145866 2015-06-10 19:47:23 -0700 [GTK] Attach popup menu to web view widget 2017-04-11 04:24:01 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jadahl aperez aperez bugs-noreply cdumez cgarcia commit-queue mcatanzaro ysuzuki zan oldest_to_newest 1101201 0 jadahl 2015-06-10 19:47:23 -0700 In order to avoid having the GDK backend guess what the parent widget of the popup is, set it explicitly. This makes popup menus work more reliably under Wayland, as a popup menu is always required to be attached to a parent window. 1101210 1 254704 jadahl 2015-06-10 20:18:52 -0700 Created attachment 254704 Patch 1107464 2 mcatanzaro 2015-07-07 10:03:04 -0700 Zan, can you review this please? It's a one-liner. I will just say that the patch should use nullptr instead of 0. 1108127 3 254704 zan 2015-07-08 23:02:26 -0700 Comment on attachment 254704 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. 1296004 4 aperez 2017-04-10 09:36:16 -0700 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? 1296228 5 jadahl 2017-04-10 17:19:07 -0700 (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. 1296304 6 aperez 2017-04-11 00:48:34 -0700 (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 :-) 1296305 7 aperez 2017-04-11 00:50:52 -0700 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. 1296337 8 aperez 2017-04-11 03:29:09 -0700 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. 1296338 9 306793 aperez 2017-04-11 03:32:45 -0700 Created attachment 306793 Patch 1296347 10 306793 commit-queue 2017-04-11 04:19:57 -0700 Comment on attachment 306793 Patch Clearing flags on attachment: 306793 Committed r215225: <http://trac.webkit.org/changeset/215225> 1296348 11 commit-queue 2017-04-11 04:19:58 -0700 All reviewed patches have been landed. Closing bug. 1296349 12 jadahl 2017-04-11 04:24:01 -0700 (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. 254704 2015-06-10 20:18:52 -0700 2017-04-11 03:32:35 -0700 Patch bug-145866-20150611111822.patch text/plain 1785 jadahl U3VidmVyc2lvbiBSZXZpc2lvbjogMTg1MzEwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMzQ5YTg4MDFlZWZlZjUz Zjk4MzU2NjlmZWEyMDI3YTc5ZjRiNDczNy4uMTQwYzI5MGVlZDI3OGNmYzkzZTUxMmVhYzdhNGRh ZWExZTMwYzlhYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE1LTA2LTEwICBKb25h cyDDhWRhaGwgIDxqYWRhaGxAZ21haWwuY29tPgorCisgICAgICAgIFtHVEtdIEF0dGFjaCBwb3B1 cCBtZW51IHRvIHdlYiB2aWV3IHdpZGdldAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MTQ1ODY2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvZ3RrL1dlYlBvcHVwTWVudVByb3h5R3RrLmNw cDoKKyAgICAgICAgKFdlYktpdDo6V2ViUG9wdXBNZW51UHJveHlHdGs6OnNob3dQb3B1cE1lbnUp OgorCiAyMDE1LTA2LTA3ICBHeXV5b3VuZyBLaW0gIDxneXV5b3VuZy5raW1Ad2Via2l0Lm9yZz4K IAogICAgICAgICBbRUZMXVtHVEtdIEZpeCBidWlsZCAgYnJlYWsgc2luY2UgcjE4NTI2MgpkaWZm IC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL2d0ay9XZWJQb3B1cE1lbnVQcm94eUd0 ay5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvZ3RrL1dlYlBvcHVwTWVudVByb3h5R3Rr LmNwcAppbmRleCBjY2U5ZjM2YTM5YWI3OTc4MWJlNjEzZWI5NGRkNTRiN2MyMzdjOTY5Li4zODc2 ODUyZDk0ZmFmNDFhN2EwZWQ0OWZjOTlhNzNkMGMwZjMxY2I3IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0Mi9VSVByb2Nlc3MvZ3RrL1dlYlBvcHVwTWVudVByb3h5R3RrLmNwcAorKysgYi9Tb3Vy Y2UvV2ViS2l0Mi9VSVByb2Nlc3MvZ3RrL1dlYlBvcHVwTWVudVByb3h5R3RrLmNwcApAQCAtMTMw LDYgKzEzMCw3IEBAIHZvaWQgV2ViUG9wdXBNZW51UHJveHlHdGs6OnNob3dQb3B1cE1lbnUoY29u c3QgSW50UmVjdCYgcmVjdCwgVGV4dERpcmVjdGlvbiwgZG91CiAgICAgZ3Vsb25nIHVubWFwSGFu ZGxlciA9IGdfc2lnbmFsX2Nvbm5lY3QobV9wb3B1cCwgInVubWFwIiwgR19DQUxMQkFDSyhtZW51 VW5tYXBwZWQpLCB0aGlzKTsKIAogICAgIGNvbnN0IEdka0V2ZW50KiBldmVudCA9IG1fY2xpZW50 LT5jdXJyZW50bHlQcm9jZXNzZWRNb3VzZURvd25FdmVudCgpID8gbV9jbGllbnQtPmN1cnJlbnRs eVByb2Nlc3NlZE1vdXNlRG93bkV2ZW50KCktPm5hdGl2ZUV2ZW50KCkgOiBudWxscHRyOworICAg IGd0a19tZW51X2F0dGFjaF90b193aWRnZXQoR1RLX01FTlUobV9wb3B1cCksIEdUS19XSURHRVQo bV93ZWJWaWV3KSwgMCk7CiAgICAgZ3RrX21lbnVfcG9wdXBfZm9yX2RldmljZShHVEtfTUVOVSht X3BvcHVwKSwgZXZlbnQgPyBnZGtfZXZlbnRfZ2V0X2RldmljZShldmVudCkgOiBudWxscHRyLCBu dWxscHRyLCBudWxscHRyLAogICAgICAgICBbXShHdGtNZW51KiwgZ2ludCogeCwgZ2ludCogeSwg Z2Jvb2xlYW4qIHB1c2hJbiwgZ3BvaW50ZXIgdXNlckRhdGEpIHsKICAgICAgICAgICAgIC8vIFdl IGNhbiBwYXNzIGEgcG9pbnRlciB0byB0aGUgbWVudVBvc2l0aW9uIGxvY2FsIHZhcmlhYmxlIGJl Y2F1c2UgdGhlIG5lc3RlZCBtYWluIGxvb3AgZW5zdXJlcyB0aGlzIGlzIGNhbGxlZCBpbiB0aGUg ZnVuY3Rpb24gY29udGV4dC4K 306793 2017-04-11 03:32:45 -0700 2017-04-11 04:19:57 -0700 Patch bug-145866-20170411133244.patch text/plain 1899 aperez U3VidmVyc2lvbiBSZXZpc2lvbjogMjE1MjIzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMzYxMmJmYjdlMTZiZDky Y2M4NzJmMzZjNWYxODcxOTkxNWNiZmUyZS4uNGRmZDVjMWQ1MDJmMjc0ZDdhYzYzYjM3N2JmMzA2 MjgyODExZTEzNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDE3LTA0LTExICBBZHJp YW4gUGVyZXogZGUgQ2FzdHJvICA8YXBlcmV6QGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dUS10g QXR0YWNoIHBvcHVwIG1lbnUgdG8gd2ViIHZpZXcgd2lkZ2V0CisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDU4NjYKKworICAgICAgICBVc2UgZ3RrX21l bnVfYXR0YWNoX3RvX3dpZGdldCgpIHRvIGxldCBHVEsrIGtub3cgdGhhdCBwb3B1cCBtZW51cyBi ZWxvbmcgdG8gYSBjZXJ0YWluIHdlYiB2aWV3LgorICAgICAgICBUaGlzIGltcHJvdmVzIHRoZSBw b3NpdGlvbmluZyBjaG9pY2VzIHRoYXQgdGhlIHRvb2xraXQgY2FuIGRvLCBhbmQgc29sdmVzIGEg bG9uZy1zdGFuZGluZyBpc3N1ZQorICAgICAgICB0aGF0IGNhdXNlZCBsb25nIHBvcHVwIG1lbnVz IHRvIGhhbmcgb3V0c2lkZSBvZiB0aGUgYXZhaWxhYmxlIGRpc3BsYXkgYXJlYSB1bmRlciBXYXls YW5kLgorCisgICAgICAgIEJhc2VkIG9uIGEgcGF0Y2ggYnkgSm9uYXMgw4VkYWhsIDxqYWRhaGxA Z21haWwuY29tPi4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICAqIFVJUHJvY2Vzcy9ndGsvV2ViUG9wdXBNZW51UHJveHlHdGsuY3BwOgorICAgICAgICAo V2ViS2l0OjpXZWJQb3B1cE1lbnVQcm94eUd0azo6c2hvd1BvcHVwTWVudSk6CisKIDIwMTctMDQt MTAgIEFsZXggQ2hyaXN0ZW5zZW4gIDxhY2hyaXN0ZW5zZW5Ad2Via2l0Lm9yZz4KIAogICAgICAg ICBSZXZlcnQgcjIxNTIxNwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL2d0 ay9XZWJQb3B1cE1lbnVQcm94eUd0ay5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvZ3Rr L1dlYlBvcHVwTWVudVByb3h5R3RrLmNwcAppbmRleCAxNmFhNDg2MmYxY2QxMzZhNmY2ZDY4ZjQ3 ZGVjMjk2NTNhNmRkNTU3Li5kMTJmYWYxYWIwNTI1MDZkOWUyZWE1NWQ1Y2M5YzhjOTNiZGZlZWQx IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvZ3RrL1dlYlBvcHVwTWVudVBy b3h5R3RrLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvZ3RrL1dlYlBvcHVwTWVu dVByb3h5R3RrLmNwcApAQCAtMTM5LDYgKzEzOSw4IEBAIHZvaWQgV2ViUG9wdXBNZW51UHJveHlH dGs6OnNob3dQb3B1cE1lbnUoY29uc3QgSW50UmVjdCYgcmVjdCwgVGV4dERpcmVjdGlvbiwgZG91 CiAgICAgICAgIG1lbnVQb3NpdGlvbi5zZXRZKG1lbnVQb3NpdGlvbi55KCkgLSByZWN0LmhlaWdo dCgpIC8gMik7CiAgICAgfQogCisgICAgZ3RrX21lbnVfYXR0YWNoX3RvX3dpZGdldChHVEtfTUVO VShtX3BvcHVwKSwgR1RLX1dJREdFVChtX3dlYlZpZXcpLCBudWxscHRyKTsKKwogICAgIGNvbnN0 IEdka0V2ZW50KiBldmVudCA9IG1fY2xpZW50LT5jdXJyZW50bHlQcm9jZXNzZWRNb3VzZURvd25F dmVudCgpID8gbV9jbGllbnQtPmN1cnJlbnRseVByb2Nlc3NlZE1vdXNlRG93bkV2ZW50KCktPm5h dGl2ZUV2ZW50KCkgOiBudWxscHRyOwogI2lmIEdUS19DSEVDS19WRVJTSU9OKDMsIDIyLCAwKQog ICAgIC8vIFNldCB0aGUgc2FtZSBwcm9wZXJ0aWVzIHRoYXQgR1RLKyB1c2VzIGl0c2VsZiBmb3Ig Y29tYm8gYm94IHBvcHVwcy4K