Repository navigation
Fix the shortcut-capture keyboard trap (#435) - #457
Merged
Merged
Conversation
Kept separate from captureHotkeyFromEvent, same reasoning that function already gives for staying framework-free: which key closes capture mode is a different question from which key makes a valid hotkey. Both were previously just "Unsupported key" like any other rejected candidate, with nothing anywhere treating them as a way out. Claude-Session: https://claude.ai/code/session_0144sxo5CJiyk99vaR7sgjuj
Every keydown was preventDefault'd while recording - including Tab, so its own default (moving focus) never ran either - and there was no Escape handler and no blur handler. A keyboard-only user who opened "Change shortcut" had no way out of it short of successfully producing one of the accepted standalone hotkeys. Escape and Tab now cancel via isCaptureCancelKey - Tab specifically isn't preventDefault'd, so the browser's own focus-move still happens right after cancelling, rather than being cancelled and then also suppressed. A window blur (Cmd-Tabbing away, or any other way the app loses focus mid-capture) cancels too, so capture mode isn't still armed for whatever key happens to land first when the app regains focus. Also added "(Esc to cancel)" to the capture prompt shown on the button while recording - the fix doesn't help much if nobody knows it's there. Claude-Session: https://claude.ai/code/session_0144sxo5CJiyk99vaR7sgjuj
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
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.
Closes #435. Fixes a real WCAG 2.1.2 "No Keyboard Trap" violation in the shortcut-capture UI, not a hypothetical one - traced through the actual code path.
The bug
HotkeySettings.tsx's "Change shortcut" flow callsevent.preventDefault()on every keydown while capturing.captureHotkeyFromEventonly accepts a standalone Option/Command/Control/Fn/Caps Lock press or F1-F19 - everything else, including Tab and Escape, came back{ ok: false, reason: "Unsupported key" }and leftrecordingtrue. There was no blur handler either. A keyboard-only user who opened capture mode had no way out of it except successfully producing one of the accepted hotkeys - Tab didn't move focus (its own default was suppressed along with everything else), and Escape did nothing.The fix
captureHotkey.ts: a new pureisCaptureCancelKey(event), checked separately fromcaptureHotkeyFromEvent- same reasoning that function's own comment already gives for staying framework-free: which key closes capture mode is a different question from which key makes a valid hotkey.HotkeySettings.tsx: Escape and Tab now cancel capture viaisCaptureCancelKey. Tab specifically isn'tpreventDefault'd, so the browser's own focus-move still happens right after cancelling. A windowblur(Cmd-Tabbing away, or any other way the app loses focus mid-capture) cancels too. The capture prompt shown on the button now says "(Esc to cancel)" - the fix doesn't help much if nobody knows it's there.Why no new tests for the React component
This project's test infra has no DOM environment configured (no jsdom, no
@testing-library/react-vite.config.tsdoesn't settest.environment, and the one existingHotkeySettings.test.tsxusesrenderToStaticMarkupspecifically to avoid needing one). Adding a DOM environment and a component-testing library for one bug fix felt like the wrong tradeoff, so instead the actual decision logic moved intocaptureHotkey.ts, which was already deliberately pure and already has real test coverage -isCaptureCancelKeygets the same treatment.HotkeySettings.tsx's own wiring stays thin and untested, consistent with how the rest of this codebase treats DOM/IPC glue versus the pure logic underneath it.Tests
captureHotkey.test.ts(+2 parameterized cases: Escape/Tab are cancels, a spread of real hotkey candidates and unrelated keys are not). Full suite (420) + typecheck + build green.🤖 Generated with Claude Code