Bug 60871 - HTML5 Number Spinbox displays a 0 in situations where a 0 is not between the min and max.
Summary: HTML5 Number Spinbox displays a 0 in situations where a 0 is not between the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-15 23:21 PDT by Naoki Takano
Modified: 2011-05-16 19:34 PDT (History)
4 users (show)

See Also:


Attachments
Patch (11.99 KB, patch)
2011-05-15 23:41 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2011-05-16 01:08 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2011-05-16 18:18 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Naoki Takano 2011-05-15 23:21:24 PDT
HTML5 Number Spinbox displays a 0 in situations where a 0 is not between the min and max.
Comment 1 Naoki Takano 2011-05-15 23:41:27 PDT
Created attachment 93614 [details]
Patch
Comment 2 Naoki Takano 2011-05-15 23:44:06 PDT
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,
Comment 3 Kent Tamura 2011-05-16 00:23:33 PDT
(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.
Comment 4 Naoki Takano 2011-05-16 00:27:56 PDT
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.
Comment 5 Kent Tamura 2011-05-16 00:29:19 PDT
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.
Comment 6 Naoki Takano 2011-05-16 01:08:32 PDT
Created attachment 93620 [details]
Patch
Comment 7 Naoki Takano 2011-05-16 01:09:36 PDT
Comment on attachment 93620 [details]
Patch

Just I changed C++ part.

Please review C++ part only.

I'll change the test tomorrow...
Comment 8 Kent Tamura 2011-05-16 02:58:09 PDT
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?
Comment 9 Naoki Takano 2011-05-16 17:44:29 PDT
(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?
Comment 10 Kent Tamura 2011-05-16 17:58:47 PDT
(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.
Comment 11 Naoki Takano 2011-05-16 18:18:06 PDT
Created attachment 93724 [details]
Patch
Comment 12 Naoki Takano 2011-05-16 18:18:27 PDT
Comment on attachment 93724 [details]
Patch

Please review again.
Comment 13 Kent Tamura 2011-05-16 18:50:43 PDT
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.
Comment 14 WebKit Commit Bot 2011-05-16 19:34:43 PDT
Comment on attachment 93724 [details]
Patch

Clearing flags on attachment: 93724

Committed r86650: <http://trac.webkit.org/changeset/86650>
Comment 15 WebKit Commit Bot 2011-05-16 19:34:47 PDT
All reviewed patches have been landed.  Closing bug.