[Webkit-unassigned] [Bug 72099] [WK2] Keyboard menu key should show context menu

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72099

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #284771|review?                     |review-
              Flags|                            |

--- Comment #27 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 284771
  --> https://bugs.webkit.org/attachment.cgi?id=284771
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160801/a8b838b4/attachment.html>