WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(4.19 KB, patch)
2011-11-03 23:58 PDT
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Patch 2
(6.86 KB, patch)
2011-12-13 02:36 PST
,
Kent Tamura
rniwa
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 113626
[details]
Patch
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
Committed
r102896
: <
http://trac.webkit.org/changeset/102896
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug