mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 08:03:28 +00:00
fix(ime): guard composition above the Escape branch, not below it
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 <help@stably.ai>
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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') {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
@@ -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') {
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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<HTMLElement>(
|
||||
`[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()
|
||||
|
||||
@@ -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<typeof vi.fn> } {
|
||||
function renderRecording(over: { onCancelRecording?: () => void } = {}): {
|
||||
button: HTMLElement
|
||||
onCapture: ReturnType<typeof vi.fn>
|
||||
} {
|
||||
const onCapture = vi.fn()
|
||||
const view = render(
|
||||
<TooltipProvider>
|
||||
@@ -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()
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user