Bug 211557 - [GTK3] Bring back usage of GtkMenu for context menus
Summary: [GTK3] Bring back usage of GtkMenu for context menus
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
: 211294 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-05-07 01:56 PDT by Adrian Perez
Modified: 2020-07-09 13:57 PDT (History)
7 users (show)

See Also:


Attachments
WIP Patch (9.89 KB, patch)
2020-05-07 17:23 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (21.10 KB, patch)
2020-05-13 13:35 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch v2 (18.83 KB, patch)
2020-05-14 09:20 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch for landing (18.86 KB, patch)
2020-05-15 00:47 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2020-05-07 01:56:36 PDT
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.
Comment 1 Adrian Perez 2020-05-07 17:23:21 PDT
Created attachment 398817 [details]
WIP Patch
Comment 2 EWS Watchlist 2020-05-07 17:23:54 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 3 Carlos Garcia Campos 2020-05-08 01:08:56 PDT
Comment on attachment 398817 [details]
WIP Patch

LGTM
Comment 4 Adrian Perez 2020-05-11 02:27:49 PDT
(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
Comment 5 Adrian Perez 2020-05-13 13:35:39 PDT
Created attachment 399294 [details]
Patch
Comment 6 Carlos Garcia Campos 2020-05-14 01:24:01 PDT
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.
Comment 7 Adrian Perez 2020-05-14 06:31:28 PDT
(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.
Comment 8 Carlos Garcia Campos 2020-05-14 07:07:01 PDT
(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.
Comment 9 Adrian Perez 2020-05-14 08:17:53 PDT
(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?
Comment 10 Carlos Garcia Campos 2020-05-14 08:25:26 PDT
(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.
Comment 11 Adrian Perez 2020-05-14 08:40:04 PDT
(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.
Comment 12 Adrian Perez 2020-05-14 09:20:47 PDT
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 13 Carlos Garcia Campos 2020-05-14 22:23:49 PDT
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
Comment 14 Adrian Perez 2020-05-15 00:47:18 PDT
Created attachment 399461 [details]
Patch for landing
Comment 15 EWS 2020-05-15 01:27:27 PDT
Committed r261735: <https://trac.webkit.org/changeset/261735>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399461 [details].
Comment 16 Jim Mason 2020-05-15 10:26:47 PDT
Looks great, thanks gents!
Comment 17 Adrian Perez 2020-07-09 13:57:22 PDT
*** Bug 211294 has been marked as a duplicate of this bug. ***