Layout Test fast/forms/input-step-as-double.html fails after running touch event tests

• Component: Forms
• Status: RESOLVED FIXED

[Ryosuke Niwa]

fast/forms/input-step-as-double.html is flaky on Chromium and Qt:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Finput-step-as-double.html
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fforms%2Finput-step-as-double.html

diff:
-PASS numberInput.value is "0.5"
+FAIL numberInput.value should be 0.5. Was -0.5.
 PASS successfullyParsed is true

[Sachin Puranik]

Created attachment 113151 [details]
Fixing the flakey ness in the test case

Fixed the flakeyness in the test case for the chrome/qt port. Even we click on upper or lower spin button , what we need to check is the absolute value in the text box.

[Kent Tamura]

The test sometimes passes on Chromium and sometimes fails.
Ideally we should find out the reason of the unstable result and should make the test stable.

[Ryosuke Niwa]

The problem might be that we're not forcing a layout before eventSender.mouseMoveTo after modifying innerHTML.

[Csaba Osztrogonác]

It seems that a fast/events test break it somehow. I can reproduce this fail on Qt easily: $ Tools/Scripts/old-run-webkit-tests fast/events/touch/touch-target.html fast/forms/input-step-as-double.html

[Kent Tamura]

(In reply to comment #4)
> It seems that a fast/events test break it somehow. I can reproduce this fail on Qt easily: $ Tools/Scripts/old-run-webkit-tests fast/events/touch/touch-target.html fast/forms/input-step-as-double.html

Thanks! It reproduced the problem on Chromium-mac too.

I had more investigation.

When a mouse pointer moves on a node, WebKit usually calls the following functions:
1. setHovered(true)
2. defaultEventHandler() for 'mousemove'

However, if touch-target.html is invoked before input-step-as-double.html, this order is reversed.
1. defaultEventhandler() for 'mousemove'
2. setHovered(true)

I don't know why this happens.  Anyway, SpinButtonElement::m_upDownState is reset to Indeterminate in setHovered(true).  So SpinButtonElement::defaultEventHandler() calls input-stepUpFromRenderer(-1).

See http://trac.webkit.org/changeset/63582 for the reason of resetting m_upDownState.

[Ryosuke Niwa]

(In reply to comment #5)
> When a mouse pointer moves on a node, WebKit usually calls the following functions:
> 1. setHovered(true)
> 2. defaultEventHandler() for 'mousemove'
> 
> However, if touch-target.html is invoked before input-step-as-double.html, this order is reversed.
> 1. defaultEventhandler() for 'mousemove'
> 2. setHovered(true)
> 
> I don't know why this happens.  Anyway, SpinButtonElement::m_upDownState is reset to Indeterminate in setHovered(true).  So SpinButtonElement::defaultEventHandler() calls input-stepUpFromRenderer(-1).

Can you see the difference in EventHandler::m_touchPressed ? Maybe we're not clearing some touch-related states properly.

[Kent Tamura]

(In reply to comment #6)
> (In reply to comment #5)
> > When a mouse pointer moves on a node, WebKit usually calls the following functions:
> > 1. setHovered(true)
> > 2. defaultEventHandler() for 'mousemove'
> > 
> > However, if touch-target.html is invoked before input-step-as-double.html, this order is reversed.
> > 1. defaultEventhandler() for 'mousemove'
> > 2. setHovered(true)
> > 
> > I don't know why this happens.  Anyway, SpinButtonElement::m_upDownState is reset to Indeterminate in setHovered(true).  So SpinButtonElement::defaultEventHandler() calls input-stepUpFromRenderer(-1).
> 
> Can you see the difference in EventHandler::m_touchPressed ? Maybe we're not clearing some touch-related states properly.

m_touchPressed is false in the former case, and m_touchPressed is true in the latter case.

It seems EventHandler::m_touchPressed is never cleared if the last call of EventHandler::handleTouchEvent() had one or more touches.

* Should we clear m_touchPressed?
* Shouldn't we change the setHovered()-mousemove order in the case of m_touchPressed?
* Should we support the reversed order in SpinButtonElement?

[Ryosuke Niwa]

ap, darin, smfr: do you know someone familiar with touch events? It appears that WebKit isn't clearing some touch-related states in EventHandler properly.

[Kent Tamura]

Created attachment 113626 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 113626 [details]
Patch

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

I still think we may have a bug in EventHandler not clearing m_touchPressed.

> Source/WebCore/ChangeLog:10
> +        before a mousemove event in the element. It is not true for touch
> +        events.

Could you fit events on the previous line to avoid awkward line break?

> Source/WebCore/ChangeLog:13
> +        setHovered(true), and should reset it when the mouse pointer moves
> +        out.

Ditto.

> Source/WebCore/ChangeLog:15
> +        This change fixes the flakiness of fast/forms/input-step-as-double.html.

Can we possibly add a test case for this?

> Source/WebCore/ChangeLog:19
> +        (WebCore::SpinButtonElement::defaultEventHandler):
> +        Add an assertion that m_upDownState should not be Indetermiante.

We normally start a sentence on the same line as a function name after :.

[Csaba Osztrogonác]

I skipped it on Qt until fix: http://trac.webkit.org/changeset/102104.
Please unskip it if you lands a proper fix for this bug.

[Ryosuke Niwa]

Comment on attachment 113626 [details]
Patch

I think this fix is fine although I still fishy about touch states.

[Kent Tamura]

Created attachment 118983 [details]
Patch 2

Added a test

[Kent Tamura]

Comment on attachment 113626 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        events.
> 
> Could you fit events on the previous line to avoid awkward line break?

I think we have no consensus about text wrapping in ChangeLog.

>> Source/WebCore/ChangeLog:15
>> +        This change fixes the flakiness of fast/forms/input-step-as-double.html.
> 
> Can we possibly add a test case for this?

I added a test in the updated patch.

[Ryosuke Niwa]

Comment on attachment 118983 [details]
Patch 2

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

> Source/WebCore/ChangeLog:13
> +        before a mousemove event in the element. It is not true for touch
> +        events.
> +        We should not reset m_upDownState to Indetermiante in
> +        setHovered(true), and should reset it when the mouse pointer moves
> +        out.

It's really unpleasant to see "events." and "out." wrapped awkwardly like this. Please fix it before landing it.
While we don't mandate the number of colums to be included in each line, I think readability and cleanness of code, comment, and changelog are very important.

[Kent Tamura]

Committed r102896: <http://trac.webkit.org/changeset/102896>