Created attachment 254704 [details] Patch

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

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.

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?

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

(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 :-)

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.

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.

Created attachment 306793 [details] Patch

Comment on attachment 306793 [details] Patch Clearing flags on attachment: 306793 Committed r215225: <http://trac.webkit.org/changeset/215225>

All reviewed patches have been landed. Closing bug.

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