GNOME Bugzilla – Bug 763627
menu-traditional puts popover beneath the text editor widget
Last modified: 2016-03-25 16:28:55 UTC
Created attachment 323887 [details] Screenshot demonstrating the issue on X11 Description: In X11, when using the Open popover menu, it appears beneath the texty editor widget in gedit. It works fine in Wayland (for once!) though. Steps to reproducer: 1. Log in GNOME on X11 2. Start gedit 3. Click on "Open" Actual result: The popover widget is beneath the text widget Expected result: The popover pops over :)
Created attachment 323888 [details] Same on Wayland is OK
Ah, nevermind, a new build works fine, I suspect it was related to the use of "menu-traditional.ui" in gedit which was selected by default, but now it works. Either way, not a bug in gtk+ and gedit seems fixed now. closing.
git bisect shows this commit in gedit is the problem: https://git.gnome.org/browse/gedit/commit/?id=24e4e79d Basically, when menu-traditional is used, the popover menu is shown beneath the editor widget as seen in attachment 323887 [details] whereas when the default UI used, it's fine.
I'm not seeing the overlapping, however I cannot use the mouse with the popover's contents.
hard to see how this could be gedit's fault
(In reply to Garrett Regier from comment #4) > I'm not seeing the overlapping, however I cannot use the mouse with the > popover's contents. Yes, I see that on Fedora 24 whereas I see attachment 323887 [details] in my jhbuild env... (In reply to Matthias Clasen from comment #5) > hard to see how this could be gedit's fault Granted, but the issue first appeared when gedit forced the menu-traditional UI in non-gnome environments whereas the default UI works just fine so there must be something in the UI that confuses popovers somehow.
(In reply to Matthias Clasen from comment #5) > hard to see how this could be gedit's fault Yeah, looking at the code in gtk/gtkpopover.c and gtk/gtkwindow.c, I hardly see how this could be gedit fault indeed, it should simply not happen. Yet I wonder why that happens, of the surface it looks like bug 756670 which wa fixed with commit 9d1b8df (among others) and yet this still doesn't work in all cases. I suspect this is related to the use of an overlay, but I failed to come up with a simple reproducer. Anyhow, moving back to gtk.
I thin kwhat happens is that the GtkOverlay uses its own window and keeps raising it: here: https://git.gnome.org/browse/gtk+/tree/gtk/gtkoverlay.c#n298 here: https://git.gnome.org/browse/gtk+/tree/gtk/gtkoverlay.c#n459 Whereas the Popover window is mapped but not raised: here: https://git.gnome.org/browse/gtk+/tree/gtk/gtkwindow.c#n6170 here: https://git.gnome.org/browse/gtk+/tree/gtk/gtkwindow.c#n7498 Therefore such a patch fixes the issue, but also makes animation look bleh... so I am not sure: diff --git a/gtk/gtkwindow.c b/gtk/gtkwindow.c index 7449668..ed0a532 100644 --- a/gtk/gtkwindow.c +++ b/gtk/gtkwindow.c @@ -7499,7 +7499,7 @@ popover_size_allocate (GtkWidget *widget, gtk_widget_is_visible (widget)) { if (!gdk_window_is_visible (popover->window)) - gdk_window_show_unraised (popover->window); + gdk_window_show (popover->window); } else if (gdk_window_is_visible (popover->window)) gdk_window_hide (popover->window);
Created attachment 324666 [details] [review] [PATCH] popover: raise when showing In the lack of a better solution ...
Created attachment 324667 [details] [review] [PATCH] popover: raise when showing v2: raise on map and on size allocate as well, otherwise it can still end up being covered by the overlay.
(In reply to Olivier Fourdan from comment #10) > Created attachment 324667 [details] [review] [review] > [PATCH] popover: raise when showing > > v2: raise on map and on size allocate as well, otherwise it can still end up > being covered by the overlay. See also bug #756670 :), this patch seems a partial revert of the one in comment #1 there. I think it's a more durable solution to avoid widgets from raising windows after those are mapped, or doing so in a controlled way (eg. using gdk_window_restack()).
(In reply to Carlos Garnacho from comment #11) > (In reply to Olivier Fourdan from comment #10) > > Created attachment 324667 [details] [review] [review] [review] > > [PATCH] popover: raise when showing > > > > v2: raise on map and on size allocate as well, otherwise it can still end up > > being covered by the overlay. > > See also bug #756670 :), this patch seems a partial revert of the one in > comment #1 there. Yes, right, I mentioned bug #756670 in comment 7 but the fix there is apparently not sufficient as popovers may still fail to show above other widgets. > I think it's a more durable solution to avoid widgets from raising windows > after those are mapped, or doing so in a controlled way (eg. using > gdk_window_restack()). gdk_window_restack() takes a sibling, and the window from one widget is not necessarily known to another (like the popover has no idea the overlay has mapped a window above, for example). We could walk the window tree and determine the top most window and restack the popover above the one from the overlay, but that would be just like using a raise (which is a restack above the top most window). As for controlling the way widgets restack their windows, it would be like some sort of window stack manager? My fear is that would end up in gtkwindow (just like popovers) and I am not sure this is an ideal solution either.
(In reply to Olivier Fourdan from comment #12) > (In reply to Carlos Garnacho from comment #11) > > (In reply to Olivier Fourdan from comment #10) > > > Created attachment 324667 [details] [review] [review] [review] [review] > > > [PATCH] popover: raise when showing > > > > > > v2: raise on map and on size allocate as well, otherwise it can still end up > > > being covered by the overlay. > > > > See also bug #756670 :), this patch seems a partial revert of the one in > > comment #1 there. > > Yes, right, I mentioned bug #756670 in comment 7 but the fix there is > apparently not sufficient as popovers may still fail to show above other > widgets. Oh, missed that :). > > > I think it's a more durable solution to avoid widgets from raising windows > > after those are mapped, or doing so in a controlled way (eg. using > > gdk_window_restack()). > > gdk_window_restack() takes a sibling, and the window from one widget is not > necessarily known to another (like the popover has no idea the overlay has > mapped a window above, for example). Yes, I didn't suggest gdk_window_restack() to stack widget windows relative to other widgets', but in order to perform intra-widget restacking. > > We could walk the window tree and determine the top most window and restack > the popover above the one from the overlay, but that would be just like > using a raise (which is a restack above the top most window). I think the problem here is that if gdk_window_show() is spammed in size allocate, you effectively have widgets contending to be on top on each resize, this is mostly useless (the situations where you really want visual restacking are very contained, eg. GtkNotebook page switches, GtkStack switches,...), and could lead to unnecessary processing (if recursing within windowed children widgets raises the window, you'll be effectively causing spurious invalidations as windows get on top of others. Doing that used to be fine (although not totally understandable IMO) because widgets didn't usually overlap with others, so the net result of all that restacking during ::size-allocate was always the same, it's been a while we're firmly out of that territory though :). > > As for controlling the way widgets restack their windows, it would be like > some sort of window stack manager? My fear is that would end up in gtkwindow > (just like popovers) and I am not sure this is an ideal solution either. Agreed, and will be short lived anyway if GdkWindows are meant to go eventually :). What I'm proposing is making widgets educated enough to avoid unneeded restacking.
(In reply to Carlos Garnacho from comment #13) > [...] > I think the problem here is that if gdk_window_show() is spammed in size > allocate, you effectively have widgets contending to be on top on each > resize, this is mostly useless (the situations where you really want visual > restacking are very contained, eg. GtkNotebook page switches, GtkStack > switches,...), and could lead to unnecessary processing (if recursing within > windowed children widgets raises the window, you'll be effectively causing > spurious invalidations as windows get on top of others. Ah, OK, yes, that's a very good point indeed. > [...] > Agreed, and will be short lived anyway if GdkWindows are meant to go > eventually :). What I'm proposing is making widgets educated enough to avoid > unneeded restacking. Problem is that even if no widget tries to raise its window, the order in which the widgets get realized still induces a stacking (because newly created windows get automatically placed on top of the window stack, at least on X11) For example, in the case of gedit with menu-traditional.ui (i.e. this bug), the popover is realized before the overlay, so the overlay window will always end up above the popover window, even if we manage to avoid any restacking. And gtk_window_restack_popovers() doesn't help here.
Created attachment 324760 [details] [review] [PATCH v3] popover: raise when showing Updated patch. 1. gtk_window_restack_popovers() would raise the popover window only if visible, but there is nothing wrong with restacking a window which is not visible, and that actually helps as the popover window is not necessarily mapped/visible when the restack is issued. 2. gtk_window_restack_popovers() is called only on gtk_window_size_allocate() which is not sufficient to guarantee that the popover window is placed on top of other widgets. => the attached patch tries to fix this by calling gtk_window_restack_popovers() from popover_map() and raising the popover windows even if the popover is not visible.
Review of attachment 324760 [details] [review]: Looks good :), and matches with what I said in comment #11 (stacking must happen on ::map), I missed it was popovers which failed to do that though... The GtkOverlay change might be separated to a different commit though. I anyway agree with the sentiment of killing extra whitespaces.
(In reply to Carlos Garnacho from comment #16) > Review of attachment 324760 [details] [review] [review]: > > Looks good :), and matches with what I said in comment #11 (stacking must > happen on ::map), I missed it was popovers which failed to do that though... > > The GtkOverlay change might be separated to a different commit though. I > anyway agree with the sentiment of killing extra whitespaces. Oh sorry, it was not meant to be included in the patch!
Comment on attachment 324760 [details] [review] [PATCH v3] popover: raise when showing Removed the whitespace clean and pusshed attachment 324760 [details] [review] as 5d34cf6 popover: raise when showing