[webkit-reviews] review denied: [Bug 101786] Clear MousePressed state on context menu to avoid initiating a drag : [Attachment 175487] Patch

Wed Nov 21 11:53:10 PST 2012

Ojan Vafai <ojan at chromium.org> has denied Fady Samuel <fsamuel at chromium.org>'s
request for review:
Bug 101786: Clear MousePressed state on context menu to avoid initiating a drag
https://bugs.webkit.org/show_bug.cgi?id=101786

Attachment 175487: Patch
https://bugs.webkit.org/attachment.cgi?id=175487&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.