Bug 72099 – [WK2] Keyboard menu key should show context menu

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[chandra shekar vallala]

Context menu should display on menu key press.

[chandra shekar vallala]

Created attachment 114646 [details]
patch

[Alexey Proskuryakov]

Is it right to handle a raw keyboard event, or should be just handle WM_CONTEXTMENU when it comes?

[chandra shekar vallala]

(In reply to comment #2)
> Is it right to handle a raw keyboard event, or should be just handle WM_CONTEXTMENU when it comes?

In my debugging I found that Qt, Win & GTK platforms do have a mechanism of informing about context menu action from KB, but I am not sure about other ports of WebKit2. Since I am not aware of such mechanism in other ports, I went for a generic design which works for all ports and which is already being used in case of mouse right click events.

[Adam Roben (:aroben)]

Comment on attachment 114646 [details]
patch

I agree that it seems strange to be interpreting the keyboard event directly rather than letting the system translate it into a context menu event for us.

[Carlos Garcia Campos]

The patch works fine for GTK port, it shows the context menu when using context menu key or shift + F10, but there's a problem. Even though the focused node or current selection is used to determine the position of the menu, the context is not created for the focused node nor the selection. That's because the context menu uses the location given in the button event to make another hit test, and that location is under the focused node or selection (where the menu will be displayed).

[Carlos Garcia Campos]

*** Bug 75380 has been marked as a duplicate of this bug. ***

[chandra shekar vallala]

(In reply to comment #5)
> The patch works fine for GTK port, it shows the context menu when using context menu key or shift + F10, but there's a problem. Even though the focused node or current selection is used to determine the position of the menu, the context is not created for the focused node nor the selection. That's because the context menu uses the location given in the button event to make another hit test, and that location is under the focused node or selection (where the menu will be displayed).

It is a bug in EventHandler. EventHandler calculates a point(used to create a platforMouseEvent) using RenderBoxModelObject.absoluteClippedOverflowRect which might give wrong position values inside EventHandler::sendContextMenuEventForKey().

[Early Warning System Bot]

Comment on attachment 114646 [details]
patch

Attachment 114646 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11861094

[chandra shekar vallala]

Created attachment 144798 [details]
updated-patch

This patch allows the browser to show context menu when the CONTEXTMENU event comes

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Build Bot]

Comment on attachment 144798 [details]
updated-patch

Attachment 144798 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12859002

[Martin Robinson]

Comment on attachment 144798 [details]
updated-patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144798&action=review

Looks alright apart from a few small issues. r- because of the build break though.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:468
> +    WebKitWebViewBase* webViewBase = WEBKIT_WEB_VIEW_BASE(widget);
> +    WebKitWebViewBasePrivate* priv = webViewBase->priv;
> +#if ENABLE(CONTEXT_MENUS)
> +    priv->pageProxy->handleContextMenuKeyEvent();
> +#endif // ENABLE(CONTEXT_MENUS)

I think it makes sense to just write:

#if ENABLE(CONTEXT_MENUS)
WEBKIT_WEB_VIEW_BASE(widget)->priv->pageProxy->handleContextMenuKeyEvent();
#endif

Shouldn't you return true here in side the #ifdef?

[chandra shekar vallala]

Created attachment 145216 [details]
updated-patch

Updated patch as per comments.

[Carlos Garcia Campos]

Comment on attachment 145216 [details]
updated-patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145216&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:467
> +#if ENABLE(CONTEXT_MENUS)
> +    WEBKIT_WEB_VIEW_BASE(widget)->priv->pageProxy->handleContextMenuKeyEvent();
> +    return TRUE;
> +#endif // ENABLE(CONTEXT_MENUS)

Isn't context menu always enabled when building gtk port?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:619
> +    widgetClass->popup_menu = webkitWebViewPopupMenuHandler;

We use the pattern classNameSignalName, so this should be webkitWebViewBasePopupMenu()

[Build Bot]

Comment on attachment 145216 [details]
updated-patch

Attachment 145216 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12862508

[chandra shekar vallala]

Created attachment 145224 [details]
updated-patch

Updated patch as per comments.

[chandra shekar vallala]

(In reply to comment #14)
> (From update of attachment 145216 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145216&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:467
> > +#if ENABLE(CONTEXT_MENUS)
> > +    WEBKIT_WEB_VIEW_BASE(widget)->priv->pageProxy->handleContextMenuKeyEvent();
> > +    return TRUE;
> > +#endif // ENABLE(CONTEXT_MENUS)
> 
> Isn't context menu always enabled when building gtk port?
Yes, Enabled always. Gtk-port might give errors when Context menu is disabled.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:619
> > +    widgetClass->popup_menu = webkitWebViewPopupMenuHandler;
> 
> We use the pattern classNameSignalName, so this should be webkitWebViewBasePopupMenu()
Done.

[Carlos Garcia Campos]

(In reply to comment #17)
> (In reply to comment #14)
> > (From update of attachment 145216 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145216&action=review
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:467
> > > +#if ENABLE(CONTEXT_MENUS)
> > > +    WEBKIT_WEB_VIEW_BASE(widget)->priv->pageProxy->handleContextMenuKeyEvent();
> > > +    return TRUE;
> > > +#endif // ENABLE(CONTEXT_MENUS)
> > 
> > Isn't context menu always enabled when building gtk port?
> Yes, Enabled always. Gtk-port might give errors when Context menu is disabled.
> 

So we don't need the #if then, no?

[Martin Robinson]

(In reply to comment #18)

> So we don't need the #if then, no?

I believe Chandra recently submitted some patches that fix the build with CONTEXT_MENUS=0. Even if a feature is typically on in GTK+, it makes sense to allow people to maintain custom flags themselves IMO. Theoretically all combinations of flags should build.

[Carlos Garcia Campos]

(In reply to comment #19)
> (In reply to comment #18)
> 
> > So we don't need the #if then, no?
> 
> I believe Chandra recently submitted some patches that fix the build with CONTEXT_MENUS=0. Even if a feature is typically on in GTK+, it makes sense to allow people to maintain custom flags themselves IMO. Theoretically all combinations of flags should build.

Fair enough, in that case I prefer to not add an impl for popup menu when it's not enabled

[chandra shekar vallala]

Created attachment 145573 [details]
updated patch

I removed CC from WebKitWebViewBase.

[Anders Carlsson]

Comment on attachment 145573 [details]
updated patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Tomas Popela]

Created attachment 284771 [details]
Proposed patch

I rebased and updated the original patch on top of the current master and fixed the issue when no context menu was displayed if no element was focused.

[Michael Catanzaro]

Comment on attachment 284771 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284771&action=review

Ah great, I noticed this was broken, it's nice to have a patch.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:827
> +    if ((event->keyval == GDK_KEY_Menu) || (((event)->state & GDK_SHIFT_MASK) && GDK_KEY_F10))

Remove the extra parens around event here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:4567
> +void WebPageProxy::handleContextMenuKeyEvent()

Carlos, owners, do you think we should use PLATFORM(GTK) guards here, since it's only used by GTK? It's not really platform specific, but only used by a single platform, so I'm not sure.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2157
> +    Frame& frame = m_page->focusController().focusedOrMainFrame();

You have to adjust the #ifdefs here to avoid the unused variable build failure on Mac. You should guard the entire implementation of the function with #if !PLATFORM(MAC).

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2160
> +#if !PLATFORM(MAC)

But TBH I'm not sure if it's the right #if, we have iOS nowadays after all. Maybe !PLATFORM(COCOA)? Or maybe put the whole function in a PLATFORM(GTK) guard? Let's see what Carlos and Apple say to use here.

[Darin Adler]

Comment on attachment 284771 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284771&action=review

> Source/WebCore/page/EventHandler.cpp:2376
> -    m_elementUnderMouse = targetElement;
> +    if (fireMouseOverOut || targetElement)
> +        m_elementUnderMouse = targetElement;

This change is mysterious. The comment in the change log above basically says “we need to do it this way so the code works”, but that is not enough. Leaving an element set as m_elementUnderMouse seems like it would be incorrect, even if it does make this bug go away. If you want to make this change please explain further how you researched further and thought it through and why this change is correct for all cases, beyond the fact that it makes the bug go away in the case we are fixing here. Also, after thinking it through we can figure out what test cases help us verify the change is correct or at least harmless.

[Carlos Garcia Campos]

(In reply to comment #24)
> Comment on attachment 284771 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284771&action=review
> 
> Ah great, I noticed this was broken, it's nice to have a patch.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:827
> > +    if ((event->keyval == GDK_KEY_Menu) || (((event)->state & GDK_SHIFT_MASK) && GDK_KEY_F10))
> 
> Remove the extra parens around event here.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:4567
> > +void WebPageProxy::handleContextMenuKeyEvent()
> 
> Carlos, owners, do you think we should use PLATFORM(GTK) guards here, since
> it's only used by GTK? It's not really platform specific, but only used by a
> single platform, so I'm not sure.

Cross-platform code that builds fine in all ports and doesn't cause other ports to do any extra work, like in this case, shouldn't have any ifdef in my opinion. 

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2157
> > +    Frame& frame = m_page->focusController().focusedOrMainFrame();
> 
> You have to adjust the #ifdefs here to avoid the unused variable build
> failure on Mac. You should guard the entire implementation of the function
> with #if !PLATFORM(MAC).
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2160
> > +#if !PLATFORM(MAC)
> 
> But TBH I'm not sure if it's the right #if, we have iOS nowadays after all.
> Maybe !PLATFORM(COCOA)? Or maybe put the whole function in a PLATFORM(GTK)
> guard? Let's see what Carlos and Apple say to use here.

I don't understand why there's a platform ifdef there.

[Carlos Garcia Campos]

Comment on attachment 284771 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284771&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests as this should be covered by existing context menu tests.

So, if it's covered by existing tests, what tests pass after this change? Can we unskip any test, or are those currently failing with no expectations?. I think it would be great for the GTK+ port if we add a test case for this to our current context menu unit tests.

>> Source/WebCore/page/EventHandler.cpp:2376
>> -    m_elementUnderMouse = targetElement;
>> +    if (fireMouseOverOut || targetElement)
>> +        m_elementUnderMouse = targetElement;
> 
> This change is mysterious. The comment in the change log above basically says “we need to do it this way so the code works”, but that is not enough. Leaving an element set as m_elementUnderMouse seems like it would be incorrect, even if it does make this bug go away. If you want to make this change please explain further how you researched further and thought it through and why this change is correct for all cases, beyond the fact that it makes the bug go away in the case we are fixing here. Also, after thinking it through we can figure out what test cases help us verify the change is correct or at least harmless.

I don't understand this change either . . .

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:828
> +        priv->contextMenuEvent.reset(gdk_event_copy(reinterpret_cast<GdkEvent*>(event)));

This is problematic. API users might expect the event to be a button event. This will cause ephy to crash for sure see:

embed_event = ephy_embed_event_new ((GdkEventButton *)event, hit_test_result);

It's true that in the documentation we say the event is the one that triggered the context menu, and in this case is a keyboard event, so it makes sense to use this. Another problem I see here is that you are using the default keybindings that trigger the popup-menu signal, but it could also be triggered by others, I think. I also wonder if we should prevent sending this key event to the web process, since it's going to be handled by context menu key event, or returning TRUE from webkitWebViewBasePopupMenu already prevents this?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:929
> +    priv->pageProxy->handleContextMenuKeyEvent();

I wonder if gtk_get_current_event() would return the key event that triggered this, and then we don't need to do that in webkitWebViewBaseKeyPressEvent. I think we really need a unit tests for all these things in GTK+.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:4567
>> +void WebPageProxy::handleContextMenuKeyEvent()
> 
> Carlos, owners, do you think we should use PLATFORM(GTK) guards here, since it's only used by GTK? It's not really platform specific, but only used by a single platform, so I'm not sure.

I don't think so.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2155
> +void WebPage::contextMenuKeyEvent()

I think we could use contextMenuForKeyEvent or something similar, for consistency with the webcore name.

[Tomas Popela]

I decided to move the issue of no context menu when no element to focused to another bug, to not block this one.

(In reply to comment #27)
> So, if it's covered by existing tests, what tests pass after this change?
> Can we unskip any test, or are those currently failing with no
> expectations?. I think it would be great for the GTK+ port if we add a test
> case for this to our current context menu unit tests.

I will test the popup-menu handling in TestContextMenu. Also there are manual tests that are testing this issue:

ManualTests/keyboard-menukey-event.html
ManualTests/win/contextmenu-key.html
ManualTests/win/contextmenu-key2.html
 
> I also wonder if we should prevent
> sending this key event to the web process, since it's going to be handled by
> context menu key event, or returning TRUE from webkitWebViewBasePopupMenu
> already prevents this?

Actually the key-press-event is emitted before the popup-menu one so we don't need to handle this at all.

[Tomas Popela]

Created attachment 290188 [details]
Proposed patch

[Michael Catanzaro]

Comment on attachment 290188 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290188&action=review

It introduces a linker error on Mac, r- for that. You should investigate why you can't use WebCore::EventHandler::sendContextMenuEventForKey in WebPage.cpp.

> Source/WebCore/ChangeLog:23
> +        the only right way if some element was focused on the page. If there

the only right way -> only the right way

> Source/WebCore/page/EventHandler.cpp:2825
> +        // Display the context menu in the middle of focused element as Chromium does.
> +        location = IntPoint(clippedRect.x() + clippedRect.width() / 2, clippedRect.y() + clippedRect.height() / 2);

FWIW I really don't think this is the desired behavior, in GTK+ apps the context menu is displayed with the pointer at one of the four corners, depending on how much space is available on the screen. But this is not a big deal, and centering the menu is certainly easier, I suppose....

[Tomas Popela]

Created attachment 290201 [details]
Fix the build on Mac

I addressed the review comments from Michael (thanks!) and added a comment why I centered the menu in the middle of element.

[Carlos Garcia Campos]

Comment on attachment 290201 [details]
Fix the build on Mac

View in context: https://bugs.webkit.org/attachment.cgi?id=290201&action=review

I think this should be testable. It would be better if you split the patch in two, one for the webcore only changes with a layout test that checks that the context menu is shown when triggered by the keyboard, checking all the cases: focused node, selected content, no focused element, etc. This test should probably be skipped initially for GTK+ until the other patch lands. And another patch for the GTK+ specific part.

> Source/WebCore/ChangeLog:27
> +        Correctly send the mouse event that used for showing the context menu.
> +        Previously the event was immediately dispatched as it is, but this was
> +        only the right way if some element was focused on the page. If there
> +        was no focused element or non-empty text range then the event lacked
> +        the right node, where it was supposed to be shown. The correct node
> +        is determined and added to the event in the sendContextMenuEvent() so
> +        we have to use this function to send the event.

So the thing is, are we sure we also want to do all other things done by sendContextMenuEvent() in the keyboard case?

> Source/WebCore/page/EventHandler.cpp:2823
> +        // FIXME On WebKit2 this returns quite a bigger rectangle than the element

Is this really a problem specific to WebKit2? Use : after FIXME.

> Source/WebCore/page/EventHandler.cpp:2859
> -    return !dispatchMouseEvent(eventNames().contextmenuEvent, targetNode, true, 0, platformMouseEvent, false);
> +    return sendContextMenuEvent(platformMouseEvent);

My guess is that what we really need here is calling Document::prepareMouseEvent() so maybe we can do that here instead of reusing sendContextMenuEvent(), because I'm not sure we want to do all other tings done there.

[Tomas Popela]

(In reply to comment #32)
> I think this should be testable. It would be better if you split the patch
> in two, one for the webcore only changes with a layout test that checks that
> the context menu is shown when triggered by the keyboard, checking all the
> cases: focused node, selected content, no focused element, etc. This test
> should probably be skipped initially for GTK+ until the other patch lands.
> And another patch for the GTK+ specific part.

Sounds good, I will create it.

> > Source/WebCore/ChangeLog:27
> > +        Correctly send the mouse event that used for showing the context menu.
> > +        Previously the event was immediately dispatched as it is, but this was
> > +        only the right way if some element was focused on the page. If there
> > +        was no focused element or non-empty text range then the event lacked
> > +        the right node, where it was supposed to be shown. The correct node
> > +        is determined and added to the event in the sendContextMenuEvent() so
> > +        we have to use this function to send the event.
> 
> So the thing is, are we sure we also want to do all other things done by
> sendContextMenuEvent() in the keyboard case?
> 
> > Source/WebCore/page/EventHandler.cpp:2859
> > -    return !dispatchMouseEvent(eventNames().contextmenuEvent, targetNode, true, 0, platformMouseEvent, false);
> > +    return sendContextMenuEvent(platformMouseEvent);
> 
> My guess is that what we really need here is calling
> Document::prepareMouseEvent() so maybe we can do that here instead of
> reusing sendContextMenuEvent(), because I'm not sure we want to do all other
> tings done there.

I'm not sure about this one, but I would trust the Chromium devs (that actually wrote that WebKit code) and they are using sendContextMenuEvent() - https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/EventHandler.cpp?rcl=f3a4c639191b2bd7b43581550b970e50d0f188d0&l=1906

[Tomas Popela]

Created attachment 302256 [details]
Patch

Add a new layout test, that is skipped on Mac. Also now is the context menu correctly positioned if shown on focused element (and not centered inside it as previously)

[Build Bot]

Comment on attachment 302256 [details]
Patch

Attachment 302256 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3166107

New failing tests:
fast/events/context-activated-by-key-event.html

[Build Bot]

Created attachment 302263 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Tomas Popela]

Created attachment 302928 [details]
Patch

Skip test on ios-simulator-wk2

[Tomas Popela]

Ping reviewers..

[Carlos Garcia Campos]

Comment on attachment 302928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302928&action=review

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

Normally the reviewed by line goes before the explanation.

> Source/WebKit2/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1622
> +     * a #GdkEventButton of type #GDK_BUTTON_PRESS when the context menu

#GDK_BUTTON_PRESS -> %GDK_BUTTON_PRESS

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1625
> +     * a #GdkEventKey of type #GDK_KEY_PRESS if the keyboard was used to show

#GDK_KEY_PRESS -> %GDK_KEY_PRESS

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1626
> +     * the menu/

menu/ -> menu.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1629
> +     * a generic #GdkEvent of type #GDK_NOTHING when the #GtkWidget:popup-menu

#GDK_NOTHING -> %GDK_NOTHING

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:484
> +    // Load the test HTML again to test the popup-menu signal handling.
> +    test->loadHtml(linksHTML, "file:///");
> +    test->waitUntilLoadFinished();
> +
> +    test->m_expectedMenuType = ContextMenuDefaultTest::Selection;
> +    test->showContextMenuTriggeredByPopupEventAndWaitUntilFinished();
> +
> +    // And for the last time to test the context menu key.
> +    test->loadHtml(linksHTML, "file:///");
> +    test->waitUntilLoadFinished();
> +
> +    test->m_expectedMenuType = ContextMenuDefaultTest::Selection;
> +    test->showContextMenuTriggeredByContextMenuKeyAndWaitUntilFinished();

So, these pass if they don't timeout, I guess. I would move them to their own test case, though. Or to the same one, but different than testContextMenuDefaultMenu

> LayoutTests/platform/ios-simulator-wk2/TestExpectations:1921
> +# Skipped because the is no copy key

What?

[Tomas Popela]

Committed r213278: <http://trac.webkit.org/changeset/213278>