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.
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
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.
Created attachment 55579 [details] Patch with style fixes
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) {
Created attachment 55607 [details] Only initialize the EventSender when loading the top frame
Created attachment 55642 [details] Add leapForward support to the DRT
Created attachment 55648 [details] Small EventSender cleanups
Created attachment 55653 [details] Add leapForward support to the GTK+ DRT
(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).
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.
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.
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.
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.
Created attachment 55933 [details] Send the proper mouse events during clicking
Created attachment 56076 [details] Send the proper mouse events during clicking (update) I've attached an updated version of the previous patch.
Comment on attachment 55648 [details] Small EventSender cleanups Great stuff.
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.
Attachment 55648 [details] was posted by a committer and has review+, assigning to Martin Robinson for commit.
Created attachment 57235 [details] Rudimentary leapForward support
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.
(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.
Created attachment 57317 [details] Rudimentary leapForward support (small fix)
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.
I've checked the second one, after checking with marting a couple of things they look good and both tests work.
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 on attachment 57317 [details] Rudimentary leapForward support (small fix) r=me
Committed r60478: <http://trac.webkit.org/changeset/60478>
Committed r60479: <http://trac.webkit.org/changeset/60479>
(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.
All patches landed as of r60482
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