Fix Xwayland keyboard grabs for applications like dmenu#221
Merged
paperbenni merged 1 commit intomainfrom Mar 13, 2026
Merged
Conversation
Added support for the zwp_xwayland_keyboard_grab_v1 protocol in the Wayland backend. This allows X11/Xwayland applications like dmenu to properly acquire exclusive keyboard grabs without dropping focus or requiring the user to hold down launcher keys. Integrated XWaylandKeyboardGrabState into WaylandState, implemented XWaylandKeyboardGrabHandler to route grabs to the correct window, and wired up the protocol handler with delegate_xwayland_keyboard_grab!. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds support for Smithay's zwp_xwayland_keyboard_grab_v1 protocol in the Wayland backend by wiring the new XWaylandKeyboardGrabState into WaylandState, implementing the XWaylandKeyboardGrabHandler to resolve keyboard focus from wl_surface to internal windows, and delegating the new protocol globally so Xwayland clients like dmenu can reliably obtain exclusive keyboard grabs. Sequence diagram for Xwayland dmenu keyboard grab using zwp_xwayland_keyboard_grab_v1sequenceDiagram
actor User
participant Compositor_WaylandState as Compositor_WaylandState
participant Xwayland
participant DmenuXwaylandApp as Dmenu_Xwayland_app
participant XWaylandKeyboardGrabProto as XWaylandKeyboardGrab_proto
User->>Compositor_WaylandState: Keybinding pressed
Compositor_WaylandState->>Xwayland: Launch dmenu Xwayland client
Xwayland->>DmenuXwaylandApp: Start X11 application
DmenuXwaylandApp->>Xwayland: Request exclusive keyboard grab
Xwayland->>XWaylandKeyboardGrabProto: zwp_xwayland_keyboard_grab_v1 request
XWaylandKeyboardGrabProto->>Compositor_WaylandState: keyboard_focus_for_xsurface(surface)
Compositor_WaylandState->>Compositor_WaylandState: win = window_id_for_surface(surface)
Compositor_WaylandState->>Compositor_WaylandState: window = window_index.get(win)
Compositor_WaylandState-->>XWaylandKeyboardGrabProto: KeyboardFocusTarget_Window(window)
XWaylandKeyboardGrabProto-->>Xwayland: Keyboard grab established
User->>DmenuXwaylandApp: Type characters
DmenuXwaylandApp-->>User: Receives all keystrokes without drops
Class diagram for updated WaylandState with XWaylandKeyboardGrab supportclassDiagram
class WaylandState {
+OutputManagerState output_manager_state
+DataDeviceState data_device_state
+XWaylandShellState xwayland_shell_state
+XWaylandKeyboardGrabState xwayland_keyboard_grab_state
+WlrLayerShellState wlr_layer_shell_state
+DmabufState dmabuf_state
+Option_dmabuf_global dmabuf_global
+KeyboardFocusTarget keyboard_focus_for_xsurface(surface)
+WindowId window_id_for_surface(surface)
+WindowIndexMap window_index
}
class XWaylandKeyboardGrabState {
+new_WaylandState(display_handle)
}
class XWaylandKeyboardGrabHandler {
<<interface>>
+keyboard_focus_for_xsurface(surface) KeyboardFocusTarget
}
class KeyboardFocusTarget {
<<enum>>
Window
}
class Window {
}
class WindowIndexMap {
<<map>>
WindowId ~ Window
}
WaylandState ..|> XWaylandKeyboardGrabHandler
WaylandState --> XWaylandKeyboardGrabState : has
WaylandState --> WindowIndexMap : has
WindowIndexMap --> Window : maps_to
XWaylandKeyboardGrabState ..> WaylandState : generic_new
KeyboardFocusTarget --> Window : wraps
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe changes add XWayland keyboard grab protocol support to the Wayland compositor backend by introducing state management, implementing the XWaylandKeyboardGrabHandler trait, and registering the corresponding Wayland delegate. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
keyboard_focus_for_xsurface, consider whether you can return a reference-basedKeyboardFocusTargetinstead of cloning the window to avoid potential unnecessary allocations ifWindowis large or clone-expensive. - In
WaylandState, you might want to group the newxwayland_keyboard_grab_statefield next to other Xwayland-related fields (e.g.,xwayland_shell_state) consistently in both the struct definition and initializer to keep related protocol state localized and easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `keyboard_focus_for_xsurface`, consider whether you can return a reference-based `KeyboardFocusTarget` instead of cloning the window to avoid potential unnecessary allocations if `Window` is large or clone-expensive.
- In `WaylandState`, you might want to group the new `xwayland_keyboard_grab_state` field next to other Xwayland-related fields (e.g., `xwayland_shell_state`) consistently in both the struct definition and initializer to keep related protocol state localized and easier to reason about.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/wayland/compositor/handlers.rs`:
- Around line 753-755: The current focus path uses window_id_for_surface ->
window_index.get(...) and drops the grab when window_index lacks entries for
unmanaged X11 overlays; update the logic in the handler so that after let win =
self.window_id_for_surface(surface)? you attempt window_index.get(&win) and, if
that returns None, fall back to checking the compositor's space mapping (e.g.
self.space or equivalent structure that holds mapped override-redirect/X11
overlay windows) to find the window record for win and return
Some(KeyboardFocusTarget::Window(cloned_window)) instead of failing; ensure you
only treat it as missing if neither window_index nor the space mapping contain
win so the keyboard grab is not dropped for overlay windows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6f0ad3e-896c-455a-b609-19b00dc27d61
📒 Files selected for processing (3)
src/backend/wayland/compositor/handlers.rssrc/backend/wayland/compositor/mod.rssrc/backend/wayland/compositor/state.rs
Comment on lines
+753
to
+755
| let win = self.window_id_for_surface(surface)?; | ||
| let window = self.window_index.get(&win)?; | ||
| Some(KeyboardFocusTarget::Window(window.clone())) |
There was a problem hiding this comment.
Overlay X11 windows are currently excluded from keyboard-grab focus lookup.
At Line 753, focus resolution goes through window_id_for_surface + window_index. Unmanaged X11 overlays (e.g., dmenu/override-redirect) are mapped in space but not indexed there, so this can return None and drop the grab.
Proposed fix
impl XWaylandKeyboardGrabHandler for WaylandState {
fn keyboard_focus_for_xsurface(
&self,
surface: &smithay::reexports::wayland_server::protocol::wl_surface::WlSurface,
) -> Option<Self::KeyboardFocus> {
- let win = self.window_id_for_surface(surface)?;
- let window = self.window_index.get(&win)?;
- Some(KeyboardFocusTarget::Window(window.clone()))
+ if let Some(window) = self
+ .window_id_for_surface(surface)
+ .and_then(|win| self.window_index.get(&win))
+ {
+ return Some(KeyboardFocusTarget::Window(window.clone()));
+ }
+
+ // Fallback for unmanaged X11 overlays mapped directly in Space.
+ self.space
+ .elements()
+ .find(|w| {
+ w.wl_surface().as_deref() == Some(surface)
+ || w
+ .surface_under((0.0, 0.0), WindowSurfaceType::ALL)
+ .map(|(hit_surface, _)| hit_surface == *surface)
+ .unwrap_or(false)
+ })
+ .cloned()
+ .map(KeyboardFocusTarget::Window)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let win = self.window_id_for_surface(surface)?; | |
| let window = self.window_index.get(&win)?; | |
| Some(KeyboardFocusTarget::Window(window.clone())) | |
| impl XWaylandKeyboardGrabHandler for WaylandState { | |
| fn keyboard_focus_for_xsurface( | |
| &self, | |
| surface: &smithay::reexports::wayland_server::protocol::wl_surface::WlSurface, | |
| ) -> Option<Self::KeyboardFocus> { | |
| if let Some(window) = self | |
| .window_id_for_surface(surface) | |
| .and_then(|win| self.window_index.get(&win)) | |
| { | |
| return Some(KeyboardFocusTarget::Window(window.clone())); | |
| } | |
| // Fallback for unmanaged X11 overlays mapped directly in Space. | |
| self.space | |
| .elements() | |
| .find(|w| { | |
| w.wl_surface().as_deref() == Some(surface) | |
| || w | |
| .surface_under((0.0, 0.0), WindowSurfaceType::ALL) | |
| .map(|(hit_surface, _)| hit_surface == *surface) | |
| .unwrap_or(false) | |
| }) | |
| .cloned() | |
| .map(KeyboardFocusTarget::Window) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/wayland/compositor/handlers.rs` around lines 753 - 755, The
current focus path uses window_id_for_surface -> window_index.get(...) and drops
the grab when window_index lacks entries for unmanaged X11 overlays; update the
logic in the handler so that after let win =
self.window_id_for_surface(surface)? you attempt window_index.get(&win) and, if
that returns None, fall back to checking the compositor's space mapping (e.g.
self.space or equivalent structure that holds mapped override-redirect/X11
overlay windows) to find the window record for win and return
Some(KeyboardFocusTarget::Window(cloned_window)) instead of failing; ensure you
only treat it as missing if neither window_index nor the space mapping contain
win so the keyboard grab is not dropped for overlay windows.
This was referenced Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added support for the
zwp_xwayland_keyboard_grab_v1protocol in the Smithay Wayland backend. This fixes an issue where Xwayland applications (like dmenu forks launched via keybindings) fail to secure an exclusive keyboard grab, leading to dropped keystrokes or requiring the user to hold down the launcher shortcut.Changes include:
XWaylandKeyboardGrabStatetoWaylandState.XWaylandKeyboardGrabHandlerforWaylandStateto properly map aWlSurfaceto its correspondingWindowusingwindow_id_for_surface.delegate_xwayland_keyboard_grab!macro to wire up the protocol globally.PR created automatically by Jules for task 689128807718661313 started by @paperbenni
Summary by Sourcery
Add support for the XWayland keyboard grab protocol in the Wayland backend to ensure XWayland clients can acquire proper exclusive keyboard focus.
New Features:
Summary by CodeRabbit