Bug 48976

I found that LayoutTests assume that the current behavior is correct. But I could not find any description on it. Does someone know whether there are some descriptions on it in the HTML5 Spec? I tried writing a patch for more natural behavior. In LayoutTests/fast/forms/input-stepup-stepdown.html : For <input type="number" step="2" min="0" value="1" /> stepUp() => should be 4. (Looks like 2 is more natural.) For <input type="number" step="10" min="0" value="9" /> stepUp(9) => should be 100. (Looks like 90 is more natural.) In addition, stepDown() is expected to show a non-similar behavior. For example : For <input type="number" step="10" min="0" value="19" /> stepDown() => should be 10.

Created attachment 73883 [details] Patch

Attachment 73883 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/forms/input-stepup-stepdown-expected.txt', u'LayoutTests/fast/forms/script-tests/input-stepup-stepdown.js', u'WebCore/ChangeLog', u'WebCore/html/HTMLInputElement.cpp']" exit_code: 1 WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 73884 [details] Patch

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#dom-input-stepup shows the algorithm of stepUp(). In short words, If the current value is step-mismatched, the new value should be step-mismatched. We don't need to apply this rule to HTMLInputElement::stepUpFromRenderer().

(In reply to comment #5) Thank you, Kent-san. I see. To follow the spec, the current implementation (without my patch) should be changed so that : 1. <input type="number" step="1" min="0" max="9.9" value="8.7" /> 2. stepUp() => 9.7 Though it looks a little unnatural... Are there any other opinions from someone else? In addition, the planned fix for the bug 45491 looks also to be changed.

(In reply to comment #6) > (In reply to comment #5) > Thank you, Kent-san. > > I see. To follow the spec, the current implementation (without my patch) should be changed so that : > > 1. <input type="number" step="1" min="0" max="9.9" value="8.7" /> > 2. stepUp() => 9.7 > > Though it looks a little unnatural... Are there any other opinions from someone else? stepUp()/stepDown(), which are JavaScript API, should follow the specification, and I think a page author have the responsibility of validity of the resultant values. The behavior of HTMLInputElement::stepUpFromRenderer(), which is the function called by spin-button click, should be "natural".

(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > Thank you, Kent-san. > > > > I see. To follow the spec, the current implementation (without my patch) should be changed so that : > > > > 1. <input type="number" step="1" min="0" max="9.9" value="8.7" /> > > 2. stepUp() => 9.7 > > > > Though it looks a little unnatural... Are there any other opinions from someone else? > > stepUp()/stepDown(), which are JavaScript API, should follow the specification, and I think a page author have the responsibility of validity of the resultant values. > > The behavior of HTMLInputElement::stepUpFromRenderer(), which is the function called by spin-button click, should be "natural". Ok, - I removed any stepwise adjustment for stepUp()/stepDown(). (removing numerical errors is alive.) - I added stepwise adjustment for stepUpFromRenderer() with "natural" behaviors for step-mismatched values. Then, do you have any ideas to test stepUpFromRenderer()? I guess that sending an event (e.g. key up/down) will work, but a simpler way may be better.

(In reply to comment #8) > Then, do you have any ideas to test stepUpFromRenderer()? I guess that sending an event (e.g. key up/down) will work, but a simpler way may be better. We need to send events. Key events are easier than mouse events. LayoutTests/fast/forms/script-tests/input-number-keyoperation.js would be helpful.

(In reply to comment #9) > We need to send events. Key events are easier than mouse events. LayoutTests/fast/forms/script-tests/input-number-keyoperation.js would be helpful. LayoutTests/fast/forms/range-keyoperation.html might be better. It doesn't use eventSender.

(In reply to comment #10) > LayoutTests/fast/forms/range-keyoperation.html might be better. It doesn't use eventSender. Kent-san, Thank you. Tests with events worked fine. I found another issue to be discussed. 1. <input type="number" min="0" value="0" step="0.003921568627450980" max="1" /> 2. stepUp() for 255 times. 3. stepDown() for 255 times. => 3.838e-15 (0 is expected.) My patch removes numeric errors with "decimal places" as the bug 48308 does. (The maximum decimal places of step and the current value, not base.) This error removing does not work for the case above. The decimal places of step are large. We could remove this rounding error with stepwise adjustment at stepUp()/stepDown(). This example is shown at HTML5 - 4.10.7.2.11 The step attribute. I've added Gavin

Created attachment 73968 [details] Patch

I committed before writing completes at #11. What do you think on it? In addition, I've submitted a patch just for discussion. Tests fail for this patch.

(In reply to comment #11) > 1. <input type="number" min="0" value="0" step="0.003921568627450980" max="1" /> > 2. stepUp() for 255 times. > 3. stepDown() for 255 times. => 3.838e-15 (0 is expected.) > > My patch removes numeric errors with "decimal places" as the bug 48308 does. (The maximum decimal places of step and the current value, not base.) > > This error removing does not work for the case above. The decimal places of step are large. We could remove this rounding error with stepwise adjustment at stepUp()/stepDown(). > > This example is shown at HTML5 - 4.10.7.2.11 The step attribute. The example in the specification is step=0.00392156863. The above step value, 0.003921568627450980, is too precise for single-precision and we should decrease "decimalPaces" value for such values. I'm not sure what algorithm we should implement yet. (In reply to comment #13) > In addition, I've submitted a patch just for discussion. Tests fail for this patch. Do not set review? flag to unready patch.

Comment on attachment 73968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73968&action=review > LayoutTests/fast/forms/script-tests/input-stepup-stepdown.js:58 > +function stepUpFromRenderer(value, step, max, optionalStepCount) { I'd like to separate tests for stepUpFromRenderer() from input-stepup-stepdown.html because input-stepup-stepdown.html contains only standardized behaviors. > WebCore/html/HTMLInputElement.h:246 > + enum StepwiseAdjustment { NOADJUSTMENT, STEPWISEADJUSTMENT }; See http://webkit.org/coding/coding-style.html > 10. Enum members should user InterCaps with an initial capital letter.

(In reply to comment #15) Thanks for comments and advices on the flag and style. I found a worse result for the 0.00392156863. 1. <input type="number" min="0" value="0" step="0.00392156863" /> (max removed.) 2. stepUp() for 255 times. => 1.00000000065 This is an essential numeric error. 0.00392156863 * 255 is completely equal to 1.00000000065. I guess we can avoid it only with stepwise adjustment at stepUp(). The only reason to round 1.00000000065 is stepwise. Another option is ignoring it. We can delegate such rounding-error handling to a page author, as you said "a page author have the responsibility of validity of the resultant values". It happens just on JavaScript stepUp()/stepDown() API. It doesn't happen on the renderer. But it is less useful (stepUp()/stepDown() accumulates rounding errors) and non-compliant with the spec's example.

Hmm. Can you check Opera's behavior?

(In reply to comment #17) Hi Kent-san, Opera behaved like my "natural" behaviors : 1. <input type="number" step="1" min="0" max="9.9" value="8.7" /> 2. stepUp() => 9 1. <input type="number" step="1" min="0" max="9.9" value="8.7" /> 2. stepDown() => 8 1. <input type="number" step="1" min="0" max="10.9" value="8.7" /> 2. stepUp() => 9 1. <input type="number" step="1" min="0" max="10.9" value="8.7" /> 2. stepDown() => 8 1. <input type="number" step="2" min="0" value="1" /> 2. stepUp() => 2 1. <input type="number" step="10" min="0" value="9" /> 2. stepUp(9) => 90 1. <input type="number" step="10" min="0" value="19" /> 2. stepDown() => 10 1. <input type="number" min="0" value="0" step="0.00392156863" /> (No "max") 2. stepUp() for 255 times. => 1

(In reply to comment #18) They were results from renderer. From JavaScript API, it looks like not working for step-mismatched values. In fast/forms/input-stepup-stepdown.html : FAIL stepUp("1", "2", "") should be 3. Was 1. PASS input.stepDown(); input.value is "1" FAIL input.min = "0"; stepUp("9", "10", "", 9) should be 99. Threw exception Error: INVALID_MODIFICATION_ERR FAIL stepDown("19", "10", "0") should be 9. Was 19. FAIL stepUp("89", "10", "99") should be 99. Was 89. I'll investigate a little more.

(In reply to comment #19) JavaScript APIs showed different results. It does nothing for step-mismatching vallues. 0.00392156863 stepUp() by 255 times => 1. 1. <input type="number" step="1" min="0" max="9.9" value="8.7" /> 2. stepUp() => 8.7 1. <input type="number" step="1" min="0" max="9.9" value="8.7" /> 2. stepDown() => 8.7 1. <input type="number" step="1" min="0" max="10.9" value="8.7" /> 2. stepUp() => 8.7 1. <input type="number" step="1" min="0" max="10.9" value="8.7" /> 2. stepDown() => 8.7 1. <input type="number" step="2" min="0" value="1" /> 2. stepUp() => 1 1. <input type="number" step="10" min="0" value="9" /> 2. stepUp(9) => 9 1. <input type="number" step="10" min="0" value="19" /> 2. stepDown() => 19 1. <input type="number" min="0" value="0" step="0.00392156863" /> (No "max") 2. stepUp() for 255 times. => 1

I realized it doesn't work correctly for 0.00392156863 even with stepwise adjustment in stepUp(). Though it works for 0.003921568627450980. But Opera handles 0.00392156863. My guess is that they round floating-point numbers to smaller decimal places.

Well, we are discussing multiple issues. * "Natural" behavior The specification doesn't ask so, and Opera doesn't work so. We shouldn't apply such behavior to stepUp() and stepDown() API. On the other hand, UI should work so. It should be handled in Bug 45491. * 0.00392156863 * 255 rounding This is also out of scope of this bug. We had better make another bug entry. * stepUp()/stepDown() behavior with step-mismatching values We should focus on it in this bug. IMO, we should follow the specification, not Opera. Opera might change its behavior in the future.

(In reply to comment #22) Ok. The top one and bottom one are strongly related. In summary : > * "Natural" behavior > The specification doesn't ask so, and Opera doesn't work so. We shouldn't apply such behavior to stepUp() and stepDown() API. > On the other hand, UI should work so. It should be handled in Bug 45491. For this, - remove already existing behaviors (before my patch) on stepwise adjustment for JS API => It is included in fixing this bug. - add (or fix) stepwise adjustment for UI => It should be done in Bug 45491. > * 0.00392156863 * 255 rounding > This is also out of scope of this bug. We had better make another bug entry. I agree. I found we can calculate it if just using IEEE 754 single precision numbers. We should re-consider precisions and how to print them in another bug. > * stepUp()/stepDown() behavior with step-mismatching values > We should focus on it in this bug. > IMO, we should follow the specification, not Opera. Opera might change its behavior in the future. Ok, this is, actually, removing stepwise adjustment for JS API. So the latest patch is near to the specification except for UI changes in stepUpFromRenderer(). (UI changes should be done in Bug 45491.)

(In reply to comment #23) In addition, however, I guess the original purpose of (existing) stepwise adjustment is to remove accumulated rounding errors, not to adjust step-mismatching values. By removing the adjustment, I'm afraid that rounding errors may start to accumulate by stepUp()s. What do you think on it?

(In reply to comment #24) > (In reply to comment #23) > In addition, however, I guess the original purpose of (existing) stepwise adjustment is to remove accumulated rounding errors, not to adjust step-mismatching values. > > By removing the adjustment, I'm afraid that rounding errors may start to accumulate by stepUp()s. What do you think on it? How about adjusting new value only if the original value is matched to the step value?

(In reply to comment #25) > How about adjusting new value only if the original value is matched to the step value? Thank you. It's a nice idea.

Created attachment 74186 [details] Patch

(In reply to comment #27) Submitted a patch to remove already existing stepwise adjustment. It works along the spec for step-mismatching values. No difference for step-matching values. Changes for UI will be done in Bug 45491. Handling 0.00392156863 * 255 will be done in another bug. (Not filed yet.)

Comment on attachment 74186 [details] Patch ok.

Comment on attachment 74186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74186&action=review not ok :-) > WebCore/ChangeLog:8 > + Fixed behaviors for already step-mismatched values. Need to describe how you fixed and/or new behavior.

Created attachment 74190 [details] Patch

(In reply to comment #30) Thanks. :) Added descriptions.

Submitted a bug for 0.00392156863 at https://bugs.webkit.org/show_bug.cgi?id=49782.

Comment on attachment 74190 [details] Patch Clearing flags on attachment: 74190 Committed r72377: <http://trac.webkit.org/changeset/72377>

All reviewed patches have been landed. Closing bug.