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

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted 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 175500: Patch
https://bugs.webkit.org/attachment.cgi?id=175500&action=review

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