Summary: | input[type=number] does not increment/decrement integers with trailing decimal characters | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||||
Component: | Forms | Assignee: | 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
Ryosuke Niwa
2015-09-04 18:56:36 PDT
Created attachment 262940 [details]
Patch
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). 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. Please take feedback into consideration and re-upload. Testing turned up that numbers like "-.2" were not parsed as valid input, so added support for that, too. Created attachment 263031 [details]
Patch
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. (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. Created attachment 263198 [details]
Patch
Comment on attachment 263198 [details]
Patch
Is Decimal::fromString used anywhere else other than <input type=number>?
It's called from parseToDecimalForNumberType, which in turn is called from NumberInputType, RangeInputType, SliderThumbElement, and StepRange. 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. Created attachment 264161 [details]
Patch
This patch includes tests for the additional elements that Darin called attention to. 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. Created attachment 264642 [details]
Patch
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? Created attachment 264666 [details]
Patch
Created attachment 264667 [details]
Patch
Comment on attachment 264667 [details] Patch Clearing flags on attachment: 264667 Committed r191940: <http://trac.webkit.org/changeset/191940> All reviewed patches have been landed. Closing bug. |