Bug 31841 – Linux Chromium sends too many click events

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Evan Stade]

for example, double clicking on the forward button on google calendar will send the calendar forward 3 months instead of 2.

[Evan Stade]

Created attachment 43790 [details]
fix

[Evan Martin]

Comment on attachment 43790 [details]
fix

> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog	(revision 51343)
> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Evan Stade  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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;

[Evan Stade]

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.

[Evan Martin]

This seems good to me, though I am not a webkit reviewer.
The statics make me a bit sad, but not so much.

[David Levin]

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.

[Evan Stade]

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.

[WebKit Commit Bot]

Comment on attachment 43817 [details]
more style

Clearing flags on attachment: 43817

Committed r51364: <http://trac.webkit.org/changeset/51364>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.