After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 776225 - [wayland] dropdown placed somewhere in the screen
[wayland] dropdown placed somewhere in the screen
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Wayland
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2016-12-18 04:46 UTC by Mohammed Sadiq
Modified: 2017-01-17 08:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtk+ menu position bug (image) (53.17 KB, image/png)
2016-12-18 04:46 UTC, Mohammed Sadiq
  Details
wayland: Handle subsurface as popup parent (1.61 KB, patch)
2017-01-05 08:06 UTC, Jonas Ådahl
none Details | Review
[PATCH gtk-3-22] wayland: translate subsurfaces coordinates (2.99 KB, patch)
2017-01-05 08:53 UTC, Olivier Fourdan
rejected Details | Review
wayland: Handle subsurface as popup parent (1.94 KB, patch)
2017-01-05 09:33 UTC, Jonas Ådahl
committed Details | Review
[PATCH master] wayland: Handle subsurface as popup parent (1.92 KB, patch)
2017-01-16 14:16 UTC, Olivier Fourdan
accepted-commit_now Details | Review

Description Mohammed Sadiq 2016-12-18 04:46:58 UTC
Created attachment 342134 [details]
gtk+ menu position bug (image)

Dropdown sometimes placed somewhere in the screen.

Please see the attached screenshot from gnome-recipes.
Comment 1 Olivier Fourdan 2017-01-02 09:27:11 UTC
I cannot reproduce this using current gtk-3-22 branch with current recipes.

Can you give more details your setup and on how to reproduce? In comment 0 you mention it  happens sometimes, does it mean it's random?
Comment 2 Olivier Fourdan 2017-01-03 17:04:51 UTC
Quick update, to reproduce, one needs to have combo inside a popover, which is not so common but can be found in recipes at commit c127b94.

The problem is that gtk_menu_position() will position the menu relative to the widget using the widget's window, but when it's inside a popover, the popover is itself a subsurface, and the popover relative position is not taken into account.

In other words, the menu is placed relative to the widget location but applied to the toplevel window, not the actual subsurface when using a popover.

Not sure how to fix this though...
Comment 3 Olivier Fourdan 2017-01-05 07:31:24 UTC
It's even worse than the combo box, I just realised that even the context popup menu on text fields is misplaced as well, it's located according to the toplevel coordinates instead of the subsurface coordinates (it doesn't show up at the pointer location).

I had /some/ patch for the combo issue but it makes the text field context menu even more wrong. Meh.
Comment 4 Jonas Ådahl 2017-01-05 08:06:31 UTC
Created attachment 342924 [details] [review]
wayland: Handle subsurface as popup parent

When a subsurface is used as a parent of a popup, GDK needs to traverse
up to the transient-for as the next parent, to properly find the parent
used by the popup positioner. This is because the parent of a popup
must always either be an xdg_popup or an xdg_surface, but traversing
the "parent" (in GDK terms) upwards from a subsurface will end up on
the fake root window before we hit the actual parent (in Wayland terms).

---

I have only tested this with the widget factory (I found a place where a popup is a child of a popover on Page 3, by pressing "Open" then opening a context menu in the text entry. This patch fixes that case, and I suspect other misplaced popups that had to pass by a subsurface should be fixed as well, if there is no other issue involved as well.
Comment 5 Olivier Fourdan 2017-01-05 08:53:53 UTC
Created attachment 342930 [details] [review]
[PATCH gtk-3-22] wayland: translate subsurfaces coordinates

When mapping a popup window from a popover, which under Wayland is
translated as a subsurface with the root window as parent, the menu
appears misplaced, located according to the toplevel  location instead
of the subsurface location.

When the parent of a popup window is a subsurface, translate the
coordinates back to the toplevel coordinates so that move_to_rect()
correctly locates the popup in the subsurface.

--

Note, it's *meh* but it appears to do the job... i.e., I'm not proud of this patch but failed to find a better solution without a lot of potential impact elsewhere...
Comment 6 Olivier Fourdan 2017-01-05 09:01:00 UTC
Review of attachment 342930 [details] [review]:

Self rejecting my own patch, actually, I prefer Jonas' patch.
Comment 7 Olivier Fourdan 2017-01-05 09:02:51 UTC
Review of attachment 342924 [details] [review]:

Yeah, I ended up with the same conclusions using a different patch, but I prefer yours - Tested, works fine with the original test case.
Comment 8 Olivier Fourdan 2017-01-05 09:05:19 UTC
Review of attachment 342924 [details] [review]:

Couldn't we save one of the two calls to get_effective_parents() by using a temp var instead?
Comment 9 Jonas Ådahl 2017-01-05 09:08:43 UTC
Review of attachment 342924 [details] [review]:

Yea, and should probably look at impl->display_server.wl_subsurface rather than parent->window_type to be more reliable. Should maybe un-obfuscate that while loop while at it.
Comment 10 Jonas Ådahl 2017-01-05 09:33:07 UTC
Created attachment 342933 [details] [review]
wayland: Handle subsurface as popup parent

When a subsurface is used as a parent of a popup, GDK needs to traverse
up to the transient-for as the next parent, to properly find the parent
used by the popup positioner. This is because the parent of a popup
must always either be an xdg_popup or an xdg_surface, but traversing
the "parent" (in GDK terms) upwards from a subsurface will end up on
the fake root window before we hit the actual parent (in Wayland terms).

----

This one untangles the while loop and uses a temp variable for the effectev parent. Got slightly messier, but also maybe easier to read; not sure which one I prefer.
Comment 11 Olivier Fourdan 2017-01-05 09:52:29 UTC
Review of attachment 342933 [details] [review]:

I like this one better, but leave it to you to chose - Either way, both patches works.
Comment 12 Jonas Ådahl 2017-01-06 03:08:59 UTC
Comment on attachment 342933 [details] [review]
wayland: Handle subsurface as popup parent

Attachment 342933 [details] pushed as 5bae71f - wayland: Handle subsurface as popup parent
Comment 13 Jonas Ådahl 2017-01-06 03:56:04 UTC
Leaving open until a similar patch has landed on master. Can't do that quite yet; I crash on mapping a popover on master now (something about a damage region being NULL), so I can't test.
Comment 14 Olivier Fourdan 2017-01-16 14:16:36 UTC
Created attachment 343556 [details] [review]
[PATCH master] wayland: Handle subsurface as popup parent

Forward port of attachment 342933 [details] [review] (git 5bae71f) to master.

gdk_window_get_effective_parent() has been removed along with the removal of offscreen windows (commit 70935f) so this patch simply use gdk_window_get_parent() instead of gdk_window_get_effective_parent() in gtk-3-22

Seems to work, popovers are currently misplaced and font rendering seems broken on master but this is unrelated to this patch.
Comment 15 Jonas Ådahl 2017-01-17 04:40:55 UTC
Review of attachment 343556 [details] [review]:

Makes sense to me.
Comment 16 Olivier Fourdan 2017-01-17 08:48:39 UTC
Comment on attachment 343556 [details] [review]
[PATCH master] wayland: Handle subsurface as popup parent

attachment 343556 [details] [review] pushed to git master as commit 7ca6d75 - wayland: Handle subsurface as popup parent