Bug 148867 – input[type=number] does not increment/decrement integers with trailing decimal characters

• Component: Forms
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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

[Radar WebKit Bug Importer]

<rdar://problem/22589693>

[Keith Rollin]

Created attachment 262940 [details]
Patch

[Chris Dumez]

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).

[Keith Rollin]

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.

[Chris Dumez]

Please take feedback into consideration and re-upload.

[Keith Rollin]

Testing turned up that numbers like "-.2" were not parsed as valid input, so added support for that, too.

[Keith Rollin]

Created attachment 263031 [details]
Patch

[Chris Dumez]

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.

[Keith Rollin]

(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.

[Keith Rollin]

Created attachment 263198 [details]
Patch

[Darin Adler]

Comment on attachment 263198 [details]
Patch

Is Decimal::fromString used anywhere else other than <input type=number>?

[Keith Rollin]

It's called from parseToDecimalForNumberType, which in turn is called from NumberInputType, RangeInputType, SliderThumbElement, and StepRange.

[Chris Dumez]

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 [e-mail].

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 [e-mail].

ditto.

[Keith Rollin]

Created attachment 264161 [details]
Patch

[Keith Rollin]

This patch includes tests for the additional elements that Darin called attention to.

[Darin Adler]

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.

[Keith Rollin]

Created attachment 264642 [details]
Patch

[Chris Dumez]

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?

[Keith Rollin]

Created attachment 264666 [details]
Patch

[Keith Rollin]

Created attachment 264667 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 264667 [details]
Patch

Clearing flags on attachment: 264667

Committed r191940: <http://trac.webkit.org/changeset/191940>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.