Bug 48976 – stepUp/stepDown for values in step-mismatching state for input elements

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dai Mikurube]

These are not described in the spec. But I wrote reasonably "Expected" behaviors below.
Opinions for these "Expected" behaviors are also welcome.

This bug was discussed at bug 48308. [https://bugs.webkit.org/show_bug.cgi?id=48308]


[Case A]
Steps to reproduce:
1. <input type="number" step="1" min="0" max="9.9" value="8.7" />
2. stepUp()

Expected:
9

Actually happens:
8.7 (no change)


[Case B]
Steps to reproduce:
1. <input type="number" step="1" min="0" max="10.9" value="8.7" /> 
2. stepUp()

Expected:
9

Actually happens:
10

[Dai Mikurube]

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.

[Dai Mikurube]

Created attachment 73883 [details]
Patch

[WebKit Review Bot]

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.

[Dai Mikurube]

Created attachment 73884 [details]
Patch

[Kent Tamura]

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

[Dai Mikurube]

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

[Kent Tamura]

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

[Dai Mikurube]

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

[Kent Tamura]

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

[Kent Tamura]

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

[Dai Mikurube]

(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

[Dai Mikurube]

Created attachment 73968 [details]
Patch

[Dai Mikurube]

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.

[Kent Tamura]

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

[Kent Tamura]

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.

[Dai Mikurube]

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

[Kent Tamura]

Hmm.  Can you check Opera's behavior?

[Dai Mikurube]

(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

[Dai Mikurube]

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

[Dai Mikurube]

(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

[Dai Mikurube]

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.

[Kent Tamura]

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.

[Dai Mikurube]

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

[Dai Mikurube]

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

[Kent Tamura]

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

[Dai Mikurube]

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

[Dai Mikurube]

Created attachment 74186 [details]
Patch

[Dai Mikurube]

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

[Kent Tamura]

Comment on attachment 74186 [details]
Patch

ok.

[Kent Tamura]

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.

[Dai Mikurube]

Created attachment 74190 [details]
Patch

[Dai Mikurube]

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

[Dai Mikurube]

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

[WebKit Commit Bot]

Comment on attachment 74190 [details]
Patch

Clearing flags on attachment: 74190

Committed r72377: <http://trac.webkit.org/changeset/72377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.