xdg_popup.grab() and parent vs. commit ordering

[YaLTeR]

Smithay calls the compositor's grab() handler right away, wherein the compositor is supposed to check xdg-popup's parent for validity:

The parent of a grabbing popup must either be an xdg_toplevel surface or another xdg_popup with an explicit grab.

However, Qt layer-shell seems to grab the popup right before assigning it a parent, breaking this logic:

[ 392910.890] {Default Queue}  -> xdg_wm_base#3.get_xdg_surface(new id xdg_surface#52, wl_surface#45)
[ 392910.986] {Default Queue}  -> xdg_surface#52.get_popup(new id xdg_popup#54, nil, xdg_positioner#53)
[ 392911.014] {Default Queue}  -> xdg_popup#54.grab(wl_seat#15, 168583)
[ 392911.030] {Default Queue}  -> zwlr_layer_surface_v1#43.get_popup(xdg_popup#54)
[ 392911.089] {Default Queue}  -> wl_surface#45.commit()

[ 392911.209] {Default Queue}  -> xdg_popup#54.reposition(xdg_positioner#55, 1)
[ 392911.934] {Default Queue} xdg_popup#54.configure(2121, 25, 173, 129)
[ 392911.947] {Default Queue} xdg_surface#52.configure(168585)
[ 392911.959] {Default Queue}  -> xdg_surface#52.ack_configure(168585)
[ 392911.999] {Default Queue} xdg_popup#54.repositioned(1)
[ 392912.012] {Default Queue} xdg_popup#54.configure(2121, 25, 173, 129)
[ 392912.023] {Default Queue} xdg_surface#52.configure(168586)
[ 392912.082] {Default Queue}  -> xdg_surface#52.ack_configure(168586)
[ 392912.119] {Default Queue}  -> xdg_surface#52.set_window_geometry(0, 0, 173, 129)
[ 392912.150] {Default Queue}  -> xdg_surface#52.set_window_geometry(0, 0, 173, 129)
[ 392913.165] {Default Queue}  -> wl_surface#45.commit()

Therefore, perhaps Smithay should delay calling the grab() handler until the commit?

Apparently, this ordering works fine in kwin. If not, then this would be a Qt bug instead. I'm not sure the protocol is clear on this.

cc @cmeissl @ids1024

[outfoxxed]

However, Qt layer-shell seems to

Its any Qt context menu. My layer impl overrides the standard per-role things for attaching a menu, which should happen in the same order regardless of the surface's role.

Confirmed to work in KWin as well.

[YaLTeR]

Toplevel menus are unaffected because they set parent right in the get_popup() request:

[1205257.427] {Default Queue}  -> xdg_surface#53.get_popup(new id xdg_popup#55, xdg_surface#39, xdg_positioner#54)
[1205257.451] {Default Queue}  -> xdg_popup#55.grab(wl_seat#15, 207898)
[1205257.511] {Default Queue}  -> wl_surface#47.commit()

[outfoxxed]

It's odd that the handling varies by surface role, but that appears to be correct. XDG shell has special handling, everything else uses this: https://code.qt.io/cgit/qt/qtwayland.git/tree/src/client/qwaylandwindow.cpp?h=v6.9.0#n1812 (as of 6.9.0)

[YaLTeR]

It's because that argument is an xdg-surface and layer-shell surfaces aren't xdg-surfaces so they can't go there

[ids1024]

https://wayland.app/protocols/xdg-shell#xdg_popup says

The client must call wl_surface.commit on the corresponding wl_surface for the xdg_popup state to take effect.

In the top level documentation for xdg_popup. Which I guess suggests, given no further clarification, that this applies to all the (not very many) requests defined on xdg_popup, including ::grab.

So by that interpretation, deferring this until commit sounds right.

Perhaps it would be good to send an MR to wayland-protocols to make it a little clearer, though. The documentation for wl_surface itself has specific working for each request about how the double-buffering works.

[YaLTeR]

Fixed in #1817