Bug 235509 - [Forms] Value doesn't change for stepUp()/stepDown() with out of range values
Summary: [Forms] Value doesn't change for stepUp()/stepDown() with out of range values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zsun
URL:
Keywords: InRadar
: 192587 233721 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-01-24 06:07 PST by zsun
Modified: 2022-05-30 17:38 PDT (History)
14 users (show)

See Also:


Attachments
Patch (10.26 KB, patch)
2022-01-24 06:29 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (39.71 KB, patch)
2022-02-03 05:25 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (39.64 KB, patch)
2022-02-03 07:19 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (15.03 KB, patch)
2022-02-03 18:26 PST, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zsun 2022-01-24 06:07:51 PST
Currently WebKit throws "invalid state" for these cases. As per section 10 in https://html.spec.whatwg.org/multipage/input.html#dom-input-stepup, we should do "return", which  is to indicate that value doesn't change.
Comment 1 zsun 2022-01-24 06:29:14 PST
Created attachment 449807 [details]
Patch
Comment 2 zsun 2022-01-24 08:27:49 PST
There are a few tests failed in fast/forms/, these tests are expecting "invalid state".

I guess it probably makes sense to reach an agreement to use "return {}" rather than "return Exception { InvalidStateError }" for out of range for stepup() and stepdown() first, before putting effort on updating these tests?

Any comments? Thank you!
Comment 3 Radar WebKit Bug Importer 2022-01-31 06:08:19 PST
<rdar://problem/88266239>
Comment 4 zsun 2022-02-03 05:25:50 PST
Created attachment 450765 [details]
Patch
Comment 5 Chris Dumez 2022-02-03 07:10:19 PST
Comment on attachment 450765 [details]
Patch

Looks like there are some legit failures on WK1: https://ews-build.s3-us-west-2.amazonaws.com/macOS-Catalina-Release-WK1-Tests-EWS/r450765-25455/results.html
Comment 6 zsun 2022-02-03 07:19:44 PST
Created attachment 450770 [details]
Patch
Comment 7 Chris Dumez 2022-02-03 07:23:01 PST
Comment on attachment 450770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450770&action=review

> LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/input-stepdown-expected.txt:7
> +FAIL Calling stepDown() on input - date - where value < min should not modify value. The object is in an invalid state.

Why is WK1 behaving differently here? The code running should be the same.

I suspect those WK1 baselines are wrong.

> LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/input-stepup-expected.txt:8
> +FAIL Calling stepUp() on input -date- where value > max should not modify value. The object is in an invalid state.

ditto.
Comment 8 Chris Dumez 2022-02-03 07:39:21 PST
Comment on attachment 450770 [details]
Patch

r=me. Based on EWS output, those Mac-wk1 baselines are correct so r+. That said, I am kinda curious why our behavior is worse on WK1 here.
Comment 9 zsun 2022-02-03 07:59:44 PST
(In reply to Chris Dumez from comment #8)
> Comment on attachment 450770 [details]
> Patch
> 
> r=me. Based on EWS output, those Mac-wk1 baselines are correct so r+.

Thanks very much for the review!

> I am kinda curious why our behavior is worse on WK1 here.

Probably not "worse". One sub-test (number) is now passing for WK1 while for other platforms all underflow/overflow sub-tests are passing.
Comment 10 Chris Dumez 2022-02-03 08:00:54 PST
(In reply to zsun from comment #9)
> (In reply to Chris Dumez from comment #8)
> > Comment on attachment 450770 [details]
> > Patch
> > 
> > r=me. Based on EWS output, those Mac-wk1 baselines are correct so r+.
> 
> Thanks very much for the review!
> 
> > I am kinda curious why our behavior is worse on WK1 here.
> 
> Probably not "worse". One sub-test (number) is now passing for WK1 while for
> other platforms all underflow/overflow sub-tests are passing.

I am not saying worse because of your patch. However, it seems WK2 got more new PASSes from your patch than WK1 no?
Comment 11 zsun 2022-02-03 11:15:54 PST
(In reply to Chris Dumez from comment #10)
> (In reply to zsun from comment #9)
> > (In reply to Chris Dumez from comment #8)
> > > Comment on attachment 450770 [details]
> > > Patch
> > > 
> > > r=me. Based on EWS output, those Mac-wk1 baselines are correct so r+.
> > 
> > Thanks very much for the review!
> > 
> > > I am kinda curious why our behavior is worse on WK1 here.
> > 
> > Probably not "worse". One sub-test (number) is now passing for WK1 while for
> > other platforms all underflow/overflow sub-tests are passing.
> 
> I am not saying worse because of your patch. However, it seems WK2 got more
> new PASSes from your patch than WK1 no?

Yes, you are right :)
Comment 12 EWS 2022-02-03 13:36:03 PST
Committed r289075 (246781@main): <https://commits.webkit.org/246781@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450770 [details].
Comment 13 Sam Weinig 2022-02-03 18:26:56 PST
Reopening to attach new patch.
Comment 14 Sam Weinig 2022-02-03 18:26:58 PST
Created attachment 450847 [details]
Patch
Comment 15 zsun 2022-02-04 00:51:53 PST
(In reply to Sam Weinig from comment #14)
> Created attachment 450847 [details]
> Patch

Sam, is the patch you attached related to this issue?
Comment 16 Chris Dumez 2022-02-04 07:19:36 PST
Comment on attachment 450847 [details]
Patch

He uploaded to the wrong bug.
Comment 17 Brent Fulgham 2022-02-04 14:51:21 PST
Moving back to Resolved/Fixed.
Comment 18 Tim Nguyen (:ntim) 2022-05-11 08:19:42 PDT
*** Bug 233721 has been marked as a duplicate of this bug. ***
Comment 19 pascoe@apple.com 2022-05-30 17:38:30 PDT
*** Bug 192587 has been marked as a duplicate of this bug. ***