Limit physical key fallback to non-Latin layout output#4469
Conversation
Shortcut matching accepted both the layout-derived key and the physical KeyA-KeyZ position for every keypress. On remapped Latin layouts one physical key then triggered shortcuts for two different letters, and a shortcut like mod+D consumed the key a user needs for mod+A. Keep the physical fallback only when the layout produces something other than a Latin letter, which is the case it was added for: non-Latin layouts and Option-modified symbols on macOS. Fixes pingdotgg#4434
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
ApprovabilityVerdict: Approved Straightforward bug fix adding a regex guard to prevent physical key fallback when the layout already outputs a Latin letter. Change is well-scoped, clearly documented, and includes tests covering both the fix and preserved behavior for non-Latin layouts. You can customize Macroscope's approvability policy. Learn more. |
What Changed
Shortcut matching only falls back to the physical
KeyA-KeyZposition when the layout-derived key is not itself a Latin letter. Two regression tests cover a non-Latin layout (still matched by position) and a remapped Latin layout (matched by layout output only).Why
resolveEventKeysaccepted both the layout key and the physical code for every keypress. On a remapped Latin layout one physical key then triggered shortcuts for two different letters at once: with a layout where physical D types "a", amod+Dbinding fires onmod+Aand the user can no longer select all text.The physical fallback exists for a real reason (Cyrillic and other non-Latin layouts, plus Option-modified symbols on macOS, produce keys that ASCII-defined shortcuts can never match), so it stays for exactly those cases: when
event.keyis not a Latin letter. When the layout already types Latin letters it speaks the shortcut vocabulary and is trusted alone.Fixes #4434
Testing
vp test run apps/web/src/keybindings.test.tspasses (43/43), including the existing Option-modifiedKeyBcase and the two new layout testspnpm typecheckin apps/web cleanvp linton changed files cleanChecklist
Note
Low Risk
Small, targeted change to keyboard shortcut matching with explicit tests; no auth, data, or API surface changes.
Overview
Shortcut resolution no longer treats physical
KeyA–KeyZposition as an alternate match when the keyboard layout already produces a Latin letter.resolveEventKeysstill falls back to the physical letter for non-Latin layout output (e.g. Cyrillic) and Option-modified symbols on macOS, whereevent.keyis not a single Latin letter. On remapped Latin layouts, matching uses layout output only, so one keypress cannot satisfy two different letter bindings (e.g. physical D typingano longer firesmod+D).Regression tests cover non-Latin layout matching by position and remapped Latin layouts ignoring physical code when layout key differs.
Reviewed by Cursor Bugbot for commit dc1345e. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Limit physical key code fallback in
resolveEventKeysto non-Latin layout outputPreviously,
resolveEventKeysalways added the physical letter derived fromevent.codeas a fallback candidate, which caused shortcuts to fire incorrectly when the keyboard layout produced a different Latin letter than the physical key label (e.g. key'a'on a key with codeKeyDwould match a binding for'd').Macroscope summarized dc1345e.