Bug 71181

Summary: Layout Test fast/forms/input-step-as-double.html fails after running touch event tests
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, benm, darin, ossy, simon.fraser, steveblock, tkent, tonikitoo, webkit-sed
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fixing the flakey ness in the test case
none
Patch
none
Patch 2 rniwa: review+

Description Ryosuke Niwa 2011-10-29 14:29:45 PDT
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
Comment 1 Sachin Puranik 2011-11-01 02:44:57 PDT
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.
Comment 2 Kent Tamura 2011-11-01 06:20:05 PDT
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.
Comment 3 Ryosuke Niwa 2011-11-01 06:44:24 PDT
The problem might be that we're not forcing a layout before eventSender.mouseMoveTo after modifying innerHTML.
Comment 4 Csaba Osztrogon√°c 2011-11-02 10:26:38 PDT
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
Comment 5 Kent Tamura 2011-11-03 22:50:59 PDT
(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.
Comment 6 Ryosuke Niwa 2011-11-03 23:07:27 PDT
(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.
Comment 7 Kent Tamura 2011-11-03 23:35:24 PDT
(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?
Comment 8 Ryosuke Niwa 2011-11-03 23:40:49 PDT
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.
Comment 9 Kent Tamura 2011-11-03 23:58:08 PDT
Created attachment 113626 [details]
Patch
Comment 10 Ryosuke Niwa 2011-11-04 00:08:20 PDT
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 :.
Comment 11 Csaba Osztrogon√°c 2011-12-06 00:06:54 PST
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.
Comment 12 Ryosuke Niwa 2011-12-08 16:48:35 PST
Comment on attachment 113626 [details]
Patch

I think this fix is fine although I still fishy about touch states.
Comment 13 Kent Tamura 2011-12-13 02:36:49 PST
Created attachment 118983 [details]
Patch 2

Added a test
Comment 14 Kent Tamura 2011-12-13 02:38:57 PST
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.
Comment 15 Ryosuke Niwa 2011-12-13 17:24:22 PST
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.
Comment 16 Kent Tamura 2011-12-14 23:01:06 PST
Committed r102896: <http://trac.webkit.org/changeset/102896>