Summary: | [GTK] Switch to use a popup window with a tree view instead of a menu for option menu default implementation | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | aperez, berto, bugs-noreply, bugzilla, cadubentzen, calvaris, ews-watchlist, gustavo, mcatanzaro | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=186032 https://bugs.webkit.org/show_bug.cgi?id=171803 https://bugs.webkit.org/show_bug.cgi?id=16450 https://bugs.webkit.org/show_bug.cgi?id=165072 https://bugs.webkit.org/show_bug.cgi?id=197947 |
||||||
Attachments: |
|
Description
Carlos Garcia Campos
2018-05-31 05:16:56 PDT
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> |