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
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.
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.
The problem might be that we're not forcing a layout before eventSender.mouseMoveTo after modifying innerHTML.
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
(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.
(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.
(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?
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.
Created attachment 113626 [details] Patch
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 :.
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 on attachment 113626 [details] Patch I think this fix is fine although I still fishy about touch states.
Created attachment 118983 [details] Patch 2 Added a test
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 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.
Committed r102896: <http://trac.webkit.org/changeset/102896>