Correcting menu button event trigger location

Correcting menu button event trigger location

BUG=64415
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172336

[anish.p, 2014-04-22 08:30:55]

I tested manually with various elements like link, input etc. It worked as
desired.
But, how should I test it using LayoutTests? eventSender.contextClick() can be
used to send context click. but eventSender.keyDown("menu") does not return
anything to compare.

[tkent, 2014-04-23 01:19:27]

On 2014/04/22 08:30:55, anish.p wrote:
> I tested manually with various elements like link, input etc. It worked as
> desired.
> But, how should I test it using LayoutTests? eventSender.contextClick() can be
> used to send context click. but eventSender.keyDown("menu") does not return
> anything to compare.

Does keyDown("menu") dispatches a 'contextmenu' event?

[anish.p, 2014-04-23 03:36:51]

> Does keyDown("menu") dispatches a 'contextmenu' event?

Yes it does. 
But I am sorry, I did not get the point. 
I thought, I need to check if context menu items are different for different
elements. At least, editable/non-editable difference should be verified.
eventSender.contextClick() returns the list of items but keyDown("menu") does
not.
Am I missing something?
Thanks.

[tkent, 2014-04-23 03:42:42]

On 2014/04/23 03:36:51, anish.p wrote:
> > Does keyDown("menu") dispatches a 'contextmenu' event?
> 
> Yes it does. 
> But I am sorry, I did not get the point. 
> I thought, I need to check if context menu items are different for different
> elements. At least, editable/non-editable difference should be verified.
> eventSender.contextClick() returns the list of items but keyDown("menu") does
> not.
> Am I missing something?
> Thanks.

The bug is that event.target is a wrong element, right?  We can verify a
contextmenu event has a correct target element.

[anish.p, 2014-04-23 04:24:16]

On 2014/04/23 03:42:42, tkent wrote:
> On 2014/04/23 03:36:51, anish.p wrote:
> > > Does keyDown("menu") dispatches a 'contextmenu' event?
> > 
> > Yes it does. 
> > But I am sorry, I did not get the point. 
> > I thought, I need to check if context menu items are different for different
> > elements. At least, editable/non-editable difference should be verified.
> > eventSender.contextClick() returns the list of items but keyDown("menu")
does
> > not.
> > Am I missing something?
> > Thanks.
> 
> The bug is that event.target is a wrong element, right?  We can verify a
> contextmenu event has a correct target element.

Added a LayoutTest for the same. PTAL.
Yes, the bug was that target is wrong. I thought, just simple layout test case
may not be sufficient.
Since, even existing implementation might pass the test.

[tkent, 2014-04-23 05:25:10]

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
File LayoutTests/fast/events/menu-key-context-menu.html (right):

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu.html:4: Test for context menu
customized for element type
This doesn't test it.
It should be something like "Test for contextmenu event target."

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu.html:8: <p>This test checks if
customized context menu is displayed for various elements.</p>
Ditto.

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu.html:9: <a href="" id="anchor"
oncontextmenu="handleContextMenu(event);">Test</a>
The oncontextmenu attribute means the handler is called only when the event
target is this element.
So, checking if handleContextMenu() is called twice would be enough.

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu.html:21:
document.getElementById("result").innerHTML +=  "PASS";
- <div id="result"></div> -> <div id="result">FAIL</div>
- += -> = in this line
- remove ...innerHTML +=  "FAIL" below.

[anish.p, 2014-04-23 05:43:59]

PTAL. Thanks.

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
File LayoutTests/fast/events/menu-key-context-menu.html (right):

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu.html:4: Test for context menu
customized for element type
On 2014/04/23 05:25:10, tkent wrote:
> This doesn't test it.
> It should be something like "Test for contextmenu event target."

Done.

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu.html:8: <p>This test checks if
customized context menu is displayed for various elements.</p>
On 2014/04/23 05:25:10, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu.html:9: <a href="" id="anchor"
oncontextmenu="handleContextMenu(event);">Test</a>
On 2014/04/23 05:25:10, tkent wrote:
> The oncontextmenu attribute means the handler is called only when the event
> target is this element.
> So, checking if handleContextMenu() is called twice would be enough.

Done.

https://codereview.chromium.org/247023002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu.html:21:
document.getElementById("result").innerHTML +=  "PASS";
On 2014/04/23 05:25:10, tkent wrote:
> - <div id="result"></div> -> <div id="result">FAIL</div>
> - += -> = in this line
> - remove ...innerHTML +=  "FAIL" below.

Done.

[tkent, 2014-04-23 05:45:48]

lgtm

[commit-bot: I haz the power, 2014-04-23 05:46:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/anish.p@samsung.com/247023002/40001

[commit-bot: I haz the power, 2014-04-23 07:31:40]

Change committed as 172336