fix: Support Xwayland exclusive keyboard grabs#244
Conversation
…e Smithay's `keyboard_handle.is_grabbed()` in the keyboard input handler to delegate input to active keyboard grabs (e.g., from `zwp_xwayland_keyboard_grab_v1` used by rofi or dmenu). This replaces the fragile, manual suppression of WM shortcuts based on window types (Launcher/Overlay), ensuring full protocol compatibility for exclusive keyboard access. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
|
👋 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 GuideAdjusts Wayland keyboard handling to rely on Smithay keyboard grabs instead of window classification for determining when WM shortcuts are allowed, and removes the now-unused shortcut suppression helper. Sequence diagram for updated Wayland keyboard shortcut handlingsequenceDiagram
participant InputBackend
participant handle_keyboard
participant KeyboardHandle
participant WaylandState
participant FocusTarget
participant LayerSurfaceCachedState
InputBackend->>handle_keyboard: keyboard_key_event
handle_keyboard->>KeyboardHandle: is_grabbed()
alt keyboard is grabbed
handle_keyboard-->>handle_keyboard: wm_shortcuts_allowed = false
else keyboard not grabbed
handle_keyboard->>KeyboardHandle: current_focus()
KeyboardHandle-->>handle_keyboard: KeyboardFocusTarget
alt focus is None
handle_keyboard-->>handle_keyboard: wm_shortcuts_allowed = true
else focus is Window
handle_keyboard-->>handle_keyboard: wm_shortcuts_allowed = true
else focus is WlSurface
handle_keyboard->>LayerSurfaceCachedState: with_states(surface)
LayerSurfaceCachedState-->>handle_keyboard: keyboard_interactivity
alt keyboard_interactivity == Exclusive
handle_keyboard-->>handle_keyboard: wm_shortcuts_allowed = false
else keyboard_interactivity != Exclusive
handle_keyboard-->>handle_keyboard: wm_shortcuts_allowed = true
end
else focus is Popup
handle_keyboard-->>handle_keyboard: wm_shortcuts_allowed = false
end
end
handle_keyboard-->>InputBackend: process key with wm_shortcuts_allowed
Class diagram for updated Wayland keyboard and window classificationclassDiagram
class WaylandState {
// classify_window(window: Window) -> WindowType
// should_suppress_shortcuts_for(window: Window) -> bool (removed)
}
class Window {
}
class WindowType {
<<enumeration>>
Normal
Overlay
Launcher
Unmanaged
Dying
}
WaylandState --> WindowType : classify_window
WaylandState --> Window : operates_on
class KeyboardHandle {
is_grabbed() bool
current_focus() KeyboardFocusTarget
}
class KeyboardFocusTarget {
<<enumeration>>
Window
WlSurface
Popup
None
}
KeyboardHandle --> KeyboardFocusTarget : current_focus
class LayerSurfaceCachedState {
current() LayerSurfaceState
}
class LayerSurfaceState {
keyboard_interactivity KeyboardInteractivity
}
class KeyboardInteractivity {
<<enumeration>>
Exclusive
Other
}
LayerSurfaceCachedState --> LayerSurfaceState : current
LayerSurfaceState --> KeyboardInteractivity : uses
class handle_keyboard {
handle_keyboard(state: WaylandState, keyboard_handle: KeyboardHandle, event: KeyboardKeyEvent)
}
class KeyboardKeyEvent {
key_code() u32
state() KeyState
}
class KeyState {
}
handle_keyboard --> WaylandState : reads_state
handle_keyboard --> KeyboardHandle : queries_focus_and_grab
handle_keyboard --> KeyboardKeyEvent : reads_key
handle_keyboard --> LayerSurfaceCachedState : with_states(surface)
LayerSurfaceCachedState <.. handle_keyboard : used_for_keyboard_interactivity
WaylandState ..|> handle_keyboard : no_longer_called_should_suppress_shortcuts_for
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved window-classification-based shortcut suppression and integrated Smithay's keyboard-shortcuts-inhibit state; keyboard handling now decides shortcut allowance from grab state, inhibitor state, and focus target type, and XWayland grab focus fallback was adjusted to allow surface-level grabs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Compositor as WaylandState
participant Inhibit as KeyboardShortcutsInhibitState
participant SurfaceOrWin as FocusTarget
Client->>Compositor: Key event (press)
Compositor->>Compositor: check keyboard_handle.is_grabbed()
alt grabbed
Compositor-->>Client: deliver (shortcuts disabled)
else not grabbed
Compositor->>Inhibit: keyboard_shortcuts_inhibited()?
alt inhibited
Compositor-->>Client: deliver (shortcuts disabled)
else not inhibited
Compositor->>SurfaceOrWin: determine focus target (Window/WlSurface/Popup)
alt Window
Compositor-->>Client: deliver (shortcuts allowed)
else WlSurface (non-exclusive)
Compositor-->>Client: deliver (possible shortcuts)
else Popup
Compositor-->>Client: deliver (shortcuts disabled)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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:
- The new
wm_shortcuts_allowedlogic now treats any keyboard grab (keyboard_handle.is_grabbed()) as fully suppressing WM shortcuts; if Smithay distinguishes between exclusive and non-exclusive grabs, consider checking specifically for exclusive grabs to avoid over-suppressing shortcuts. - With
should_suppress_shortcuts_forremoved,WindowType::Overlay/Launcher/Unmanagedare no longer used for shortcut routing; if these types still have semantic meaning elsewhere, consider whether any of that behavior should be preserved for non-grabbed windows or if the classifications can be simplified.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `wm_shortcuts_allowed` logic now treats any keyboard grab (`keyboard_handle.is_grabbed()`) as fully suppressing WM shortcuts; if Smithay distinguishes between exclusive and non-exclusive grabs, consider checking specifically for exclusive grabs to avoid over-suppressing shortcuts.
- With `should_suppress_shortcuts_for` removed, `WindowType::Overlay`/`Launcher`/`Unmanaged` are no longer used for shortcut routing; if these types still have semantic meaning elsewhere, consider whether any of that behavior should be preserved for non-grabbed windows or if the classifications can be simplified.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@jules the problem is still happening, maybe it is somewhere else |
|
@jules |
Good catch! It turns out |
|
@jules this is not just affecting keys which have window manager shortcuts. even keys which have no window manager shortcuts do not get passed to rofi (A NATIVE WAYLAND APPLICATION, NOT RUNNING IN XWAYLAND). Fuzzel straight up closes without doing anything, likely because the key grab fails |
…\n\n- Removed ad-hoc manual WM shortcut suppression based on window type heuristics.\n- Integrated Smithay's `zwp_keyboard_shortcuts_inhibit_v1` protocol to respect native Wayland shortcut inhibitors.\n- Updated `XWaylandKeyboardGrabHandler` to correctly fallback to the raw `WlSurface` for unmanaged windows or dummy surfaces, preventing `dmenu` or `rofi` grabs from being silently dropped.\n- Updated `handle_keyboard` to natively disable WM shortcuts globally when `keyboard_handle.is_grabbed()` is true or when `seat.keyboard_shortcuts_inhibited()` is true. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
![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/wayland/input/keyboard/mod.rs`:
- Around line 27-39: The WlSurface match arm currently assumes every WlSurface
has LayerSurfaceCachedState and unconditionally calls
.get::<LayerSurfaceCachedState>(), which panics for raw grab surfaces; instead,
change the WlSurface branch in the keyboard focus check (the match on
keyboard_handle.current_focus()) to first try to fetch LayerSurfaceCachedState
via with_states(s, |states|
states.cached_state.get::<LayerSurfaceCachedState>()), and if that returns
Some(layer_state) read layer_state.current().keyboard_interactivity and compare
to KeyboardInteractivity::Exclusive, but if it returns None (raw grab surface)
treat it as non-exclusive (i.e., allow shortcuts/return true). This ensures
KeyboardFocusTarget::WlSurface is split into layer-backed vs raw grab handling
without panicking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2957f186-50f1-42c1-97a2-8b7dd991b426
📒 Files selected for processing (6)
src/backend/wayland/compositor/handlers.rssrc/backend/wayland/compositor/mod.rssrc/backend/wayland/compositor/state.rssrc/wayland/input/keyboard/mod.rstest_inhibitor.rstest_xwayland_grab.rs
✅ Files skipped from review due to trivial changes (2)
- test_inhibitor.rs
- test_xwayland_grab.rs
|
@jules In |
…itors\n\n- Fixes a panic in `handle_keyboard` where `LayerSurfaceCachedState` was retrieved blindly from any `WlSurface` focus target. Now uses `.has::<LayerSurfaceCachedState>()` safely.\n- Fully implement `zwp_keyboard_shortcuts_inhibit_v1` using `KeyboardShortcutsInhibitState`, allowing native Wayland applications like `fuzzel` or native `rofi` to explicitly grab the keyboard. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
1 participant
Fixes exclusive keyboard grabs (like those used by rofi or dmenu via Xwayland) on the Wayland backend by properly routing events based on active Smithay keyboard grabs instead of ad-hoc window type classification.
PR created automatically by Jules for task 11526415645579586335 started by @paperbenni
Summary by Sourcery
Route keyboard shortcut handling based on actual Wayland keyboard grabs rather than ad-hoc window classification for Xwayland clients.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes
Tests