swaywm / sway

i3-compatible Wayland compositor
https://swaywm.org
MIT License
14.55k stars 1.11k forks source link

Sway does not handle subsurfaces that extend beyond the parent's bounding box very well (common in gtk applications) #4924

Closed tmccombs closed 2 years ago

tmccombs commented 4 years ago

Relevant portion:

2020-01-18 02:15:28 - [sway/desktop/transaction.c:489] Transaction 0x558676e2c7f0 is ready
2020-01-18 02:15:28 - [sway/desktop/transaction.c:280] Applying transaction 0x558676e2c7f0
2020-01-18 02:15:32 - [types/seat/wlr_seat_pointer.c:362] button_count=1 grab_serial=0 serial=132
2020-01-18 02:15:32 - [sway/desktop/transaction.c:411] Transaction 0x558676e2c7f0 committing with 1 instructions
2020-01-18 02:15:32 - [sway/desktop/transaction.c:280] Applying transaction 0x558676e2c7f0
2020-01-18 02:15:32 - [types/seat/wlr_seat_pointer.c:362] button_count=0 grab_serial=132 serial=133
2020-01-18 02:15:38 - [sway/input/cursor.c:576] denying request to set cursor from unfocused client
2020-01-18 02:15:46 - [sway/ipc-server.c:334] Sending window::focus event

Steps to reproduce:

Run the popover example code found here: https://python-gtk-3-tutorial.readthedocs.io/en/latest/popover.html#menu-popover (reproduced here):

It is more obvious if you change the border width on line 46 (for the app window) to something smaller, like 10.

Once the window opens switch to floating mode (Mod+Shift+space in default config). Then click on the button to make the popover appear. Although the popover displays outside of the parent window, it doesn't seem to receive mouse events, and if focus_follows_mouse is enabled, as soon as you move over the popover, the parent window (and the popover itself) loses focus. Clicking also seems to send the click event to the window underneath the popover.

Update

After more investigation I've determined this is because GTK uses subsurfaces, rather than popup surfaces or toplevel surfaces for menus and popovers. And sway seems to have some assumptions that subsurfaces fit inside the bounding box of the parent top-level surface. This results in the following undesirable behavior:

emersion commented 4 years ago

Can you try master?

tmccombs commented 4 years ago

Yep. Still happens on master.

tmccombs commented 4 years ago

Oh, I also forgot to mention, if the popover overlaps with the parent window, then the window border of the parent window is drawn on top of the popover.

tmccombs commented 4 years ago

Is there other information that would be helpful? I've tried looking at the gtk/gdk source code but haven't been able to figure out how the overlay maps to the wayland protocols yet, or how it is different than a GtkMenu (which doesn't have this problem).

ascent12 commented 4 years ago

It does appear to be sway or wlroots related. The popup is not receiving any wl_pointer (or any) wayland messages with WAYLAND_DEBUG=1.

tmccombs commented 4 years ago

It also seems to work as you would expect in weston.

tmccombs commented 4 years ago

It's not just popovers. I think it is any "popup" type window. I can reproduce the issue with the following program, which just creates a new window with the "popup" type and transient for the main window:

import gi
gi.require_version('Gtk', '3.0')
from gi.repository import Gio, Gtk

class AppWindow(Gtk.ApplicationWindow):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

        outerbox = Gtk.Box(spacing=6, orientation=Gtk.Orientation.VERTICAL)
        self.add(outerbox)
        outerbox.show()

        button = Gtk.Button(label="Click me")
        button.connect('clicked', open_window)
        button.show()

        outerbox.pack_end(button, False, True, 0)

        self.set_border_width(10)

def open_window(attachto):
    button = Gtk.Button(label="Click me 2")
    button.set_size_request(300, 30)
    button.connect('clicked', lambda x: print("Clicked 2"))
    win = Gtk.Window.new(Gtk.WindowType.POPUP)
    win.attached_to = attachto
    win.set_transient_for(attachto.get_toplevel())
    win.add(button)
    win.show_all()

if __name__ == "__main__":
    mainwindow = AppWindow(title="Main Window")
    mainwindow.connect('destroy', Gtk.main_quit)
    mainwindow.show_all()

    Gtk.main()

It appears to be for all transient windows that extend beyond the parent window.

tmccombs commented 4 years ago

After some debugging, I've discovered that in wlr_xdg_surface_surface_at, the surface->popups list is empty. Which seems strange to me. Even after looking through the gtk and gdk source code I can't figure out what that would be. At first I though maybe it was creating a subsurface instead of a popup, but I can't find any subsurfaces that don't have a width or height other than 0 for the test application. Setting breakpoints on the xdg_surface_handle_get_popup functions also doesn't ever break.

tmccombs commented 4 years ago

Ok, so I think it is creating a subsurface, not a popup surface. Here's where I can I think it is creating the new window in the output with WAYLAND_DEBUG=1 (on the application):

[644382.633]  -> wl_compositor@6.create_surface(new id wl_surface@32)
[644382.771]  -> wl_subcompositor@7.get_subsurface(new id wl_subsurface@33, wl_surface@32, wl_surface@24)
[644382.800]  -> wl_subsurface@33.set_position(0, 0)
[644383.073]  -> wl_shm@4.create_pool(new id wl_shm_pool@38, fd 16, 40800)
[644383.101]  -> wl_shm_pool@38.create_buffer(new id wl_buffer@39, 0, 300, 34, 1200, 0)
[644384.005]  -> wl_surface@32.attach(wl_buffer@39, 0, 0)
[644384.042]  -> wl_surface@32.set_buffer_scale(1)
[644384.050]  -> wl_surface@32.damage(0, 0, 300, 34)
[644384.066]  -> wl_compositor@6.create_region(new id wl_region@40)
[644384.074]  -> wl_region@40.add(0, 0, 300, 34)
[644384.091]  -> wl_surface@32.set_opaque_region(wl_region@40)
[644384.098]  -> wl_region@40.destroy()
[644384.105]  -> wl_surface@32.set_input_region(nil)
[644384.132]  -> wl_surface@32.frame(new id wl_callback@41)
[644384.149]  -> wl_surface@32.commit()
tmccombs commented 4 years ago

Good news: I found the problem. Bad news: I'm not sure what the best way to fix it is (mostly because of my unfamiliarity with sway's codebase).

So the issue is in the container_at function, and the functions it calls. So, surface_is_popup returns false for these subsurfaces. At first I thought that was wrong, but then I realized, that might not actually be what we want since I don't know if these popovers should have higher priority than floating windows above them. So, the problem is actually in how it checks the location for normal containers.

I think floating_container_at is definitely broken. It should probably call surface_at_view instead of just relying on the bounding box of the floating container itself. But what if the floating container isn't a view, and has a subsurface that extends beyond the floating container's bounding box?

And then I'm not sure about tiling windows. If a tiling window has a subsurface that extend beyond it's bounding box, should that receive events events only if the parent view is focused?

I'd be happy to implement a fix, but would like a little direction from a maintainer on what my approach should be.

I haven't figured out why the border is drawn on top of the subsurface yet, maybe that should be a different issue?

tmccombs commented 4 years ago

I'm also confused about this line:

    // Tiling (focused)
    if (focus && focus->view && !is_floating) {

Why is there a !is_floating there?

tmccombs commented 4 years ago

This gnome bug is related: https://bugzilla.gnome.org/show_bug.cgi?id=759738

Apparantly gtk now uses subsurfaces rather than popups in many cases. (for menus, tooltips, popovers, etc.)

I also discovered while playing around with this some more that if a subsurface of a tiled window extends beyond the applications bounding box it isn't rendered outside the bounding box at all. This can mean that menus aren't fully visible.

This seems like it might be a bigger task to fix than I thought. Maybe subsurfaces that extend beyond the bounding box of the parents should be marked somehow, and then treated kind of like popup windows?

tmccombs commented 4 years ago

@RyanDwyer @ddevault what are your thoughts?

RyanDwyer commented 4 years ago

We've always had problems with this. It's been difficult to get right for all cases.

I wonder if it would be easier with a floating window manager style approach for this part. Eg. store pointers to all surfaces, no matter their type, in a list along with their global coordinates. Include containers in the list too. Keep the list sorted by z-index similar to how a floating window manager would do it. Then for surface_at just traverse the list linearly from the top until you find something that intersects the coordinate. Keeping the list up to date could be tricky though, especially with transactions involved.

Why is there a !is_floating there?

Views are either floating or tiling. As per the comment on the line above it's checking if the focused view is tiling, or in other words, not floating.

tmccombs commented 4 years ago

Views are either floating or tiling. As per the comment on the line above it's checking if the focused view is tiling, or in other words, not floating.

Sorry, I should have been more specific. I understand that the !is_floating is checking if it is tiling. My question is, why do we call surface_at_view on the focused view if it is tiling, but not if it is floating? This seems especially strange since subsurfaces apparantly aren't even drawn beyond their bounding boxes.

RyanDwyer commented 4 years ago

Because everything floating has already been handled by the call to floating_container_at just before it. If it reaches the tiling checks then it is certain that the coordinates are not over a floating container. The !is_floating condition is just an optimisation to avoid rechecking the focused view if it happens to be floating.

tmccombs commented 4 years ago

My PR was closed in favor of clipping surfaces to the rectangle of the view (#5451). I think that will make this problem even worse.

This may be related to https://github.com/Alexays/Waybar/issues/771. where if the layer is waybar is "bottom", then tooltips are hidden and systray popups are truncated (see screenshot). I'm worried that if #5451 is merged, even if the layer is set to top these pop-ups, which I think use subsurfaces won't work.

image

emersion commented 4 years ago

Yes, this is the correct behaviour. Waybar needs to switch to xdg-popup instead of subsurfaces.

tmccombs commented 4 years ago

Waybar needs to switch to xdg-popup instead of subsurfaces.

That means it can't use GTK3 (at least not for popups). This behavior will be fixed in GTK4, but it sounds like they aren't willing to backport using xdg-popup to GTK3, and GTK4 hasn't been released yet. And it will probably take while for applications to migrate to GTK4, and some might not migrate at all.

emersion commented 4 years ago
  1. I don't see why that's the case. gtk-layer-shell supports xdg-popup.
  2. It doesn't seem like a good idea to fix GTK's limitations by adding workarounds in compositors.
tmccombs commented 4 years ago

gtk-layer-shell supports xdg-popup

I'm not sure what you mean by that. I don't see anything in gtk-layer-shell that explicitly supports xdg-popup. It just takes a GtkWindow, and GtkWindows created using API's for Popovers and tooltips use subsurfaces, not xdg-popup in gtk3. I've looked into what it would take to force Gtk to use xdg-popup instead of subsurfaces in an application, and it would basically require re-implementing the tooltip and popover apis.

It doesn't seem like a good idea to fix GTK's limitations by adding workarounds in compositors.

This particular behaviour seems to be underspecified by wayland. From my reading both Gtk and sway's interpretations are valid. I can try bringing this up with Gtk again. But the problem is Gtk can make a similar argument "It doesn't seem like a good idea to fix Sway's limitations by making a large change that might break applications in other compositors [that we care about more]."

tmccombs commented 4 years ago

For reference:

https://gitlab.gnome.org/GNOME/gtk/-/issues/1097

tmccombs commented 4 years ago

From the above issue:

Note that popovers must move alongside their parent widgets, unlike menus, which in practice don't. Working around bugs in compositors unlikely to be a motivation for spending time on this for anyone in the gtk team.

https://gitlab.gnome.org/GNOME/gtk/-/issues/1097#note_867752

emersion commented 4 years ago

I'm not sure what you mean by that. I don't see anything in gtk-layer-shell that explicitly supports xdg-popup.

https://github.com/wmww/gtk-layer-shell/blob/ae088382b4be876e1bba0432e977ff60d1b1ffda/src/xdg-popup-surface.c#L24

This particular behaviour seems to be underspecified by wayland.

Yes: there's no guarantee subsurfaces outside of the window geometry will be displayed. It may be hidden by another surface, or may be hidden because off-screen, or may be hidden because any other reason.

From my reading both Gtk and sway's interpretations are valid.

No. If GTK needs to display a surface outside of the window geometry and over other surfaces, using a subsurface is incorrect.

tmccombs commented 4 years ago

Well this is embarrassing. 'After reading, the discussion in the Gtk issue I looked at the output from WAYLAND_DEBUG=1 waybar and it looks like it may actually be using xdg-popup anyway, which means this particular behavior may be a different bug, possibly relating to how popups are rendered for layers? I'll open a seperate bug for that.

kennylevinsen commented 4 years ago

The options at this point:

  1. Change Gtk+ to create popovers in a way that by the spec guarantees the expected behavior (e.g. xdg-popup).
  2. Change the spec to make Gtk+'s assumptions guaranteed, and update sway to comply with the spec change.
  3. Leave the spec, and thus sway, as is. In other words, do nothing.

It seems like this issue is not priority for the Gtk team, as they're understandably focused on Gtk4 (which already fixes this). Things also started off on the wrong foot, which does not help.

If the spec is not changed, and sway's current interpretation remains correct/passable, then it is healthier for the Wayland community to leave it as is in order to create a more heterogenous compositor environment which helps applications and toolkits identify incorrect assumptions about protocol details.

Wayland applications and toolkits must never assume anything other than what is explicitly written in the protocol specifications, and a compositor should never concern itself with applications and frameworks but instead just implement the exact details of its supported protocols.

tmccombs commented 4 years ago

Change the spec to make Gtk+'s assumptions guaranteed, and update sway to comply with the spec change.

I've created an issue with wayland-protocols to clarify this issue: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/issues/24

Riteo commented 2 years ago

What's the status of this issue? On Godot due to some architectural limitations we can't implement xdg_popups (we can't guarantee that the popups get deleted in stack order) and so we'll have to either work with subsurfaces or nothing. Also they map better anyways to what it expects (random freeform borderless windows without any automatic positioners or stacking) but this issue makes the situation a little more troublesome when it comes to usability on sway.

tmccombs commented 2 years ago

I'm pretty sure the situation is still that subsurfaces that extend beyond the bounds of the parent window are clipped. At least, if they are tiled.

kennylevinsen commented 2 years ago

What's the status of this issue?

No change planned.

On Godot due to some architectural limitations we can't implement xdg_popups

If what you are trying to implement in Godot is a context menu or mouseover thing, then xdg_popup is the only interface that should be considered. Subsurfaces are unsuitable for such use even in compositors which allow overflowing the parent as they lack things like position constraints that are required to make a context menu or mouseovers usable.

Riteo commented 2 years ago

@kennylevinsen you are correct by saying that godot uses them primarily for context menus and mouseover things, but they aren't their only use, as they reuse a component which instantiates "normal" borderless windows which the user can manipulate to their liking, that have a very different structure than xdg_popups (godot expects "full" trees while xdg_popups stack relative to an xdg_toplevel) with mechanisms and limitations (like with in-between-stack deletion) that can't be overridden.

Regarding the positioner and constraint, in the current godot windowing api we can't infer any good data for setting them and godot repositions everything automatically anyways.

In conclusion, xdg_popups are too "automatic" and godot needs a more "low-level" windowing primitive, which subsurfaces would cover neatly IMO.

Edit: forgot to say that if this isn't the right place to talk about this, just let me know and I'll move into the respective ticket.

kennylevinsen commented 2 years ago

godot uses them primarily for context menus and mouseover things, but they aren't their only use

If it is supposed to be part of the parent window's content rather than a distinct entity, then subsurfaces are fine. They are no good for creating new window entities of any type.

Regarding the positioner and constraint, in the current godot windowing api we can't infer any good data for setting them and godot repositions everything automatically anyways.

How does this repositioning work? Wayland clients do not have access to information that allows them to position popups correctly in response to external constraints. For example, there is intentionally no way to know where a window is placed, making it impossible to know if a popup needs to be repositioned to not be cut off by output geometries.

In conclusion, xdg_popups are too "automatic" and godot needs a more "low-level" windowing primitive, which subsurfaces would cover neatly IMO.

"low-level window primitives" that allows a client to do whatever is against the general Wayland philosophy, a big part of which is to keep all policy decisions (such as popup constraining) within the Wayland display server rather than up to clients.

The best forum for discussions around existing Wayland protocols as well as design of new ones is either https://gitlab.freedesktop.org/wayland/wayland-protocols/ or #wayland on irc.oftc.net. Feel free to open a discussion in either of those places.

Riteo commented 2 years ago

They are no good for creating new window entities of any type.

I see.

For example, there is intentionally no way to know where a window is placed, making it impossible to know if a popup needs to be repositioned to not be cut off by output geometries.

I had no idea that positioners handled output borders. Godot only repositioned borderless windows relative to itself on resize, it has no idea about output borders.

"low-level window primitives" that allows a client to do whatever is against the general Wayland philosophy, a big part of which is to keep all policy decisions (such as popup constraining) within the Wayland display server rather than up to clients.

Right, I can see what you mean.

Feel free to open a discussion in either of those places.

Sure, once I find some time I'll at least mention this.

Anyways, in the worst case, Godot has a single-window mode that I can always fallback to, so this isn't absolutely vital.

kennylevinsen commented 2 years ago

popups get deleted in stack order

https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/167 has been opened to relax the destroy order requirements.

Riteo commented 2 years ago

@kennylevinsen cool! This would still imply that popups are only "stacked", without children, popup trees or anything, right?