Bug 38853 - [GTK] Double clicks cause three button press events
Summary: [GTK] Double clicks cause three button press events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on:
Blocks: 39040 39844
  Show dependency treegraph
 
Reported: 2010-05-10 09:40 PDT by Martin Robinson
Modified: 2010-06-01 10:54 PDT (History)
5 users (show)

See Also:


Attachments
Add leapForward support to the GTK+ DRT (20.62 KB, patch)
2010-05-10 11:17 PDT, Martin Robinson
mrobinson: commit-queue-
Details | Formatted Diff | Diff
Patch with style fixes (20.62 KB, patch)
2010-05-10 11:31 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Only initialize the EventSender when loading the top frame (5.27 KB, patch)
2010-05-10 15:10 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Add leapForward support to the DRT (16.97 KB, patch)
2010-05-10 19:09 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Small EventSender cleanups (7.08 KB, patch)
2010-05-10 19:32 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Add leapForward support to the GTK+ DRT (20.62 KB, patch)
2010-05-10 20:50 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Take into account the timeOffset when synthesizing events in the DRT (2.97 KB, patch)
2010-05-12 17:53 PDT, Martin Robinson
mrobinson: commit-queue-
Details | Formatted Diff | Diff
Take into account the timeOffset when synthesizing events in the DRT (3.60 KB, patch)
2010-05-12 17:59 PDT, Martin Robinson
mrobinson: commit-queue-
Details | Formatted Diff | Diff
Take into account the timeOffset when synthesizing events in the DRT (3.90 KB, patch)
2010-05-12 18:07 PDT, Martin Robinson
mrobinson: commit-queue-
Details | Formatted Diff | Diff
Send the proper mouse events during clicking (3.90 KB, patch)
2010-05-12 18:22 PDT, Martin Robinson
mrobinson: commit-queue-
Details | Formatted Diff | Diff
Send the proper mouse events during clicking (17.14 KB, patch)
2010-05-12 20:35 PDT, Martin Robinson
mrobinson: commit-queue-
Details | Formatted Diff | Diff
Send the proper mouse events during clicking (update) (16.15 KB, patch)
2010-05-14 09:07 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Combine two related patches (19.58 KB, patch)
2010-05-14 11:57 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Rudimentary leapForward support (18.11 KB, patch)
2010-05-27 06:32 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Rudimentary leapForward support (small fix) (18.00 KB, patch)
2010-05-28 04:50 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2010-05-10 09:40:01 PDT
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.
Comment 1 Martin Robinson 2010-05-10 11:17:14 PDT
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
Comment 2 WebKit Review Bot 2010-05-10 11:22:20 PDT
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.
Comment 3 Martin Robinson 2010-05-10 11:31:43 PDT
Created attachment 55579 [details]
Patch with style fixes
Comment 4 Alejandro G. Castro 2010-05-10 12:46:20 PDT
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) {
Comment 5 Martin Robinson 2010-05-10 15:10:35 PDT
Created attachment 55607 [details]
Only initialize the EventSender when loading the top frame
Comment 6 Martin Robinson 2010-05-10 19:09:42 PDT
Created attachment 55642 [details]
Add leapForward support to the DRT
Comment 7 Martin Robinson 2010-05-10 19:32:11 PDT
Created attachment 55648 [details]
Small EventSender cleanups
Comment 8 Martin Robinson 2010-05-10 20:50:52 PDT
Created attachment 55653 [details]
Add leapForward support to the GTK+ DRT
Comment 9 Martin Robinson 2010-05-10 20:52:37 PDT
(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).
Comment 10 Martin Robinson 2010-05-12 17:53:55 PDT
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.
Comment 11 Martin Robinson 2010-05-12 17:59:27 PDT
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.
Comment 12 Martin Robinson 2010-05-12 18:07:31 PDT
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.
Comment 13 Martin Robinson 2010-05-12 18:22:19 PDT
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.
Comment 14 Martin Robinson 2010-05-12 20:35:17 PDT
Created attachment 55933 [details]
Send the proper mouse events during clicking
Comment 15 Martin Robinson 2010-05-14 09:07:51 PDT
Created attachment 56076 [details]
Send the proper mouse events during clicking (update)

I've attached an updated version of the previous patch.
Comment 16 Xan Lopez 2010-05-14 09:47:49 PDT
Comment on attachment 55648 [details]
Small EventSender cleanups

Great stuff.
Comment 17 Martin Robinson 2010-05-14 11:57:38 PDT
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.
Comment 18 Adam Barth 2010-05-14 23:31:16 PDT
Attachment 55648 [details] was posted by a committer and has review+, assigning to Martin Robinson for commit.
Comment 19 Martin Robinson 2010-05-27 06:32:51 PDT
Created attachment 57235 [details]
Rudimentary leapForward support
Comment 20 Martin Robinson 2010-05-27 06:37:03 PDT
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.
Comment 21 Alejandro G. Castro 2010-05-27 10:57:32 PDT
(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.
Comment 22 Martin Robinson 2010-05-28 04:50:03 PDT
Created attachment 57317 [details]
Rudimentary leapForward support (small fix)
Comment 23 Martin Robinson 2010-05-28 04:51:25 PDT
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.
Comment 24 Alejandro G. Castro 2010-06-01 04:02:51 PDT
I've checked the second one, after checking with marting a couple of things they look good and both tests work.
Comment 25 Xan Lopez 2010-06-01 04:43:32 PDT
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.
Comment 26 Xan Lopez 2010-06-01 04:54:57 PDT
Comment on attachment 57317 [details]
Rudimentary leapForward support (small fix)

r=me
Comment 27 Martin Robinson 2010-06-01 07:59:02 PDT
Committed r60478: <http://trac.webkit.org/changeset/60478>
Comment 28 Martin Robinson 2010-06-01 08:47:41 PDT
Committed r60479: <http://trac.webkit.org/changeset/60479>
Comment 29 Martin Robinson 2010-06-01 10:18:13 PDT
(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.
Comment 30 Martin Robinson 2010-06-01 10:30:52 PDT
All patches landed as of r60482
Comment 31 WebKit Review Bot 2010-06-01 10:54:19 PDT
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