Bug 31841 - Linux Chromium sends too many click events
Summary: Linux Chromium sends too many click events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-24 11:10 PST by Evan Stade
Modified: 2009-11-24 15:46 PST (History)
2 users (show)

See Also:


Attachments
fix (2.86 KB, patch)
2009-11-24 11:15 PST, Evan Stade
no flags Details | Formatted Diff | Diff
style fixes (2.82 KB, patch)
2009-11-24 11:31 PST, Evan Stade
levin: review-
Details | Formatted Diff | Diff
more style (2.81 KB, patch)
2009-11-24 15:22 PST, Evan Stade
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Stade 2009-11-24 11:10:27 PST
for example, double clicking on the forward button on google calendar will send the calendar forward 3 months instead of 2.
Comment 1 Evan Stade 2009-11-24 11:15:01 PST
Created attachment 43790 [details]
fix
Comment 2 Evan Martin 2009-11-24 11:19:45 PST
Comment on attachment 43790 [details]
fix

> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog	(revision 51343)
> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> +2009-11-24  Evan Stade  <estade@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Linux Chromium sends too many click events
> +        https://bugs.webkit.org/show_bug.cgi?id=31841
> +
> +        Manually count number of clicks for double/triple click events. This
> +        makes us match firefox on http://www.quirksmode.org/js/events_mouse.html

Cap Firefox?

> +        Chromium side of this patch is here:
> +        http://codereview.chromium.org/431031/show
> +
> +        * src/gtk/WebInputEventFactory.cpp:
> +        (WebKit::WebInputEventFactory::mouseEvent):
> +
>  2009-11-24  Adam Barth  <abarth@webkit.org>
>  
>          Reviewed by Dimitri Glazkov.
> Index: WebKit/chromium/src/gtk/WebInputEventFactory.cpp
> ===================================================================
> --- WebKit/chromium/src/gtk/WebInputEventFactory.cpp	(revision 51343)
> +++ WebKit/chromium/src/gtk/WebInputEventFactory.cpp	(working copy)
> @@ -38,10 +38,22 @@
>  
>  #include <gdk/gdk.h>
>  #include <gdk/gdkkeysyms.h>
> +#include <gtk/gtk.h>
>  #include <gtk/gtkversion.h>
>  
>  #include <wtf/Assertions.h>
>  
> +namespace {
> +
> +gint GetDoubleClickTime() {
> +  static GtkSettings* settings = gtk_settings_get_default();
> +  gint double_click_time = 250;
> +  g_object_get(G_OBJECT(settings), "gtk-double-click-time", &double_click_time, NULL);
> +  return double_click_time;
> +}

Style: getDoubleClickTime, four spaces, variable naming.
Why static gobject?  Maybe the double click time should be static?
Does this work in a sandboxed renderer?  I didn't think you could get the real gtk settings.

> +
> +}  // namespace
> +
>  namespace WebKit {
>  
>  static double gdkEventTimeToWebEventTime(guint32 time)
> @@ -321,24 +333,36 @@ WebMouseEvent WebInputEventFactory::mous
>      result.clickCount = 0;
>  
>      switch (event->type) {
> -    case GDK_3BUTTON_PRESS:
> -        ++result.clickCount;
> -        // fallthrough
> -    case GDK_2BUTTON_PRESS:
> -        ++result.clickCount;
> -        // fallthrough
>      case GDK_BUTTON_PRESS:
>          result.type = WebInputEvent::MouseDown;
> -        ++result.clickCount;
>          break;
>      case GDK_BUTTON_RELEASE:
>          result.type = WebInputEvent::MouseUp;
>          break;
> -
> +    case GDK_3BUTTON_PRESS:
> +        // fallthrough

I think the comment isn't necessary when there's no code in the case.

> +    case GDK_2BUTTON_PRESS:
> +        // fallthrough
>      default:
>          ASSERT_NOT_REACHED();
>      };
>  
> +    if (GDK_BUTTON_PRESS == event->type) {
> +        static int numClicks = 0;
> +        static GdkWindow* eventWindow = NULL;
> +        static gint lastLeftClickTime = 0;
> +
> +        gint time_diff = event->time - lastLeftClickTime;
> +        if (eventWindow == event->window && time_diff < GetDoubleClickTime())
> +            numClicks++;
> +        else
> +            numClicks = 1;
> +
> +        result.clickCount = numClicks;
> +        eventWindow = event->window;
> +        lastLeftClickTime = event->time;
> +    }
> +
>      result.button = WebMouseEvent::ButtonNone;
>      if (event->button == 1)
>          result.button = WebMouseEvent::ButtonLeft;
Comment 3 Evan Stade 2009-11-24 11:31:52 PST
Created attachment 43792 [details]
style fixes

style fixed

1. The getDoubleClickTime function is copied from chrome code. The first variable is static to avoid unnecessary get_settings calls, although I admit this optimization might not buy us much. The result is not static because it could potentially change.
2. This code does not execute in the renderer, it executes in the browser process only.
Comment 4 Evan Martin 2009-11-24 11:55:01 PST
This seems good to me, though I am not a webkit reviewer.
The statics make me a bit sad, but not so much.
Comment 5 David Levin 2009-11-24 15:12:24 PST
Comment on attachment 43792 [details]
style fixes

r- for no layout test.
You can send clicks from within layout tests, so it seems like it would be possible to add a layout test. As an absolutely last resort, there is always WebKit/WebCore/manual-tests


> Index: WebKit/chromium/src/gtk/WebInputEventFactory.cpp
> +gint getDoubleClickTime() {

The brace needs to go on the next line.

> +    g_object_get(G_OBJECT(settings), "gtk-double-click-time", &doubleClickTime, NULL);

Use 0 instead of NULL.

> +    if (GDK_BUTTON_PRESS == event->type) {
> +        static int numClicks = 0;
> +        static GdkWindow* eventWindow = NULL;

Use 0 instead of NULL.
Comment 6 Evan Stade 2009-11-24 15:22:22 PST
Created attachment 43817 [details]
more style

I don't think it's possible to add a layout test because this is system event conversion. Layout tests would bypass this code and create the WebMouseEvent directly. The only way to test this would be with a Chromium UI test.
Comment 7 WebKit Commit Bot 2009-11-24 15:46:31 PST
Comment on attachment 43817 [details]
more style

Clearing flags on attachment: 43817

Committed r51364: <http://trac.webkit.org/changeset/51364>
Comment 8 WebKit Commit Bot 2009-11-24 15:46:36 PST
All reviewed patches have been landed.  Closing bug.