Bug 211557

Summary:

[GTK3] Bring back usage of GtkMenu for context menus

Product:

WebKit

Reporter:

Adrian Perez <aperez>

Component:

WebKitGTK

Assignee:

Adrian Perez <aperez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

berto, bugs-noreply, cgarcia, ews-watchlist, gustavo, jmason, mcatanzaro

Priority:

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=211241
https://bugs.webkit.org/show_bug.cgi?id=211294

Attachments:

Description	Flags
WIP Patch	none
Patch	none
Patch v2	none
Patch for landing	none

Adrian Perez

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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Adrian Perez

Comment 1 2020-05-07 17:23:21 PDT

Created attachment 398817 [details] WIP Patch

EWS Watchlist

Comment 2 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

Carlos Garcia Campos

Comment 3 2020-05-08 01:08:56 PDT

Comment on attachment 398817 [details] WIP Patch LGTM

Adrian Perez

Comment 4 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

Adrian Perez

Comment 5 2020-05-13 13:35:39 PDT

Created attachment 399294 [details] Patch

Carlos Garcia Campos

Comment 6 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.

Adrian Perez

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

Carlos Garcia Campos

Comment 8 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.

Adrian Perez

Comment 9 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?

Carlos Garcia Campos

Comment 10 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.

Adrian Perez

Comment 11 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.

Adrian Perez

Comment 12 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.

Carlos Garcia Campos

Comment 13 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

Adrian Perez

Comment 14 2020-05-15 00:47:18 PDT

Created attachment 399461 [details] Patch for landing

EWS

Comment 15 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].

Jim Mason

Comment 16 2020-05-15 10:26:47 PDT

Looks great, thanks gents!

Adrian Perez

Comment 17 2020-07-09 13:57:22 PDT

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

Note You need to log in before you can comment on or make changes to this bug.