WebKitWebViewBase has a ClickCounter helper class to handle single-clicks, double-clicks, triple-clicks and so on: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/gtk/GtkClickCounter.h WebKitTestRunner uses EventSenderProxy to create and dispatch GdkEvents with the mouse event. In TestController::resetStateToConsistentValues(), the EventSenderProxy is destroyed after every test run, which should clear existing click counts, previous click locations and other mouse event related state. However, since the internal ClickCounter is not cleared, and as it is used in webkitWebViewBaseButtonPressEvent to create NativeWebMouseEvent from the GdkEvents received, these new NativeWebMouseEvents carry the existing click count. The tests are simply flaky: when they are run in their own WebKitTestRunner, they will pass. This shouldn't affect the regular browsing behaviour, as the ClickCounter uses the event time and click location to tell whether a click is a different event or part of the previous one. However, I still think it should be fixed, as it can be misleading when analysing test results. I will now upload a patch that resets the click counter in the resetStateToConsistentValues, and also in the WEBKIT_LOAD_STARTED state. Let me know what you think. You can try this yourself by running Tools/Scripts/run-webkit-tests --repeat-each=2 --gtk -2 editing/selection/anchor-focus1.html \ editing/selection/anchor-focus2.html \ editing/selection/anchor-focus3.html \ editing/selection/block-with-positioned-lastchild.html \ editing/selection/caret-after-keypress.html \ editing/selection/crash-on-shift-click.html \ editing/selection/doubleclick-beside-cr-span.html \ editing/selection/doubleclick-crash.html \ editing/selection/doubleclick-inline-first-last-contenteditable.html \ editing/selection/doubleclick-whitespace.html \ editing/selection/drag-select-1.html \ editing/selection/drag-select-rapidly.html \ editing/selection/drag-start-event-client-x-y.html \ editing/selection/editable-links.html \ editing/selection/expanding-selections.html \ editing/selection/expanding-selections2.html \ editing/selection/fake-doubleclick.html \ editing/selection/fake-drag.html \ editing/selection/focus-and-display-none.html \ editing/selection/focus-crash.html \ editing/selection/hit-test-anonymous.html \ editing/selection/hit-test-on-text-with-line-height.html \ editing/selection/inline-closest-leaf-child.html \ editing/selection/last-empty-inline.html \ editing/selection/mixed-editability-1.html \ editing/selection/move-begin-end.html \ editing/selection/paragraph-granularity.html \ editing/selection/rtl-move-selection-right-left.html \ editing/selection/select-across-readonly-input-1.html \ editing/selection/select-across-readonly-input-2.html \ editing/selection/select-across-readonly-input-3.html \ editing/selection/select-across-readonly-input-4.html \ editing/selection/select-across-readonly-input-5.html \ editing/selection/select-all-iframe.html \ editing/selection/select-bidi-run.html \ editing/selection/select-from-textfield-outwards.html \ editing/selection/select-line-break-with-opposite-directionality.html \ editing/selection/select-out-of-editable.html \ editing/selection/select-out-of-floated-contenteditable.html \ editing/selection/select-out-of-floated-input.html \ editing/selection/select-out-of-floated-textarea.html \ editing/selection/selection-actions.html I took this list from http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20WK2%20%28Tests%29/builds/7907/steps/layout-test/logs/stdio. I removed editing/selection/empty-cell-right-click.html, which seems to cause a different issue and that I am investigating on its own.
Created attachment 209863 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 209863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209863&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1472 > + // Reset the click counter > + webkitWebViewBaseResetClickCounter(WEBKIT_WEB_VIEW_BASE(webView)); This is probably not the right time to do this. Just because a new load started doesn't mean the old page isn't active. Maybe a better time would be after the document changes. It might be hard to track this in the UI process, so perhaps it's better to wait for the first repaint that includes the new page. This should probably also be in WebKitWebViewBase.
(In reply to comment #3) > (From update of attachment 209863 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209863&action=review > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1472 > > + // Reset the click counter > > + webkitWebViewBaseResetClickCounter(WEBKIT_WEB_VIEW_BASE(webView)); > > This is probably not the right time to do this. Just because a new load started doesn't mean the old page isn't active. Maybe a better time would be after the document changes. It might be hard to track this in the UI process, so perhaps it's better to wait for the first repaint that includes the new page. This should probably also be in WebKitWebViewBase. As I explain in comment #0, that is probably the less important change: in the regular browsing, the ClickCounter is really unlikely to cause a wrong behaviour. And when WebKitTestRunner is used, webkitWebViewLoadChanged is never reached. So I wonder what's your opinion on the other changes in the patch: do you think that could be a good approach? (Of course, I will address your comment as well)
Created attachment 211757 [details] Patch Removed the call to reset ClickCounter from the WebKitWebView
*** Bug 121952 has been marked as a duplicate of this bug. ***
OK: I've talked to Martin, and he agrees to have this split into two issues. We would file a new bug for the issue in the general case, and land this fix for the layout tests.
Comment on attachment 211757 [details] Patch Looks good to me, but I think a WK2 owner needs to verify the changes to platform independent code.
(In reply to comment #8) > (From update of attachment 211757 [details]) > Looks good to me, but I think a WK2 owner needs to verify the changes to platform independent code. Sam, Alexey, could any of you take a look at the changes in Tools/WebKitTestRunner/PlatformWebView.h and Tools/WebKitTestRunner/TestController.cpp? Thanks!
Could you please clarify why this is a responsibility of WKTR? On the face of it, it seems that the counter should be reset during regular page navigations, so the fix would be in WebKit2/gtk.
(In reply to comment #10) > Could you please clarify why this is a responsibility of WKTR? On the face of it, it seems that the counter should be reset during regular page navigations, so the fix would be in WebKit2/gtk. You are right: the proper fix would be in WebKit2/GTK (we filed bug #122551 about it), but I thought of this as a partial fix that would get the layout tests working properly again. Fixing it in WebKit2/GTK was the first approach I tried -unsuccessfully-, but I am not sure where exactly it makes sense to proceed and reset it (see also Martin's comment #3). I don't mind trying again, but would appreciate some suggestions about where/when to handle this and reset the counter.
Comment on attachment 211757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211757&action=review I'm OK with landing this change. > Tools/WebKitTestRunner/TestController.cpp:635 > +#if PLATFORM(GTK) > + m_mainWebView->resetStateToConsistentValues(); > +#endif Perhaps this should be named less generally if we don't expect more code in the reset function - and we don't. I'd just call this resetGtkClickCounter(), and add a FIXME pointing to bug 122551.
We need to get the tests running, so let's just land this and figure out a better way to fix it later.
Comment on attachment 211757 [details] Patch This also needs to unskip the tests.
Why is this needed, given that bug 122551 is already fixed? That bug was supposed to fix this for real, as opposed to this workaround.
I will try to talk to Quique, who fixed bug #122551, and see if this patch is still needed.
I have just talked to Quique and he showed me https://bugs.webkit.org/show_bug.cgi?id=122551#c7. According to that comment, his patch in bug #122551 addressed this problem in the proper way, so the patch here shouldn't be needed any more. I will still try to see if the patch here makes any difference: if it doesn't, we can clear the flags and close it.
It look like the patch for 122551 did not actually unskip the tests. *** This bug has been marked as a duplicate of bug 122551 ***
Comment on attachment 211757 [details] Patch Clearing the flags.