Bug 211241 - [GTK4] Disable arrow on context menu popover
Summary: [GTK4] Disable arrow on context menu popover
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-30 11:08 PDT by Alexander Mikhaylenko
Modified: 2020-05-07 02:07 PDT (History)
5 users (show)

See Also:


Attachments
Screenshot (156.44 KB, image/png)
2020-04-30 11:08 PDT, Alexander Mikhaylenko
no flags Details
Patch (2.75 KB, patch)
2020-04-30 15:40 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 Alexander Mikhaylenko 2020-04-30 11:08:45 PDT
Created attachment 398067 [details]
Screenshot

GTK doesn't do it for context menus in GtkEntry/GtkTextView, see screenshot.

It's debatable what's the "right" behavior on GTK3 though, as it's all over the place.
Comment 1 Adrian Perez 2020-04-30 14:35:56 PDT
(In reply to Alexander Mikhaylenko from comment #0)
> Created attachment 398067 [details]
> Screenshot
> 
> GTK doesn't do it for context menus in GtkEntry/GtkTextView, see screenshot.
> 
> It's debatable what's the "right" behavior on GTK3 though, as it's all over
> the place.

I suppose we can just disable the arrow to make the context menus look like
the rest of context menus in GTK4, and I would disable it for both versions
for the sake of coherence (and avoiding #ifdefs in the code).
Comment 2 Adrian Perez 2020-04-30 15:33:42 PDT
(In reply to Adrian Perez from comment #1)
> (In reply to Alexander Mikhaylenko from comment #0)
> > Created attachment 398067 [details]
> > Screenshot
> > 
> > GTK doesn't do it for context menus in GtkEntry/GtkTextView, see screenshot.
> > 
> > It's debatable what's the "right" behavior on GTK3 though, as it's all over
> > the place.
> 
> I suppose we can just disable the arrow to make the context menus look like
> the rest of context menus in GTK4, and I would disable it for both versions
> for the sake of coherence (and avoiding #ifdefs in the code).

Actually, now that I checked it seems that there is no API in GTK3 to disable
the popover arrows and they are unconditionally drawn. For GTK4 we can use
gtk_popover_set_has_arrow() to disable them.
Comment 3 Alexander Mikhaylenko 2020-04-30 15:34:44 PDT
Oh well, then GTK4 only.
Comment 4 Adrian Perez 2020-04-30 15:40:15 PDT
Created attachment 398105 [details]
Patch
Comment 5 Adrian Perez 2020-04-30 15:50:14 PDT
(In reply to Alexander Mikhaylenko from comment #3)
> Oh well, then GTK4 only.

I have filed an issue requesting the gtk_popover_{get,set}_has_arrow()
for GTK3: https://gitlab.gnome.org/GNOME/gtk/-/issues/2679 — it would be
neat to have, but I guess it's not a huge deal.
Comment 6 EWS 2020-05-01 01:08:17 PDT
Committed r260986: <https://trac.webkit.org/changeset/260986>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398105 [details].
Comment 7 Michael Catanzaro 2020-05-01 05:37:38 PDT
We should probably only use this API in GTK 4, since it doesn't do what we want in GTK 3.
Comment 8 Adrian Perez 2020-05-01 06:37:39 PDT
(In reply to Michael Catanzaro from comment #7)
> We should probably only use this API in GTK 4, since it doesn't do what we
> want in GTK 3.

It doesn't even exist in GTK3. Have you seen the stub in GtkVersioning.h?
Comment 9 Michael Catanzaro 2020-05-01 07:49:22 PDT
I mean: let's not use an API that makes an arrow appear.
Comment 10 Adrian Perez 2020-05-01 07:59:07 PDT
(In reply to Michael Catanzaro from comment #9)
> I mean: let's not use an API that makes an arrow appear.

Maybe I am not explaining myself properly:

 - GTK3: We cannot control visibility of the arrow (it's always painted).
 - GTK4: There is gtk_popover_set_has_arrow(), used to disable it.
 - In GtkVersioning.h, an empty gtk_popover_set_has_arrow() is added
   *only used for GTK3* so the code in WebContextMenuProxyGtk.cpp does
   not need #ifdefs.

So, no, an arrow is never made to appear. On the contrary: it's always
there by default, and on GTK4 we make it go away 🤷️
Comment 11 Michael Catanzaro 2020-05-01 08:15:39 PDT
Look, in 2.28 there was no arrow on the context menu. Now going forward there is going to be an arrow. Do we really want to deal with complaints from users about this? It looks silly.
Comment 12 Adrian Perez 2020-05-01 08:23:38 PDT
(In reply to Michael Catanzaro from comment #11)
> Look, in 2.28 there was no arrow on the context menu. Now going forward
> there is going to be an arrow. Do we really want to deal with complaints
> from users about this? It looks silly.

We don't want to maintain different code paths for GTK3 and GTK4. If you
want my personal opinion, adding gtk_popover_set_has_arrow() to GTK3 does
not look too complicated, but my proposal for that has been shot down
already: https://gitlab.gnome.org/GNOME/gtk/-/issues/2679
Comment 13 Alexander Mikhaylenko 2020-05-01 09:58:05 PDT
Popover is going to look different from a GtkMenu in any case, so not a big deal whether it has arrow in GTK3 really.

GTK4 is another story because it itself does context menus as popovers with no arrow, like the patch does.
Comment 14 Jim Mason 2020-05-05 02:33:41 PDT
(In reply to Adrian Perez from comment #12)
> (In reply to Michael Catanzaro from comment #11)
> > Look, in 2.28 there was no arrow on the context menu. Now going forward
> > there is going to be an arrow. Do we really want to deal with complaints
> > from users about this? It looks silly.
>
> We don't want to maintain different code paths for GTK3 and GTK4. If you
> want my personal opinion, adding gtk_popover_set_has_arrow() to GTK3 does
> not look too complicated, but my proposal for that has been shot down
> already: https://gitlab.gnome.org/GNOME/gtk/-/issues/2679

Even if the proposal had been accepted, I have a doubt.  Maybe not everyone wants to or is able to upgrade to the latest gtk3 release to pick up the backported function to compile or run webkit.

Also, in addition to the visual hackishness of the nipple artefact, the new context menu implementation breaks mouse click semantics.  I've opened Bug 211294 to track that.
Comment 15 Adrian Perez 2020-05-07 02:07:56 PDT
(In reply to Jim Mason from comment #14)
> (In reply to Adrian Perez from comment #12)
> > (In reply to Michael Catanzaro from comment #11)
> > > Look, in 2.28 there was no arrow on the context menu. Now going forward
> > > there is going to be an arrow. Do we really want to deal with complaints
> > > from users about this? It looks silly.
> >
> > We don't want to maintain different code paths for GTK3 and GTK4. If you
> > want my personal opinion, adding gtk_popover_set_has_arrow() to GTK3 does
> > not look too complicated, but my proposal for that has been shot down
> > already: https://gitlab.gnome.org/GNOME/gtk/-/issues/2679
> 
> Even if the proposal had been accepted, I have a doubt.  Maybe not everyone
> wants to or is able to upgrade to the latest gtk3 release to pick up the
> backported function to compile or run webkit.
> 
> Also, in addition to the visual hackishness of the nipple artefact, the new
> context menu implementation breaks mouse click semantics.  I've opened Bug
> 211294 to track that.

After some discussion with Carlos García, I have opened bug #211557 to
bring back the GtkMenuShell-based context menus for GTK3. That will fix
the regressions and get rid of the arrow.

Note that GTK4 will keep using the GtkPopoverMenu, so if there are context
menu behaviors which would be desirable to keep (like selecting items with
the right button), then those should be reported as bugs in the GTK project,
at https://gitlab.gnome.org/GNOME/gtk/-/issues

I hope doing things in this way will work out for the best 🙂️