RESOLVED FIXED 71181
Layout Test fast/forms/input-step-as-double.html fails after running touch event tests
https://bugs.webkit.org/show_bug.cgi?id=71181
Summary Layout Test fast/forms/input-step-as-double.html fails after running touch ev...
Ryosuke Niwa
Reported 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
Attachments
Fixing the flakey ness in the test case (2.29 KB, patch)
2011-11-01 02:44 PDT, Sachin Puranik
no flags
Patch (4.19 KB, patch)
2011-11-03 23:58 PDT, Kent Tamura
no flags
Patch 2 (6.86 KB, patch)
2011-12-13 02:36 PST, Kent Tamura
rniwa: review+
Sachin Puranik
Comment 1 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.
Kent Tamura
Comment 2 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.
Ryosuke Niwa
Comment 3 2011-11-01 06:44:24 PDT
The problem might be that we're not forcing a layout before eventSender.mouseMoveTo after modifying innerHTML.
Csaba Osztrogonác
Comment 4 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
Kent Tamura
Comment 5 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.
Ryosuke Niwa
Comment 6 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.
Kent Tamura
Comment 7 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?
Ryosuke Niwa
Comment 8 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.
Kent Tamura
Comment 9 2011-11-03 23:58:08 PDT
Ryosuke Niwa
Comment 10 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 :.
Csaba Osztrogonác
Comment 11 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.
Ryosuke Niwa
Comment 12 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.
Kent Tamura
Comment 13 2011-12-13 02:36:49 PST
Created attachment 118983 [details] Patch 2 Added a test
Kent Tamura
Comment 14 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.
Ryosuke Niwa
Comment 15 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.
Kent Tamura
Comment 16 2011-12-14 23:01:06 PST
Note You need to log in before you can comment on or make changes to this bug.