Bug 101786 - Clear MousePressed state on context menu to avoid initiating a drag
Summary: Clear MousePressed state on context menu to avoid initiating a drag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-09 13:14 PST by Fady Samuel
Modified: 2012-11-21 14:34 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.38 KB, patch)
2012-11-09 13:14 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (14.05 KB, patch)
2012-11-21 09:14 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.69 KB, patch)
2012-11-21 09:20 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.53 KB, patch)
2012-11-21 09:22 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.26 KB, patch)
2012-11-21 09:48 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.28 KB, patch)
2012-11-21 11:22 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.03 KB, patch)
2012-11-21 12:43 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.52 KB, patch)
2012-11-21 13:23 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.56 KB, patch)
2012-11-21 13:30 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.55 KB, patch)
2012-11-21 13:38 PST, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2012-11-09 13:14:01 PST
Clear MousePressed state on context menu to avoid initiating a drag
Comment 1 Fady Samuel 2012-11-09 13:14:20 PST
Created attachment 173351 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-11-10 22:42:28 PST
Can you please post steps to reproduce for a bug you are fixing? Also, please explain the lack of regression test.
Comment 3 Fady Samuel 2012-11-21 09:14:30 PST
Created attachment 175458 [details]
Patch
Comment 4 Fady Samuel 2012-11-21 09:20:01 PST
Created attachment 175460 [details]
Patch
Comment 5 Fady Samuel 2012-11-21 09:22:53 PST
Created attachment 175461 [details]
Patch
Comment 6 Fady Samuel 2012-11-21 09:48:44 PST
Created attachment 175467 [details]
Patch
Comment 7 jochen 2012-11-21 11:04:00 PST
Comment on attachment 175467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175467&action=review

> Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:855
> +    if (result) // Could be 0 if invoked asynchronously.

I don't think this method is ever invoked asynchronously

> LayoutTests/platform/chromium/fast/events/context-nodrag.html:1
> +<html>

please add a doctype

> LayoutTests/platform/chromium/fast/events/context-nodrag.html:28
> +       document.getElementById('result').appendChild(document.createTextNode('FAIL'));

nit. everywhere else you use double quotes, also wrong indent
Comment 8 Fady Samuel 2012-11-21 11:22:03 PST
Created attachment 175487 [details]
Patch
Comment 9 Fady Samuel 2012-11-21 11:24:28 PST
Comment on attachment 175467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175467&action=review

>> Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:855
>> +    if (result) // Could be 0 if invoked asynchronously.
> 
> I don't think this method is ever invoked asynchronously

Yup, copypasta error.

>> LayoutTests/platform/chromium/fast/events/context-nodrag.html:1
>> +<html>
> 
> please add a doctype

Done.

>> LayoutTests/platform/chromium/fast/events/context-nodrag.html:28
>> +       document.getElementById('result').appendChild(document.createTextNode('FAIL'));
> 
> nit. everywhere else you use double quotes, also wrong indent

Done.
Comment 10 Ojan Vafai 2012-11-21 11:55:12 PST
Comment on attachment 175487 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175487&action=review

The EventHandler.cpp change looks correct to me. I'll r+ once we work out the test issues and the ChangeLog descriptions are updated.

> Source/WebCore/ChangeLog:7
> +

This needs a bit more description of what the change is actually doing.

Also, you should include in the ChangeLog what the platform behavior is for this set of actions (i.e. right-click when the left-mouse button is down).

In my testing, Windows and Linux stop the drag operation when you bring up the context menu (matching this patch). Mac doesn't bring up the context menu at all if you have the left mouse button down. That's a separate bug we should file since we (both Chrome and Safari) currently do bring it up.

In short, I think the behavioral change here is correct per platform convention. Your ChangeLog description should just explain that. Ideally in fewer, easier to understand words than my crappy description here. :)

> LayoutTests/ChangeLog:13
> +        If a user initiates a drag, and then brings up the context menu, and then cancels
> +        the context menu, then the drag operation will continue. This feels awkward.

You need a "without releaseing the left mouse button" somewhere in here. That was the part I missed when trying to reproduce this.

Also, more important than it's awkwardness is that it doesn't match platform conventions.

> LayoutTests/ChangeLog:17
> +        * platform/chromium/fast/events/context-nodrag.html: Added.

Why is this in platform/chromium? The bug this is fixing isn't specific to chrome. I could reproduce on Apple Mac. For the platforms that don't implement contextMouseDown, you can skip the test for now.

Even better, maybe contextClick should be changed to match platform behavior. On Apple-Mac, it already only sends a mousedown. How about we change the implementation of contextClick to do mousedown+mouseup only on Windows?

In either case, the test will still pass for the platforms that send a mouseup as well, right?

> LayoutTests/platform/chromium/fast/events/context-nodrag.html:1
> +<!doctype html>

This should be:
<!DOCTYPE html>

That's the official HTML doctype and is what we use in all our tests.

> LayoutTests/platform/chromium/fast/events/context-nodrag.html:4
> +<div id="box" style="border:1px dotted red">This tests to make sure that right clicking does not start a drag.</div>

You don't need this border, do you?

> LayoutTests/platform/chromium/fast/events/context-nodrag.html:12
> +     var box, x, y;

Nit: typically we'd just put the var before each line (i.e. lines 13, 14 and 15).

> LayoutTests/platform/chromium/fast/events/context-nodrag.html:32
> +     if (selection)
> +         document.getElementById("result").appendChild(document.createTextNode("FAIL"));
> +     else
> +         document.getElementById("result").appendChild(document.createTextNode("PASS"));
> +     testRunner.dumpAsText();

Please import js-test-pre.js instead of doing this. Then you also won't need the 'result' paragraph. You just call shouldBeTrue('!window.getSelection().isCollapsed').

In addition to being less code, it gives a more helpful error message when it fails.
Comment 11 Ojan Vafai 2012-11-21 11:58:28 PST
(In reply to comment #10)
> Mac doesn't bring up the context menu at all if you have the left mouse button down. That's a separate bug we should file since we (both Chrome and Safari) currently do bring it up.

Actually, Mac is inconsistent here. TextEdit doesn't bring up the context menu, but right clicking on the desktop with the left mouse button down, does bring up the context menu. Not sure what the right platform behavior is, but I think this patch is still fine for Mac.
Comment 12 Fady Samuel 2012-11-21 12:43:20 PST
Created attachment 175500 [details]
Patch
Comment 13 Fady Samuel 2012-11-21 12:43:58 PST
Comment on attachment 175487 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175487&action=review

>> Source/WebCore/ChangeLog:7
>> +
> 
> This needs a bit more description of what the change is actually doing.
> 
> Also, you should include in the ChangeLog what the platform behavior is for this set of actions (i.e. right-click when the left-mouse button is down).
> 
> In my testing, Windows and Linux stop the drag operation when you bring up the context menu (matching this patch). Mac doesn't bring up the context menu at all if you have the left mouse button down. That's a separate bug we should file since we (both Chrome and Safari) currently do bring it up.
> 
> In short, I think the behavioral change here is correct per platform convention. Your ChangeLog description should just explain that. Ideally in fewer, easier to understand words than my crappy description here. :)

Done.

>> LayoutTests/ChangeLog:13
>> +        the context menu, then the drag operation will continue. This feels awkward.
> 
> You need a "without releaseing the left mouse button" somewhere in here. That was the part I missed when trying to reproduce this.
> 
> Also, more important than it's awkwardness is that it doesn't match platform conventions.

Done.

>> LayoutTests/ChangeLog:17
>> +        * platform/chromium/fast/events/context-nodrag.html: Added.
> 
> Why is this in platform/chromium? The bug this is fixing isn't specific to chrome. I could reproduce on Apple Mac. For the platforms that don't implement contextMouseDown, you can skip the test for now.
> 
> Even better, maybe contextClick should be changed to match platform behavior. On Apple-Mac, it already only sends a mousedown. How about we change the implementation of contextClick to do mousedown+mouseup only on Windows?
> 
> In either case, the test will still pass for the platforms that send a mouseup as well, right?

Done, moved, and modified contextClick.

>> LayoutTests/platform/chromium/fast/events/context-nodrag.html:1
>> +<!doctype html>
> 
> This should be:
> <!DOCTYPE html>
> 
> That's the official HTML doctype and is what we use in all our tests.

Done.

>> LayoutTests/platform/chromium/fast/events/context-nodrag.html:4
>> +<div id="box" style="border:1px dotted red">This tests to make sure that right clicking does not start a drag.</div>
> 
> You don't need this border, do you?

Done.

>> LayoutTests/platform/chromium/fast/events/context-nodrag.html:12
>> +     var box, x, y;
> 
> Nit: typically we'd just put the var before each line (i.e. lines 13, 14 and 15).

Done.

>> LayoutTests/platform/chromium/fast/events/context-nodrag.html:32
>> +     testRunner.dumpAsText();
> 
> Please import js-test-pre.js instead of doing this. Then you also won't need the 'result' paragraph. You just call shouldBeTrue('!window.getSelection().isCollapsed').
> 
> In addition to being less code, it gives a more helpful error message when it fails.

Done.
Comment 14 Ojan Vafai 2012-11-21 12:53:06 PST
Comment on attachment 175500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175500&action=review

Looks great. Just a couple nits about the test. Feel free to commit with those fixes.

> Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:839
> +    if (pressedButton == WebMouseEvent::ButtonNone)

I'm confused by this if-statement. Aren't context-clicks always done with the right mouse button? I guess the concept that we can only have a single button pressed is just flawed. This probably deserves a short comment saying that it's a hack to work around only allowing a single pressed button since we want to test the case where both the left and mouse buttons are pressed.

> LayoutTests/fast/events/context-nodrag.html:8
> +<p id="description"></p>
> +<div id="console"></div>

You don't need these elements. js-test-pre will insert them for you.

> LayoutTests/fast/events/context-nodrag.html:11
> +     description("This tests to make sure that right clicking does not start a drag.");

How about: This tests to make sure that right clicking when the left-mouse button is pressed disables the drag.

> LayoutTests/fast/events/context-nodrag.html:12
> +     var box = document.getElementById("description");

Better to create an element the js-test-pre doesn't muck with should js-test-pre change in the future.
Comment 15 Ojan Vafai 2012-11-21 12:55:55 PST
Comment on attachment 175500 [details]
Patch

Actually, one more thing, we should probably also clear m_mousePressed from sendContextMenuEventForKey if you open the context menu from the keyboard during a drag.
Comment 16 Fady Samuel 2012-11-21 13:23:04 PST
Created attachment 175508 [details]
Patch
Comment 17 Ojan Vafai 2012-11-21 13:28:44 PST
Comment on attachment 175508 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175508&action=review

> LayoutTests/fast/events/context-nodrag-expected.txt:5
> +

Does this need to be updated? I would expect the text context of the box to be here.
Comment 18 Fady Samuel 2012-11-21 13:30:44 PST
Created attachment 175509 [details]
Patch
Comment 19 Fady Samuel 2012-11-21 13:31:20 PST
Comment on attachment 175500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175500&action=review

>> Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:839
>> +    if (pressedButton == WebMouseEvent::ButtonNone)
> 
> I'm confused by this if-statement. Aren't context-clicks always done with the right mouse button? I guess the concept that we can only have a single button pressed is just flawed. This probably deserves a short comment saying that it's a hack to work around only allowing a single pressed button since we want to test the case where both the left and mouse buttons are pressed.

Done.

>> LayoutTests/fast/events/context-nodrag.html:8
>> +<div id="console"></div>
> 
> You don't need these elements. js-test-pre will insert them for you.

Done.

>> LayoutTests/fast/events/context-nodrag.html:11
>> +     description("This tests to make sure that right clicking does not start a drag.");
> 
> How about: This tests to make sure that right clicking when the left-mouse button is pressed disables the drag.

Done.

>> LayoutTests/fast/events/context-nodrag.html:12
>> +     var box = document.getElementById("description");
> 
> Better to create an element the js-test-pre doesn't muck with should js-test-pre change in the future.

Done.
Comment 20 Fady Samuel 2012-11-21 13:31:51 PST
(In reply to comment #15)
> (From update of attachment 175500 [details])
> Actually, one more thing, we should probably also clear m_mousePressed from sendContextMenuEventForKey if you open the context menu from the keyboard during a drag.

Done.
Comment 21 Fady Samuel 2012-11-21 13:32:28 PST
(In reply to comment #17)
> (From update of attachment 175508 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175508&action=review
> 
> > LayoutTests/fast/events/context-nodrag-expected.txt:5
> > +
> 
> Does this need to be updated? I would expect the text context of the box to be here.

Yes, updated. Sorry, I typed webkit-patch upload too soon :-) Uploaded a corrected patch.
Comment 22 Ojan Vafai 2012-11-21 13:35:22 PST
Comment on attachment 175509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175509&action=review

Looks good.

> Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:839
> +    // Is this is a hack to work around only allowing a single pressed button since we want to

Typo: s/Is this/This
Comment 23 Fady Samuel 2012-11-21 13:38:48 PST
Created attachment 175512 [details]
Patch
Comment 24 WebKit Review Bot 2012-11-21 14:34:43 PST
Comment on attachment 175512 [details]
Patch

Clearing flags on attachment: 175512

Committed r135436: <http://trac.webkit.org/changeset/135436>
Comment 25 WebKit Review Bot 2012-11-21 14:34:48 PST
All reviewed patches have been landed.  Closing bug.