Bug 170553

The same goes for gtk_menu_popup_for_device(), which we are also using and it has been deprecated as well.

Created attachment 306416 [details] Patch

I have still pending check the patch once the build I am doing finishes, but I think it should be correct. I am not 100% sure about the call to gtk_menu_popup_at_rect(), so I am uploading it for early review. My understanding of the GTK+ documentation is that: - The passed GdkRectangle is the top-left coordinates where the menu is shown to be shown, relative to the webview, and the size. - The gravity parameter for the rectangle anchor is NORTH_WEST because that's the (x,y) position calculated for the top-left corner of the rectangle. - The gravity parameter for the reference point of the reference widget (in this case the webview) does not matter because the menu is a popup menu (GDK_WINDOW_TYPE_HINT_POPUP_MENU), which does not get “attached” to the reference widget. (In contrast, if this were a menu for a menubar, the top-left of the popup would be attached to the bottom-left of the item clicked — but that would be a GDK_WINDOW_TYPE_HINT_MENU.) The rest of changes are straightforward, and the new code path is even simpler than the old one, which is still used for GTK+ <3.22

Comment on attachment 306416 [details] Patch LGTM. Volker, can you test it to make sure it works please?

Created attachment 306432 [details] Patch

(In reply to Adrian Perez from comment #5) > Created attachment 306432 [details] > Patch This version now builds. There was a couple of mistakes in the previous version of the patch.

Created attachment 306444 [details] popup menu without patch

Created attachment 306445 [details] popup menu with patch

I forgot to mention the context menu is OK with that patch. Only the popup menus are still off, as can be seen in the pictures.

(In reply to Volker Sobek from comment #9) > I forgot to mention the context menu is OK with that patch. Only the popup > menus are still off, as can be seen in the pictures. Thanks for attaching the images and testing out the patch. I didn't have time to try the patch yesterday, and today I have just tried it myself indeed I get the same as you. I have to debug the calculation for the coordinates and size of the GdkRectangle and the other parameters passed to gtk_menu_poup_at_rect(). At least the contextual menus are already working fine. This is an issue that had been annoying me as well when using a Wayland session, but somehow I didn't stop to try and fix it before. So nice that you reported the issue and pointed towards the solution!

Created attachment 306500 [details] Popup menu with new patch version on X11 I got this working fine in X11! It works both with 1x scaling (normal screens) and 2x scaling (HiDPI screens). I'm going to test it under Wayland as well, but I am quite confident that it will work because the new version of the patch leaves all the position calculation to GTK+ and removes usage of the non-trivial positioning code we had to calculate where to place the popup menu.

Comment on attachment 306432 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306432&action=review Please do test under Wayland before committing. > Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:127 > + const GdkRectangle position = { menuPosition.x(), menuPosition.y(), requisition.width, requisition.height }; Got an extra space (two spaces) before requisition.width here > Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:128 > + gtk_menu_popup_at_rect(GTK_MENU(m_popup), gtk_widget_get_window(m_webView) , &position, GDK_GRAVITY_NORTH_WEST, GDK_GRAVITY_NORTH_WEST, event); Got an extra space before the comma here

Created attachment 306502 [details] Patch

(In reply to Michael Catanzaro from comment #12) > Comment on attachment 306432 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306432&action=review > > Please do test under Wayland before committing. I just did, and it works just fine. Leaving the calculations to GTK+ was indeed was a good move, and I am happy to not have to use our own code to position the popup anymore :-) > > Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:127 > > + const GdkRectangle position = { menuPosition.x(), menuPosition.y(), requisition.width, requisition.height }; > > Got an extra space (two spaces) before requisition.width here In the latest version of the patch “requisition” is not used anymore. > > Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:128 > > + gtk_menu_popup_at_rect(GTK_MENU(m_popup), gtk_widget_get_window(m_webView) , &position, GDK_GRAVITY_NORTH_WEST, GDK_GRAVITY_NORTH_WEST, event); > > Got an extra space before the comma here ACK, will fix.

Created attachment 306505 [details] Patch

(In reply to Adrian Perez from comment #15) > Created attachment 306505 [details] > Patch This version works fine, but when popping up the menu it, the previously selected item does not appear below the mouse pointer when popping it up. I think changing the gravity of the menu to GDK_GRAVITY_WEST will try to center the menu. I'll see if that's enough to get the active item below the mouse, though it could be that some of the tricky positioning code that iterates over the menu children is needed — I'll try my best to re-enable as little as possible from that.

(In reply to Adrian Perez from comment #16) > (In reply to Adrian Perez from comment #15) > > Created attachment 306505 [details] > > Patch > > This version works fine, but when popping up the menu it, the previously > selected item does not appear below the mouse pointer when popping it up. > I think changing the gravity of the menu to GDK_GRAVITY_WEST will try to > center the menu. I'll see if that's enough to get the active item below > the mouse, though it could be that some of the tricky positioning code > that iterates over the menu children is needed — I'll try my best to > re-enable as little as possible from that. Why is this desirable in the first place? What is the objective/benefit to the user of doing this? I never understood the idea behind it, it rather always seemed like something is broken and made the menu prone to accidentally selecting something wrong, at least for me. If the user doesn't want to change the item after opening the menu, they can just release the press (as it is now) as the previously selected item will continue to be selected. The patch works correctly in all cases here, did quite some testing, Wayland and X. Tested with a patched webkitgtk-2.16.1.

(In reply to Volker Sobek from comment #17) > (In reply to Adrian Perez from comment #16) > > (In reply to Adrian Perez from comment #15) > > > Created attachment 306505 [details] > > > Patch > > > > This version works fine, but when popping up the menu it, the previously > > selected item does not appear below the mouse pointer when popping it up. > > I think changing the gravity of the menu to GDK_GRAVITY_WEST will try to > > center the menu. I'll see if that's enough to get the active item below > > the mouse, though it could be that some of the tricky positioning code > > that iterates over the menu children is needed — I'll try my best to > > re-enable as little as possible from that. > > Why is this desirable in the first place? What is the objective/benefit to > the user of doing this? [...] WebKitGTK+ tries to behave as much as possible like the rest of the GTK+ toolkit. In GTK+ when a combo box is clicked, when the popup menu appears it has the currently selected item below the mouse pointer. We try to do that as well for consistency. > [...] I never understood the idea behind it, it rather always seemed > like something is broken and made the menu prone to accidentally > selecting something wrong, at least for me. If the user doesn't want > to change the item after opening the menu, they can just release the > press (as it is now) as the previously selected item will continue to be > selected. This is something that I personally agree with. I do not like much how combo boxes behave in GTK+ — but that's how they are, so we should try behave as closely to the combo box popup menus from the toolkit, for the sake of consistency ;-) > The patch works correctly in all cases here, did quite some testing, Wayland > and X. Tested with a patched webkitgtk-2.16.1. Thanks *a lot* for testing. I have only tried building from “master” and it's useful to know that it would be also fine if we wanted to apply the patch in a point release.

Created attachment 306694 [details] Patch

(In reply to Adrian Perez from comment #19) > Created attachment 306694 [details] > Patch This version of the patch also makes the popup appear with the active item centered below the mouse. (Well, not always, but at least it behaves the same as the existing code which is still used for GTK+ older than 3.22.0) So I think this should be the version to land.

Comment on attachment 306694 [details] Patch Clearing flags on attachment: 306694 Committed r215190: <http://trac.webkit.org/changeset/215190>

All reviewed patches have been landed. Closing bug.

Reverted r215190 for reason: Broke product select element on GNOME Bugzilla Committed r218798: <http://trac.webkit.org/changeset/218798>

Today I stumbled upon this issue reported to the Sway Wayland compositor, which seems might be related to this WebKitGTK+ bug: https://github.com/swaywm/sway/issues/1922 That one is about the Web view context menu not appearing when in fullscreen, which I think would be fixed by the part of the reverted patch that made changes to “WebContextMenuProxyGtk.cpp”. I think we can split that part of the patch and land it, because it won't affect the popups for <select> elements which were broken by the patch version that was reverted. I'll split that part of the patch into a new bug for landing.

(In reply to Adrian Perez from comment #24) > [...] I'll split that part of the patch into a new bug for landing. Or, better: land the Web view context menu for this bug (which is originally about the context menu after all) and split out the part about the Web popup menus to a separate bug.

Created attachment 341451 [details] Patch Patch fixing only the context menus (as per bug title+description).

Comment on attachment 341451 [details] Patch With this patch applied, trying to open the menu using the keyboard (for example with the dedicated context menu that some keyboards have) results in the following errors: (MiniBrowser:26141): Gtk-WARNING **: no trigger event for menu popup (MiniBrowser:26141): Gtk-CRITICAL **: gtk_menu_popup_at_rect: assertion 'GDK_IS_WINDOW (rect_window)' failed Let's not land this yet, I'll try to figure out what to do in that case.

Created attachment 396713 [details] Patch

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

Created attachment 396714 [details] Screenshot of new context menu based on GtkPopoverMenu

Created attachment 397745 [details] Patch

Created attachment 397748 [details] Patch This should fix the WPE build

Comment on attachment 397748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397748&action=review > Source/WebCore/ChangeLog:8 > + * platform/gtk/GtkVersioning.h: Add replacements for GtkPopover functions which are no longet available in GTK4. We should add GtkVersioning.h as an exception to be skipped by the style checker. It can be done in a separate patch. > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1662 > +static void activeContextMenuUnmapped(GtkWidget* widget, WebKitWebViewBase* webViewBase) I would rename this as Closed since this is no longer unmap callback. > Tools/TestWebKitAPI/Tests/WebKitGtk/TestContextMenu.cpp:31 > + WTF::Function<bool(GtkWidget*)> predicate; You don't need WTF:: > Tools/TestWebKitAPI/Tests/WebKitGtk/TestContextMenu.cpp:32 > + WTF::Vector<GtkWidget*> result { }; Ditto, nor the initialization.

Created attachment 397947 [details] Patch for landing

Committed r260889: <https://trac.webkit.org/changeset/260889> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397947 [details].

(In reply to EWS from comment #35) > Committed r260889: <https://trac.webkit.org/changeset/260889> > > All reviewed patches have been landed. Closing bug and clearing flags on > attachment 397947 [details]. This has caused GTK API Failures: /webkit/WebKitWebView/populate-menu: ** ERROR:../../Tools/TestWebKitAPI/Tests/WebKitGtk/TestContextMenu.cpp:703:void testContextMenuPopulateMenu(ContextMenuCustomTest *, gconstpointer): 'test->m_activated' should be TRUE Aborted

(In reply to Carlos Alberto Lopez Perez from comment #36) > (In reply to EWS from comment #35) > > Committed r260889: <https://trac.webkit.org/changeset/260889> > > > > All reviewed patches have been landed. Closing bug and clearing flags on > > attachment 397947 [details]. > > This has caused GTK API Failures: > > /webkit/WebKitWebView/populate-menu: ** > ERROR:../../Tools/TestWebKitAPI/Tests/WebKitGtk/TestContextMenu.cpp:703:void > testContextMenuPopulateMenu(ContextMenuCustomTest *, gconstpointer): > 'test->m_activated' should be TRUE > Aborted Mmh, I made sure tests passed locally before submitting, could it be an issue which shows only in the JHBuild environment? I'll try and take a look ¬_¬

(In reply to Adrian Perez from comment #37) > (In reply to Carlos Alberto Lopez Perez from comment #36) > > (In reply to EWS from comment #35) > > > Committed r260889: <https://trac.webkit.org/changeset/260889> > > > > > > All reviewed patches have been landed. Closing bug and clearing flags on > > > attachment 397947 [details]. > > > > This has caused GTK API Failures: > > > > /webkit/WebKitWebView/populate-menu: ** > > ERROR:../../Tools/TestWebKitAPI/Tests/WebKitGtk/TestContextMenu.cpp:703:void > > testContextMenuPopulateMenu(ContextMenuCustomTest *, gconstpointer): > > 'test->m_activated' should be TRUE > > Aborted > > Mmh, I made sure tests passed locally before submitting, could it be an > issue which shows only in the JHBuild environment? I'll try and take a > look ¬_¬ Okay, I managed to reproduce this locally and it's puzzling because I have no idea what could have changed between earlier and now… hopefully it's not timing-related.

(In reply to Adrian Perez from comment #38) > (In reply to Adrian Perez from comment #37) > > (In reply to Carlos Alberto Lopez Perez from comment #36) > > > (In reply to EWS from comment #35) > > > > Committed r260889: <https://trac.webkit.org/changeset/260889> > > > > > > > > All reviewed patches have been landed. Closing bug and clearing flags on > > > > attachment 397947 [details]. > > > > > > This has caused GTK API Failures: > > > > > > /webkit/WebKitWebView/populate-menu: ** > > > ERROR:../../Tools/TestWebKitAPI/Tests/WebKitGtk/TestContextMenu.cpp:703:void > > > testContextMenuPopulateMenu(ContextMenuCustomTest *, gconstpointer): > > > 'test->m_activated' should be TRUE > > > Aborted > > > > Mmh, I made sure tests passed locally before submitting, could it be an > > issue which shows only in the JHBuild environment? I'll try and take a > > look ¬_¬ > > Okay, I managed to reproduce this locally and it's puzzling because I have > no idea what could have changed between earlier and now… hopefully it's not > timing-related. I have already an idea of what is the cause, and opened bug #211203 to handle fixing the regression. Thanks for the heads up!

(In reply to EWS from comment #35) > Committed r260889: <https://trac.webkit.org/changeset/260889> > > All reviewed patches have been landed. Closing bug and clearing flags on > attachment 397947 [details]. For me under GTK 3.24.11 / X11, r260889 breaks the right-click context popup in a couple ways. First, it has a nipple on top (like a dropdown menu) where before it was a rectangle as expected. More problemmatic, all of the menu items in the popup are disabled. I am uploading a couple screenshots, 'sample r260888.png' (working) and 'sample r260889.png' (broken).

Created attachment 398061 [details] sample r260888

Created attachment 398062 [details] sample r260889

Created attachment 398064 [details] sample r260937 (In reply to Jim Mason from comment #40) > (In reply to EWS from comment #35) > > Committed r260889: <https://trac.webkit.org/changeset/260889> > > > > All reviewed patches have been landed. Closing bug and clearing flags on > > attachment 397947 [details]. > For me under GTK 3.24.11 / X11, r260889 breaks the right-click context popup > in a couple ways. First, it has a nipple on top (like a dropdown menu) > where before it was a rectangle as expected. > > More problemmatic, all of the menu items in the popup are disabled. I am > uploading a couple screenshots, 'sample r260888.png' (working) and 'sample > r260889.png' (broken). Trying a later build, the disabled menu items have been fixed but the nipple is still there. Screenshot attached.

(In reply to Jim Mason from comment #43) > Created attachment 398064 [details] > sample r260937 > > (In reply to Jim Mason from comment #40) > > (In reply to EWS from comment #35) > > > Committed r260889: <https://trac.webkit.org/changeset/260889> > > > > > > All reviewed patches have been landed. Closing bug and clearing flags on > > > attachment 397947 [details]. > > > > For me under GTK 3.24.11 / X11, r260889 breaks the right-click context popup > > in a couple ways. First, it has a nipple on top (like a dropdown menu) > > where before it was a rectangle as expected. This is because now we are using a GtkPopoverMenu, which has an arrow by default pointing to location where the menu was opened. In GTK4 the context menus don't have the arrow enabled, and we may do the same for WebKitGTK's context menus, see bug #211241 for that. > > More problemmatic, all of the menu items in the popup are disabled. I am > > uploading a couple screenshots, 'sample r260888.png' (working) and 'sample > > r260889.png' (broken). > > Trying a later build, the disabled menu items have been fixed but the nipple > is still there. Screenshot attached. This we noticed a bit after the patch landed, and has been fixed with the patch for bug #211203. Thanks for taking your time to report the issues nevertheless!

(In reply to Carlos Garcia Campos from comment #33) > Comment on attachment 397748 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=397748&action=review > > > Source/WebCore/ChangeLog:8 > > + * platform/gtk/GtkVersioning.h: Add replacements for GtkPopover functions which are no longet available in GTK4. > > We should add GtkVersioning.h as an exception to be skipped by the style > checker. It can be done in a separate patch. Done, the patch for this is in bug #211262

Right-click selection of menu items has been affected as well. See Bug 211294