[GTK] Attach popup menu to web view widget

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jonas Ådahl]

In order to avoid having the GDK backend guess what the parent widget of the popup is, set it explicitly. This makes popup menus work more reliably under Wayland, as a popup menu is always required to be attached to a parent window.

[Jonas Ådahl]

Created attachment 254704 [details]
Patch

[Michael Catanzaro]

Zan, can you review this please? It's a one-liner. I will just say that the patch should use nullptr instead of 0.

[Zan Dobersek]

Comment on attachment 254704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254704&action=review

> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:133
> +    gtk_menu_attach_to_widget(GTK_MENU(m_popup), GTK_WIDGET(m_webView), 0);

Right, nullptr is preferred.

[Adrian Perez]

This is related to bug #170553, which is to move to non-deprecated
functions which handle positioning of popups more reliably. I think
it is still desirable to attach the popup to the web view because
even when we will be using gtk_menu_popup_at_{rect,widget,pointer}
those functions do not attach the menu to the widget (at least that's
my understanding from reading the GTK+ documentation).

Jonas, will you be updating this patch, or do you want me to update
it and land it for you?

[Jonas Ådahl]

(In reply to Adrian Perez from comment #4)
> This is related to bug #170553, which is to move to non-deprecated
> functions which handle positioning of popups more reliably. I think
> it is still desirable to attach the popup to the web view because
> even when we will be using gtk_menu_popup_at_{rect,widget,pointer}
> those functions do not attach the menu to the widget (at least that's
> my understanding from reading the GTK+ documentation).
> 
> Jonas, will you be updating this patch, or do you want me to update
> it and land it for you?

Seems I had forgotten about this. If you could rebase and land, that would be great, as I don't have a recent tree checked out nor built. If your time is limited, I'll try to get it done in the coming days.

[Adrian Perez]

(In reply to Jonas Ådahl from comment #5)
> (In reply to Adrian Perez from comment #4)
> > This is related to bug #170553, which is to move to non-deprecated
> > functions which handle positioning of popups more reliably. I think
> > it is still desirable to attach the popup to the web view because
> > even when we will be using gtk_menu_popup_at_{rect,widget,pointer}
> > those functions do not attach the menu to the widget (at least that's
> > my understanding from reading the GTK+ documentation).
> > 
> > Jonas, will you be updating this patch, or do you want me to update
> > it and land it for you?
> 
> Seems I had forgotten about this. If you could rebase and land, that would
> be great, as I don't have a recent tree checked out nor built. If your time
> is limited, I'll try to get it done in the coming days.

I have been hunting popup menu bugs the last days, so I'll just go ahead
and get this landed — I'll credit you in the log. Thanks for the patch and
for stopping by for commenting :-)

[Adrian Perez]

FWIW, this does not apply cleanly after the patch for bug #170553 has
landed and needed a rebase. I am now waiting for a build that I can
smoketest before posting an updated version.

[Adrian Perez]

This seems to fix the remaining issue that we had with popup menus
in Wayland: after applying this on current “master”, menus with a
lot of elements are correctly resized and repositioned to fit in the
current screen when running *under Mutter* (and hence GNOME Shell).

Under Weston, long menus still bleed outside of the screen. Most likely
my Weston is too old for the version of the “xdg_popup” interface (part
of “xdg_shell”) that GTK+ is using. Probably this could happen with
other Wayland compositors as well. I think this is fine: we do all
that is possible from our part with what GTK+ provides, and IMHO the
ball would be now in the court of the compositors that don't implement
the “xdg_popup” interface.

I'll upload the patch in a moment.

[Adrian Perez]

Created attachment 306793 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 306793 [details]
Patch

Clearing flags on attachment: 306793

Committed r215225: <http://trac.webkit.org/changeset/215225>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonas Ådahl]

(In reply to Adrian Perez from comment #8)
> This seems to fix the remaining issue that we had with popup menus
> in Wayland: after applying this on current “master”, menus with a
> lot of elements are correctly resized and repositioned to fit in the
> current screen when running *under Mutter* (and hence GNOME Shell).
> 
> Under Weston, long menus still bleed outside of the screen. Most likely
> my Weston is too old for the version of the “xdg_popup” interface (part
> of “xdg_shell”) that GTK+ is using. Probably this could happen with
> other Wayland compositors as well. I think this is fine: we do all
> that is possible from our part with what GTK+ provides, and IMHO the
> ball would be now in the court of the compositors that don't implement
> the “xdg_popup” interface.

This is only because libweston-desktop (used by "weston") only having partially implemented xdg-shell (last time I checked this was the case). It implements the positioning, but not the constraining, which is fine according to spec, but less good from a UX point of view. There is nothing WebKit or GTK+ should do about anything about however.

> 
> I'll upload the patch in a moment.