Bug 64786 – The value of a number input form continues to increase/decrease even if we disable the input form.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kentaro Hara]

Assume the following html:

<input type="number" id="number" value="2011" onmouseup="moseup()">
<script type="text/javascript">
function mouseup() {
    document.getElementById("number").disabled = "disabled";
    setTimeout(function() {
        document.getElementById("number").disabled = undefined;
    }, 1000);
}
</script>

Click the spin button of the number input form.

Expected behavior: The value becomes 2010.

Actual behavior: The value becomes 2010. After 1000ms, the value continues to decrease automatically as if we are still clicking the spin button. 

See also http://code.google.com/p/chromium/issues/detail?id=74960.

[Kentaro Hara]

Created attachment 101288 [details]
Patch

[Kent Tamura]

Comment on attachment 101288 [details]
Patch

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

> LayoutTests/ChangeLog:20
> +        Reviewed by NOBODY (OOPS!).

This line should be in the next of the bug URL.

> Source/WebCore/html/HTMLInputElement.cpp:795
> +    else if (attr->name() == disabledAttr)
> +        m_inputType->disabledAttributeChanged();
> +    else if (attr->name() == readonlyAttr)
> +        m_inputType->readonlyAttributeChanged();

This will make some regressions because HTMLFormControlElement::parseMappedAttribute() won't be called for disabled and readonly.

> Source/WebCore/html/TextFieldInputType.cpp:248
> +    m_innerSpinButton->stopRepeating();
> +}
> +
> +void TextFieldInputType::readonlyAttributeChanged()
> +{
> +    m_innerSpinButton->stopRepeating();

m_innerSpinButton can be NULL.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:336
> +void SpinButtonElement::stopRepeating()
> +{
> +    stopRepeatingTimer();
> +}

How about calling stopRepeatingTimer() in repeatingTimerFired()? If we do so, we don't need to add disabledAttributeChanged() and readonlyAttribtueChanged().

[Kentaro Hara]

Created attachment 101293 [details]
Patch

[Kentaro Hara]

Comment on attachment 101288 [details]
Patch

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

>> LayoutTests/ChangeLog:20
>> +        Reviewed by NOBODY (OOPS!).
> 
> This line should be in the next of the bug URL.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:795
>> +        m_inputType->readonlyAttributeChanged();
> 
> This will make some regressions because HTMLFormControlElement::parseMappedAttribute() won't be called for disabled and readonly.

Removed this method, going away this issue.

>> Source/WebCore/html/TextFieldInputType.cpp:248
>> +    m_innerSpinButton->stopRepeating();
> 
> m_innerSpinButton can be NULL.

Removed this method, going away this issue.

>> Source/WebCore/html/shadow/TextControlInnerElements.cpp:336
>> +}
> 
> How about calling stopRepeatingTimer() in repeatingTimerFired()? If we do so, we don't need to add disabledAttributeChanged() and readonlyAttribtueChanged().

Great idea! But I think that stopRepeatingTimer() is necessary not only in repeatingTimerFired() but also in defaultEventHandler(). There is a possibility that 'mouseup' event happens and then the input form gets enabled again, before the first repeatingTimerFired() is invoked.

[Kent Tamura]

Comment on attachment 101293 [details]
Patch

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

> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly-expected.txt:14
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +
> +Test on a readonly number input form:
> +delay = 1 ms
> +PASS input.value is "1234566"

"TEST COMPLETE" followed by actual tests sounds very strange.
You had better use jsTestisAsync and finishJSTest().  See fast/forms/autofocus-keygen.html.

> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:25
> +var test_inputs;
> +var test_delays;

We usually use camelCase for variables.

[Kent Tamura]

(In reply to comment #4)
> > How about calling stopRepeatingTimer() in repeatingTimerFired()? If we do so, we don't need to add disabledAttributeChanged() and readonlyAttribtueChanged().
> 
> Great idea! But I think that stopRepeatingTimer() is necessary not only in repeatingTimerFired() but also in defaultEventHandler(). There is a possibility that 'mouseup' event happens and then the input form gets enabled again, before the first repeatingTimerFired() is invoked.

I see. But adding stopRepeatingTimer() in defaultEventHandler() looks inelegant. I think the idea of disabledAttributeChanged() and readonlyAttribtueChanged() was better.

BTW, when the input becomes disabled/readonly, is mouse capturing released correctly? (Is  setCapturingMouseEventsNode(0) called?)

[Kentaro Hara]

Created attachment 101302 [details]
Patch

[Kentaro Hara]

Comment on attachment 101288 [details]
Patch

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

>>> Source/WebCore/html/HTMLInputElement.cpp:795
>>> +        m_inputType->readonlyAttributeChanged();
>> 
>> This will make some regressions because HTMLFormControlElement::parseMappedAttribute() won't be called for disabled and readonly.
> 
> Removed this method, going away this issue.

Added "HTMLTextFormControlElement::parseMappedAttribute(attr)" here.

>>> Source/WebCore/html/TextFieldInputType.cpp:248
>>> +    m_innerSpinButton->stopRepeating();
>> 
>> m_innerSpinButton can be NULL.
> 
> Removed this method, going away this issue.

Added "if(m_innerSpinButton)" here.

[Kentaro Hara]

Comment on attachment 101293 [details]
Patch

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

>> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly-expected.txt:14
>> +PASS input.value is "1234566"
> 
> "TEST COMPLETE" followed by actual tests sounds very strange.
> You had better use jsTestisAsync and finishJSTest().  See fast/forms/autofocus-keygen.html.

Done.

>> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:25
>> +var test_delays;
> 
> We usually use camelCase for variables.

Sorry again... Done.

[Kentaro Hara]

Comment on attachment 101302 [details]
Patch

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

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:313
> +            releaseCapture();

releaseCapture() executes stopRepeatingTimer() even if m_capturing is false, but I think that it is acceptable.

[Kent Tamura]

Comment on attachment 101302 [details]
Patch

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

> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:60
> +        setTimeout(function() {
> +            shouldBeEqualToString('input.value', "1234566");
> +            nextDelayTest();
> +        }, 600);

Why do we wait for 600msec? 
I'm afraid this test will be a very slow test.

> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:65
> +    input.readOnly = "readonly";

The type of HTMLInputElement::readOnly is boolean, not string.

> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:72
> +    input.disabled = "disabled";

The type of HTMLInputElement::disabled is boolean, not string.

> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:81
> +    input.disabled = undefined;
> +    input.readOnly = undefined;

ditto.

> Source/WebCore/ChangeLog:25
> +        (WebCore::HTMLElement::releaseCapture): A virtual method.

"A virtual method." isn't helpful.
We should write what is changed, reasons of the change, or a purpose of the function.

> Source/WebCore/ChangeLog:31
> +        (WebCore::InputType::disabledAttributeChanged): A virtual method.
> +        (WebCore::InputType::readonlyAttributeChanged): A virtual method.

ditto.

> Source/WebCore/html/HTMLElement.h:87
> +    virtual void releaseCapture();
> +

I don't think we need to add this function to HTMLElement.
TextFieldInputType can know m_innerSpinButton type is SpinButtonElement.

[Kentaro Hara]

Created attachment 101426 [details]
Patch

[Kentaro Hara]

Comment on attachment 101302 [details]
Patch

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

>> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:60
>> +        }, 600);
> 
> Why do we wait for 600msec? 
> I'm afraid this test will be a very slow test.

The time from when the spin button is clicked till when the repeating starts is defined as initialAutoscrollTimerDelay(), and the velocity of the repeating is defined as autoscrollTimerDelay(). These values differ depending on platforms, and in Linux Chromium, initialAutoscrollTimerDelay is 250ms and autoscrollTimerDelay is 50ms. So, at least, 250ms + 50ms = 300ms is needed to fire the first repeating. I changed the 600ms to 500ms.

>> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:65
>> +    input.readOnly = "readonly";
> 
> The type of HTMLInputElement::readOnly is boolean, not string.

Done.

>> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:72
>> +    input.disabled = "disabled";
> 
> The type of HTMLInputElement::disabled is boolean, not string.

Done.

>> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:81
>> +    input.readOnly = undefined;
> 
> ditto.

Done.

>> Source/WebCore/ChangeLog:25
>> +        (WebCore::HTMLElement::releaseCapture): A virtual method.
> 
> "A virtual method." isn't helpful.
> We should write what is changed, reasons of the change, or a purpose of the function.

Done.

>> Source/WebCore/ChangeLog:31
>> +        (WebCore::InputType::readonlyAttributeChanged): A virtual method.
> 
> ditto.

Done.

>> Source/WebCore/html/HTMLElement.h:87
>> +
> 
> I don't think we need to add this function to HTMLElement.
> TextFieldInputType can know m_innerSpinButton type is SpinButtonElement.

Removed it.

[Kent Tamura]

Comment on attachment 101426 [details]
Patch

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

> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:60
> +        setTimeout(function() {
> +            shouldBeEqualToString('input.value', "1234566");
> +            nextDelayTest();
> +        }, 500);

We can know the first increment/decrement by 'input' event. Can we avoid setTime() by 'input' event handler?

> Source/WebCore/html/TextFieldInputType.cpp:244
> +    if (m_innerSpinButton)
> +        static_cast<SpinButtonElement*>(innerSpinButtonElement())->releaseCapture();

We can avoid static_cast by changing the type of m_innerSpinButton from RefPtr<HTMLElement> to RefPtr<SpinButtonElement>.

> Source/WebCore/html/TextFieldInputType.cpp:250
> +        static_cast<SpinButtonElement*>(innerSpinButtonElement())->releaseCapture();

ditto.

[Kentaro Hara]

> > LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:60
> > +        setTimeout(function() {
> > +            shouldBeEqualToString('input.value', "1234566");
> > +            nextDelayTest();
> > +        }, 500);
> 
> We can know the first increment/decrement by 'input' event. Can we avoid setTime() by 'input' event handler?

I guess that what we need to check is whether the second increment/decrement happens or not. How can we know it by watching the 'input' event?

We need to distinguish the following two cases:

Actual behavior (without this patch):
(1) 'mousedown' -> 'mouseup' -> 'input'  : the first 'input' event
(2) the repeating timer is fired  : the second 'input' event

Expected behavior:
(1) 'mousedown' -> 'mouseup' -> 'input'  : the first 'input' event
(2) nothing happens.

[Kent Tamura]

(In reply to comment #15)
> I guess that what we need to check is whether the second increment/decrement happens or not. How can we know it by watching the 'input' event?
> 
> We need to distinguish the following two cases:
> 
> Actual behavior (without this patch):
> (1) 'mousedown' -> 'mouseup' -> 'input'  : the first 'input' event
> (2) the repeating timer is fired  : the second 'input' event
> 
> Expected behavior:
> (1) 'mousedown' -> 'mouseup' -> 'input'  : the first 'input' event
> (2) nothing happens.

Ok, let's use setTimeout(..., 51 or more) in the 'input' handler.
Smaller timeout is safer than 600ms/500ms timeout.

[Kentaro Hara]

> > I guess that what we need to check is whether the second increment/decrement happens or not. How can we know it by watching the 'input' event?
> > 
> > We need to distinguish the following two cases:
> > 
> > Actual behavior (without this patch):
> > (1) 'mousedown' -> 'mouseup' -> 'input'  : the first 'input' event
> > (2) the repeating timer is fired  : the second 'input' event
> > 
> > Expected behavior:
> > (1) 'mousedown' -> 'mouseup' -> 'input'  : the first 'input' event
> > (2) nothing happens.
> 
> Ok, let's use setTimeout(..., 51 or more) in the 'input' handler.
> Smaller timeout is safer than 600ms/500ms timeout.

Sorry but I am confused... Why can we shorten the time by using setTimeout() in the 'input' event?

(1) the 'mousedown' event is triggered. 
(2) the 'mouseup' event is triggered. 
(3) the 'input' event is triggered. 
(4) the repeating timer is fired first. 
(5) the repeating timer is fired second. 

In my understanding, initialAutoscrollTimerDelay (250ms) is the time between (2) and (4), and autoscrollTimerDelay (50ms) is the time between (4) and (5). And the time between (2) and (3) is small enough. In this way, even if we use setTimeout() in the 'input' event, I think that we cannot shorten the waiting time less than 250ms. 

In case where the delay is long, we may be able to shorten the time by using setTimeout() inside the setTimeout() in the 'mouseup' event, but I am afraid that it degrades readability.

[Kent Tamura]

(In reply to comment #17)
> Sorry but I am confused... Why can we shorten the time by using setTimeout() in the 'input' event?

I was confused with your test scenario and the original scenario in crbug.com/74960. In the original scenario, the input was disabled in 'input' handler.  I'm sorry for the confusion.

I thought we could use a shorter timeout by disabling the input in the second 'input' event like:

1. mousedown
   1.1. 'input' is dispatched
   1.2. start the repeating timer
2. wait for the next 'input' event (actually 250msec)
3. repeatingTimerFired()
   3.1 the second 'input' is fired
   3.2 Disable the input in the 'input' handler
4. mouseup
5. re-enable the input
6. wait for 50msec
7. repeatingTimerFired() should not be called.  The value should not be changed since the last 'input' event.

These steps look complicated, and not so faster.

Ok, let's keep your test code as is.  The test might have flaky timeouts in buildbots because of the slowness.  If it happened, I would remove the test.

[Kentaro Hara]

Created attachment 101442 [details]
Patch

[Kentaro Hara]

Comment on attachment 101426 [details]
Patch

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

>> Source/WebCore/html/TextFieldInputType.cpp:244
>> +        static_cast<SpinButtonElement*>(innerSpinButtonElement())->releaseCapture();
> 
> We can avoid static_cast by changing the type of m_innerSpinButton from RefPtr<HTMLElement> to RefPtr<SpinButtonElement>.

Done.

>> Source/WebCore/html/TextFieldInputType.cpp:250
>> +        static_cast<SpinButtonElement*>(innerSpinButtonElement())->releaseCapture();
> 
> ditto.

Done.

[Kentaro Hara]

> Ok, let's keep your test code as is.  The test might have flaky timeouts in buildbots because of the slowness.  If it happened, I would remove the test.

Thanks. 

> > Sorry but I am confused... Why can we shorten the time by using setTimeout() in the 'input' event?
> 
> I was confused with your test scenario and the original scenario in crbug.com/74960. In the original scenario, the input was disabled in 'input' handler.  I'm sorry for the confusion.

It is possible to write a test with the 'input' event, but I am using the 'mouseup' event instead of the 'input' event, because it would be a simpler way to test the change that we made in this patch. 

Please assume the following script:

<input type="number" id="input" oninput="inputEventDispatched()" />
<script>
var input = $("input");
var testDelays = [1, 10, 100];
...;
function nextDelayTest() {
    delay = testDelays.shift();
    initializeInputAttributes(input, 1234567);
    clickSpinDownButton(input);
    setTimeout(function() {
        shouldBeEqualToString('input.value', "1234566");
        nextDelayTest();
    }, 500);
}

function inputEventDispatched() {
    input.readOnly = true;
    setTimeout(function() {
        input.readOnly = false;
    }, delay);
}
</script>

For the previous WebKit code (without my patch), the above script will not work as we expect. This is because |input| is a global variable. In the previous WebKit code, the repeating timer is fired again and again, and the 'input' event is triggered whenever the repeating timer is fired and the number is changed. Thus, even if we move on to the next delay test, inputEventDispatched() of the previous delay test still continues to be invoked, which unintentionally changes |input.readOnly| for the current delay test. To prevent this, we need to prepare an input form and the variable to refer to the input form for each delay test and for each readonly/disabled test, which would make the script messy. (Or we need to add or remove event handlers for the 'input' event appropriately.)

On the other hand, in case of the 'mouseup' event, the above problem does not happen since the 'mouseup' event is not triggered when the repeating timer is fired.

[Kent Tamura]

Comment on attachment 101442 [details]
Patch

ok.

Thank you for fixing this!

[WebKit Review Bot]

Comment on attachment 101442 [details]
Patch

Clearing flags on attachment: 101442

Committed r91353: <http://trac.webkit.org/changeset/91353>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gabor Loki]

The test fails on Qt. See the followings:

Build info:
  http://build.webkit.org/builders/Qt%20Linux%20Release/builds/35575

Test difference:
  http://build.webkit.org/results/Qt%20Linux%20Release/r91353%20(35575)/fast/forms/spin-button-gets-disabled-or-readonly-pretty-diff.html

--- fast/forms/spin-button-gets-disabled-or-readonly-expected.txt 
+++ fast/forms/spin-button-gets-disabled-or-readonly-actual.txt 
@@ -8,19 +8,19 @@
 
 Test on a readonly number input form:
 delay = 1 ms
-PASS input.value is "1234566"
+FAIL input.value should be 1234566. Was 1234567.
 delay = 10 ms
-PASS input.value is "1234566"
+FAIL input.value should be 1234566. Was 1234567.
 delay = 100 ms
-PASS input.value is "1234566"
+FAIL input.value should be 1234566. Was 1234567.
 
 Test on a disabled number input form:
 delay = 1 ms
-PASS input.value is "1234566"
+FAIL input.value should be 1234566. Was 1234567.
 delay = 10 ms
-PASS input.value is "1234566"
+FAIL input.value should be 1234566. Was 1234567.
 delay = 100 ms
-PASS input.value is "1234566"
+FAIL input.value should be 1234566. Was 1234567.
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Kent Tamura]

(In reply to comment #25)
> The test fails on Qt. See the followings:

Qt seems to have no spinbutton implementation. (See LayoutTests/platform/qt/Skipped line 2007)
So, this failure is reasonable.

[Gabor Loki]

> Qt seems to have no spinbutton implementation. (See LayoutTests/platform/qt/Skipped line 2007)

Ohh, my bad, sorry.

[Jessie Berlin]

(In reply to comment #26)
> (In reply to comment #25)
> > The test fails on Qt. See the followings:
> 
> Qt seems to have no spinbutton implementation. (See LayoutTests/platform/qt/Skipped line 2007)
> So, this failure is reasonable.

In the future, could you make sure that any such tests are added to both the Qt and Windows skipped lists when the patch is originally landed?

I am just about to add it to the Windows skipped list, since Windows doesn't have a spinbutton implementation either.

Thanks!

Jessie