Bug 211557 – [GTK3] Bring back usage of GtkMenu for context menus

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Adrian Perez]

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.

[Adrian Perez]

Created attachment 398817 [details]
WIP Patch

[EWS Watchlist]

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

[Carlos Garcia Campos]

Comment on attachment 398817 [details]
WIP Patch

LGTM

[Adrian Perez]

(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

[Adrian Perez]

Created attachment 399294 [details]
Patch

[Carlos Garcia Campos]

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.

[Adrian Perez]

(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.

[Carlos Garcia Campos]

(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.

[Adrian Perez]

(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?

[Carlos Garcia Campos]

(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.

[Adrian Perez]

(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.

[Adrian Perez]

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.

[Carlos Garcia Campos]

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

[Adrian Perez]

Created attachment 399461 [details]
Patch for landing

[EWS]

Committed r261735: <https://trac.webkit.org/changeset/261735>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399461 [details].

[Jim Mason]

Looks great, thanks gents!

[Adrian Perez]

*** Bug 211294 has been marked as a duplicate of this bug. ***