GNOME Bugzilla – Bug 769402
regression in menu positioning on wayland
Last modified: 2016-08-18 08:53:12 UTC
Created attachment 332502 [details] screenshot of the issue on wayland I have some custom menu positioning code in Builder to align GtkTreeView menus with the top of the row. While the toplevel menu position is still correct, the submenus position (which we do not alter the position of) is off by about ~300 pixels. I haven not checked to see if this is also an issue on Xorg yet.
gtk version/commit ?
commit 0989614d2d sorry for the short commit id, on wayland and c&p is broken in epiphany.
Confirmed with testpopupat. Jonas is looking into it.
Created attachment 332693 [details] [review] gdkwindow: Use toplevel for getting root cords in move_to_rect() The Wayland backend manages a set of fake root coordinate spaces, where each non-relative positioned toplevel (i.e. not popups, popovers, tooltips etc) make up the basis of separate fake root coordinate spaces. This means that the Wayland backend doesn't have the abilitiy get a proper root coordinate when querying on a non-toplevel GdkWindow. To avoid this issue, first find the toplevel, while translating the anchor rect coordinates so that they are in the toplevel window coordinate space. Then use this toplevel to translate the coordinates to root window coordinate space.
Created attachment 332694 [details] [review] wayland: Use effective toplevel as popup parent When using the set transient-for as a popup parent, fetch the effective toplevel instead, otherwise we will position against the wrong coordinate.
Created attachment 332695 [details] [review] wayland: Don't traverse transient-ofs when faking root coordinate space The position of each transient-of will be in fake-root coordinate space; thus we should not accumulate all the positions making it an offset; each window is already in fake root coordinate space.
Created attachment 332696 [details] [review] wayland: Don't append shadow width to window size The window size is the size without the drop shadow included. Until after this patch, we'd temporarly set a larger window size, until it was overriden by something else updating the size.
CC:ing William because this touches the generic implementation of move_to_rect. CC:ing Carlos because he was the one introducing the resize-to-include-shadow-width.
Created attachment 332789 [details] [review] wayland: Don't include shadow margins in window size The GdkWindow::width and GdkWindow::height fields represent the window geometry size, not including any possible shadow margin. The Wayland backend ignored this and appended it anyway, causing various problems here and there. This fixes an relative popup positioning when a menu expands upwards or to the right from a anchor rectangle. The reason was that since the size was "in flux" between gtk/'s and gdk/wayland/'s understanding of size, if the position calculation happened to be done in state, but actual positioning in the other, the position was incorrect. --- This patch replaces "wayland: Don't append shadow width to window size".
Review of attachment 332693 [details] [review]: Thanks, just a couple questions, but these patches do fix the problem. I'm still a bit hesitant to alter the default implementation for the sake of the Wayland backend, which will have its own implementation at some point. But if it still needs a lot of work, we can do it to fix the regression. I didn't see any problems with this patch under X11 or Mir. ::: gdk/gdkwindowimpl.c @@ +187,3 @@ + { + if (_gdk_window_is_toplevel (window)) +{ Is this needed if we already know window->window_type == GDK_WINDOW_CHILD? @@ +191,3 @@ + gdk_window_coords_to_parent (window, xf, yf, &xf, &yf); + + gdouble yf = y; Should we use gdk_window_get_effective_parent (window) instead? The docs for gdk_window_coords_to_parent () say that for off-screen windows, it converts to the window's embedder instead.
(In reply to William Hua from comment #10) > Review of attachment 332693 [details] [review] [review]: > > Thanks, just a couple questions, but these patches do fix the problem. > > I'm still a bit hesitant to alter the default implementation for the sake of > the Wayland backend, which will have its own implementation at some point. > But if it still needs a lot of work, we can do it to fix the regression. I > didn't see any problems with this patch under X11 or Mir. Well, we could add a dumb implementation to the Wayland backend right now, but it'd regress sligthly (for example a window placed at poisition (0, 0) on a single monitor gets mostly the correct menu placement simply because the real position happens to be the same as the "fake root coordinate". > > ::: gdk/gdkwindowimpl.c > @@ +187,3 @@ > + { > + if (_gdk_window_is_toplevel (window)) > +{ > > Is this needed if we already know window->window_type == GDK_WINDOW_CHILD? I'm not entirely sure. I used the same loop semantics as gdk_window_get_toplevel() which includes this part. > > @@ +191,3 @@ > + gdk_window_coords_to_parent (window, xf, yf, &xf, &yf); > + > + gdouble yf = y; > > Should we use gdk_window_get_effective_parent (window) instead? > > The docs for gdk_window_coords_to_parent () say that for off-screen windows, > it converts to the window's embedder instead. Good point. Should probably use gdk_window_get_effective_parent() above then as well.
Created attachment 332910 [details] [review] gdkwindow: Use toplevel for getting root cords in move_to_rect() The Wayland backend manages a set of fake root coordinate spaces, where each non-relative positioned toplevel (i.e. not popups, popovers, tooltips etc) make up the basis of separate fake root coordinate spaces. This means that the Wayland backend doesn't have the abilitiy get a proper root coordinate when querying on a non-toplevel GdkWindow. To avoid this issue, first find the toplevel, while translating the anchor rect coordinates so that they are in the toplevel window coordinate space. Then use this toplevel to translate the coordinates to root window coordinate space.
Created attachment 332911 [details] [review] wayland: Postpone processing move_to_rect params until showing At the time of move_to_rect() is called, not all state may have been set up on the impl gdk window, causing the position to sometimes be slightly offset due to drap shadow margins. For now, work around this by postponing the processing of the move_to_rect() parameters until showing, when its more likely that all state (such as shadow margin) has been set correctly. ---- I give up on getting resizing working properly in the Wayland backend for now, and work around it by postponing the calculations that depend on a fully set up state. The problem is that, if I understand things correctly, GdkWindow::width/height are including shadow margin dimensions, which causes problems when changing state changes the existance of drop shadows. For example, assume that we have a maximized window thus without drop shadow. We get a configure request telling GTK to unmaximize the window and use the size of the configure request (i.e. the requested window geometry - window size excluding shadow margin). GDK will then use this size, append the 0-sized shadow margin, and emit a GDK_CONFIGURE request with the new size. GTK will then add a shadow region, but without resizing the allocation, resulting in a slightly smaller window. Maximize->unmaximize->maximize a few of times and your window is suddenly tiny. We'll see the same problem when going the other way: assume we have a unmaximized window, and the compositor asks it to maximize itself. It'll take the window geometry dimension it got from the compositor, append the shadow, and emit a GDK_CONFIGURE event. GTK will then take the dimension of the configure event, remove the shadow without resizing the allocation, resulting in a too large window. A possible solution is to add private state to GDK_CONFIGURE, with a window *geometry* rect set, that is always excluding shadow width. Not sure I like this solution very much, because it'll leave the dimension of the GdkConfigure event still somewhat useless.
Review of attachment 332910 [details] [review]: Looks good, just remove _gdk_window_is_toplevel () that isn't needed any more. ::: gdk/gdkinternals.h @@ +545,3 @@ gboolean _gdk_window_has_impl (GdkWindow *window); GdkWindow * _gdk_window_get_impl_window (GdkWindow *window); +gboolean _gdk_window_is_toplevel (GdkWindow *window); This isn't necessary any more. ::: gdk/gdkwindow.c @@ +666,2 @@ gboolean +_gdk_window_is_toplevel (GdkWindow *window) Same with this.
Is this patchset still needed if we land the xdg-shell v6 support ?
(In reply to Matthias Clasen from comment #15) > Is this patchset still needed if we land the xdg-shell v6 support ? The xdg-shell unstable v6 patchset builds upon these patches. The last one is not needed per se but and could be squashed into the last of the v6 patch set, but landing the patches in this bug prior to the v6 patches would be good to so we can have a working state before porting.
Removed the unused function before pushing Attachment 332694 [details] pushed as 1f7094a - wayland: Use effective toplevel as popup parent Attachment 332695 [details] pushed as 4e0ebd0 - wayland: Don't traverse transient-ofs when faking root coordinate space Attachment 332910 [details] pushed as f6929cf - gdkwindow: Use toplevel for getting root cords in move_to_rect() Attachment 332911 [details] pushed as cc019de - wayland: Postpone processing move_to_rect params until showing