wayland/shell/xdg: Delay grab handler until pre-commit

Qt layer-shell calls grab() first, only then sets popup parent via
zwlr_layer_surface_v1::get_popup(). Before this commit, grab() would call the
handler immediately, which would see that the popup has no parent, and fail.
However, it works on other compositors.

The xdg_popup docs don't mention the desired behavior explicitly, though this
line could be interpreted in favor of "applying" the grab request at commit
time:

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

Fixes grabs from first-level popups on layer-shell Qt clients such as
pcmanfm-qt --desktop and lxqt-panel.

Author: YaLTeR

CHANGELOG.md

@@ -65,6 +65,7 @@ struct XdgPopupSurfaceRoleAttributes {
 -    configure_serial: Option<Serial>,
 -    current: PopupState,
 -    current_serial: Option<Serial>,
+-    committed: bool,
 -    last_acked: Option<PopupState>,
 +    last_acked: Option<PopupConfigure>,
     // ...

src/desktop/wayland/popup/manager.rs

@@ -9,7 +9,7 @@ use crate::{
 };
 use std::sync::{Arc, Mutex};
 use tracing::trace;
-use wayland_protocols::xdg::shell::server::{xdg_popup, xdg_wm_base};
+use wayland_protocols::xdg::shell::server::xdg_wm_base;
 use wayland_server::{protocol::wl_surface::WlSurface, Resource};
 
 use super::{PopupGrab, PopupGrabError, PopupGrabInner, PopupKind};
@@ -74,23 +74,7 @@ impl PopupManager {
         );
 
         match popup {
-            PopupKind::Xdg(ref xdg) => {
-                let surface = xdg.wl_surface();
-                let committed = with_states(surface, |states| {
-                    states
-                        .data_map
-                        .get::<XdgPopupSurfaceData>()
-                        .unwrap()
-                        .lock()
-                        .unwrap()
-                        .committed
-                });
-
-                if committed {
-                    surface.post_error(xdg_popup::Error::InvalidGrab, "xdg_popup already is mapped");
-                    return Err(PopupGrabError::InvalidGrab);
-                }
-            }
+            PopupKind::Xdg(ref _xdg) => (),
             PopupKind::InputMethod(ref _input_method) => {
                 return Err(PopupGrabError::InvalidGrab);
             }

src/wayland/shell/xdg/handlers/surface/popup.rs

@@ -4,7 +4,7 @@ use crate::{
     input::SeatHandler,
     utils::Serial,
     wayland::{
-        compositor,
+        compositor::{self, with_states},
         shell::xdg::{PopupCachedState, SurfaceCachedState, XdgPopupSurfaceData, XdgPositionerUserData},
     },
 };
@@ -45,7 +45,15 @@ where
 
                 let serial = Serial::from(serial);
 
-                XdgShellHandler::grab(state, handle, seat, serial);
+                with_states(handle.wl_surface(), |states| {
+                    states
+                        .data_map
+                        .get::<XdgPopupSurfaceData>()
+                        .unwrap()
+                        .lock()
+                        .unwrap()
+                        .requested_grab = Some((seat, serial));
+                });
             }
             xdg_popup::Request::Reposition { positioner, token } => {
                 let handle = crate::wayland::shell::xdg::PopupSurface {

src/wayland/shell/xdg/mod.rs

@@ -426,11 +426,6 @@ xdg_role!(
     /// for the xdg_popup state to take effect.
     #[derive(Debug)]
     XdgPopupSurfaceRoleAttributes {
-        // FIXME: "parent" should move into PopupCachedState, and "committed" should be removed in
-        // favor of something like PopupCachedState::last_acked.is_some(). Along the way, popup
-        // grabs should be made to apply at commit time, rather than immediately upon
-        // xdg_popup.grab(). This is required to fix Qt layer-shell popup grabs for example.
-
         /// Holds the parent for the xdg_popup.
         ///
         /// The parent is allowed to remain unset as long
@@ -443,12 +438,8 @@ xdg_role!(
         /// the xdg_popup role when no parent is set.
         pub parent: Option<wl_surface::WlSurface>,
 
-        /// Defines if the surface has received at least one commit
-        ///
-        /// This can be used to check for protocol errors, like
-        /// checking if a popup requested a grab after it has been
-        /// mapped.
-        pub committed: bool
+        /// If the popup requested a grab, this is the seat and serial.
+        requested_grab: Option<(wl_seat::WlSeat, Serial)>
     },
     /// Represents the xdg_popup pending state
     #[derive(Debug, Default, Clone)]
@@ -1969,15 +1960,15 @@ impl PopupSurface {
     ) {
         let _span = trace_span!("xdg-popup pre-commit", surface = %surface.id()).entered();
 
-        let error = compositor::with_states(surface, |states| {
+        let res = compositor::with_states(surface, |states| {
             let mut role = states
                 .data_map
                 .get::<XdgPopupSurfaceData>()
                 .unwrap()
                 .lock()
                 .unwrap();
             if role.parent.is_none() {
-                return Some((
+                return Err((
                     xdg_surface::Error::NotConstructed,
                     "xdg_popup must have parent before mapping",
                 ));
@@ -1990,6 +1981,19 @@ impl PopupSurface {
             // surface is not allowed to attach a buffer without acking the initial configure.
             let had_buffer_before = pending.last_acked.is_some();
 
+            let grab = role.requested_grab.take();
+            if grab.is_some() && had_buffer_before {
+                let popups = &state.xdg_shell_state().known_popups;
+                if let Some(popup) = popups.iter().find(|handle| handle.wl_surface == *surface) {
+                    popup
+                        .shell_surface
+                        .post_error(xdg_popup::Error::InvalidGrab, "tried to grab after being mapped");
+                } else {
+                    error!("surface missing from known popups");
+                }
+                return Ok(None);
+            }
+
             let mut guard_surface = states.cached_state.get::<SurfaceAttributes>();
             let has_buffer = match &guard_surface.pending().buffer {
                 Some(BufferAssignment::NewBuffer(_)) => true,
@@ -2002,7 +2006,7 @@ impl PopupSurface {
 
             if has_buffer {
                 let Some(last_acked) = role.last_acked else {
-                    return Some((
+                    return Err((
                         xdg_surface::Error::UnconfiguredBuffer,
                         "must ack the initial configure before attaching buffer",
                     ));
@@ -2031,19 +2035,31 @@ impl PopupSurface {
                 *guard_popup.pending() = Default::default();
             }
 
-            None
+            Ok(grab)
         });
 
-        if let Some((error, msg)) = error {
-            let popups = &state.xdg_shell_state().known_popups;
-            if let Some(popup) = popups.iter().find(|handle| handle.wl_surface == *surface) {
-                let data = popup
-                    .shell_surface
-                    .data::<self::handlers::XdgShellSurfaceUserData>()
-                    .unwrap();
-                data.xdg_surface.post_error(error, msg);
-            } else {
-                error!("surface missing from known popups");
+        match res {
+            Ok(Some((seat, serial))) => {
+                let popups = &state.xdg_shell_state().known_popups;
+                if let Some(popup) = popups.iter().find(|handle| handle.wl_surface == *surface) {
+                    let popup = popup.clone();
+                    XdgShellHandler::grab(state, popup, seat, serial);
+                } else {
+                    error!("surface missing from known popups");
+                }
+            }
+            Ok(None) => (),
+            Err((error, msg)) => {
+                let popups = &state.xdg_shell_state().known_popups;
+                if let Some(popup) = popups.iter().find(|handle| handle.wl_surface == *surface) {
+                    let data = popup
+                        .shell_surface
+                        .data::<self::handlers::XdgShellSurfaceUserData>()
+                        .unwrap();
+                    data.xdg_surface.post_error(error, msg);
+                } else {
+                    error!("surface missing from known popups");
+                }
             }
         }
     }