Bug 186146 - [GTK] Switch to use a popup window with a tree view instead of a menu for option menu default implementation
Summary: [GTK] Switch to use a popup window with a tree view instead of a menu for opt...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2018-05-31 05:16 PDT by Carlos Garcia Campos
Modified: 2019-05-16 06:56 PDT (History)
9 users (show)

See Also:


Attachments
Patch (33.57 KB, patch)
2018-05-31 05:19 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-05-31 05:16:56 PDT
We have successfully used it in ephy enough time.
Comment 1 Carlos Garcia Campos 2018-05-31 05:19:28 PDT
Created attachment 341660 [details]
Patch
Comment 2 Build Bot 2018-05-31 05:22:03 PDT
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 3 Adrian Perez 2018-05-31 06:01:18 PDT
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.)
Comment 4 Adrian Perez 2018-05-31 06:04:29 PDT
I believe that landing this would fix bug #186032 as well.
Comment 5 Adrian Perez 2018-05-31 06:09:24 PDT
(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.
Comment 6 Adrian Perez 2018-05-31 06:19:45 PDT
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 7 Michael Catanzaro 2018-05-31 07:18:23 PDT
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.
Comment 8 Carlos Garcia Campos 2018-06-01 00:44:44 PDT
Committed r232390: <https://trac.webkit.org/changeset/232390>