Bug 38853 – [GTK] Double clicks cause three button press events

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

I believe that triple clicks also cause 5. This, and the lack of leapForward support in the DRT, cause us to fail fast/events/click-count.html.

[Martin Robinson]

Created attachment 55577 [details]
Add leapForward support to the GTK+ DRT

Attached a patch to add leapForward support to the DRT. The behavior of leapForward is described here: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

[WebKit Review Bot]

Attachment 55577 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/gtk/EventSender.cpp:130:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Created attachment 55579 [details]
Patch with style fixes

[Alejandro G. Castro]

Could the patch be divided like:
  - Refactoring, these changes look nice: prepareMouseButtonEvent, sendOrQueueEvent, 
  - Fix click problems
  - leapForward support, why is this required? Could you just leave the main loop iterate as usual? 

+            while (!doneLeapingForward)
+                gtk_main_iteration();

Does this change in the condition inside makeEventSender solves the clicks problem or a problem with the replaying queue?

-    if (!replayingSavedEvents) {
-        // This function can be called in the middle of a test, even
-        // while replaying saved events. Resetting these while doing that
-        // can break things.
+    if (isTopFrame) {

[Martin Robinson]

Created attachment 55607 [details]
Only initialize the EventSender when loading the top frame

[Martin Robinson]

Created attachment 55642 [details]
Add leapForward support to the DRT

[Martin Robinson]

Created attachment 55648 [details]
Small EventSender cleanups

[Martin Robinson]

Created attachment 55653 [details]
Add leapForward support to the GTK+ DRT

[Martin Robinson]

(In reply to comment #4)
> Could the patch be divided like:
>   - Refactoring, these changes look nice: prepareMouseButtonEvent, sendOrQueueEvent, 
>   - Fix click problems

Thanks for the review!

I've split the patch into two:
1. One to add rudimentary leapForward support which includes the initialization fix.
2. Another which includes the style fixes and add prepareMouseButtonEvent.

>   - leapForward support, why is this required? Could you just leave the main loop iterate as usual? 
> 
> +            while (!doneLeapingForward)
> +                gtk_main_iteration();
> 

I think you're right here. I've reworked the logic a bit. It's simpler now, because we don't need a lot of the drag-and-drop stuff yet (perhaps until we add dropping support to the DRT and perhaps never).

[Martin Robinson]

Created attachment 55917 [details]
Take into account the timeOffset when synthesizing events in the DRT

Added an additional change which adjust event time relative to the timeOffset controlled via leapForward. This will ensure that clicks going into WebKit will not be mistaken for double, quadruple, quintuple, sextuple, etc clicks.

[Martin Robinson]

Created attachment 55918 [details]
Take into account the timeOffset when synthesizing events in the DRT

Added a patch with a small addition to the previous one.

[Martin Robinson]

Created attachment 55919 [details]
Take into account the timeOffset when synthesizing events in the DRT

My sincere apologies for the patch spam. This should be correct version. It's been a long day.

[Martin Robinson]

Created attachment 55922 [details]
Send the proper mouse events during clicking

I've added a patch that fixes the test. Click counting is now very similar to that of Win32, which also does not support native click counts above 2.

[Martin Robinson]

Created attachment 55933 [details]
Send the proper mouse events during clicking

[Martin Robinson]

Created attachment 56076 [details]
Send the proper mouse events during clicking (update)

I've attached an updated version of the previous patch.

[Xan Lopez]

Comment on attachment 55648 [details]
Small EventSender cleanups

Great stuff.

[Martin Robinson]

Created attachment 56092 [details]
Combine two related patches

I've combined the leapForward support patch and the timeOffset update. These changes
are very closely related and should probably be in the same patch.

[Adam Barth]

Attachment 55648 [details] was posted by a committer and has review+, assigning to Martin Robinson for commit.

[Martin Robinson]

Created attachment 57235 [details]
Rudimentary leapForward support

[Martin Robinson]

Comment on attachment 56092 [details]
Combine two related patches

Xan and Alex expressed some concern that sending events with the time component as GDK_CURRENT_TIME + timeOffset may have unintended consequences. I've attached a new patch which confines it to only mouseUp and mouseDown event simulation. These events are sent directly to the widget and should not affect GDK or GTK+ internal state.

[Alejandro G. Castro]

(In reply to comment #19)
> Created an attachment (id=57235) [details]
> Rudimentary leapForward support

Patch looks good to me, it has the leapForward feature and a lot of really nice refactoring that makes sense to me (refactor events sending in functions, remove variables, fix makeSenderEvent function). Nitpicking, there is a blank line added in the first line of the method mouseDownCallback.

[Martin Robinson]

Created attachment 57317 [details]
Rudimentary leapForward support (small fix)

[Martin Robinson]

Comment on attachment 57235 [details]
Rudimentary leapForward support

I've attached a new patch which doesn't remove the missing line and alos fixes the issue where the timeOffset was used during mouse motion events, but not mouse down events.

[Alejandro G. Castro]

I've checked the second one, after checking with marting a couple of things they look good and both tests work.

[Xan Lopez]

Comment on attachment 56076 [details]
Send the proper mouse events during clicking (update)

>+    static gint doubleClickDistance = 0;
>+    static gint doubleClickTime = 0;
>+    if (!doubleClickDistance) {
>+        GtkSettings* settings = gtk_settings_get_for_screen(gdk_drawable_get_screen(widget->window));
>+        g_object_get(settings, "gtk-double-click-distance", &doubleClickDistance, NULL);
>+        g_object_get(settings, "gtk-double-click-time", &doubleClickTime, NULL);
>+    }

You should connect to the notify signal in case the values are updated. You can do this in a follow-up.

>+
>+    PlatformMouseEvent platformEvent(event);
>+
>+    if ((event->type == GDK_2BUTTON_PRESS || event->type == GDK_3BUTTON_PRESS)
>+        || ((abs(event->x - previousPoint.x()) < doubleClickDistance)
>+            && (abs(event->y - previousPoint.y()) < doubleClickDistance)
>+            && (event->time - previousTime < static_cast<guint>(doubleClickTime))
>+            && (event->button == previousButton)))
>+        currentClickCount++;
>+    else
>+        currentClickCount = 1;

We should have a comment explaining why do we have to do the manual check for increased click count (since GTK+ only checks up to triple click).


LGTM otherwise.

[Xan Lopez]

Comment on attachment 57317 [details]
Rudimentary leapForward support (small fix)

r=me

[Martin Robinson]

Committed r60478: <http://trac.webkit.org/changeset/60478>

[Martin Robinson]

Committed r60479: <http://trac.webkit.org/changeset/60479>

[Martin Robinson]

(In reply to comment #25)

Thanks for the review!

> You should connect to the notify signal in case the values are updated. You can do this in a follow-up.

Xan and I decided that it's better in this case to simply not cache these values. If we run into performance disadvantages later, we can fix them then.

[Martin Robinson]

All patches landed as of r60482

[WebKit Review Bot]

http://trac.webkit.org/changeset/60485 might have broken Chromium Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/60483
http://trac.webkit.org/changeset/60484
http://trac.webkit.org/changeset/60485
http://trac.webkit.org/changeset/60486