Bug 48976

Summary: stepUp/stepDown for values in step-mismatching state for input elements
Product: WebKit Reporter: Dai Mikurube <dmikurube>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dai Mikurube 2010-11-03 22:52:39 PDT
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
Comment 1 Dai Mikurube 2010-11-15 04:01:21 PST
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.
Comment 2 Dai Mikurube 2010-11-15 04:05:19 PST
Created attachment 73883 [details]
Patch
Comment 3 WebKit Review Bot 2010-11-15 04:06:55 PST
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.
Comment 4 Dai Mikurube 2010-11-15 04:09:14 PST
Created attachment 73884 [details]
Patch
Comment 5 Kent Tamura 2010-11-15 04:12:38 PST
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().
Comment 6 Dai Mikurube 2010-11-15 04:29:18 PST
(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.
Comment 7 Kent Tamura 2010-11-15 04:50:50 PST
(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".
Comment 8 Dai Mikurube 2010-11-15 21:19:58 PST
(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.
Comment 9 Kent Tamura 2010-11-15 21:28:17 PST
(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.
Comment 10 Kent Tamura 2010-11-15 21:34:29 PST
(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.
Comment 11 Dai Mikurube 2010-11-15 23:00:38 PST
(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
Comment 12 Dai Mikurube 2010-11-15 23:50:40 PST
Created attachment 73968 [details]
Patch
Comment 13 Dai Mikurube 2010-11-15 23:53:07 PST
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.
Comment 14 Kent Tamura 2010-11-16 01:31:42 PST
(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 15 Kent Tamura 2010-11-16 01:38:49 PST
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.
Comment 16 Dai Mikurube 2010-11-16 02:15:10 PST
(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.
Comment 17 Kent Tamura 2010-11-16 17:57:31 PST
Hmm.  Can you check Opera's behavior?
Comment 18 Dai Mikurube 2010-11-16 18:42:47 PST
(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
Comment 19 Dai Mikurube 2010-11-16 18:48:41 PST
(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.
Comment 20 Dai Mikurube 2010-11-16 19:10:58 PST
(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
Comment 21 Dai Mikurube 2010-11-16 19:49:13 PST
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.
Comment 22 Kent Tamura 2010-11-16 21:24:24 PST
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.
Comment 23 Dai Mikurube 2010-11-16 22:35:49 PST
(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.)
Comment 24 Dai Mikurube 2010-11-16 22:41:06 PST
(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?
Comment 25 Kent Tamura 2010-11-16 22:56:49 PST
(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?
Comment 26 Dai Mikurube 2010-11-17 00:12:53 PST
(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.
Comment 27 Dai Mikurube 2010-11-17 17:56:02 PST
Created attachment 74186 [details]
Patch
Comment 28 Dai Mikurube 2010-11-17 18:00:50 PST
(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 29 Kent Tamura 2010-11-17 18:28:53 PST
Comment on attachment 74186 [details]
Patch

ok.
Comment 30 Kent Tamura 2010-11-17 18:30:13 PST
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.
Comment 31 Dai Mikurube 2010-11-17 18:39:02 PST
Created attachment 74190 [details]
Patch
Comment 32 Dai Mikurube 2010-11-17 18:39:50 PST
(In reply to comment #30)
Thanks. :)
Added descriptions.
Comment 33 Dai Mikurube 2010-11-18 19:57:14 PST
Submitted a bug for 0.00392156863 at https://bugs.webkit.org/show_bug.cgi?id=49782.
Comment 34 WebKit Commit Bot 2010-11-19 04:13:55 PST
Comment on attachment 74190 [details]
Patch

Clearing flags on attachment: 74190

Committed r72377: <http://trac.webkit.org/changeset/72377>
Comment 35 WebKit Commit Bot 2010-11-19 04:14:02 PST
All reviewed patches have been landed.  Closing bug.