fix(wayland): shifted-keysym drops and a present-failure key desync by bearyjd · Pull Request #429 · emoon/rust_minifb

bearyjd · 2026-08-28T11:36:41Z

Two real Wayland keyboard bugs found while building a remote-desktop client
(navette) that forwards physical key state over the network -- both make
the difference between "one dropped event" and "a stuck key" much sharper
than they would be for a purely local app, which is likely why they hadn't
surfaced before.

1. Shifted-level keysyms are silently dropped

handle_key's match table only covers unshifted keysyms (lowercase
letters, unshifted punctuation). XKB resolves a key through whichever
shift level is currently active, so every shifted keysym -- capital
letters, !, @, {, etc. -- falls through to _ => return and never
reaches set_key_state.

Two consequences on a real keyboard:

Shift+key does not register at all (the press itself is dropped).
A press/release pair that straddles a Shift transition -- ordinary
typing rollover: press a letter, press Shift for the next word, release
the first letter while Shift is still down -- drops only the release.
The key then reads as permanently held in KeyHandler's state.

Fix maps each shifted keysym back to the same Key its unshifted form
already uses, matching the shift-invariant physical-key semantics every
other minifb backend has.

2. A failed present skips the key-repeat timer's advance

update_with_buffer_stride returned early via ? on
check_buffer_size/update_framebuffer before ever calling
self.update() -- the only call site of key_handler.update(), which
advances each key's held-duration timer. A caller whose own error handling
for the Err doesn't also call update() (natural, since the point of
that branch is usually just log-and-retry) leaves the timer frozen for one
cycle. If a key had duration 0.0 (first poll after going down), the next
successful poll sees the same 0.0 again and reports it as freshly
pressed -- a duplicate keystroke with no visible cause tying it back to
the earlier failed present.

Fix runs update() unconditionally (captures the present result in a
closure first), so a failed present costs a frame but never desyncs input.

Testing

Both fixes are exercised live in navette (a client that opens a real
window and forwards every keystroke over a real network connection to a
remote Wayland app), where the shifted-keysym fix was verified end-to-end:
capitals and shifted punctuation, previously untypeable, now work
correctly. cargo test and cargo build --release are clean on this
branch. I didn't add new unit tests here since handle_key takes a raw
XKB state pointer that isn't easily constructed in a unit test without a
real XKB context -- happy to add coverage if there's a preferred pattern
for this in the existing suite.

Two commits, one per fix, for easier review.

XKB resolves a key through whichever shift level is currently active, so holding Shift changes which keysym a press/release reports even though the physical key hasn't changed. handle_key's match table only covered the unshifted keysyms (lowercase letters, unshifted punctuation), so every shifted variant fell through to the `_ => return` fallback and was silently dropped before ever reaching set_key_state. Two consequences on a real keyboard: - Shift+key does not register at all -- no capital letters, no shifted punctuation -- since the press event itself is dropped. - A press/release pair straddling a Shift transition (ordinary typing rollover: press a letter, press Shift for the next word, release the first letter while Shift is still down) drops only the release. The key then reads as permanently held in KeyHandler's state, and downstream consumers relying on wl_keyboard-style press/release semantics see a key that's stuck down until the process exits or the window loses focus. Maps each shifted keysym back to the same Key its unshifted form already uses, matching the shift-invariant physical-key semantics every other minifb backend already has.

update_with_buffer_stride returned early via `?` on check_buffer_size or update_framebuffer before ever calling self.update() -- the sole call site of key_handler.update(), which advances each key's held-duration timer. A caller whose own error handling for update_with_buffer's Err doesn't also call plain update() (a very natural thing not to do, since the point of the error branch is usually just to log and retry the frame) leaves that timer frozen at its current value for one cycle. If a key was already held with duration exactly 0.0 (i.e. this was the very first poll after it went down), the next successful update_with_buffer poll sees the same 0.0 and is_key_index_pressed reports it as freshly pressed again -- a spurious duplicate keystroke with no visible cause tying it back to the present that failed a cycle earlier. Runs update() unconditionally now, after capturing the present result in a closure, so a failed present costs a frame but never desyncs input state.

…ow poll KeyHandler::update() does two things that belong on opposite sides of the Wayland backend's keyboard-event application, but update() runs entirely before it: try_dispatch_events(); // this cycle's key events are queued key_handler.update(); // advances state from the PREVIOUS keys[] for event in iter_keyboard_events() { ... set_key_state(...) } - keys_prev[i] = keys[i] is the snapshot is_key_index_released compares against (keys_prev && !keys). It has to be taken BEFORE this cycle's events, or a release written into keys makes both sides false and the edge is gone before the caller can read it. - keys_down_duration[i] gates is_key_index_pressed, which reports a key only while the duration is exactly 0.0. It has to be advanced AFTER this cycle's events, or a key pressed in this batch is still at its initial -1.0 when the caller reads. Because both run before the batch, a press is never reported on the cycle it arrives -- only if the key is still down at the next update. That is invisible at a fast poll, where an ordinary keypress spans many cycles. It breaks as soon as a caller's poll cycle is long enough that the release lands in the very next batch: the press is lost outright, an unpaired release is reported, and a key whose last observed edge was a press stays latched down with is_key_down() still returning true. A consumer that forwards key state onward then has a key held forever, and anything driving its own key repeat off that state repeats it forever. Measured with a harness that mirrors a real caller's poll loop while injecting identical input (20 taps, 50ms hold) at two cycle lengths: cycle dup presses unpaired releases latched at exit 8ms 0 0 no 60ms 0 10 yes 60ms* 0 0 no (* with this change) Moving update() wholesale to after the loop is not a fix -- it trades the lost presses for lost releases, since keys_prev then syncs to the post-release state (measured: 17 duplicate presses, 0 releases, still latched). The two phases genuinely have to be separated. So split update() into snapshot_prev() and advance_durations(), keep update() as both in sequence, and have the Wayland backend call the two around its keyboard-event loop. The X11, macOS, Windows and Redox backends apply their key events outside that window and keep calling update(), unchanged. One residual is inherent to the level-based key model and not addressed here: a press and release that both land inside a single poll cycle net out and neither edge is reported. That costs a dropped key, but the key no longer latches, so it cannot become a stuck repeat.

bearyjd · 2026-08-28T19:28:14Z

Added a third commit to this branch: 0b54200 — fix(wayland): split key_handler update so both key edges survive a slow poll.

This one is related to the present-failure fix already in the PR but is a distinct bug, found while chasing a "typing repeats characters under load" report downstream.

Window::update() calls key_handler.update() before applying the keyboard events it just dispatched. KeyHandler::update() does two things that belong on opposite sides of that application:

keys_prev[i] = keys[i] — what is_key_index_released compares against (keys_prev && !keys). Must be snapshotted before the batch, or a release written into keys makes both sides false and the edge is destroyed before the caller reads it.
keys_down_duration[i] — is_key_index_pressed reports a key only while this is exactly 0.0. Must be advanced after the batch, or a key pressed in it is still at the initial -1.0 at read time.

Running both before the batch means a press is only reported if the key is still down at the next update. That's invisible at a fast poll (an ordinary keypress spans many cycles) and is why this hasn't bitten before. It breaks once a caller's poll cycle is slow enough that the release arrives in the next batch — the press is lost, an unpaired release is reported, and a key whose last observed edge was a press stays latched with is_key_down() still true.

Measured with a harness mirroring a real poll loop, identical input (20 taps, 50 ms hold), varying only cycle length:

poll cycle	unpaired releases	latched at exit
8 ms	0	no
60 ms	10	yes
60 ms + this fix	0	no

Worth noting the obvious one-line fix is wrong: moving update() wholesale to after the loop just trades lost presses for lost releases (measured 17 duplicate presses, 0 releases, still latched), because keys_prev then syncs to the post-release state. The phases have to be split.

Only the Wayland backend changes call sites; X11/macOS/Windows/Redox apply key events outside that window and keep calling update() unchanged. cargo test passes (35 tests); the 3 clippy warnings in wayland.rs are pre-existing (verified identical with and without the patch).

One residual left alone, inherent to the level-based key model: a press and release both landing inside one poll cycle net out and neither is reported — a dropped key, but no latch, so it can't become a stuck repeat.

The repeat was never navette's code. It is a third bug in minifb's Wayland backend: Window::update() runs key_handler.update() before applying the keyboard events it just dispatched, but that call both snapshots the release edge (keys_prev) and advances the press timer (keys_down_duration) -- two things belonging on opposite sides of event application. A press is therefore only reported if the key survives to the next update. Invisible at an 8ms poll, where an ordinary keypress spans ~6 cycles, which is why 145 tests and three review passes never caught it. It breaks once a resize makes a cycle slow enough that the release lands in the next batch: the press is lost, an unpaired release is reported, and a key whose last observed edge was a press stays latched with is_key_down() still true -- so native.rs's stale-key self-heal cannot see it either. The guest never gets the release and repeats forever. Fixed in the fork (0b54200) by splitting KeyHandler::update() into snapshot_prev() and advance_durations() around the Wayland backend's event loop; update() stays as both in sequence so X11/macOS/Windows/Redox are untouched. Upstream PR emoon/rust_minifb#429 carries all three fixes. Moving update() wholesale after the loop is not a fix -- it trades lost presses for lost releases, since keys_prev then syncs to the post-release state. Measured with a harness mirroring the viewer's poll loop, injecting 480 key edges over four phases (single keys, rollover across Shift, sustained 3-key and deep 4-key overlap) at a 60ms cycle. Identical input, only minifb differing: pre-fix 68 unpaired releases with T at 33 presses against 84 releases; post-fix zero unpaired releases and every key balanced exactly. Also fixes the pointer misalignment found in the same live-testing round (positions now rescale against the last presented frame, not the live window size), forwards minifb's typematic repeat no longer (KeyRepeat::No), and hardens the input path: releases retried under backpressure, redundant presses collapsed at the bridge, InputState::release_all_held on worker exit. Two bugs found by review and fixed here rather than shipped: - Every tracing::debug! in the workspace was dead at runtime. Both binaries called fmt::init() without tracing-subscriber's env-filter feature, so RUST_LOG was ignored and the level pinned at info. This is why an earlier round of instrumentation "produced no evidence" -- it could not have. Enabling the feature alone would swap one bug for another, since from_default_env falls back to error when RUST_LOG is unset; both binaries now default to info with RUST_LOG override. - Absolute-state inputs were excluded from redelivery, so dropping one latched the guest permanently -- the same bug class this change exists to fix. KeyboardModifiers and ViewportResize describe a whole condition, and both producers short-circuit on equality, so nothing re-derives them: a dropped modifier update left the guest holding Shift with no resync path anywhere. The distinction that matters is edge vs absolute state, not press vs release. Edges are retried; absolute state is coalesced, since a state queued a tick ago can land after a newer one and clobber it. The backpressure policy now lives in navette_viewer::relay rather than the binary -- it was the only new stateful logic and it sat where it could not be tested, which is why the latch shipped. It has 10 tests including a classification tripwire, so a new absolute-state variant added to the protocol must be classified deliberately instead of silently falling through to "drop". Coverage verified by mutation: reverting the fix fails 4 of them. The viewer's real poll-cycle duration is now measured rather than assumed: p50 17ms, p90 30ms, max 626ms, with resize p90 8000 at 125ms. 14 cycles in one 60-second run were long enough to swallow an entire keypress, which makes moving decode/convert off the poll thread required work rather than optional -- recorded as the top open M2 item. docs/HANDOFF.md and docs/ROADMAP.md are now tracked; they existed only as untracked files, so the project's written memory was one git clean from gone. Workspace at 161 passed / 1 ignored, up from 145. Open follow-ups are recorded in .claude/PRPs/reviews/pr-7-review.md rather than left in scrollback.

Copilot

🟡 Changes recommended

Hard-coded US shifted-symbol mappings can report and leave incorrect keys stuck on non-US layouts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes Wayland keyboard event drops and input-state desynchronization.

Changes:

Adds shifted keysym mappings.
Splits key-state snapshot and duration advancement.
Processes input even when framebuffer presentation fails.

File summaries

File	Description
`src/key_handler.rs`	Separates key-state update phases.
`src/os/posix/wayland.rs`	Updates key mapping and present-failure handling.
`src/os/posix/xkb_keysyms.rs`	Defines shifted XKB keysyms.

Review details

Files reviewed: 3/3 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+                key::XKB_KEY_parenleft => Key::Key9,
+                key::XKB_KEY_parenright => Key::Key0,
+
+                key::XKB_KEY_quotedbl => Key::Apostrophe,


+        // `update()` is also where input events and the key-repeat timer
+        // advance -- it must run whether or not the present above
+        // succeeded. A caller that only calls `self.update()` on the Err
+        // branch (or not at all) leaves `key_handler`'s internal duration
+        // tracking frozen for a cycle: the next successful poll then sees
+        // an already-held key's timer still at its initial `0.0` and
+        // reports it as freshly pressed again, a spurious duplicate
+        // keystroke with no visible cause tying it to the failed present.


emoon · 2026-08-29T07:23:46Z

Thanks for the PR. The Copilot comment is valid, but that I will sorted that out in a follow up fix insetad.

The shifted-keysym table from #429 hardcodes US pairings. On a German layout Shift+2 is quotedbl, so the press sets Key::Apostrophe while the release after Shift is up arrives as 2 and clears Key::Key2, leaving Apostrophe held. Take Key from level 0 of the active layout, as x11.rs does with XLookupKeysym(&ev.key, 0). Character input keeps the state-resolved keysym. Also fixes the numpad with NumLock on, where the active keysym is KP_1..KP_0 and was dropped, and set_key_repeat_rate, which forwarded to set_key_repeat_delay. Tests drive handle_key through an embedded keymap fixture; the rollover and numpad cases fail without the fix.

The shifted-keysym table from #429 hardcodes US pairings. On a German layout Shift+2 is quotedbl, so the press sets Key::Apostrophe while the release after Shift is up arrives as 2 and clears Key::Key2, leaving Apostrophe held. Take Key from level 0 of the active layout, as x11.rs does with XLookupKeysym(&ev.key, 0). Character input keeps the state-resolved keysym. Level 0 alone is not enough: the compositor can switch layout group while a key is held, resolving the release through a different group. Record the Key each keycode resolved to on press and clear that on release. Also fixes the numpad with NumLock on, where the active keysym is KP_1..KP_0 and was dropped; the keypad decimal key, whose level-0 name is KP_Delete rather than KP_Decimal; and set_key_repeat_rate, which forwarded to set_key_repeat_delay. Tests drive handle_key through an embedded keymap fixture; each case fails without its fix.

bearyjd · 2026-08-29T12:45:54Z

Thanks for the quick turnaround on this @emoon — merged inside a day, and it let us delete our fork immediately rather than carrying it. Much appreciated.

Since it might be useful back to you: we'd built a harness to characterise the third bug, and we re-ran it against master at 3a711ad (i.e. including #430 and #431) before repinning, because both land in key handling. It injects 480 key edges through virtual-keyboard-v1 into a nested sway, in four phases — single keys, rollover across Shift, sustained three-key overlap, deep four-key overlap — while the client polls at a deliberately slowed 60 ms cycle, which is what makes the edge-ordering bug reachable.

Result on current master:

key	presses	releases
T	84	84
E	50	50
S	40	40
LeftShift	35	35

Zero duplicate presses, zero unpaired releases, no key latched down at exit.

Also worth noting: #431 giving Key an explicit #[repr(u8)] formalises something downstream code was already depending on — we map minifb Keys to evdev codes, and that relied on the discriminants being what they now provably are. Good change to have made explicit.

One residual we documented on our side rather than pushing upstream, in case it's ever of interest: with a level-based key model, a press and release that both land inside a single poll cycle net out and neither edge is reported. That costs a dropped key, but importantly it can't latch one, so it degrades safely. We handled it by keeping our poll cycle short instead.

emoon/rust_minifb#429 landed upstream as 7724f43 on 2026-08-29 with all three Wayland keyboard fixes found during navette's live testing: shifted-level keysyms silently dropped, a failed present skipping the key-repeat timer, and key_ 8BD0 handler.update() running before the keyboard events it must account for -- the root cause of the resize + rapid-typing repeat. The fork is no longer needed and the maintenance liability it represented goes away. Still a git pin rather than a version pin: the newest crates.io release is 0.28.0 from 2025-01-20, seven months before the merge, so nothing published contains these fixes yet. Move to a plain version as soon as one ships. The pin is two commits past ours and both touch key handling, so it was verified rather than assumed. #430 reworked keysym derivation; #431 gave Key an explicit #[repr(u8)], which formalises exactly what native.rs's evdev mapping already relied on through transmute -- a tightening, not a break. Ran the 480-edge rollover harness against the pinned SHA at a 60ms poll cycle (single keys, rollover across Shift, sustained three-key and deep four-key overlap): zero duplicate presses, zero unpaired releases, nothing latched at exit, every key balanced -- T 84/84, E 50/50, S 40/40, LeftShift 35/35. 162 passed, 1 ignored; clippy and fmt clean.

bearyjd added 3 commits August 27, 2026 22:44

bearyjd mentioned this pull request Aug 28, 2026

fix: root-cause and fix the resize + rapid-typing key repeat bearyjd/navette#7

Merged

6 of 7 tasks

emoon requested a balanced review from Copilot August 29, 2026 06:03

Copilot started reviewing on behalf of emoon August 29, 2026 06:04

View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

emoon merged commit 7724f43 into emoon:master Aug 29, 2026
4 checks passed

emoon mentioned this pull request Aug 29, 2026

fix(wayland): derive Key from the level-0 keysym #430

Merged

emoon mentioned this pull request Aug 29, 2026

[Left/RightShift + <key>] not picked up by get_keys on wayland #349

Closed

bearyjd mentioned this pull request Aug 29, 2026

chore(viewer): drop the minifb fork now that upstream has merged bearyjd/navette#9

Merged

4 of 5 tasks

Navigation Menu

Repository navigation

fix(wayland): shifted-keysym drops and a present-failure key desync - #429

fix(wayland): shifted-keysym drops and a present-failure key desync#429
emoon merged 3 commits into
emoon:masterfrom
bearyjd:navette-wayland-key-fixes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

1. Shifted-level keysyms are silently dropped

2. A failed present skips the key-repeat timer's advance

Testing

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants