From c2e59f6aff35ff1a8e34542b63e2fa32c1df10b2 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 2 Aug 2026 04:14:16 -0700 Subject: [PATCH] fix(ime): guard composition above the Escape branch, not below it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An Escape that dismisses an IME candidate window carries the composition markers, so a handler that runs its Escape branch before consulting the guard acts on a keystroke the user aimed at the IME — discarding a comment draft, reverting a search, or cancelling shortcut recording. Eight keydown handlers guarded Enter but dispatched Escape first. Two of them (the reviewer picker and the workspace-name field) also left the arrows unguarded, so candidate navigation moved the app's selection too. The marker sweep that found the dropped-isComposing adapters cannot see this: the markers are present and the guard is called, just one branch too late. Ordering is now pinned as a structural invariant over every keydown handler in src/, which is what a per-component test could not do. Co-authored-by: Orca --- src/renderer/src/components/TaskPage.tsx | 6 ++ .../browser-pane/markup/MarkupOverlay.tsx | 10 +- .../diff-comments/DiffCommentCard.tsx | 7 +- .../diff-comments/DiffCommentPopover.tsx | 10 +- .../src/components/editor/MarkdownPreview.tsx | 7 +- .../github-project/ProjectViewWrapper.tsx | 8 +- ...SmartWorkspaceNameField.ime-enter.test.tsx | 64 +++++++++++++ .../new-workspace/SmartWorkspaceNameField.tsx | 16 ++-- .../ShortcutRecorderButton.ime.test.tsx | 31 ++++++- .../settings/ShortcutRecorderButton.tsx | 13 ++- .../src/lib/ime-escape-guard-ordering.test.ts | 91 +++++++++++++++++++ 11 files changed, 239 insertions(+), 24 deletions(-) create mode 100644 src/renderer/src/lib/ime-escape-guard-ordering.test.ts diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index 0017e68b3d4..5c457c43e68 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -2558,6 +2558,12 @@ function PRReviewCell({ aria-label={translate('auto.components.TaskPage.0b9b04f4b5', 'Type or choose a user')} aria-autocomplete="list" onKeyDown={(event) => { + // Above the dispatch: while a candidate window is open the arrows move the + // candidate, Enter commits it and Escape dismisses it — none of the three may also + // drive the reviewer picker. + if (isImeCompositionKeyDown(event)) { + return + } if (event.key === 'ArrowDown' && actionableReviewerRows.length > 0) { event.preventDefault() setActiveReviewerIndex((current) => (current + 1) % actionableReviewerRows.length) diff --git a/src/renderer/src/components/browser-pane/markup/MarkupOverlay.tsx b/src/renderer/src/components/browser-pane/markup/MarkupOverlay.tsx index 27ea5fa6878..888a0085dda 100644 --- a/src/renderer/src/components/browser-pane/markup/MarkupOverlay.tsx +++ b/src/renderer/src/components/browser-pane/markup/MarkupOverlay.tsx @@ -82,9 +82,13 @@ export function MarkupOverlay({ // Why: keep keystrokes local — without this the browser pane's global // key handlers can swallow typing before it reaches the input. event.stopPropagation() - // Why: during IME composition (e.g. Japanese conversion), Enter - // confirms the candidate — it must NOT also commit the annotation. - if (event.key === 'Enter' && !isImeCompositionKeyDown(event)) { + // Why: during IME composition (e.g. Japanese conversion), Enter confirms the + // candidate and Escape dismisses it — neither may also commit or discard the + // annotation. Guarded above the dispatch so both are suppressed together. + if (isImeCompositionKeyDown(event)) { + return + } + if (event.key === 'Enter') { event.preventDefault() editor.commitPendingText(event.currentTarget.value) } else if (event.key === 'Escape') { diff --git a/src/renderer/src/components/diff-comments/DiffCommentCard.tsx b/src/renderer/src/components/diff-comments/DiffCommentCard.tsx index f8ef8c15609..4f75528bbea 100644 --- a/src/renderer/src/components/diff-comments/DiffCommentCard.tsx +++ b/src/renderer/src/components/diff-comments/DiffCommentCard.tsx @@ -288,12 +288,17 @@ export function DiffCommentCard({ onContentResizeRef.current?.() }} onKeyDown={(e) => { + // Above the dispatch: Escape dismisses an IME candidate window, and discarding + // the draft on it would lose text the user never asked to throw away. + if (isImeCompositionKeyDown(e)) { + return + } if (e.key === 'Escape') { e.preventDefault() handleCancel() return } - if (e.key === 'Enter' && !isImeCompositionKeyDown(e) && !e.shiftKey) { + if (e.key === 'Enter' && !e.shiftKey) { e.preventDefault() if (!canSubmit) { return diff --git a/src/renderer/src/components/diff-comments/DiffCommentPopover.tsx b/src/renderer/src/components/diff-comments/DiffCommentPopover.tsx index 0074cc2ac0c..84b91760412 100644 --- a/src/renderer/src/components/diff-comments/DiffCommentPopover.tsx +++ b/src/renderer/src/components/diff-comments/DiffCommentPopover.tsx @@ -209,13 +209,19 @@ export function DiffCommentPopover({ autoResize(e.currentTarget) }} onKeyDown={(e) => { + // Why: a composing Enter commits the CJK candidate and a composing Escape dismisses + // it — neither may submit or discard the note. Guarded above the dispatch so the + // Escape branch cannot run ahead of the check. + if (isImeCompositionKeyDown(e)) { + return + } if (e.key === 'Escape') { e.preventDefault() onCancel() return } - // Why: Shift+Enter inserts a newline; skip isComposing so IME composition Enter doesn't submit a half-typed CJK note. - if (e.key === 'Enter' && !isImeCompositionKeyDown(e) && !e.shiftKey) { + // Why: Shift+Enter inserts a newline. + if (e.key === 'Enter' && !e.shiftKey) { e.preventDefault() if (submitting) { return diff --git a/src/renderer/src/components/editor/MarkdownPreview.tsx b/src/renderer/src/components/editor/MarkdownPreview.tsx index 7b9b0e42c5b..6f3e53b5365 100644 --- a/src/renderer/src/components/editor/MarkdownPreview.tsx +++ b/src/renderer/src/components/editor/MarkdownPreview.tsx @@ -2121,12 +2121,17 @@ function MarkdownAnnotationComposer({ el.style.height = `${Math.min(el.scrollHeight, 240)}px` }} onKeyDown={(event) => { + // Above the dispatch: Escape dismisses an IME candidate window, and discarding the + // note on it would lose text the user never asked to throw away. + if (isImeCompositionKeyDown(event)) { + return + } if (event.key === 'Escape') { event.preventDefault() onCancel() return } - if (event.key === 'Enter' && !isImeCompositionKeyDown(event) && !event.shiftKey) { + if (event.key === 'Enter' && !event.shiftKey) { event.preventDefault() void submit() } diff --git a/src/renderer/src/components/github-project/ProjectViewWrapper.tsx b/src/renderer/src/components/github-project/ProjectViewWrapper.tsx index 00a39f71c7c..e4d5c3ea438 100644 --- a/src/renderer/src/components/github-project/ProjectViewWrapper.tsx +++ b/src/renderer/src/components/github-project/ProjectViewWrapper.tsx @@ -1103,10 +1103,12 @@ function ProjectSearchInput({ value={value} onChange={(e) => setValue(e.target.value)} onKeyDown={(e) => { + // Above the dispatch: a composing Escape dismisses the candidate window, and reverting + // and blurring the search on it would throw away a half-typed CJK query. + if (isImeCompositionKeyDown(e)) { + return + } if (e.key === 'Enter') { - if (isImeCompositionKeyDown(e)) { - return - } e.preventDefault() apply(value) } else if (e.key === 'Escape') { diff --git a/src/renderer/src/components/new-workspace/SmartWorkspaceNameField.ime-enter.test.tsx b/src/renderer/src/components/new-workspace/SmartWorkspaceNameField.ime-enter.test.tsx index fd828b0b202..a14b3e9650d 100644 --- a/src/renderer/src/components/new-workspace/SmartWorkspaceNameField.ime-enter.test.tsx +++ b/src/renderer/src/components/new-workspace/SmartWorkspaceNameField.ime-enter.test.tsx @@ -199,6 +199,70 @@ function pressEnter( }) } +function pressEscape( + input: HTMLInputElement, + init?: KeyboardEventInit & { keyCode?: number } +): void { + const event = new KeyboardEvent('keydown', { + key: 'Escape', + bubbles: true, + cancelable: true, + ...init + }) + Object.defineProperty(event, 'keyCode', { value: init?.keyCode ?? 27 }) + act(() => { + input.dispatchEvent(event) + }) +} + +// Escape is the key that dismisses an IME candidate window, so it arrives marked while a +// composition is live. Closing the emoji menu on it would consume a keystroke the user aimed at +// the IME. Whether the menu survived is read through the Enter that follows. +describe('SmartWorkspaceNameField IME Escape guard', () => { + function openEmojiMenu(input: HTMLInputElement): void { + act(() => { + input.focus() + input.setSelectionRange(12, 12) + input.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + } + + it('keeps the emoji menu open when Escape is marked as composing', () => { + const onValueChange = vi.fn() + const input = renderEmojiField('Launch :wink', onValueChange) + openEmojiMenu(input) + + pressEscape(input, { isComposing: true }) + pressEnter(input) + + expect(onValueChange).toHaveBeenCalledWith('Launch 😉') + }) + + it('keeps the emoji menu open when Escape is reported as keyCode 229', () => { + const onValueChange = vi.fn() + const input = renderEmojiField('Launch :wink', onValueChange) + openEmojiMenu(input) + + pressEscape(input, { keyCode: 229 }) + pressEnter(input) + + expect(onValueChange).toHaveBeenCalledWith('Launch 😉') + }) + + // The paired negative: an ordinary Escape must still dismiss the menu, or the two tests above + // would pass with the guard deleted. + it('still dismisses the emoji menu on an ordinary Escape', () => { + const onValueChange = vi.fn() + const input = renderEmojiField('Launch :wink', onValueChange) + openEmojiMenu(input) + + pressEscape(input) + pressEnter(input) + + expect(onValueChange).not.toHaveBeenCalledWith('Launch 😉') + }) +}) + describe('SmartWorkspaceNameField IME Enter guard', () => { it('ignores the Enter that commits a CJK IME composition (isComposing)', () => { const onPlainEnter = vi.fn() diff --git a/src/renderer/src/components/new-workspace/SmartWorkspaceNameField.tsx b/src/renderer/src/components/new-workspace/SmartWorkspaceNameField.tsx index ead339690ea..a7b328c0bdf 100644 --- a/src/renderer/src/components/new-workspace/SmartWorkspaceNameField.tsx +++ b/src/renderer/src/components/new-workspace/SmartWorkspaceNameField.tsx @@ -1811,6 +1811,14 @@ export default function SmartWorkspaceNameField({ tryOpenSourcePopover() }} onKeyDown={(event) => { + // Why: an Enter that only commits a CJK IME candidate must not select a row + // or advance focus — moving focus mid-composition makes Chromium re-commit + // the composed character into the controlled input, duplicating the last + // syllable (e.g. 배포 → 배포포). Escape and the arrows belong to the + // candidate window for the same reason, so the check sits above all of them. + if (isImeCompositionKeyDown(event)) { + return + } if (event.key === 'Tab' && event.shiftKey) { const activeTrigger = tabsListRef.current?.querySelector( `[data-smart-name-mode="${mode}"]` @@ -1841,14 +1849,6 @@ export default function SmartWorkspaceNameField({ !event.ctrlKey && !event.shiftKey ) { - // Why: an Enter that only commits a CJK IME candidate - // must not select a row or advance focus — moving focus - // mid-composition makes Chromium re-commit the composed - // character into the controlled input, duplicating the - // last syllable (e.g. 배포 → 배포포). - if (isImeCompositionKeyDown(event)) { - return - } if (emojiMenuOpen && selectedEmojiSuggestion) { event.preventDefault() event.stopPropagation() diff --git a/src/renderer/src/components/settings/ShortcutRecorderButton.ime.test.tsx b/src/renderer/src/components/settings/ShortcutRecorderButton.ime.test.tsx index 4c96f353737..5719125622c 100644 --- a/src/renderer/src/components/settings/ShortcutRecorderButton.ime.test.tsx +++ b/src/renderer/src/components/settings/ShortcutRecorderButton.ime.test.tsx @@ -15,7 +15,10 @@ afterEach(cleanup) // the easiest place to forget that an IME-owned keystroke carries no chord: its `key` is 'Process' // or 'Unidentified', so capturing one writes a binding that can never fire. describe('ShortcutRecorderButton while an IME owns the keystroke', () => { - function renderRecording(): { button: HTMLElement; onCapture: ReturnType } { + function renderRecording(over: { onCancelRecording?: () => void } = {}): { + button: HTMLElement + onCapture: ReturnType + } { const onCapture = vi.fn() const view = render( @@ -29,7 +32,7 @@ describe('ShortcutRecorderButton while an IME owns the keystroke', () => { bindingCount={1} recording onStartRecording={vi.fn()} - onCancelRecording={vi.fn()} + onCancelRecording={over.onCancelRecording ?? vi.fn()} onCapture={onCapture} onClearError={vi.fn()} /> @@ -82,6 +85,30 @@ describe('ShortcutRecorderButton while an IME owns the keystroke', () => { expect(onCapture).not.toHaveBeenCalled() }) + // Escape is what dismisses an IME candidate window, so it arrives marked while a composition is + // live. Cancelling recording on it would consume a keystroke the user aimed at the IME. + it.each([ + ['isComposing', { isComposing: true, keyCode: 27 }], + ['keyCode 229', { keyCode: 229 }] + ])('does not cancel recording on an Escape marked by %s', (_marker, over) => { + const onCancelRecording = vi.fn() + const { button } = renderRecording({ onCancelRecording }) + + fireEvent.keyDown(button, { key: 'Escape', ...over }) + + expect(onCancelRecording).not.toHaveBeenCalled() + }) + + // The paired negative: without it, both cases above pass with the guard deleted. + it('still cancels recording on an ordinary Escape', () => { + const onCancelRecording = vi.fn() + const { button } = renderRecording({ onCancelRecording }) + + fireEvent.keyDown(button, { key: 'Escape', keyCode: 27 }) + + expect(onCancelRecording).toHaveBeenCalledTimes(1) + }) + it('still records a real double-tap', () => { const { button, onCapture } = renderRecording() diff --git a/src/renderer/src/components/settings/ShortcutRecorderButton.tsx b/src/renderer/src/components/settings/ShortcutRecorderButton.tsx index 98b1f0da581..89618a02eef 100644 --- a/src/renderer/src/components/settings/ShortcutRecorderButton.tsx +++ b/src/renderer/src/components/settings/ShortcutRecorderButton.tsx @@ -87,17 +87,22 @@ export function ShortcutRecorderButton({ event.preventDefault() event.stopPropagation() + // Read before the Escape branch: the Escape that dismisses a candidate window is marked, and + // cancelling recording on it would consume a keystroke the user aimed at the IME. + // Fed to the detector rather than short-circuiting above it: withholding a composing press + // leaves the gesture armed, so a later real press would complete a tap nobody made. + const imeOwned = isImeCompositionKeyDown(event) + if (event.key === 'Escape') { doubleTapDetectorRef.current?.reset() + if (imeOwned) { + return + } onClearError(actionId) onCancelRecording() return } - // Fed to the detector rather than short-circuiting above it: withholding a composing press - // leaves the gesture armed, so a later real press would complete a tap nobody made. - const imeOwned = isImeCompositionKeyDown(event) - // A modifier press never captures on its own — the detector decides whether // it completes a double-tap, leaving normal chords to capture on their key. if (modifierFromKeyEvent(event.code, event.key) !== null) { diff --git a/src/renderer/src/lib/ime-escape-guard-ordering.test.ts b/src/renderer/src/lib/ime-escape-guard-ordering.test.ts new file mode 100644 index 00000000000..69c2bb010e4 --- /dev/null +++ b/src/renderer/src/lib/ime-escape-guard-ordering.test.ts @@ -0,0 +1,91 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +// The marker sweep that found the dropped-`isComposing` adapters is blind to this defect: the +// markers are present and the guard is called, just one branch too late. An Escape that dismisses +// an IME candidate window is marked, so a handler that runs its Escape branch first discards the +// draft, resets the search, or cancels recording on a keystroke the user aimed at the IME. +// +// Ordering is a property of the source, not of any one component's behaviour, so it is pinned +// here once rather than re-tested in every composer that happens to handle Escape. + +const ROOTS = ['src/renderer/src', 'src/main', 'src/shared'] +const GUARD = 'isImeCompositionKeyDown' +const ESCAPE = "=== 'Escape'" + +function sourceFiles(dir: string): string[] { + const found: string[] = [] + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const path = join(dir, entry.name) + if (entry.isDirectory()) { + if (entry.name !== 'node_modules' && entry.name !== '__snapshots__') { + found.push(...sourceFiles(path)) + } + continue + } + if (!/\.tsx?$/.test(entry.name) || /\.(test|spec)\.tsx?$/.test(entry.name)) { + continue + } + found.push(path) + } + return found +} + +// Returns the body of every keydown handler in `source`, brace-matched from its opening `{`. +function keydownHandlerBodies(source: string): string[] { + const bodies: string[] = [] + const starts = /onKeyDown(?:Capture)?=\{|(?:function|const)\s+\w*[Kk]ey[Dd]own\w*\s*[=(]/g + for (let match = starts.exec(source); match !== null; match = starts.exec(source)) { + const open = source.indexOf('{', match.index + match[0].length - 1) + if (open === -1) { + continue + } + let depth = 0 + for (let i = open; i < source.length; i++) { + if (source[i] === '{') { + depth++ + } else if (source[i] === '}') { + depth-- + if (depth === 0) { + bodies.push(source.slice(open, i + 1)) + break + } + } + } + } + return bodies +} + +describe('composition guards sit above the Escape branch', () => { + const offenders: string[] = [] + for (const root of ROOTS) { + for (const file of sourceFiles(root)) { + const source = readFileSync(file, 'utf8') + if (!source.includes(GUARD) || !source.includes(ESCAPE)) { + continue + } + for (const body of keydownHandlerBodies(source)) { + const guardAt = body.indexOf(GUARD) + const escapeAt = body.indexOf(ESCAPE) + if (guardAt !== -1 && escapeAt !== -1 && escapeAt < guardAt) { + offenders.push(file) + } + } + } + } + + it('finds no keydown handler that dispatches Escape before consulting the guard', () => { + expect(offenders).toEqual([]) + }) + + // Guards against the check silently matching nothing — a scanner that parses no handlers + // would report zero offenders forever. + it('actually scanned guarded keydown handlers', () => { + const scanned = ROOTS.flatMap(sourceFiles).filter((file) => { + const source = readFileSync(file, 'utf8') + return source.includes(GUARD) && keydownHandlerBodies(source).some((b) => b.includes(GUARD)) + }) + expect(scanned.length).toBeGreaterThan(8) + }) +})