Bug 72099 - [WK2] Keyboard menu key should show context menu
Summary: [WK2] Keyboard menu key should show context menu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Tomas Popela
URL:
Keywords:
: 75380 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-11-11 01:02 PST by chandra shekar vallala
Modified: 2017-03-02 06:19 PST (History)
11 users (show)

See Also:


Attachments
patch (3.28 KB, patch)
2011-11-11 01:22 PST, chandra shekar vallala
no flags Details | Formatted Diff | Diff
updated-patch (6.24 KB, patch)
2012-05-30 06:50 PDT, chandra shekar vallala
no flags Details | Formatted Diff | Diff
updated-patch (6.17 KB, patch)
2012-05-31 23:32 PDT, chandra shekar vallala
no flags Details | Formatted Diff | Diff
updated-patch (6.22 KB, patch)
2012-06-01 00:19 PDT, chandra shekar vallala
no flags Details | Formatted Diff | Diff
updated patch (5.66 KB, patch)
2012-06-04 06:17 PDT, chandra shekar vallala
no flags Details | Formatted Diff | Diff
Proposed patch (8.25 KB, patch)
2016-07-28 04:03 PDT, Tomas Popela
cgarcia: review-
mcatanzaro: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (16.30 KB, patch)
2016-09-29 01:02 PDT, Tomas Popela
mcatanzaro: review-
mcatanzaro: commit-queue-
Details | Formatted Diff | Diff
Fix the build on Mac (17.78 KB, patch)
2016-09-29 06:43 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff
Patch (22.92 KB, patch)
2017-02-21 07:13 PST, Tomas Popela
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (838.72 KB, application/zip)
2017-02-21 08:33 PST, Build Bot
no flags Details
Patch (23.70 KB, patch)
2017-02-28 02:41 PST, Tomas Popela
cgarcia: review+
cgarcia: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chandra shekar vallala 2011-11-11 01:02:27 PST
Context menu should display on menu key press.
Comment 1 chandra shekar vallala 2011-11-11 01:22:32 PST
Created attachment 114646 [details]
patch
Comment 2 Alexey Proskuryakov 2011-11-11 10:35:00 PST
Is it right to handle a raw keyboard event, or should be just handle WM_CONTEXTMENU when it comes?
Comment 3 chandra shekar vallala 2011-11-14 05:52:38 PST
(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.
Comment 4 Adam Roben (:aroben) 2011-11-14 08:42:39 PST
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.
Comment 5 Carlos Garcia Campos 2012-01-04 03:08:05 PST
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).
Comment 6 Carlos Garcia Campos 2012-01-04 03:08:54 PST
*** Bug 75380 has been marked as a duplicate of this bug. ***
Comment 7 chandra shekar vallala 2012-01-04 06:22:52 PST
(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().
Comment 8 Early Warning System Bot 2012-03-07 19:43:22 PST
Comment on attachment 114646 [details]
patch

Attachment 114646 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11861094
Comment 9 chandra shekar vallala 2012-05-30 06:50:49 PDT
Created attachment 144798 [details]
updated-patch

This patch allows the browser to show context menu when the CONTEXTMENU event comes
Comment 10 WebKit Review Bot 2012-05-30 06:56:13 PDT
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
Comment 11 Build Bot 2012-05-30 07:13:37 PDT
Comment on attachment 144798 [details]
updated-patch

Attachment 144798 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12859002
Comment 12 Martin Robinson 2012-05-30 10:28:51 PDT
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?
Comment 13 chandra shekar vallala 2012-05-31 23:32:10 PDT
Created attachment 145216 [details]
updated-patch

Updated patch as per comments.
Comment 14 Carlos Garcia Campos 2012-05-31 23:41:53 PDT
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()
Comment 15 Build Bot 2012-05-31 23:53:47 PDT
Comment on attachment 145216 [details]
updated-patch

Attachment 145216 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12862508
Comment 16 chandra shekar vallala 2012-06-01 00:19:26 PDT
Created attachment 145224 [details]
updated-patch

Updated patch as per comments.
Comment 17 chandra shekar vallala 2012-06-01 00:28:29 PDT
(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.
Comment 18 Carlos Garcia Campos 2012-06-01 00:40:04 PDT
(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?
Comment 19 Martin Robinson 2012-06-01 08:57:53 PDT
(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.
Comment 20 Carlos Garcia Campos 2012-06-01 09:04:49 PDT
(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
Comment 21 chandra shekar vallala 2012-06-04 06:17:24 PDT
Created attachment 145573 [details]
updated patch

I removed CC from WebKitWebViewBase.
Comment 22 Anders Carlsson 2014-02-05 10:55:12 PST
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.
Comment 23 Tomas Popela 2016-07-28 04:03:17 PDT
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.
Comment 24 Michael Catanzaro 2016-07-28 10:56:01 PDT
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.
Comment 25 Darin Adler 2016-07-31 21:40:31 PDT
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.
Comment 26 Carlos Garcia Campos 2016-07-31 23:07:28 PDT
(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.
Comment 27 Carlos Garcia Campos 2016-07-31 23:35:58 PDT
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.
Comment 28 Tomas Popela 2016-09-28 04:19:36 PDT
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.
Comment 29 Tomas Popela 2016-09-29 01:02:57 PDT
Created attachment 290188 [details]
Proposed patch
Comment 30 Michael Catanzaro 2016-09-29 01:45:02 PDT
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....
Comment 31 Tomas Popela 2016-09-29 06:43:48 PDT
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.
Comment 32 Carlos Garcia Campos 2016-10-17 02:00:09 PDT
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.
Comment 33 Tomas Popela 2017-02-21 06:57:13 PST
(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
Comment 34 Tomas Popela 2017-02-21 07:13:06 PST
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)
Comment 35 Build Bot 2017-02-21 08:33:46 PST
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
Comment 36 Build Bot 2017-02-21 08:33:51 PST
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
Comment 37 Tomas Popela 2017-02-28 02:41:59 PST
Created attachment 302928 [details]
Patch

Skip test on ios-simulator-wk2
Comment 38 Tomas Popela 2017-03-02 02:41:31 PST
Ping reviewers..
Comment 39 Carlos Garcia Campos 2017-03-02 03:04:37 PST
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?
Comment 40 Tomas Popela 2017-03-02 06:19:53 PST
Committed r213278: <http://trac.webkit.org/changeset/213278>