GNOME Bugzilla – Bug 786209
X11: GtkPopover positioning doesn't "avoid" CSD window shadows
Last modified: 2017-08-24 20:45:36 UTC
Created attachment 357490 [details] Screenshot showing the bug GtkPopover must be constrained to the parent window size on X11 (bug 747509). However, when determining where to place the popover, the parent window size including CSD and shadows is used. This means the popover can appear outside the window clickable area (but still within the shadow). Since the shadow is not clickable, this means part of the popover cannot be clicked, and input events will fall through to whatever is behind in the window stack. See attached screenshot for an example. In the screenshot, the popover window has opened partially outside the application window, but note it is still within the shadow area. If I click on buttons 1-6 it will work correctly. But clicking on 7,8,9 will not work (except for a tiny sliver at the top). The click will go to the desktop instead. Also note that this only happens when clicking the second to last row of cells. The bottom row of cells are close enough to the bottom that the popover will not fit within the window+shadow, and so the popover appears above the cell instead, as expected. The bug can be reproduced more easily by overriding the theme CSS to set extra large shadows, e.g. window decoration { box-shadow: 100px 100px }
IMO GtkPopover should use the border allocation of the window when deciding where to position itself, not the entire allocation including shadow, though it's not immediately evident to me how that could be done (beyond ugliness like e.g. using the allocation of its GtkBox child instead)
It's simpler than that. gtk_popover_update_position() doesn't care about the window's position, only its width/height. So, using gtk_window_get_size() seems superior to using gtk_widget_get_allocation(). The following patch makes this change, and so appears to fix this bug, without any regressions that I can find. Now, in gnome-sudoku, where previously the Popover would've gotten positioned downwards over the shadow and thus broken its buttons, now it gets positioned upwards, so the buttons work.
Created attachment 357520 [details] [review] Popover: Exclude shadow from available window size Using the allocation includes shadows in the size, which we use to assess where to position the Popover. This could break input on X11, where thePopover must be positioned within its parent window: shadows don’t receive input, but it was possible to end up positioning in them. Fix this by using the size from gtk_window_get_size(), which excludes shadows, CSD, etc. This ensures we position in a fully usable area.
I'll try to test this patch, thanks. A thing that sticks out to me is you apparently haven't changed the top/left origin of the area. Doesn't this mean the constraint area will now be the correct size, but aligned to the top-left of the shadow?
Comment on attachment 357520 [details] [review] Popover: Exclude shadow from available window size Yup, you seem to be correct. Applying a large negative box-shadow at the left of the popover test (tests/testpopover.c) and setting up the buttons to open the popover to the left indicates that the problem still occurs there.
Created attachment 357523 [details] [review] testpopover: Set right initial values for controls The CheckButtons and ComboBoxes were defaulting to empty, rather than to the initial value of the properties they control. Set the value on them.
Created attachment 357524 [details] [review] testpopover: Use HeaderBar to get CSD decorations This helps test whether the Popover positioning gets messed up by the presence of CSD shadow or other accessories around the content area.
(In reply to Daniel Boles from comment #1) > IMO GtkPopover should use the border allocation of the window when deciding > where to position itself, not the entire allocation including shadow, though > it's not immediately evident to me how that could be done (beyond ugliness > like e.g. using the allocation of its GtkBox child instead) The GtkBox thing doesn't seem to work, and (A) GTK+ 4 has gtk_widget_get_(border|margin|content|etc)_allocation() but GTK+ 3 does not (B) GTK+ 3 does have gtk_css_gadget_get_*_allocation() but GtkWindow does not use a gadget. Could we either add conversions of those functions to GTK+ 3, or if they're not going to be needed often enough to justify it, is there an equivalent way to do what they're doing already in GTK+ 3?
...or get_outer_allocation(), since get_(border|content|margin)_allocation() have just been removed from GTK+ 4. I'm trying to figure out whether it's possible to imitate this in GTK+ 3 using any existing (public or private) API.
What's wrong with gtk_widget_get_allocation + gtk_window_get_shadow_width?
Thanks for the reply, Timm. The answer is: probably nothing, but I just didn't realise _gtk_window_get_shadow_width() existed! I tend to forget about private headers. So, thanks for the pointer. So hopefully this is easily sorted by declaring a GtkBorder = { 0 }, doing get_shadow_width() to it, and adjusting the current overlap checks accordingly. I'll work on a patch, probably tonight.
Created attachment 357569 [details] [review] Popover: Include window shadows in overshoot calcs .update_position() enforces that non-Wayland platforms must position a Popover within its parent Window. We use the allocation of the Window to translate the position and check for overshoot on each of its sides. Calling Widget.get_allocation() of a CSD Window includes its shadows. But shadows were not excluded from the area in which we can position. Thus, Popovers could get positioned in the shadow of CSD windows, where, at least on X11, no input is received. Therefore, positioning a Popover over a shadow meant its child widgets within that area became unusable. Fix by calling Window.get_shadow() and including it in the overshoot on each side. This adjusts for how the allocation includes shadows, making overshoots with and without shadows the same. Thus, we avoid considering shadows as viable for positioning, favouring a side where input works. -- Thanks again for the direction, Timm. In my tests using testpopover, this produces identical overshoots for all sides and directions with (contrived) shadows on or off, and thus avoids positioning within them like we were. Still, if you can review and give it the OK, that'd be appreciated. Also, I haven't trawled through gtkpopover.c to figure out whether shadows might be biting us anywhere else. I guess we'll find out when we find out...
Review of attachment 357524 [details] [review]: (It didn't seem worth adding more controls to create a shadow... that's easy enough so long as we have CSD for the shadow CSS to work.)
Created attachment 357574 [details] [review] testpopover: Sync initial vals of controls & props The ComboBoxes were initially empty, rather than reflecting the initial values of the properties. The CheckButtons were only correct by chance. Fix this by setting the initial values on the widgets and binding them to the properties using SYNC_CREATE, so the two are always synced up. -- This seems like a cleaner way of doing it (and is used in other tests).
Review of attachment 357569 [details] [review]: ::: gtk/gtkpopover.c @@ +1043,3 @@ GtkWidget *widget = GTK_WIDGET (popover); GtkAllocation window_alloc; + GtkBorder window_shadow = { 0 }; The zero-init isn't necessary; _gtk_window_get_shadow_width() inits the out parameter in all cases.
Comment on attachment 357524 [details] [review] testpopover: Use HeaderBar to get CSD decorations why not
Comment on attachment 357569 [details] [review] Popover: Include window shadows in overshoot calcs ack'd by Matthias and Timm on IRC
Thanks for the report!