Bug 11138 - Incorrect mouse event generation on Windows
Summary: Incorrect mouse event generation on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 420+
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 11142
  Show dependency treegraph
 
Reported: 2006-10-03 15:18 PDT by Don Gibson
Modified: 2006-10-05 11:01 PDT (History)
1 user (show)

See Also:


Attachments
patch v1 (7.73 KB, patch)
2006-10-03 15:24 PDT, Don Gibson
no flags Details | Formatted Diff | Diff
patch v2 (2.82 KB, patch)
2006-10-03 15:47 PDT, Don Gibson
aroben: review-
Details | Formatted Diff | Diff
patch v3 (3.71 KB, patch)
2006-10-03 17:17 PDT, Don Gibson
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Gibson 2006-10-03 15:18:48 PDT
Mouse events are not generated correctly in the Windows port:
* Clicks are only reported for the left button
* BUTTONUP events are reported on the wrong button
* Some events are reported with <garbage> as the button

Patch to fix these issues coming shortly.
Comment 1 Don Gibson 2006-10-03 15:24:15 PDT
Created attachment 10883 [details]
patch v1

This passes the actual button message along to the PlatformMouseEvent constructor, which uses it to set the button state, click count, etc. correctly.

Clicks are now produced for all three buttons.  Click counting should match Windows' behavior (from Spy++): a MOUSEUP of one button between clicks of another does not reset the click count for the second button, but a MOUSEDOWN does.
Comment 2 Don Gibson 2006-10-03 15:47:19 PDT
Created attachment 10884 [details]
patch v2

Updated to latest trunk... most of the "set the correct button" portion of this bug was fixed yesterday by aroben.  The remaining bit of that changed here is to set m_button to something sane for non-click messages (like WM_MOUSEMOVE).  Probably unnecessary, but it seems better than leaving m_button uninitialized.

That leaves getting click handling correct as the thing taking up most of this patch.
Comment 3 Adam Roben (:aroben) 2006-10-03 16:16:43 PDT
Comment on attachment 10884 [details]
patch v2

r=me. Thanks!
Comment 4 Adam Roben (:aroben) 2006-10-03 16:18:45 PDT
Comment on attachment 10884 [details]
patch v2

Oops, code is fine, but we need a ChangeLog entry.
Comment 5 Hunter L. Williams 2006-10-03 17:12:17 PDT
>+        case WM_LBUTTONDBLCLK:  // For these messages, the OS ensures that the
>+        case WM_MBUTTONDBLCLK:  // previous BUTTONDOWN was for the same button.
>+        case WM_RBUTTONDBLCLK:
>+            {
>+                DWORD curTime = GetTickCount();
>+                if (curTime - globalPrevClickTime > GetDoubleClickTime() ||
>+                    m_position != globalPrevClickPosition)
>+                    globalClickCount = 0;
>+                globalPrevClickTime = curTime;
>+            }
>+            globalPrevClickButton = m_button;
>+            globalPrevClickPosition = m_position;
>+            m_clickCount = ++globalClickCount;
>+            return;

Isn't a DBLCLK 2 clicks, not one? I saw code further down in WebCore that had if (event.event().clickCount() == 3) that did triple click handling. It seems that with this, for a triple click you'll get one DBLCLK and one LBUTTONDOWN (say) and thus only get m_clickCount = 2... or is clickCount incremented somewhere else independently for WM_*DBLCLK?
Comment 6 Don Gibson 2006-10-03 17:17:03 PDT
Created attachment 10885 [details]
patch v3

Now with 100% more ChangeLog
Comment 7 Don Gibson 2006-10-03 17:20:03 PDT
(In reply to comment #5)
> Isn't a DBLCLK 2 clicks, not one? I saw code further down in WebCore that had
> if (event.event().clickCount() == 3) that did triple click handling. It seems
> that with this, for a triple click you'll get one DBLCLK and one LBUTTONDOWN
> (say) and thus only get m_clickCount = 2... or is clickCount incremented
> somewhere else independently for WM_*DBLCLK?

WM_LMOUSEDBLCLK is sent in place of the second WM_LBUTTONDOWN on a double click (Windows doesn't queue up clicks to decide whether you're single- or double-clicking before sending messages).  So the message sequence on a double click is:
WM_LBUTTONDOWN
WM_LBUTTONUP
WM_LBUTTONDBLCLK
WM_LBUTTONUP
Thus, a double click message is still just one click.  We could avoid click counting altogether and just handle the messages as "double" and "single" clicks more easily if we didn't need to worry about dispatching triple-clicks, which Windows doesn't understand.
Comment 8 Hunter L. Williams 2006-10-03 17:28:37 PDT
OK, it's one click on Windows, but in Frame::handleMousePressEvent, there's this test:

if (event.event().clickCount() >= 3) {
  handleMousePressEventTripleClick(event);
  ...

which means that on Mac at least the event is initialized with clickCount = 3 for the triple click case, which means we probably need to do that here too. 

When you enumerate the messages that get sent, do you mean that Windows counts the first DOWN/UP action as the first component of the double click? i.e. are they coalesced? Do DOWN, UP, DBLCLK get sent for all doubleclick actions? Is the following UP the only message sent for the third click? (What happened to the DOWN for that one?)
Comment 9 Hunter L. Williams 2006-10-03 17:53:23 PDT
Apologies, I was confused. I was thinking about another issue, which I have filed in bug 11144.
Comment 10 Don Gibson 2006-10-04 11:02:16 PDT
Comment on attachment 10885 [details]
patch v3

Forgot to ask for review, oops
Comment 11 Adam Roben (:aroben) 2006-10-04 11:15:19 PDT
Comment on attachment 10885 [details]
patch v3

r=me
Comment 12 Alexey Proskuryakov 2006-10-05 11:01:09 PDT
Committed revision 16811.