Clear MousePressed state on context menu to avoid initiating a drag
Created attachment 173351 [details] Patch
Can you please post steps to reproduce for a bug you are fixing? Also, please explain the lack of regression test.
Created attachment 175458 [details] Patch
Created attachment 175460 [details] Patch
Created attachment 175461 [details] Patch
Created attachment 175467 [details] Patch
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
Created attachment 175487 [details] Patch
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 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.
(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.
Created attachment 175500 [details] Patch
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 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 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.
Created attachment 175508 [details] Patch
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.
Created attachment 175509 [details] Patch
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.
(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.
(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 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
Created attachment 175512 [details] Patch
Comment on attachment 175512 [details] Patch Clearing flags on attachment: 175512 Committed r135436: <http://trac.webkit.org/changeset/135436>
All reviewed patches have been landed. Closing bug.