Bug 148867

Summary: input[type=number] does not increment/decrement integers with trailing decimal characters
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: FormsAssignee: Keith Rollin <krollin>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, gyuyoung.kim, krollin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ryosuke Niwa 2015-09-04 18:56:36 PDT
See https://html.spec.whatwg.org/multipage/infrastructure.html#valid-floating-point-number

e.g. strings such as "1." should be treated as an invalid value and sanitized to "".

There are many other issues found in
LayoutTests/http/tests/w3c/html/semantics/forms/the-input-element/number.html
Comment 1 Radar WebKit Bug Importer 2015-09-04 18:57:05 PDT
<rdar://problem/22589693>
Comment 2 Keith Rollin 2015-10-12 16:46:58 PDT
Created attachment 262940 [details]
Patch
Comment 3 Chris Dumez 2015-10-12 17:17:42 PDT
Comment on attachment 262940 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        input[type=number] does not sanitize value properly

The bug title needs to be updated to reflect your actual change.

> Source/WebCore/ChangeLog:14
> +        Updated the following tests:

test (singular)

> Source/WebCore/ChangeLog:15
> +        * fast/forms/number/number-stepup-stepdown-from-renderer-expected.txt:

Drop this line.

> Source/WebCore/ChangeLog:16
> +        * fast/forms/number/number-stepup-stepdown-from-renderer.html:

Use - instead of * to avoid confusing our tools and drop the semicolon at the end.

> LayoutTests/fast/forms/number/number-stepup-stepdown-from-renderer.html:110
> +debug('Normal cases with unusual input')

I think it would be interesting to add coverage for values with a 0 after the dot (e.g. '1.0') to show that the results are consistent. We also probably want to cover a different step interval than 1 (e.g. '1.0' and '1.' with a step interval of 0.1 so we end up with 1.1).
Comment 4 Keith Rollin 2015-10-12 19:10:16 PDT
Updating the bug to reflect the result of some analysis. The test case in the original report turned up five tests that were marked as FAIL. Four of them also failed in Firefox and Chrome, and so were considered behavior that we wanted to keep in the name of compatibility. The remaining case -- one that reported that "1." should be considered invalid -- failed in Safari and Chrome, but not Firefox. Upon considering that case, taking into account general usage, conventions in other languages, cross-browser compatibility, and general usefulness -- we've decided to keep the current behavior of treating "1." as valid.

It turns out that the HTML spec is ambiguous with regards to whether or not "###." should be considered valid or not. A bug has been filed to track this issue (https://github.com/whatwg/html/issues/250).

That aside, upon further testing, we noticed that attempting to subsequently change the value with the up/down arrows in the UI or the stepUp/Down methods on the input object failed. These operations considered any number with the form "###." as zero and so incremented them to "1" or decremented them to "-1". This bug now addresses that issue.
Comment 5 Chris Dumez 2015-10-13 12:53:11 PDT
Please take feedback into consideration and re-upload.
Comment 6 Keith Rollin 2015-10-13 15:32:42 PDT
Testing turned up that numbers like "-.2" were not parsed as valid input, so added support for that, too.
Comment 7 Keith Rollin 2015-10-13 15:56:47 PDT
Created attachment 263031 [details]
Patch
Comment 8 Chris Dumez 2015-10-14 09:45:04 PDT
Comment on attachment 263031 [details]
Patch

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

r=me with nit.

> LayoutTests/fast/forms/number/number-stepup-stepdown.html:70
> +

If you want nicer separation in the output as well, you can add a debug(""); line as well.

> LayoutTests/fast/forms/number/number-stepup-stepdown.html:115
> +//shouldBe('stepUp("1.1", 1, null)', '"2.1"');

I would leave those out since they are duplicates. Having commented out checks seems weird.
Comment 9 Keith Rollin 2015-10-14 10:57:03 PDT
(In reply to comment #8)
> If you want nicer separation in the output as well, you can add a debug("");
> line as well.

It sounds to me like you consider this optional, to do at my discretion. I'll leave it out for now, but keep it in mind for next time.

> I would leave those out since they are duplicates. Having commented out
> checks seems weird.

Wilco.
Comment 10 Keith Rollin 2015-10-15 15:16:01 PDT
Created attachment 263198 [details]
Patch
Comment 11 Darin Adler 2015-10-15 16:33:35 PDT
Comment on attachment 263198 [details]
Patch

Is Decimal::fromString used anywhere else other than <input type=number>?
Comment 12 Keith Rollin 2015-10-16 09:51:30 PDT
It's called from parseToDecimalForNumberType, which in turn is called from NumberInputType, RangeInputType, SliderThumbElement, and StepRange.
Comment 13 Chris Dumez 2015-10-16 10:06:48 PDT
Comment on attachment 263198 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Reviewed by cdumez@apple.com.

I don't think email will work. This is supposed to be "by Chris Dumez."

> Source/WebCore/platform/Decimal.cpp:827
> +            HandleCharAndBreak('.', StateDot);

The comment in the header says:
"""
    // fromString supports following syntax EBNF:
    //  number ::= sign? digit+ ('.' digit*) (exponent-marker sign? digit+)?
    //          | sign? '.' digit+ (exponent-marker sign? digit+)?
    //  sign ::= '+' | '-'
    //  exponent-marker ::= 'e' | 'E'
    //  digit ::= '0' | '1' | ... | '9'
    // Note: fromString doesn't support "infinity" and "nan".
"""

So, your changes seem to match the behavior claimed by the method documentation.

> LayoutTests/ChangeLog:7
> +        Reviewed by cdumez@apple.com.

ditto.
Comment 14 Keith Rollin 2015-10-27 14:40:30 PDT
Created attachment 264161 [details]
Patch
Comment 15 Keith Rollin 2015-10-27 14:41:19 PDT
This patch includes tests for the additional elements that Darin called attention to.
Comment 16 Darin Adler 2015-10-31 10:59:34 PDT
Comment on attachment 264161 [details]
Patch

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

> LayoutTests/platform/mac/fast/forms/range/input-appearance-range-expected.txt:3
> +layer at (0,0) size 800x303

This change in expected result isn’t mentioned in the change log. I think we should change this test to be a reference test instead of a render tree and pixel dumping test at some point in the future.
Comment 17 Keith Rollin 2015-11-02 15:44:18 PST
Created attachment 264642 [details]
Patch
Comment 18 Chris Dumez 2015-11-02 16:11:45 PST
Comment on attachment 264642 [details]
Patch

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

> LayoutTests/fast/forms/range/input-appearance-range-decimals-expected.html:3
> +<div><input type=range value="0.4" min="-0.4" max="1.0"></div>

Why the div?

> LayoutTests/fast/forms/range/input-appearance-range-decimals.html:3
> +<!-- These numbers should parse the samed as 0.4, -0.4, and 1.0 -->

typo: same

> LayoutTests/fast/forms/range/input-appearance-range-decimals.html:4
> +<div><input type=range value=".4" min="-.4" max="1."></div>

Why the div?
Comment 19 Keith Rollin 2015-11-02 21:46:08 PST
Created attachment 264666 [details]
Patch
Comment 20 Keith Rollin 2015-11-02 21:46:39 PST
Created attachment 264667 [details]
Patch
Comment 21 WebKit Commit Bot 2015-11-02 23:22:34 PST
Comment on attachment 264667 [details]
Patch

Clearing flags on attachment: 264667

Committed r191940: <http://trac.webkit.org/changeset/191940>
Comment 22 WebKit Commit Bot 2015-11-02 23:22:39 PST
All reviewed patches have been landed.  Closing bug.