GNOME Bugzilla – Bug 757558
gtk_window_move() unreliable in Wayland?
Last modified: 2016-05-09 06:46:23 UTC
Created attachment 314779 [details] screenshot As the screenshot illustrates...
Thanks for a bug report. I can confirm this, but only on the Wayland.
After some debugging, I see that the gtk_window_move() is called with correct values, still the window is positioned in a wrong place. It seems to me it's something in the Wayland backend of the gtk3, because I cannot reproduce this with an X11 backend (GNOME or Cinnamon). I'd like to ask any gtk+ developer for an opinion, though I do not see any much difference between the GtkComboBox popup and the one used by the evolution.
Jonas, can you look at why this popup doesn't get the right treatment under Wayland ?
This seems most likely due to an unexpected GdkWindowTypeHint. Milan, do you know offhand the place where this popup is created?
Sure, for the emoticon it's created here: https://git.gnome.org/browse/evolution/tree/e-util/e-emoticon-tool-button.c#n573 and repositioned here: https://git.gnome.org/browse/evolution/tree/e-util/e-emoticon-tool-button.c#n124 I compared the code with the one for GtkComboBox and it seems to me it's pretty much the same, except we hold the popup window for the whole lifetime of the widget, while GtkComboBox seems to build and destroy the popup on demand. Please note that the X11 works properly, with the same code in the evolution.
(In reply to Milan Crha from comment #5) > Sure, for the emoticon it's created here: > https://git.gnome.org/browse/evolution/tree/e-util/e-emoticon-tool-button. > c#n573 > and repositioned here: > https://git.gnome.org/browse/evolution/tree/e-util/e-emoticon-tool-button. > c#n124 > > I compared the code with the one for GtkComboBox and it seems to me it's > pretty much the same, except we hold the popup window for the whole lifetime > of the widget, while GtkComboBox seems to build and destroy the popup on > demand. > > Please note that the X11 works properly, with the same code in the evolution. Thanks! I see that you're setting the transient_for on the popup conditionally around https://git.gnome.org/browse/evolution/tree/e-util/e-emoticon-tool-button.c#n581 . At that time the toplevel will still be NULL, so that if() is never stepped in. The transient_for window is essential for popups to appear properly positioned in wayland, so that needs to be done at some point when the button has a toplevel, maybe hierarchy_changed(), or before showing.
and FWIW, I'm not sure how much can we fix in gtk+... We've got iffy code to figure out a "transient_for" so popups are at least shown, we don't keep it any longer nor try to fake this is the transient_for we got from the user.
I think we maybe should complain more verbosely in the Wayland GDK when things won't work properly. For example, if a toplevel window is moved, we should complain saying moving doesn't work for toplevel windows. We have started to complain some when a window that should be mapped as a popup is not actually mapped as a popup, but we can also start complaining when the application tries to move an already mapped popup. We should also consider how to deal with things that should be mapped as subsurfaces, since we currently are a bit lax on the order of transiency and showing (we ad-hoc map when transiency is set for those), and that makes it hard to warn when things go wrong. I don't see how we can make it work reliably without going all GTK+ 4 though. Can we add more detailed semantics of the different window types right now and then just force applications to play with those rules in order to work?
(In reply to Carlos Garnacho from comment #6) > The transient_for window is essential for popups to appear properly > positioned in wayland, so that needs to be done at some point when the > button has a toplevel, maybe hierarchy_changed(), or before showing. I moved the gtk_window_group_add_window() and gtk_window_set_transient_for() calls from the _init() to the emoticon_tool_button_popup(), just before the call of emoticon_tool_button_reposition_window(), but it didn't help. I've a test g_print() there, which is printed, thus this code is triggered for sure. Please note that the evolution passes correct coordinates for the popup window, before the window is shown, they are just ignored under Wayland.
Created attachment 314869 [details] reproducer This is a reproducer, with extracted evolution bits into a single file. The first line contains a comment with a command line to compile it. Once run, click the "Click me" in the middle of the dialog, which shows a popup. It's placed correctly in X11, but incorrectly in Wayland. I also noticed that the tooltips above those emoticons are placed incorrectly under Wayland. The code doesn't contain the test change of moving the set_transient_for() before the show() for the popup. This is with gtk3 3.18.2.
Thanks for the reproducer. I had a look at it and I could make the popup map correctly by doing three changes: 1. set_transient_for() was moved to just before show(). The reason for this was that toplevel was NULL on init(), i.e. no transiency was established. This was warned in stderr as "Gdk-WARNING **: Couldn't map as window 0xc8e450 as popup because it doesn't have a parent" 2. I moved show() to *after* the device grab. This was warned in stderr as "Gdk-WARNING **: Couldn't map window 0xa20450 as popup because no grabbed seat found". This is needed because the popup window is shown in a response to an input event. The GDK backend needs to know where the input event came from when mapping, and the only way to do that is via device grabs. Though that was not enough. One last thing was needed. 3. I removed keyboard = ... when the source event came from a pointer. Regarding the need for part 3, there are 2 issues that needs resolving, one in GDK and one in the reproducer. In the reproducer, one should not grab the keyboard when the event came from a pointer, because that way the GDK backend thinks the keyboard was the source of the event. In effect, it will not be able to map the popup, because it doesn't have a valid serial. The other problem, in the GDK backend, is that popup mapping seems to be hardcoded to only function as a response to mouse clicks (see gdk_wayland_window_create_xdg_popup()).
Created attachment 314871 [details] [review] Diff to make the reproducer map the popup as a popup
Another issue I noticed is that mutter will consider the tooltip part of the popup window and position it according to the union of the popup and tooltip rectangles, causing the popup to move awkwardly as tooltips show and disappear. This is sadly not something we can fix without changing xdg_shell or ignoring parts of the subsurface semantics.
(In reply to Jonas Ådahl from comment #12) > Created attachment 314871 [details] [review] [review] > Diff to make the reproducer map the popup as a popup Thanks. I tested it, and the popup doesn't show at all under X11. The button is pressed down, but the popup is nowhere visible. (I wanted to adapt the Evolution code with the proposed change, but the test application fails the same.)
(In reply to Milan Crha from comment #14) > (In reply to Jonas Ådahl from comment #12) > > Created attachment 314871 [details] [review] [review] [review] > > Diff to make the reproducer map the popup as a popup > > Thanks. I tested it, and the popup doesn't show at all under X11. The button > is pressed down, but the popup is nowhere visible. (I wanted to adapt the > Evolution code with the proposed change, but the test application fails the > same.) Hmm. This is a bit awkward. On Wayland, we need to have a grab before showing the popup, because the popup need to know the device that triggered it. But when doing that on X11, at least in the reperoducer, when we try to grab before it is shown the grab fails with "GrabNotViewable", and we hide the window as a result. Matthias, Carlos, any idea how this was intended to work? Can one grab a window before it is shown on X11 some how?
(In reply to Jonas Ådahl from comment #15) > Hmm. This is a bit awkward. On Wayland, we need to have a grab before > showing the popup, because the popup need to know the device that triggered > it. But when doing that on X11, at least in the reperoducer, when we try to > grab before it is shown the grab fails with "GrabNotViewable", and we hide > the window as a result. > > Matthias, Carlos, any idea how this was intended to work? Can one grab a > window before it is shown on X11 some how? Unfortunately that's not possible. As the error name hints, the window needs to be viewable (it and all its ancestors currently mapped) in order to be used for a grab.
> Matthias, Carlos, any idea how this was intended to work? Can one grab a > window before it is shown on X11 some how? See the complications with the 'grab_transfer_window' in gtkmenu.c. Using the grab in this way was a workaround for not having a 'popup window' api in GDK that would let us combine the grabbing and the showing. Maybe it is time to add such an API ?
See also bug 759738 Now (in gtk+ 3.20) the gdk backend will use a subsurface for popup windows when possible, ie when a parent is specified, so we don't require a grab anymore. But I reckon the given reproducer is broken: 1. it creates a e_emoticon_tool_button_new (); 2. The corresponding e_emoticon_tool_button_init() tries to get the toplevel window from gtk_widget_get_toplevel (GTK_WIDGET (button)); 3. But because the button si not yet added to the window, gtk_widget_get_toplevel() will necessarily return the buttn itself, therefore GTK_IS_WINDOW (toplevel) is always wrong and gtk_window_set_transient_for(0 is never called. Too bad, because it it was, I think it would work as-is with gtk+ 3.20.
Created attachment 327242 [details] Rworked reproducer as per comment 18 When the set the transient_for() relationship correctly, it works. Yes, you do get a warning from gdk wayland backend that "No grabbed seat found, using the default one in order to map popup window 0x19d6730. You may find oddities ahead, gdk_seat_grab() should be used to simultaneously grab input and show this popup" but it does work. Can we reconsider this bug then?... and possibly close it as a dupe of bug 759738?
(In reply to Olivier Fourdan from comment #19) > Yes, you do get a warning from gdk wayland backend that "No grabbed seat > found, using the default one in order to map popup window 0x19d6730. You may > find oddities ahead, gdk_seat_grab() should be used to simultaneously grab > input and show this popup" but it does work. > > Can we reconsider this bug then?... and possibly close it as a dupe of > bug 759738? I'd be happy to, but as long as a runtime warning is printed it cannot be considered fixed. Either the code is still wrong or the runtime warning is wrong.
(In reply to Milan Crha from comment #20) > I'd be happy to, but as long as a runtime warning is printed it cannot be > considered fixed. Either the code is still wrong or the runtime warning is > wrong. But aren't these two different things? Comment #0 was about the popup being misplaced on Wayland, which is fixed with gtk+ 3.20 and does not require a grab anymore (just a parent window for the popup, as shown in attachment 327242 [details] whci works with both X11 and Wayland backend).
Created attachment 327302 [details] Reworked reproducer to make it work withotu warning. (In reply to Milan Crha from comment #20) > [...] Either the code is still wrong or the runtime warning is > wrong. And there might be some room for improvement in to the code as well, changing it as the warning message suggests make the warning go away. What about this modified version? No warning, the popup shows where expected in both X11 and Wayland.
Thanks for the reproducer update. I can confirm it works properly with the latest gtk3 (Fedora rawhide), gtk3-3.20.x and even gtk3-3.10.9, without any runtime warning about grabbing. I see some under Wayland when moving the window after the popup had been shown (and closed), about not found weakref, also the packing in Wayland is odd (there's a gap between the "Click me" and "Close" buttons, which is not shown in X11), but those things are unrelated for this bug report, because the popup window currently shows at the right place. Thus feel free to close this. As I'm unsure whether you prefer to close this as a duplicate or on its own, I'll rather keep the close for you. Thanks again.
I made changes in the evolution code based on your suggestions for 3.21.2+ and 3.20.2+ [1] and I can confirm that with it the emoticon and color chooser popups are shown at the correct place (and without runtime warnings) under both X11 and Wayland. Thanks for your help with this. [1] https://git.gnome.org/browse/evolution/commit/?id=ca1d3b8
Great, glad that helped! Closing this bug following comment 23 and comment 24.