Bug 22505 - Add a test to validate the event order for context clicks.
Summary: Add a test to validate the event order for context clicks.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-25 22:31 PST by David Levin
Modified: 2008-12-09 17:28 PST (History)
2 users (show)

See Also:


Attachments
Adds the test mentioned in the bug. (4.99 KB, patch)
2008-11-25 23:16 PST, David Levin
no flags Details | Formatted Diff | Diff
New patch. (6.98 KB, patch)
2008-12-08 16:14 PST, David Levin
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2008-11-25 22:31:07 PST
A key thing to verify is that "mouseup" shouldn't happen after "contextmenu".
Comment 1 David Levin 2008-11-25 23:16:33 PST
Created attachment 25515 [details]
Adds the test mentioned in the bug.
Comment 2 Eric Seidel (no email) 2008-12-01 14:44:54 PST
Comment on attachment 25515 [details]
Adds the test mentioned in the bug.

The test looks OK.  I'm just not sure why our results are correct?  Why would we want to send these events in different orders on different platforms?

We generally prefer tests which include the expected results over ones which just dump them.

If we could decide what the exact order for all platforms should be, then we could test for that explicitly.
Comment 3 David Levin 2008-12-01 15:26:06 PST
I thought the difference either in events sent for the different platforms was odd but thought I must not know something due to my newbie status.  Here's the survey that I did when looking at this test.

For Windows:
  Safari, Chromium, and Firefox all send a mousedown, then a mouseup, and then a contextmenu event all with event.button = 2 for all three events.

  IE sends the mousedown, mouseup with event.button =2, and then a contextmenu event with event.button = 0.

For OSX:

  Safari sends a mousedown and a contextmenu event all with event.button = 2.
  Firefox sends a mousedown, a contextmenu and then a mouseup event all with event.button = 2.

It seems to me that the correct behavior is to 
  "send a mousedown, then a mouseup, and then a contextmenu event all with event.button = 2 for all three events."

I'd be willing to change Safari on OSX to do this.  I'd modify 
    - (NSMenu *)menuForEvent:(NSEvent *)event in WebKit/mac/WebView/WebHTMLView.mm
by adding
        coreframe->eventHandler()->mouseUp(event);
right after the coreframe->eventHandler()->mouseDown(event); line.

I'll check if the contextmenu event is sent, it the mouseup is handled and mirror that behavior too (as well as add a regression test).

How does that sound?

PS fwiw, the mouseDown change was added by http://trac.webkit.org/changeset/28585 to mirror other browsers.)
Comment 4 David Levin 2008-12-07 16:47:22 PST
I had a discussion with Hixie on irc and he helped me to understand the platform difference.

mouseup shouldn't occur before contextmenu on osx because the contextmenu occurs on mousedown on OSX, so there is an expected difference for the mouseup event.

The main issue is for this test is that mouseup shouldn't occur after the contextmenu (on Windows).

Comment 5 David Levin 2008-12-07 23:43:04 PST
Comment on attachment 25515 [details]
Adds the test mentioned in the bug.

I think the patch should change to modify context-onmousedown-event-expected.txt, so I'll do that before having this reviewed further.
Comment 6 David Levin 2008-12-08 16:14:19 PST
Created attachment 25859 [details]
New patch.

On Windows, it is important for some web sites (like Google Spreadsheets) that the onmouseup event occurs before the oncontextmenu event or else the context menu disappears.

This test verifies that this event order for Windows.
Comment 7 Eric Seidel (no email) 2008-12-09 14:51:26 PST
Comment on attachment 25859 [details]
New patch.

Looks good.
Comment 8 Pam Greene (IRC:pamg) 2008-12-09 17:28:39 PST
Landed r39158.

The changes to fast/canvas/* from make-js-wrappers were already done in r39114. Removed those from the patch and ChangeLog.