Bug 186146 – [GTK] Switch to use a popup window with a tree view instead of a menu for option menu default implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

We have successfully used it in ephy enough time.

[Carlos Garcia Campos]

Created attachment 341660 [details]
Patch

[EWS Watchlist]

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

[Adrian Perez]

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

[Adrian Perez]

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

[Adrian Perez]

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

[Adrian Perez]

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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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