227328 - we get extra single-click events due to GDK

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[David Baron :dbaron:]

Because of GDK, we send out extra single-click DOM events that we shouldn't be
for double-clicks and triple-clicks.

This is because the GDK event queue works in a weird way -- when it sees a
double or triple click, it adds an additional double or triple press event after
the event for the single press.  We need to ignore these events.  There are two
options:
 * look ahead in the event queue
 * ignore GDK's double/triple-press events and synthesize our own using the
times stored in the events and GDK's interval

blizzard prefers the first of these.  I'm not sure if I can assume that if
there's a corresponding double/triple press event it will always be the next
event in the queue.

Comment 1

[David Baron :dbaron:]

Attachment: testcase

For each click of the mouse, you should see one of the following lines
(depending on whether than click has a clickCount of 1, 2, or 3):
down(1).  up.
down(2).  up.
down(3).  up.

Comment 2

[David Baron :dbaron:]

Comment on attachment 136701 [details]
testcase

Steps to reproduce: triple-click in attachment	136701 [details]

Actual results:
down(1).  up.
down(1).  down(2).  up.
down(1).  down(3).  up.

Expected results:
down(1).  up.
down(2).  up.
down(3).  up.

Comment 3

[Christopher Blizzard (:blizzard)]

We've been talking about this on irc.  I didn't realize that gdk was generating
an extra single click event for each double click.

Anyway, it should be pretty easy to look in the queue and see if there's either
a double or triple click event in the queue.  If there is, we can just assume
that this is the click associated with that event, and drop it.

Comment 4

[David Baron :dbaron:]

Attachment: more thorough testcase

Just add "  click(n)" to the steps to reproduce for the previous testcase.

Comment 5

[Christopher Blizzard (:blizzard)]

Attachment: patch

Patch that checks the next event in the queue to see if it's a 2BUTTON or
3BUTTON patch.	This seems pretty safe.  The code in gdk puts the 2BUTTON or
3BUTTON event onto the front of the queue using gdk_event_put() so it's pretty
safe to assume that the very next event will be the event we're looking for.

This works with the test case.

Comment 6

[David Baron :dbaron:]

Comment on attachment 136703 [details] [diff] [review]
patch


Looks good to me, and works fine for me.  But, since I always like to simplify
code, I'll note that

>+        if (peekedEvent->any.type == GDK_2BUTTON_PRESS ||
>+            peekedEvent->any.type == GDK_3BUTTON_PRESS) {
>+            gdk_event_free(peekedEvent);
>+            return;
>+        }
>+
>+        gdk_event_free(peekedEvent);

Could be simplified to:

GdkEventType type = peekedEvent->any.type;
gdk_event_free(peekedEvent);
if (type == GDK_2BUTTON_PRESS || type == GDK_3BUTTON_PRESS)
    return;

Comment 7

[Christopher Blizzard (:blizzard)]

Attachment: final patch

Patch that includes a fix for dbaron's comment.

Comment 8

[David Baron :dbaron:]

Comment on attachment 136724 [details] [diff] [review]
final patch

I suspect this needs to be fixed in the GTK1 port as well (unless brendan was
using a GTK2 build).

Comment 9

[David Baron :dbaron:]

It looks like the same patch should apply in the same function in
widget/src/gtk1/nsWidget.cpp (not nsWindow).  I'm still waiting for my build to
finish to test the result.  (I did test that it's needed for the GTK1 port.)

Comment 10

[David Baron :dbaron:]

Attachment: GTK1 patch

This is tested on a GTK1 build.

Comment 11

[David Baron :dbaron:]

Attachment: GTK1 patch

It would help if I diffed the right file.

Comment 12

[David Baron :dbaron:]

Fix checked in to trunk, 2003-12-21 10:54/55 -0800.