Bug 38129 – Wrong target for contextmenu events

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

Created attachment 54316 [details]
Test case

On Windows, the user can press the context menu key on their keyboard. This dispatches a MouseEvent of type 'contextmenu'. The reason that this is a MouseEvent instead of KeyEvent is mostly a legacy issue. The problem in WebKit is that we try to find the location where the lower left corner of the element is located and we dispatch and use that location in dispatching a mouse event. This causes the target of the event to be the element at that point as well as the element at that point gets painted as it is :active/:hover. The correct behavior is to use the focusedNode and we should paint the focusedNode as active.

The solution is probably to split EventHandler::sendContextMenuEvent into two cases, one for mouse events and one for key events. Reusing the mouse event code path for key events just leads to too much trouble.

[Erik Arvidsson]

I got a solution for this that solves the invalid target and the invalid focus/active artifact but the position of the context menu is still wrong. I need to find out the size of the clipped rect.

[Erik Arvidsson]

I have a working fix for this. The thing holding me back has been that the code is only for Chromium on Windows. No other port seems to support the context menu key. Another problem is that DRT does not support this so it would end up being a manual test only.

[Erik Arvidsson]

Created attachment 59925 [details]
Patch

[WebKit Review Bot]

Attachment 59925 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/page/EventHandler.cpp:2026:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2027:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2028:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2031:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2032:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2035:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2036:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2037:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2038:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2039:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2040:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/page/EventHandler.cpp:2042:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 12 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Erik Arvidsson]

Created attachment 59941 [details]
Patch

Fixed style issues

[Ojan Vafai]

Comment on attachment 59941 [details]
Patch

WebCore/manual-tests/chromium/contextmenu-key.html:76
 +      function cs(el)
Nit: this function should not be indented.

WebCore/ChangeLog:14
 +          Use manual test since DRT does not handle context menu keys.
Can you file a bug for adding this?

WebCore/page/EventHandler.cpp:2027
 +      FrameView* v = m_frame->view();
Doesn't look like you use v anywhere. Also, do you need to null-check document?

WebKit/chromium/ChangeLog:10
 +          clipped rect to determin eht position of the menu.
Typos: determin eht

WebCore/page/EventHandler.cpp:2024
 +  bool EventHandler::sendContextMenuEventForKey(const PlatformMouseEvent& event)
It's weird to me that this takes a mouse event. I think this should take a PlatformKeyEvent and then dispatch a mouse event below.

WebCore/page/EventHandler.cpp:2042
 +      return dispatchMouseEvent(eventNames().contextmenuEvent, targetNode, true, 0, event, false);
Can you add a comment here that we need to dispatch a mouse event for web compat?

WebKit/chromium/src/WebViewImpl.cpp: 
 +      coords = location + IntSize(0, -1);
This causes us to diverge from the WebKit/win implementation. Is that correct?

[Erik Arvidsson]

Created attachment 60410 [details]
Patch

Moved most of the logic into EventHandler::sendContextMenuEventForKey and updated WebKit/win/ and WebKit/chromium to use that so that they share code.

[Ojan Vafai]

Comment on attachment 60410 [details]
Patch

Nice cleanup. A few minor things. Feel free to commit after that.
---------------------------------
http://wkrietveld.appspot.com/38129/diff/2001/3001
File WebCore/ChangeLog (right):

http://wkrietveld.appspot.com/38129/diff/2001/3001#newcode12
WebCore/ChangeLog:12: +        For the keboard case We now use the focused node as the target for the
s/We/we

http://wkrietveld.appspot.com/38129/diff/2001/3003
File WebCore/page/EventHandler.cpp (right):

http://wkrietveld.appspot.com/38129/diff/2001/3003#newcode2019
WebCore/page/EventHandler.cpp:2019: swallowEvent = dispatchMouseEvent(eventNames().contextmenuEvent, mev.targetNode(), true, 0, event, false);
Does this match the behavior of other Windows browsers? If so, that's fine.

http://wkrietveld.appspot.com/38129/diff/2001/3006
File WebKit/chromium/src/WebViewImpl.cpp (right):

http://wkrietveld.appspot.com/38129/diff/2001/3006#newcode668
WebKit/chromium/src/WebViewImpl.cpp:668: FrameView* view = m_page->mainFrame()->view();
Do you need to null-check view here?

http://wkrietveld.appspot.com/38129/diff/2001/3006#newcode672
WebKit/chromium/src/WebViewImpl.cpp:672: Frame* focusedFrame = page()->focusController()->focusedOrMainFrame();
Nit: This should move down to be before line 683. Since it's not used till then.

[Erik Arvidsson]

Comment on attachment 60410 [details]
Patch

Thanks for the reviews.
---------------------------------
http://wkrietveld.appspot.com/38129/diff/2001/3001
File WebCore/ChangeLog (right):

http://wkrietveld.appspot.com/38129/diff/2001/3001#newcode12
WebCore/ChangeLog:12: +        For the keboard case We now use the focused node as the target for the
On 2010/07/02 23:14:56, ojan wrote:
> s/We/we

Done.

http://wkrietveld.appspot.com/38129/diff/2001/3003
File WebCore/page/EventHandler.cpp (right):

http://wkrietveld.appspot.com/38129/diff/2001/3003#newcode2019
WebCore/page/EventHandler.cpp:2019: swallowEvent = dispatchMouseEvent(eventNames().contextmenuEvent, mev.targetNode(), true, 0, event, false);
On 2010/07/02 23:14:56, ojan wrote:
> Does this match the behavior of other Windows browsers? If so, that's fine.

Yes. I tried it on Firefox and IE.

http://wkrietveld.appspot.com/38129/diff/2001/3006
File WebKit/chromium/src/WebViewImpl.cpp (right):

http://wkrietveld.appspot.com/38129/diff/2001/3006#newcode668
WebKit/chromium/src/WebViewImpl.cpp:668: FrameView* view = m_page->mainFrame()->view();
On 2010/07/02 23:14:56, ojan wrote:
> Do you need to null-check view here?

Removed here and in WebKit/win/

http://wkrietveld.appspot.com/38129/diff/2001/3006#newcode672
WebKit/chromium/src/WebViewImpl.cpp:672: Frame* focusedFrame = page()->focusController()->focusedOrMainFrame();
On 2010/07/02 23:14:56, ojan wrote:
> Nit: This should move down to be before line 683. Since it's not used till then.

Done.

[Erik Arvidsson]

Created attachment 60420 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 60420 [details]
Patch for landing

Rejecting patch 60420 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 60420, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
?id=60420&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=38129&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 60420 from bug 38129.
[e-mail] found in /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Adam Barth]

+ Reviewed by [e-mail].

Should be "Reviewed by Ojan Vafai".  If you posted the patch with "webkit-patch land-safely", the tool will fill in the reviewer's name for you if the bug has an R+'ed patch.

[Adam Barth]

Created attachment 60449 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 60449 [details]
Patch for landing

Clearing flags on attachment: 60449

Committed r62447: <http://trac.webkit.org/changeset/62447>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Erik Arvidsson]

Thanks Adam