Repository navigation
fix(app): match shortcuts on non-Latin keyboard layouts - #55765
ZXLyyds100 wants to merge 7 commits into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
The fix looks good to me. I ran the reproduction script from #55676 against matchKeybind using real KeyboardEvents. On the base, all 7 Russian-layout Ctrl cases fail (mod+f/p/t/w/comma/./[). With this PR, all 15 cases pass, and the alt+f control still matches. command.test.ts passes (43/43) and tsgo -b in packages/app passes. I didn't run the new Playwright component spec.
The new loop in handleKeyDown keeps the literal key first, so exact custom bindings and the palette still take priority over the physical-key fallback. The editable-target early return can't cut the loop short, because the fallback only applies when Ctrl or Meta is held. The exclusions (Alt, composition, AltGraph, Latin or ASCII remaps, named keys) leave the existing Alt handling, Windows AltGr and AZERTY/Dvorak-style layouts unchanged.
Public API: unchanged (internal keyboard matching in packages/app only).
No blocking concerns. One thing to watch: #54573 and #54708 change keyFromKeyboardEvent as well, so whichever lands second will need a rebase. A shared event.code → key map could serve all three.
Related: #55371
|
Thanks for the independent reproduction and review. The added provider-level browser regression also fails on the original code and passes with this fix, covering physical-key fallback, literal custom bindings, and palette priority. The test, check, and nix-eval workflows are currently awaiting maintainer approval. Could a maintainer approve the fork workflow runs when convenient so the required CI checks can run? |
|
AI-assisted follow-up at The conflict resolution preserves upstream's new Current local results: 63 focused command tests, 630 randomized repetitions, 971 unit + 251 browser tests, official app TypeScript, 20/20 focused Chrome component repetitions, root check and the normal push hook (35/35 tasks), changed-file lint, Prettier and whitespace checks passed. Current upstream first fails 13 command assertions and both focused browser cases. Broader browser results are not all green: unfiltered components 205 passed / 2 failed (focus ring and composer history). The same failures reproduce on byte-identical upstream production code; composer history fails 5/10 repeated controls. Built-mode shortcut/palette keepers are 3 passed / 4 failed on both trees, with a Ctrl/Mac modifier mismatch and three Windows-text/Mac-symbol expectations. The production build completed. No skips, weakened assertions, retries or unrelated fixes hide these outcomes. Earlier component counts in this comment's history are superseded observations, not claims of current success. The provider receives synthetic keyboard events in isolated macOS/Chrome 155; other dependencies use the repository's test harness. This is not Linux/Windows hosted CI, OS-level keyboard-layout verification or production acceptance. Workflow approval and human review remain independent requirements. Original/control logs and media are retained locally. The PR remains four files; this edits the existing follow-up without another review-thread reply or automatic resolution. |
There was a problem hiding this comment.
Re-checked after the merge with dev. Upstream's contextual-command selection (resolveKeybindOption with when) now runs for each candidate key inside the new loop, and the literal key is still tried first, so contextual and custom bindings keep priority over the physical-key fallback. The new component cases for contextual commands are small and clear. The unrelated lint-suppression comments are no longer in the diff.
The #55676 reproduction still fails on the base (7 of 15) and passes with this PR (15/15). command.test.ts passes 43/43, and tsc -b in packages/app passes.
Public API: unchanged.
No new concerns.
There was a problem hiding this comment.
The #55676 fix still works. On the current base, 6 of 15 reproduction cases fail; with this PR all 15 pass. command.test.ts passes (52/52), and tsc -b in packages/app passes.
The latest commit introduces a regression, though. While resolving the merge with dev, it removed the physical-bracket fallback from bracketKey, which #57301 added. Shortcuts that hold Alt and press a bracket key no longer match, because the new fallback skips any event with Alt held. That includes the default mod+alt+[ and mod+alt+] (previous and next message). See the inline comment.
Public API: unchanged.
packages/app/src/shell/commands/command.tsx:107: This removes the physical-bracket fallback thatdevadded in #57301, and the new fallback above doesn't cover what it handled. That fallback skips any event with Alt held, so bracket shortcuts that hold Alt now fail. I checked with realKeyboardEvents: - macOS US layout, Cmd+Option+[ (key: "“",code: "BracketLeft"): matchedmeta+alt+[on the base, no match with this PR. That breaks the defaultmod+alt+[/mod+alt+](previous/next message) on Mac. - Russian layout, Ctrl+Alt+х: matchedctrl+alt+[on the base, no match now. - The shortcut recorder (keyFromKeyboardEvent) now recordsctrl+хinstead ofctrl+[for that key. Could you keepdev'sbracketKeyfallback as it was and layer the new fallback on top? Please also add an Alt+bracket test row so this is covered.
There was a problem hiding this comment.
The earlier bracket regression is fixed. bracketKey matches dev again, the recorder records [ again, and new rows cover Option+bracket and Ctrl+Alt+bracket. The #55676 reproduction now passes for all the Ctrl cases. command.test.ts passes (57/57), and tsc -b in packages/app passes.
One new problem: the composition/AltGraph check now runs before keyFromKeyboardEvent, so for those events it skips the existing Alt and bracket handling as well as the new fallback. See the inline comment.
Public API: unchanged.
| } | ||
|
|
||
| function keybindKeysFromKeyboardEvent(event: KeyboardEvent) { | ||
| if (event.isComposing || event.getModifierState("AltGraph")) return [normalizeKey(event.key)] |
There was a problem hiding this comment.
This check now runs before keyFromKeyboardEvent. So for any event that reports AltGraph (or composition), it returns the raw event.key and skips the existing Alt→event.code fallback and the bracket fallback from dev, not just the new physical-key fallback.
I ran the #55676 reproduction under happy-dom, whose getModifierState("AltGraph") returns true for any Alt event:
alt+fwithkey: "а",code: "KeyF"passes on the base and fails here.meta+alt+[with“andctrl+alt+[withхpass on the base and fail here.
The test rows don't catch this because they override getModifierState. Your component spec also now expects Ctrl+Alt+х with AltGraph to stop triggering ctrl+alt+[, which works on the base. Browsers that report AltGraph together with Alt would lose these shortcuts too. I couldn't check which do; Firefox on macOS may report Option this way.
Could you compute const key = keyFromKeyboardEvent(event) first, and use isComposing / AltGraph only to return [key] before the physical fallback? Then events like these behave as they do on dev.
There was a problem hiding this comment.
The AltGraph/composition point is addressed. The guard now runs after keyFromKeyboardEvent and only skips the new physical-key fallback, so the existing Alt and bracket handling is unchanged. I re-ran the #55676 reproduction and my bracket checks with happy-dom's real getModifierState. All Russian-layout cases match, and alt+f with а, Cmd+Option+[ with “ and Ctrl+Alt+х behave as they do on the base. command.test.ts passes (63/63), and tsc -b in packages/app passes.
Public API: unchanged.
No new concerns.
Issue for this PR
Closes #55676
Type of change
What does this PR do?
Ctrl/Meta shortcuts try a non-Latin key's physical position after its literal binding. Exact custom bindings, contextual commands and palette priority are preserved. Recording, upstream bracket handling and existing Option/Ctrl+Alt behavior remain unchanged; composition/AltGraph suppress only the new fallback.
Normally merged
dev@726cf096into4f0cf1cb98d3c8abded6e5497dcbee010f447f96. The conflict resolution preserves upstream's newonSelect() === falsecontract: a declined literal or physical command leaves the event to its target, without trying another command. Added a browser regression for both cases. The PR still changes four files.Implementation, tests and self-review are AI-assisted, not independent human review. No public API, dependency, changelog or visual-layout change.
How did you verify your code works?
October 29, Bun 1.4.4, macOS/installed Chrome 155:
--rerun-each 10: 630 passed. The same regression against current upstream first produced 50 passed / 13 failed.packages/app,bun run test: 971 unit + 251 browser passed, zero failures.bun run typecheck:tsc: passed.command-keybinds.spec.ts --repeat-each=10 --workers=2 --retries=0: 20 passed. Both browser cases fail against upstream without this fix.bun run checkand the normal push hook: 35/35 tasks, existing warnings.bun run lint:changed 726cf096, four-file Prettier andgit diff --checkpassed.Not all broader browser tests are green:
These controls establish local baseline failures, not fixes to those areas. No assertion was weakened, skipped or retried into a pass; original reports/media are retained. Older component results in the comment history are not this run's results.
The fixture drives the real command provider/settings with synthetic keyboard events; other services use the repository's mocked harness. This is not OS-level Russian-layout testing, Linux/Windows CI or production verification.
Hosted observation at 12:56 UTC: this head is mergeable;
triage-pr-standardsrun40877123517succeeded.check-test/check-bot-scope/check-nix/checkruns40877130556/40877130605/40877130603/40877130542require owner approval, with zero jobs. These are not executed test passes.Visual
Conceptual release illustration, not execution evidence:
Screenshots / recordings
No layout change. Evidence is executable before/after assertions and real browser runs, not generated UI images.
Checklist
Added by the opencode bot (AI-generated).
Related: #55371