Bug 186146

Created attachment 341660 [details] Patch

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 on attachment 341660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341660&action=review This looks good to me, and having used these new popups in Epiphany already for a while I *know* that they do work much better, particularly in Wayland where the current implementation can have occasional popup window sizing and placement issues. I am for landing this, but probably it's good that someone else who knows GTK+ well (for example, Michael Catanzaro) would take a look as well. > Source/WebKit/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:270 > + // https://gitlab.gnome.org/GNOME/gtk/issues/997 I wonder whether it would be possible avoid this kind of complex code by (ab)using some GTK+ popup widget that behaves correctly on Wayland regarding to positioning -- for example using a GtkPopover and then overriding its CSS styling to make it look like a plain popup. Dunno, at any rate it's just a thought, not a request to change anything in this patch. (BTW, I commented in GTK+ bug #997 that having gdk_window_move_to_rect() public would be useful for WebKitGTK+ as well, and from touching popup layout code before I am quite confident we could use it in a few places.)

I believe that landing this would fix bug #186032 as well.

(In reply to Adrian Perez from comment #4) > I believe that landing this would fix bug #186032 as well. It would fix bug #171803, too. Note that we are still missing a layout test that checks that an <option> inside <optiongroup> can be chosen using the type-ahead find. We can repurpose use bug #171803 for adding the test once this lands.

This improves also performance for combo boxes with thousands of elements, which is an issue with the current implementation as per bug #16450 I've done a quick test with Epiphany and while there is still a noticeable <500ms delay before the popup appears, landing this patch is an amazingly huge improvement. I expect that the popup delay should be even smaller once the code is inside WebKit -- there will be less objects allocated and passed back and forth through the public API.

Comment on attachment 341660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341660&action=review > Source/WebKit/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:274 > + // FIXME: We can't ensure the menu will be on screen in Wayland. > + // https://blog.gtk.org/2016/07/15/future-of-relative-window-positioning/ > + // https://gitlab.gnome.org/GNOME/gtk/issues/997 > + if (menuPosition.x() < area.x) > + menuPosition.setX(area.x); > + else if (menuPosition.x() + menuRequisition.width > area.x + area.width) > + menuPosition.setX(area.x + area.width - menuRequisition.width); You need the important fix from https://gitlab.gnome.org/GNOME/epiphany/commit/37b9c6e69950729981c352e245772f2e51c9d78f for the multimonitor users. And I guess you'll remove EphyOptionMenu after the next release.

Committed r232390: <https://trac.webkit.org/changeset/232390>