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.
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.
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.
Created attachment 59925 [details] Patch
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.
Created attachment 59941 [details] Patch Fixed style issues
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?
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.
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.
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.
Created attachment 60420 [details] Patch for landing
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. ojan@chromium.org 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).
+ Reviewed by ojan@chromium.org. 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.
Created attachment 60449 [details] Patch for landing
Comment on attachment 60449 [details] Patch for landing Clearing flags on attachment: 60449 Committed r62447: <http://trac.webkit.org/changeset/62447>
All reviewed patches have been landed. Closing bug.
Thanks Adam