<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span> changed
<a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - [WK2] Keyboard menu key should show context menu"
href="https://bugs.webkit.org/show_bug.cgi?id=72099">bug 72099</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #284771 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - [WK2] Keyboard menu key should show context menu"
href="https://bugs.webkit.org/show_bug.cgi?id=72099#c27">Comment # 27</a>
on <a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - [WK2] Keyboard menu key should show context menu"
href="https://bugs.webkit.org/show_bug.cgi?id=72099">bug 72099</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=284771&action=diff" name="attach_284771" title="Proposed patch">attachment 284771</a> <a href="attachment.cgi?id=284771&action=edit" title="Proposed patch">[details]</a></span>
Proposed patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284771&action=review">https://bugs.webkit.org/attachment.cgi?id=284771&action=review</a>
<span class="quote">> Source/WebCore/ChangeLog:11
> + No new tests as this should be covered by existing context menu tests.</span >
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.
<span class="quote">>> 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.</span >
I don't understand this change either . . .
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:828
> + priv->contextMenuEvent.reset(gdk_event_copy(reinterpret_cast<GdkEvent*>(event)));</span >
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?
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:929
> + priv->pageProxy->handleContextMenuKeyEvent();</span >
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+.
<span class="quote">>> 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.</span >
I don't think so.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2155
> +void WebPage::contextMenuKeyEvent()</span >
I think we could use contextMenuForKeyEvent or something similar, for consistency with the webcore name.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>