Bug 101786

Summary:

Clear MousePressed state on context menu to avoid initiating a drag

Product:

WebKit

Reporter:

Fady Samuel <fsamuel>

Component:

UI Events

Assignee:

Fady Samuel <fsamuel>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, fishd, jochen, ojan, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Fady Samuel

Reported 2012-11-09 13:14:01 PST

Clear MousePressed state on context menu to avoid initiating a drag

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
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Fady Samuel

Comment 1 2012-11-09 13:14:20 PST

Created attachment 173351 [details] Patch

Alexey Proskuryakov

Comment 2 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.

Fady Samuel

Comment 3 2012-11-21 09:14:30 PST

Created attachment 175458 [details] Patch

Fady Samuel

Comment 4 2012-11-21 09:20:01 PST

Created attachment 175460 [details] Patch

Fady Samuel

Comment 5 2012-11-21 09:22:53 PST

Created attachment 175461 [details] Patch

Fady Samuel

Comment 6 2012-11-21 09:48:44 PST

Created attachment 175467 [details] Patch

jochen

Comment 7 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

Fady Samuel

Comment 8 2012-11-21 11:22:03 PST

Created attachment 175487 [details] Patch

Fady Samuel

Comment 9 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.

Ojan Vafai

Comment 10 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.

Ojan Vafai

Comment 11 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.

Fady Samuel

Comment 12 2012-11-21 12:43:20 PST

Created attachment 175500 [details] Patch

Fady Samuel

Comment 13 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.

Ojan Vafai

Comment 14 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.

Ojan Vafai

Comment 15 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.

Fady Samuel

Comment 16 2012-11-21 13:23:04 PST

Created attachment 175508 [details] Patch

Ojan Vafai

Comment 17 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.

Fady Samuel

Comment 18 2012-11-21 13:30:44 PST

Created attachment 175509 [details] Patch

Fady Samuel

Comment 19 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.

Fady Samuel

Comment 20 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.

Fady Samuel

Comment 21 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.

Ojan Vafai

Comment 22 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

Fady Samuel

Comment 23 2012-11-21 13:38:48 PST

Created attachment 175512 [details] Patch

WebKit Review Bot

Comment 24 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>

WebKit Review Bot

Comment 25 2012-11-21 14:34:48 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.