After r60889 we have been using GtkPopoverMenu on GTK3, which has brought a number of issues: * Bug #211294 - [GTK] REGRESSION(r260889): Right click not selecting menu item * Bug #211241 - [GTK4] Disable arrow on context menu popover * Context menus in most (not all!) GTK3 applications are still using GtkMenuShell, and people feel like WebKitGTK's are inconsistent. * It is not possible anymore to activate a context menu item with right button press + move mouse over an item + release button. * …and maybe others. Let's try and use GtkMenuShell on GTK3, keeping GtkPopoverMenu for GTK4.
Created attachment 398817 [details] WIP 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
Comment on attachment 398817 [details] WIP Patch LGTM
(In reply to Carlos Garcia Campos from comment #3) > Comment on attachment 398817 [details] > WIP Patch > > LGTM Thanks! I'll make the needed changes on the API tests so we can land this. IMO having a few helper functions at the top of the “WebContextMenuProxyGtk.cpp” does not look too bad :D
Created attachment 399294 [details] Patch
Comment on attachment 399294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399294&action=review > Source/WebKit/ChangeLog:8 > + WIP: Bug #211557 I guess this is no longer WIP. > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > +#include "WebKitWebViewPrivate.h" Ideally this should only know about WebKitWebViewBase, I'm not sure if this code is ever reached by the C api, though.
(In reply to Carlos Garcia Campos from comment #6) > Comment on attachment 399294 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > Source/WebKit/ChangeLog:8 > > + WIP: Bug #211557 > > I guess this is no longer WIP. Indeed 😅️ > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > +#include "WebKitWebViewPrivate.h" > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > code is ever reached by the C api, though. Yes, it's reached. Do you have any suggestion to avoid letting WebContextMenuProxyGtk know about WebKitWebView? I'll also try to come up with something.
(In reply to Adrian Perez from comment #7) > (In reply to Carlos Garcia Campos from comment #6) > > Comment on attachment 399294 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > > > Source/WebKit/ChangeLog:8 > > > + WIP: Bug #211557 > > > > I guess this is no longer WIP. > > Indeed 😅️ > > > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > > +#include "WebKitWebViewPrivate.h" > > > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > > code is ever reached by the C api, though. > > Yes, it's reached. Do you have any suggestion to avoid letting > WebContextMenuProxyGtk know about WebKitWebView? I'll also try to > come up with something. Connect to close/deactive from the API instead like we did before the popover rework.
(In reply to Carlos Garcia Campos from comment #8) > (In reply to Adrian Perez from comment #7) > > (In reply to Carlos Garcia Campos from comment #6) > > > Comment on attachment 399294 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > > > > > Source/WebKit/ChangeLog:8 > > > > + WIP: Bug #211557 > > > > > > I guess this is no longer WIP. > > > > Indeed 😅️ > > > > > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > > > +#include "WebKitWebViewPrivate.h" > > > > > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > > > code is ever reached by the C api, though. > > > > Yes, it's reached. Do you have any suggestion to avoid letting > > WebContextMenuProxyGtk know about WebKitWebView? I'll also try to > > come up with something. > > Connect to close/deactive from the API instead like we did before the > popover rework. That's precisely what I was trying to avoid, as it's ugly. How about adding a “WebContextMenuProxyGtk::connectDismissed(WTF::Function&&)” method?
(In reply to Adrian Perez from comment #9) > (In reply to Carlos Garcia Campos from comment #8) > > (In reply to Adrian Perez from comment #7) > > > (In reply to Carlos Garcia Campos from comment #6) > > > > Comment on attachment 399294 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > > > > > > > Source/WebKit/ChangeLog:8 > > > > > + WIP: Bug #211557 > > > > > > > > I guess this is no longer WIP. > > > > > > Indeed 😅️ > > > > > > > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > > > > +#include "WebKitWebViewPrivate.h" > > > > > > > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > > > > code is ever reached by the C api, though. > > > > > > Yes, it's reached. Do you have any suggestion to avoid letting > > > WebContextMenuProxyGtk know about WebKitWebView? I'll also try to > > > come up with something. > > > > Connect to close/deactive from the API instead like we did before the > > popover rework. > > That's precisely what I was trying to avoid, as it's ugly. How about adding > a “WebContextMenuProxyGtk::connectDismissed(WTF::Function&&)” method? I don't see what's ugly there, I think it's the easiest solution. If you mean it's ugly because of the ifdefs, add a helper to connect to the signal and move the ifdef to the helper.
(In reply to Carlos Garcia Campos from comment #10) > (In reply to Adrian Perez from comment #9) > > (In reply to Carlos Garcia Campos from comment #8) > > > (In reply to Adrian Perez from comment #7) > > > > (In reply to Carlos Garcia Campos from comment #6) > > > > > Comment on attachment 399294 [details] > > > > > Patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=399294&action=review > > > > > > > > > > > Source/WebKit/ChangeLog:8 > > > > > > + WIP: Bug #211557 > > > > > > > > > > I guess this is no longer WIP. > > > > > > > > Indeed 😅️ > > > > > > > > > > Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:39 > > > > > > +#include "WebKitWebViewPrivate.h" > > > > > > > > > > Ideally this should only know about WebKitWebViewBase, I'm not sure if this > > > > > code is ever reached by the C api, though. > > > > > > > > Yes, it's reached. Do you have any suggestion to avoid letting > > > > WebContextMenuProxyGtk know about WebKitWebView? I'll also try to > > > > come up with something. > > > > > > Connect to close/deactive from the API instead like we did before the > > > popover rework. > > > > That's precisely what I was trying to avoid, as it's ugly. How about adding > > a “WebContextMenuProxyGtk::connectDismissed(WTF::Function&&)” method? > > I don't see what's ugly there, I think it's the easiest solution. If you > mean it's ugly because of the ifdefs, add a helper to connect to the signal > and move the ifdef to the helper. It's partly the ifdefs, but having a WebContextMenuProxyGtk::gtkWidget() accessor is something I don't like either.
Created attachment 399367 [details] Patch v2 This moves back connecting to the context menu widget signals to WebKitWebView.cpp and WebKitWebViewBase.cpp, and uses a static constant defined in WebContextMenuProxyGtk.cpp to avoid needing more #ifdefs.
Comment on attachment 399367 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=399367&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2616 > + g_signal_emit(webView, signals[CONTEXT_MENU_DISMISSED], 0, NULL); NULL -> nullptr
Created attachment 399461 [details] Patch for landing
Committed r261735: <https://trac.webkit.org/changeset/261735> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399461 [details].
Looks great, thanks gents!
*** Bug 211294 has been marked as a duplicate of this bug. ***