Bug 60871 – HTML5 Number Spinbox displays a 0 in situations where a 0 is not between the min and max.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Naoki Takano]

HTML5 Number Spinbox displays a 0 in situations where a 0 is not between the min and max.

[Naoki Takano]

Created attachment 93614 [details]
Patch

[Naoki Takano]

Comment on attachment 93614 [details]
Patch

Please review my patch.

The original didn't have tests
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp?rev=85998

So I added.

If we don't need it, please let me know.

Thanks,

[Kent Tamura]

(In reply to comment #2)
> (From update of attachment 93614 [details])
> Please review my patch.
> 
> The original didn't have tests
> http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp?rev=85998
> 
> So I added.

No, we had.
http://trac.webkit.org/changeset/72884

Please add new test cases to LayoutTest/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js.

[Naoki Takano]

Oops, I should I ask first;-)

If you have any comment in C++ code, please give me too.

Thanks,

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 93614 [details] [details])
> > Please review my patch.
> > 
> > The original didn't have tests
> > http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp?rev=85998
> > 
> > So I added.
> 
> No, we had.
> http://trac.webkit.org/changeset/72884
> 
> Please add new test cases to LayoutTest/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js.

[Kent Tamura]

Comment on attachment 93614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93614&action=review

> Source/WebCore/html/HTMLInputElement.cpp:339
> -void HTMLInputElement::applyStep(double count, ExceptionCode& ec)
> +void HTMLInputElement::applyStep(double count, bool clearedByDefault, ExceptionCode& ec)

I don't think we need to handle such cases in applyStep().  We can handle such cases only in stepUpFromRenderer().
Also, a bool function parameter is not recommended.

[Naoki Takano]

Created attachment 93620 [details]
Patch

[Naoki Takano]

Comment on attachment 93620 [details]
Patch

Just I changed C++ part.

Please review C++ part only.

I'll change the test tomorrow...

[Kent Tamura]

Comment on attachment 93620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93620&action=review

> LayoutTests/fast/forms/numeric-input-spin.html:48
> +//     - The value should be the minimum value for both keyDown and keyUp.

Don't call them keydown / keyup.  Keydown and keyup are the names of DOM events.

> Source/WebCore/html/HTMLInputElement.cpp:1466
> +        int nextDiff = step * n;
> +        if (current < m_inputType->minimum() - nextDiff)
> +            current = m_inputType->minimum() - nextDiff;

This looks tricky.  But I understand you want to use the following step-rounding and the event dispatching.  So I think this is acceptable.

> Source/WebCore/html/HTMLInputElement.cpp:1475
> -        if (stepMismatch(currentStringValue)) {
> +        if (stepMismatch(value())) {

Instead of this change, we had better do currentStringValue=value() just after setValueAsNumber().
Anyway, this also fixes another bug http://crbug.com/76046 , right?

[Naoki Takano]

(In reply to comment #8)
> (From update of attachment 93620 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93620&action=review
> 
> > LayoutTests/fast/forms/numeric-input-spin.html:48
> > +//     - The value should be the minimum value for both keyDown and keyUp.
> 
> Don't call them keydown / keyup.  Keydown and keyup are the names of DOM events.
Ok, anyway the test is consolidated to the test you suggested;-)

> 
> > Source/WebCore/html/HTMLInputElement.cpp:1466
> > +        int nextDiff = step * n;
> > +        if (current < m_inputType->minimum() - nextDiff)
> > +            current = m_inputType->minimum() - nextDiff;
> 
> This looks tricky.  But I understand you want to use the following step-rounding and the event dispatching.  So I think this is acceptable.
Thanks,

> 
> > Source/WebCore/html/HTMLInputElement.cpp:1475
> > -        if (stepMismatch(currentStringValue)) {
> > +        if (stepMismatch(value())) {
> 
> Instead of this change, we had better do currentStringValue=value() just after setValueAsNumber().
> Anyway, this also fixes another bug http://crbug.com/76046 , right?
I didn't know the bug. But fortunately, the bug is gone after the change.

My original intention is to fix this one
//   * If 0 is in-range, but not matched to step value
//     - The value should be the larger matched value nearest to 0 if n > 0   //       e.g. <input type=number min=-100 step=3> -> 2               

Should I separate the bug entry?


Also I suppose 
> > +        if (stepMismatch(value())) {
Is better than currentStringValue=value().

Because after this step, the following steps are executed,
if (currentStringValue != value()) {
    if (m_inputType->isRangeControl())
            dispatchFormControlChangeEvent();
        else
            dispatchFormControlInputEvent();
}

So shouldn't we change currentStringValue, right?

What do you think?

[Kent Tamura]

(In reply to comment #9)
> > Instead of this change, we had better do currentStringValue=value() just after setValueAsNumber().
> > Anyway, this also fixes another bug http://crbug.com/76046 , right?
> I didn't know the bug. But fortunately, the bug is gone after the change.
> 
> My original intention is to fix this one
> //   * If 0 is in-range, but not matched to step value
> //     - The value should be the larger matched value nearest to 0 if n > 0   //       e.g. <input type=number min=-100 step=3> -> 2               
> 
> Should I separate the bug entry?

I don't think so.  This change is needed for this issue.  Let's go ahead with the single patch.

> Also I suppose 
> > > +        if (stepMismatch(value())) {
> Is better than currentStringValue=value().
> 
> Because after this step, the following steps are executed,
> if (currentStringValue != value()) {
>     if (m_inputType->isRangeControl())
>             dispatchFormControlChangeEvent();
>         else
>             dispatchFormControlInputEvent();
> }
> 
> So shouldn't we change currentStringValue, right?

Ah, you're right.

[Naoki Takano]

Created attachment 93724 [details]
Patch

[Naoki Takano]

Comment on attachment 93724 [details]
Patch

Please review again.

[Kent Tamura]

Comment on attachment 93724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93724&action=review

> LayoutTests/fast/forms/script-tests/input-stepup-stepdown-from-renderer.js:245
> +shouldBe('stepUpExplicitBounds(-4, 4, 1, "")', '"1"');
> +shouldBe('stepDownExplicitBounds(-4, 4, 1, "")', '"-1"');
> +shouldBe('stepDownExplicitBounds(0, 4, 1, "")', '"0"');
> +shouldBe('stepUpExplicitBounds(-4, 0, 1, "")', '"0"');
> +shouldBe('stepDownExplicitBounds(1, 4, 1, "")', '"1"');
> +shouldBe('stepUpExplicitBounds(1, 4, 1, "")', '"1"');
> +shouldBe('stepDownExplicitBounds(-4, -1, 1, "")', '"-1"');
> +shouldBe('stepUpExplicitBounds(-4, -1, 1, "")', '"-1"');
> +shouldBe('stepUpExplicitBounds(-100, null, 3, "")', '"2"');
> +shouldBe('stepDownExplicitBounds(-100, null, 3, "")', '"-1"');
> +shouldBe('stepUpExplicitBounds(1, 4, 1, 0)', '"1"');
> +shouldBe('stepDownExplicitBounds(1, 4, 1, 0)', '"0"');
> +shouldBe('stepDownExplicitBounds(-4, -1, 1, 0)', '"-1"');
> +shouldBe('stepUpExplicitBounds(-4, -1, 1, 0)', '"0"');
> +shouldBe('stepUpExplicitBounds(-100, null, 3, 3)', '"5"');
> +shouldBe('stepDownExplicitBounds(-100, null, 3, 3)', '"2"');

nit: The function arguments should be strings unless null.
Specifying numbers causes extra conversion.

[WebKit Commit Bot]

Comment on attachment 93724 [details]
Patch

Clearing flags on attachment: 93724

Committed r86650: <http://trac.webkit.org/changeset/86650>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.