Fix contextmenu event location for menu key in an iframe

Fix contextmenu event location for menu key in an iframe

For focusedElement contentToRootView translate should not happen.
so that we will get proper coprdinate for context menu i.e centerPoint
of clippedRect of focusedElement.

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

[Deepak, 2014-12-01 10:14:55]

deepak.m1@samsung.com changed reviewers:
+ rob.buis@chromium.org

[Deepak, 2014-12-01 10:14:55]

PTAL.

[Deepak, 2014-12-03 05:45:20]

deepak.m1@samsung.com changed reviewers:
+ arv@chromium.org

[Deepak, 2014-12-03 05:45:21]

@arv
PTAL at my changes.

[anish.p, 2014-12-03 06:23:41]

Removing myself from CC list.

[Deepak, 2014-12-05 05:41:21]

deepak.m1@samsung.com changed reviewers:
+ rbyers@chromium.org
- rob.buis@chromium.org

[Deepak, 2014-12-05 05:42:03]

@Rick Byers,
PTAL at my small changes.
Thanks

[Rick Byers, 2014-12-05 18:14:46]

Change looks reasonable but you need a test.  Perhaps you can find a similar
pattern to follow in LayoutTests/fast/events somewhere?

[Rick Byers, 2014-12-05 18:15:00]

rbyers@chromium.org changed reviewers:
- arv@chromium.org

[Rick Byers, 2014-12-05 18:15:35]

On 2014/12/03 05:45:21, Deepak wrote:
> @arv
> PTAL at my changes.

Moving arv@ to cc list - one reviewer is enough here.

[Deepak, 2014-12-08 07:30:15]

On 2014/12/05 18:15:35, Rick Byers wrote:
> On 2014/12/03 05:45:21, Deepak wrote:
> > @arv
> > PTAL at my changes.
> 
> Moving arv@ to cc list - one reviewer is enough here.

@Rick Byers
I have added test case.
When context menu comes by menu key it is on the element that is focused.
And after dismissing it by 'esc' key, focus should remain on same element.
That is not happening without patch.
PTAL at my changes.
Thanks

[Rick Byers, 2014-12-08 22:02:17]

On 2014/12/08 07:30:15, Deepak wrote:
> On 2014/12/05 18:15:35, Rick Byers wrote:
> > On 2014/12/03 05:45:21, Deepak wrote:
> > > @arv
> > > PTAL at my changes.
> > 
> > Moving arv@ to cc list - one reviewer is enough here.
> 
> @Rick Byers
> I have added test case.
> When context menu comes by menu key it is on the element that is focused.
> And after dismissing it by 'esc' key, focus should remain on same element.
> That is not happening without patch.
> PTAL at my changes.
> Thanks

This seems rather indirect.  Is this happening because the right mouse button
press is changing focus to some other element?  This seems OK to test, but you
should also test the primary thing you're fixing here - that the event is being
sent to the right node.  Eg. add a 'contextmenu' event handler and check the
properties of that event - ideally even the actual co-ordinates in the
MouseEvent.  The existing menu-key-contextmenu* tests appear to verify the event
target, but not for the 'focus' scenario.  Sadly none of the existing tests
appear to attempt to verify the co-ordinates stored in the event.

[Deepak, 2014-12-09 04:12:19]

On 2014/12/08 22:02:17, Rick Byers wrote:
> On 2014/12/08 07:30:15, Deepak wrote:
> > On 2014/12/05 18:15:35, Rick Byers wrote:
> > > On 2014/12/03 05:45:21, Deepak wrote:
> > > > @arv
> > > > PTAL at my changes.
> > > 
> > > Moving arv@ to cc list - one reviewer is enough here.
> > 
> > @Rick Byers
> > I have added test case.
> > When context menu comes by menu key it is on the element that is focused.
> > And after dismissing it by 'esc' key, focus should remain on same element.
> > That is not happening without patch.
> > PTAL at my changes.
> > Thanks
> 
> This seems rather indirect.  Is this happening because the right mouse button
> press is changing focus to some other element?  This seems OK to test, but you
> should also test the primary thing you're fixing here - that the event is
being
> sent to the right node.  Eg. add a 'contextmenu' event handler and check the
> properties of that event - ideally even the actual co-ordinates in the
> MouseEvent.  The existing menu-key-contextmenu* tests appear to verify the
event
> target, but not for the 'focus' scenario.  Sadly none of the existing tests
> appear to attempt to verify the co-ordinates stored in the event.

Thanks for review and sharing your thoughts.
I tried to get check co-ordinates stored in the event.
But I am getting some difference in y coordinate value. so we can not rely on
this.
I have added changes for checking eventTarget as suggested.

PTAL at my changes.
Thanks

[Rick Byers, 2014-12-10 03:09:25]

https://codereview.chromium.org/766143002/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/menu-key-context-menu-element-focus.html (right):

https://codereview.chromium.org/766143002/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu-element-focus.html:18: if(count
=== 2 && (e.currentTarget == anchorNode)) {
Does this test actually fail without your patch?  currentTarget==anchorNode
should always be true since the event handler was added on the anchorNode
directly.

I guess I don't understand why menu-key-context-menu.html isn't already covering
everything you're testing here.  What's fundamentally different about this test?

From the code, I would have expected that you'd need to scroll the page in order
to provoke the bug.  What if you just change menu-key-context-menu.html to add
some padding at the bottom of the document and call window.scrollTo(0, 30). 
That way the root view co-ordinate space and the contents co-ordinate space are
different.  If do that, does the test start failing without your patch (but
still pass with your patch)?

https://codereview.chromium.org/766143002/diff/40001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/766143002/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2790: shouldTranslateToRootView = true;
by the way, you don't need this variable at all anymore, right?  It would be
much simpler to just keep 'location' always in root-view co-ordinate space
(maybe even call it rootViewLocation) and just add the view->contentsToRootView
call here.

[Deepak, 2014-12-10 06:40:27]

PTAL at changes and comments
Thanks

https://codereview.chromium.org/766143002/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/menu-key-context-menu-element-focus.html (right):

https://codereview.chromium.org/766143002/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/menu-key-context-menu-element-focus.html:18: if(count
=== 2 && (e.currentTarget == anchorNode)) {
On 2014/12/10 03:09:24, Rick Byers wrote:
> Does this test actually fail without your patch?  currentTarget==anchorNode
> should always be true since the event handler was added on the anchorNode
> directly.
 No , This test is not failing without my changes.

> I guess I don't understand why menu-key-context-menu.html isn't already
covering
> everything you're testing here.  What's fundamentally different about this
test?
 Actually when you load  http://jsfiddle.net/kvckb90w/2/show/
 * Use tab key to bring focus on one of the buttons
 * Hit the context menu key
Then context menu does not come on the button but off the button and on
selecting 'esc' key, Button will lose focus and then
 again selecting 'Context menu' key context menu will come at the top left
corner and context menu items are also different.

But I am surprised that in the test case doing
document.activeElement after hitting 'esc' giving still 'anchor' element as
focused element.

> From the code, I would have expected that you'd need to scroll the page in
order
> to provoke the bug.  What if you just change menu-key-context-menu.html to add
> some padding at the bottom of the document and call window.scrollTo(0, 30). 
> That way the root view co-ordinate space and the contents co-ordinate space
are
> different.  If do that, does the test start failing without your patch (but
> still pass with your patch)? 
 I tried this, But still the test case is getting passed, as 'Anchor' element is
not losing focus after scroll.

I have added a padding in para as:
<style>
p.ex1 {
    padding: 100px 100px;
}
</style>

<a href="" id="anchor" oncontextmenu="handleContextMenu(event);">Test</a>
<div id="result">FAIL</div>
<p class="ex1">Padding.</p>

and call as:

 anchor.focus();
 eventSender.keyDown("menu");
 eventSender.keyDown("escape");
 window.scroll(0,200);
 eventSender.keyDown("menu");

but as 'Anchor' element doesn't lose focus in test case, so it is getting passed
even without my change.
But in the issue case 'Button' is losing focus, as context menu is not on the
button.

https://codereview.chromium.org/766143002/diff/40001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/766143002/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2790: shouldTranslateToRootView = true;
On 2014/12/10 03:09:25, Rick Byers wrote:
> by the way, you don't need this variable at all anymore, right?  It would be
> much simpler to just keep 'location' always in root-view co-ordinate space
> (maybe even call it rootViewLocation) and just add the
view->contentsToRootView
> call here.

Done.

[Deepak, 2014-12-10 09:26:28]

I also observed that as the "http://jsfiddle.net/kvckb90w/2/show/"
is not normal html page
as we are getting frame's menu items like 'Reload Frame' ,'view frame info' etc
also in context menu that came by right click on this page.

But we are checking in the normal html page in our test case that is the
reason that our test case is passing without our changes.
as issue is not reproduced on normal page with anchor or link or button.
It appears special case with http://jsfiddle.net/kvckb90w/2/show/ where
element lose focus when 'esc' key is pressed to dismiss context menu that came
by menu key.

[Deepak, 2014-12-10 12:52:36]

@Rick Bayers
I have added new test case, This test case is getting failed without my patch
And With the Patch Test case is getting passed.
Other layout tests are also working well.
I have also addressed comments in the Eventhandler.cpp file.
PTAL at my changes.

[Deepak, 2014-12-12 10:25:35]

On 2014/12/10 12:52:36, Deepak wrote:
@Rick Bayers
I have added new test case, This test case is getting failed without my patch
And With the Patch Test case is getting passed.
Other layout tests are also working well.
I have also addressed comments in the Eventhandler.cpp file.
PTAL at my changes.

[Deepak, 2014-12-15 12:38:17]

On 2014/12/12 10:25:35, Deepak wrote:
> On 2014/12/10 12:52:36, Deepak wrote:
> @Rick Bayers
> I have added new test case, This test case is getting failed without my patch
> And With the Patch Test case is getting passed.
> Other layout tests are also working well.
> I have also addressed comments in the Eventhandler.cpp file.
> PTAL at my changes.

PTAL.

[Deepak, 2014-12-17 13:11:39]

PTAL at my changes.
Thanks

[Rick Byers, 2014-12-18 22:28:23]

On 2014/12/10 06:40:27, Deepak wrote:
> PTAL at changes and comments
> Thanks
> 
>
https://codereview.chromium.org/766143002/diff/40001/LayoutTests/fast/events/...
> File LayoutTests/fast/events/menu-key-context-menu-element-focus.html (right):
> 
>
https://codereview.chromium.org/766143002/diff/40001/LayoutTests/fast/events/...
> LayoutTests/fast/events/menu-key-context-menu-element-focus.html:18: if(count
> === 2 && (e.currentTarget == anchorNode)) {
> On 2014/12/10 03:09:24, Rick Byers wrote:
> > Does this test actually fail without your patch?  currentTarget==anchorNode
> > should always be true since the event handler was added on the anchorNode
> > directly.
>  No , This test is not failing without my changes.
> 
> > I guess I don't understand why menu-key-context-menu.html isn't already
> covering
> > everything you're testing here.  What's fundamentally different about this
> test?
>  Actually when you load  http://jsfiddle.net/kvckb90w/2/show/
>  * Use tab key to bring focus on one of the buttons
>  * Hit the context menu key
> Then context menu does not come on the button but off the button and on
> selecting 'esc' key, Button will lose focus and then
>  again selecting 'Context menu' key context menu will come at the top left
> corner and context menu items are also different.

Right, that makes sense as a side effect of the root problem here.  The root
problem is that the 'contextmenu' event is sent with the wrong co-ordinates and
so hits the wrong node.  After you dismiss the contextmenu, focus has changed to
the point where the contextmenu event was dispatched (not exactly sure why, but
it's not important).  You should be testing the core issue, not the side effect
of what happens after 'esc' causes the context menu to close.  Eg. if someone
broke the code so that the contextmenu key did absolutely nothing at all, then
your test would still pass - that's no good.

The existing menu-key-context-menu.html test has the right basic idea here.  It
puts a contextmenu event handler on the node we expect the mouse event to reach
and verifies that the handler gets invoked.

But your test gave me the answer I was looking for - the existing test works
fine because the problem is limited to iframes (when there's no iframe involved
then view==rootView so no transformation is required, whenever view!=rootView
then we'll hit the bug).  That's the key to this bug - contextmenu events in
iframes are sent to the wrong location (I've updated the CL description to
clarify that).

So instead of testing the activeElement, please put a contextmenu event listener
in the iframe and verify it gets invoked with the correct 'target' element, and
with the correct clientX/clientY co-ordinates.  That should fail very obviously
without your patch (clientY being off by exactly the distance between the iframe
and main document).

You can see an example of testing events in iframes at
LayoutTests/fast/events/touch/gesture/gesture-tap-frame-scrollbar.html - note
that the iframe (event-delegator.html) installs an event listener that just
calls 'parent.onEventInIframe', and the main page implements that function to
log and check the results of the event.

 
> But I am surprised that in the test case doing
> document.activeElement after hitting 'esc' giving still 'anchor' element as
> focused element.
> 
> > From the code, I would have expected that you'd need to scroll the page in
> order
> > to provoke the bug.  What if you just change menu-key-context-menu.html to
add
> > some padding at the bottom of the document and call window.scrollTo(0, 30). 
> > That way the root view co-ordinate space and the contents co-ordinate space
> are
> > different.  If do that, does the test start failing without your patch (but
> > still pass with your patch)? 
>  I tried this, But still the test case is getting passed, as 'Anchor' element
is
> not losing focus after scroll.
> 
> I have added a padding in para as:
> <style>
> p.ex1 {
>     padding: 100px 100px;
> }
> </style>
> 
> <a href="" id="anchor" oncontextmenu="handleContextMenu(event);">Test</a>
> <div id="result">FAIL</div>
> <p class="ex1">Padding.</p>
> 
> and call as:
> 
>  anchor.focus();
>  eventSender.keyDown("menu");
>  eventSender.keyDown("escape");
>  window.scroll(0,200);
>  eventSender.keyDown("menu");
> 
> but as 'Anchor' element doesn't lose focus in test case, so it is getting
passed
> even without my change.
> But in the issue case 'Button' is losing focus, as context menu is not on the
> button.
> 
>
https://codereview.chromium.org/766143002/diff/40001/Source/core/page/EventHa...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/766143002/diff/40001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:2790: shouldTranslateToRootView = true;
> On 2014/12/10 03:09:25, Rick Byers wrote:
> > by the way, you don't need this variable at all anymore, right?  It would be
> > much simpler to just keep 'location' always in root-view co-ordinate space
> > (maybe even call it rootViewLocation) and just add the
> view->contentsToRootView
> > call here.
> 
> Done.

[Deepak, 2014-12-19 10:42:14]

On 2014/12/18 22:28:23, Rick Byers wrote:
> On 2014/12/10 06:40:27, Deepak wrote:
> > PTAL at changes and comments
> > Thanks
> > 
> >
>
https://codereview.chromium.org/766143002/diff/40001/LayoutTests/fast/events/...
> > File LayoutTests/fast/events/menu-key-context-menu-element-focus.html
(right):
> > 
> >
>
https://codereview.chromium.org/766143002/diff/40001/LayoutTests/fast/events/...
> > LayoutTests/fast/events/menu-key-context-menu-element-focus.html:18:
if(count
> > === 2 && (e.currentTarget == anchorNode)) {
> > On 2014/12/10 03:09:24, Rick Byers wrote:
> > > Does this test actually fail without your patch? 
currentTarget==anchorNode
> > > should always be true since the event handler was added on the anchorNode
> > > directly.
> >  No , This test is not failing without my changes.
> > 
> > > I guess I don't understand why menu-key-context-menu.html isn't already
> > covering
> > > everything you're testing here.  What's fundamentally different about this
> > test?
> >  Actually when you load  http://jsfiddle.net/kvckb90w/2/show/
> >  * Use tab key to bring focus on one of the buttons
> >  * Hit the context menu key
> > Then context menu does not come on the button but off the button and on
> > selecting 'esc' key, Button will lose focus and then
> >  again selecting 'Context menu' key context menu will come at the top left
> > corner and context menu items are also different.
> 
> Right, that makes sense as a side effect of the root problem here.  The root
> problem is that the 'contextmenu' event is sent with the wrong co-ordinates
and
> so hits the wrong node.  After you dismiss the contextmenu, focus has changed
to
> the point where the contextmenu event was dispatched (not exactly sure why,
but
> it's not important).  You should be testing the core issue, not the side
effect
> of what happens after 'esc' causes the context menu to close.  Eg. if someone
> broke the code so that the contextmenu key did absolutely nothing at all, then
> your test would still pass - that's no good.
 I agree with you.

> The existing menu-key-context-menu.html test has the right basic idea here. 
It
> puts a contextmenu event handler on the node we expect the mouse event to
reach
> and verifies that the handler gets invoked.

 I agree with you. I have done changes as suggested.

> But your test gave me the answer I was looking for - the existing test works
> fine because the problem is limited to iframes (when there's no iframe
involved
> then view==rootView so no transformation is required, whenever view!=rootView
> then we'll hit the bug).  That's the key to this bug - contextmenu events in
> iframes are sent to the wrong location (I've updated the CL description to
> clarify that).
> 
> So instead of testing the activeElement, please put a contextmenu event
listener
> in the iframe and verify it gets invoked with the correct 'target' element,
and
> with the correct clientX/clientY co-ordinates.  That should fail very
obviously
> without your patch (clientY being off by exactly the distance between the
iframe
> and main document).
  I have done changes as suggested. 
> You can see an example of testing events in iframes at
> LayoutTests/fast/events/touch/gesture/gesture-tap-frame-scrollbar.html - note
> that the iframe (event-delegator.html) installs an event listener that just
> calls 'parent.onEventInIframe', and the main page implements that function to
> log and check the results of the event.

  Done Changes as suggested, but I have to use frameset as using iframe, facing
issues in 
  button focusing as it is must for initial condition for reproducing this
issue. So used frames
  that anyways achieve our objective.
  Now I am checking the element on which event get fired as well as the event
co-ordinates
  at which event is fired.This make clear our test.
  Thanks
  PTAL

>  
> > But I am surprised that in the test case doing
> > document.activeElement after hitting 'esc' giving still 'anchor' element as
> > focused element.
> > 
> > > From the code, I would have expected that you'd need to scroll the page in
> > order
> > > to provoke the bug.  What if you just change menu-key-context-menu.html to
> add
> > > some padding at the bottom of the document and call window.scrollTo(0,
30). 
> > > That way the root view co-ordinate space and the contents co-ordinate
space
> > are
> > > different.  If do that, does the test start failing without your patch
(but
> > > still pass with your patch)? 
> >  I tried this, But still the test case is getting passed, as 'Anchor'
element
> is
> > not losing focus after scroll.
> > 
> > I have added a padding in para as:
> > <style>
> > p.ex1 {
> >     padding: 100px 100px;
> > }
> > </style>
> > 
> > <a href="" id="anchor" oncontextmenu="handleContextMenu(event);">Test</a>
> > <div id="result">FAIL</div>
> > <p class="ex1">Padding.</p>
> > 
> > and call as:
> > 
> >  anchor.focus();
> >  eventSender.keyDown("menu");
> >  eventSender.keyDown("escape");
> >  window.scroll(0,200);
> >  eventSender.keyDown("menu");
> > 
> > but as 'Anchor' element doesn't lose focus in test case, so it is getting
> passed
> > even without my change.
> > But in the issue case 'Button' is losing focus, as context menu is not on
the
> > button.
> > 
> >
>
https://codereview.chromium.org/766143002/diff/40001/Source/core/page/EventHa...
> > File Source/core/page/EventHandler.cpp (right):
> > 
> >
>
https://codereview.chromium.org/766143002/diff/40001/Source/core/page/EventHa...
> > Source/core/page/EventHandler.cpp:2790: shouldTranslateToRootView = true;
> > On 2014/12/10 03:09:25, Rick Byers wrote:
> > > by the way, you don't need this variable at all anymore, right?  It would
be
> > > much simpler to just keep 'location' always in root-view co-ordinate space
> > > (maybe even call it rootViewLocation) and just add the
> > view->contentsToRootView
> > > call here.
> > 
> > Done.

[Rick Byers, 2014-12-19 15:14:28]

Thanks, now the test makes sense!  Just a couple minor things left...

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

https://codereview.chromium.org/766143002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:3: <head>
Nit: omit html and head tags.  See
http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h...

https://codereview.chromium.org/766143002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:11: if (e.target ==
anchorNode && e.clientX === x && e.clientY === y)
this is what the 'shouldBe' helpers are for in js-test.js.  So this should
become something like:

var event;
function onEventInIframe(e) {
  shouldBe("event.target", "anchorNode");
  shouldBe("event.clientX", "expectedX");
  shouldBe("event.clientY", "expectedY");
}

Also, once you're using js-test.js, please add a 'description()' call.  See, for
example, fast/events/touch/gesture/gesture-tap-div-removed.html for a layout
test that follows the style expected for new tests.

https://codereview.chromium.org/766143002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:29: // Esc key to
hide context menu
Any reason to do this twice?  I'd suggest removing the below two lines (and
looking only at the first contextmenu event) - if I understand the problem
properly, they don't add anything to the test...

https://codereview.chromium.org/766143002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:36: <frameset
rows="30%,40%" onload="runTest()">
perhaps the frameset is necessary just to get "myframe" to be positioned
somewhere other than (0,0)?  I'd expect this to work just as well and be
simpler:

<iframe id="myframe" style="margin-top: 50px;" onload="runTest()"
src="resources/menu-key-context-menu-position-frame.html">

[Deepak, 2014-12-20 09:35:33]

1.Even while putting description() in the testcase, it is not appearing in the
expected result file.
2.while using iframes in test case getting Uncaught ReferenceError , so have to
use frames only.

Rest of the changes done as suggested.
PTAL

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

https://codereview.chromium.org/766143002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:3: <head>
On 2014/12/19 15:14:27, Rick Byers (Out until 12-5) wrote:
> Nit: omit html and head tags.  See
>
http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h...

Done.

https://codereview.chromium.org/766143002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:11: if (e.target ==
anchorNode && e.clientX === x && e.clientY === y)
On 2014/12/19 15:14:27, Rick Byers (Out until 12-5) wrote:
> this is what the 'shouldBe' helpers are for in js-test.js.  So this should
> become something like:
> 
> var event;
> function onEventInIframe(e) {
>   shouldBe("event.target", "anchorNode");
>   shouldBe("event.clientX", "expectedX");
>   shouldBe("event.clientY", "expectedY");
> }
I have tried to use description() but I am not able to get that in the expected
results files.
> Also, once you're using js-test.js, please add a 'description()' call.  See,
for
> example, fast/events/touch/gesture/gesture-tap-div-removed.html for a layout
> test that follows the style expected for new tests.

https://codereview.chromium.org/766143002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:29: // Esc key to
hide context menu
On 2014/12/19 15:14:27, Rick Byers (Out until 12-5) wrote:
> Any reason to do this twice?  I'd suggest removing the below two lines (and
> looking only at the first contextmenu event) - if I understand the problem
> properly, they don't add anything to the test...

Done.

https://codereview.chromium.org/766143002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:36: <frameset
rows="30%,40%" onload="runTest()">
On 2014/12/19 15:14:27, Rick Byers (Out until 12-5) wrote:
> perhaps the frameset is necessary just to get "myframe" to be positioned
> somewhere other than (0,0)?  I'd expect this to work just as well and be
> simpler:
> 
> <iframe id="myframe" style="margin-top: 50px;" onload="runTest()"
> src="resources/menu-key-context-menu-position-frame.html">

I am getting uncaught reference error like below 
when I have iframes in Frameset. OR I tried to used iframes without frameset.
"CONSOLE ERROR: line 38: Uncaught ReferenceError: runTest is not defined
PASS successfullyParsed is true"
So I have to use frames only.

[Deepak, 2014-12-22 12:44:16]

description() issue resolved.
I have updated changes.
PTAL.

[Deepak, 2014-12-23 05:45:47]

On 2014/12/22 12:44:16, Deepak wrote:
> description() issue resolved.
> I have updated changes.
> PTAL.

If I use just iframes then I am getting warnings in shouldbe() like
WARN: shouldBe() expects string arguments
PASS [object HTMLInputElement] is [object HTMLInputElement]
WARN: shouldBe() expects string arguments
PASS 33 is 33.
But with using frames usage it is working fine.

[Deepak, 2015-01-05 11:36:40]

On 2014/12/23 05:45:47, Deepak wrote:
> On 2014/12/22 12:44:16, Deepak wrote:
> > description() issue resolved.
> > I have updated changes.
> > PTAL.
> 
> If I use just iframes then I am getting warnings in shouldbe() like
> WARN: shouldBe() expects string arguments
> PASS [object HTMLInputElement] is [object HTMLInputElement]
> WARN: shouldBe() expects string arguments
> PASS 33 is 33.
> But with using frames usage it is working fine.

PTAL

[Rick Byers, 2015-01-05 18:16:24]

Sorry for the delay, I was out of the office for Christmas / New Years vacation
(hopefully you saw this in my username string).

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
File LayoutTests/fast/events/menu-key-context-menu-position-expected.txt
(right):

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position-expected.txt:5: 
If the test was actually working, you'd have 'PASS ...' entries here indicating
that the shouldBe calls succeeded.  Somehow your test isn't actually running -
maybe the onload is never called?

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

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:13:
shouldBe(e.target, anchorNode);
shouldBe takes expression strings to be evaluated, not values.  This is why I
wrote examples like this:
  shouldBe("event.target", "anchorNode");

One benefit here is that you probably don't really want the value of "e.clientX"
appearing in the test output - it might be slightly different on different
platforms or change for unrelated reasons.  You don't care what the value is,
just that it matches what the test expects.

I'm surprised to hear you get the "not a string" warnings only when using
iframes - I would have expected you to see those warnings unconditionally. 
Probably suggests something is broken with the test output in the frameset case.

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
File LayoutTests/fast/events/resources/menu-key-context-menu-position-frame.html
(right):

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/resources/menu-key-context-menu-position-frame.html:2:
<body>
nit: you can omit the html/body tags here too.

[Deepak, 2015-01-06 13:27:02]

*Now PASS for should be check is coming fine for all 3 comparisions.
*iframes are used instead of frameset.
PTAL and Please let me know if I missed something.
Thanks

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
File LayoutTests/fast/events/menu-key-context-menu-position-expected.txt
(right):

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position-expected.txt:5: 
On 2015/01/05 18:16:24, Rick Byers wrote:
> If the test was actually working, you'd have 'PASS ...' entries here
indicating
> that the shouldBe calls succeeded.  Somehow your test isn't actually running -
> maybe the onload is never called?

You are right, runTest was not getting called, so we are not getting PASS for
shouldbe()
This is due to frameset in body.

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

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:13:
shouldBe(e.target, anchorNode);
On 2015/01/05 18:16:24, Rick Byers wrote:
> shouldBe takes expression strings to be evaluated, not values.  This is why I
> wrote examples like this:
>   shouldBe("event.target", "anchorNode");
> 
> One benefit here is that you probably don't really want the value of
"e.clientX"
> appearing in the test output - it might be slightly different on different
> platforms or change for unrelated reasons.  You don't care what the value is,
> just that it matches what the test expects.
> 
> I'm surprised to hear you get the "not a string" warnings only when using
> iframes - I would have expected you to see those warnings unconditionally. 
> Probably suggests something is broken with the test output in the frameset
case.

Earlier the issue is we can not use frameset inside body tag. I have removed
body tag as well as frameset.
and using iframes as you suggested earlier.

Now I am using menu-key-context-menu-document.html as reference for comparing
event.target.id, 
and I need to convert event.clientX/Y to String() so that it can be used in the
shouldbe.
Now I am getting PASS for all 3 shouldbe() calls as shown in the expected
result.

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
File LayoutTests/fast/events/resources/menu-key-context-menu-position-frame.html
(right):

https://codereview.chromium.org/766143002/diff/180001/LayoutTests/fast/events...
LayoutTests/fast/events/resources/menu-key-context-menu-position-frame.html:2:
<body>
On 2015/01/05 18:16:24, Rick Byers wrote:
> nit: you can omit the html/body tags here too.

Done.

[Deepak, 2015-01-06 15:07:06]

*event.target.id I am compairing same way as happening in
menu-key-context-menu-document.html.
*I have to take separate variable clientX and clientY, This will hide numeric
values from output file, As doing 
shouldBe("event.clientX","X");
shouldBe("event.clientY","Y");
is not working properly, Please tell me if their is some way to do this
directly.
Thanks

[Rick Byers, 2015-01-07 16:57:34]

Great, looks like the test is working properly now!  Just a couple more minor
style issues.

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

https://codereview.chromium.org/766143002/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:12:
shouldBe(event.target.id, 'inputNode');
this should be a string as well.  Remember the arguments here are ALL
"expression strings".  Otherwise you get the nonsense test output like "PASS:
inputNode is inputNode" (well duh, of course it is!).

A simple way to do this is to make 'event' a global.  Eg.

var event;
function onEventInFrame(e) {
  event = e;
  shouldBe("event.id", "inputNode");
  shouldBe("event.clientX", "X");
  ...
}

https://codereview.chromium.org/766143002/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:31: <body>
nit: no need for a body tag:
http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h...

https://codereview.chromium.org/766143002/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:32: <iframe
src="resources/window-opened.html"></iframe>
what is this other iframe for?  You can probably remove it now, right?

[Deepak, 2015-01-08 10:11:29]

Thanks for review.Changes done as suggested.
PTAL.

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

https://codereview.chromium.org/766143002/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:12:
shouldBe(event.target.id, 'inputNode');
On 2015/01/07 16:57:34, Rick Byers wrote:
> this should be a string as well.  Remember the arguments here are ALL
> "expression strings".  Otherwise you get the nonsense test output like "PASS:
> inputNode is inputNode" (well duh, of course it is!).
> 
> A simple way to do this is to make 'event' a global.  Eg.
> 
> var event;
> function onEventInFrame(e) {
>   event = e;
>   shouldBe("event.id", "inputNode");
>   shouldBe("event.clientX", "X");
>   ...
> }

Done.

https://codereview.chromium.org/766143002/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:12:
shouldBe(event.target.id, 'inputNode');
On 2015/01/07 16:57:34, Rick Byers wrote:
> this should be a string as well.  Remember the arguments here are ALL
> "expression strings".  Otherwise you get the nonsense test output like "PASS:
> inputNode is inputNode" (well duh, of course it is!).
> 
> A simple way to do this is to make 'event' a global.  Eg.
> 
> var event;
> function onEventInFrame(e) {
>   event = e;
>   shouldBe("event.id", "inputNode");
>   shouldBe("event.clientX", "X");
>   ...
> }

Done.

https://codereview.chromium.org/766143002/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:31: <body>
On 2015/01/07 16:57:34, Rick Byers wrote:
> nit: no need for a body tag:
>
http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h...

Done.

https://codereview.chromium.org/766143002/diff/260001/LayoutTests/fast/events...
LayoutTests/fast/events/menu-key-context-menu-position.html:32: <iframe
src="resources/window-opened.html"></iframe>
On 2015/01/07 16:57:34, Rick Byers wrote:
> what is this other iframe for?  You can probably remove it now, right?

Done.

[Deepak, 2015-01-09 13:02:39]

PTAL.

[Rick Byers, 2015-01-09 16:21:35]

LGTM

[Deepak, 2015-01-10 04:40:19]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-01-10 04:40:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/766143002/280001

[commit-bot: I haz the power, 2015-01-10 06:23:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-10 06:23:29]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

[Deepak, 2015-01-11 03:30:22]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-01-11 03:30:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/766143002/280001

[commit-bot: I haz the power, 2015-01-11 11:26:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-11 11:26:19]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/38165)

[Deepak, 2015-01-12 09:22:39]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-01-12 09:23:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/766143002/280001

[Deepak, 2015-01-12 10:07:15]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-01-12 10:28:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/766143002/300001

[commit-bot: I haz the power, 2015-01-12 11:37:52]

Committed patchset #16 (id:300001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188215