Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Review html/semantics/forms/the-input-element/range.html #107

Closed
nt1m opened this issue Aug 12, 2022 · 7 comments
Closed

Review html/semantics/forms/the-input-element/range.html #107

nt1m opened this issue Aug 12, 2022 · 7 comments
Labels
test-change-proposal Proposal to add or remove tests for an interop area

Comments

@nt1m
Copy link
Member

nt1m commented Aug 12, 2022

** Test List **

  • html/semantics/forms/the-input-element/range.html

** Rationale **

Not suggesting any concrete action yet, but we should review html/semantics/forms/the-input-element/range.html which has subtests failing across all 3 browsers. Either this needs a spec change to reflect current browsers behavior, either we keep the test as it is. @cdumez has mentioned it's pretty simple to implement these changes in WebKit, but it would leave WebKit uninteroperable with the other 2 browser vendors.

cc @domenic and @annevk to check if there is any spec work needed here.
@nt1m nt1m added the test-change-proposal Proposal to add or remove tests for an interop area label Aug 12, 2022
@foolip foolip mentioned this issue Aug 17, 2022
Closed
@foolip
Copy link
Member

foolip commented Aug 18, 2022

@josepharhar can you take a look?

@nt1m
Copy link
Member Author

nt1m commented Aug 18, 2022

@emilio @jgraham What do you think?

josepharhar added a commit to josepharhar/wpt that referenced this issue Aug 18, 2022
@josepharhar
Fix "Skip ASCII whitespace within input" test
241e7e5 
This test sets the range input's value greater than the default maximum,
causing the value not to be set properly. By changing it to 50, the
value will be set properly.

The underlying test of skipping whitespace should still work fine since
the value assigned into the input is " 50".

This was identified in web-platform-tests/interop#107
@josepharhar josepharhar mentioned this issue Aug 18, 2022
Merged
@josepharhar
Copy link

Thanks for finding this! I am generally in favor of changing the tests/spec to match the browsers when the browsers are all doing the same thing.

The "The default scale factor is 1 even if step attribute is explicitly set to non-integer value, unless min attribute has non-integer value" test was added by @tomoyukilabs here: web-platform-tests/wpt#167
If I'm reading it correctly, the test description suggests that step should be forced to 1 when the min attribute has an integer value. The test does seem to do this by setting min to 5, but I'm not sure what the motivation is. I've never seen any bugs filed about this in chromium. Perhaps we could just remove the test.

The "Skip ASCII whitespace within input" test was added by @nox here: web-platform-tests/wpt#9084
The "123" value is larger than the default maximum of range inputs, so this test could be fixed by just changing the "123" to "50" or something. I've opened a PR to do this here: web-platform-tests/wpt#35519

@annevk
Copy link
Member

annevk commented Aug 24, 2022

I think the test is slightly incorrect (and browsers appear correct) as I explained in web-platform-tests/wpt#35519 (comment). Hope that helps.

josepharhar added a commit to web-platform-tests/wpt that referenced this issue Aug 24, 2022
@josepharhar
Fix "Skip ASCII whitespace within input" test (#35519)
bc183f1 
This test assigns an invalid value into the range input, which should
reset the input to its default value 50.

This was identified in web-platform-tests/interop#107
This was referenced Aug 24, 2022
Closed
Closed
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 5, 2022
@josepharhar @moz-wptsync-bot
Bug 1785899 [wpt PR 35519] - Fix "Skip ASCII whitespace within input"…
a88f4e3 
… test, a=testonly

Automatic update from web-platform-tests
Fix "Skip ASCII whitespace within input" test (#35519)

This test assigns an invalid value into the range input, which should
reset the input to its default value 50.

This was identified in web-platform-tests/interop#107
--

wpt-commits: bc183f1426aba64995ef40555ce405ce45183a26
wpt-pr: 35519
@nt1m
Copy link
Member Author

nt1m commented Sep 11, 2022

The default scale factor is 1 even if step attribute is explicitly set to non-integer value, unless min attribute has non-integer value

Does anyone have opinions on how to handle this subtest?

@foolip
Copy link
Member

foolip commented Sep 15, 2022

@nt1m that subtest seems wrong per spec. https://html.spec.whatwg.org/multipage/input.html#dom-input-step says that the step IDL attribute simply reflects the content attribute. The test description doesn't really make sense, and I think we should delete this subtest.

@nt1m nt1m mentioned this issue Sep 15, 2022
Merged
josepharhar added a commit to josepharhar/wpt that referenced this issue Sep 15, 2022
@josepharhar
Remove "default scale factor is 1" range.html subtest
15c5c8e 
This test fails in all 3 browsers and we decided to remove it here:
web-platform-tests/interop#107
@josepharhar josepharhar mentioned this issue Sep 15, 2022
Closed
@foolip
Copy link
Member

foolip commented Sep 15, 2022

Fixed by web-platform-tests/wpt#35914

@foolip foolip closed this as completed Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@foolip @annevk @nt1m @josepharhar