mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 08:03:28 +00:00
fix(ime): stop a composing keystroke arming a double-tap gesture
A detected double-tap is dispatched as `{ doubleTapModifier }` with no key or
modifier flags, so once one is armed the shortcut matcher has nothing left to
refuse on. Decide ownership where the gesture is built instead: an IME-owned
event now yields a null modifier, which the detector already treats as a reset.
The three renderer listeners feed that decision in rather than returning early.
Withholding the press looked safer but left the gesture armed, so a later real
press completed a tap the user never made.
Also routes the quick-commands listener through the shared dispatch adapter, so
it can no longer drop the IME markers while rebuilding the input field-by-field.
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -1911,11 +1911,10 @@ function App(): React.JSX.Element {
|
||||
}
|
||||
|
||||
const onKeyDown = (e: KeyboardEvent): void => {
|
||||
// Ahead of the detector: a composing chord must not arm a double-tap either, and the
|
||||
// synthetic input a detection produces carries no markers to be refused by later.
|
||||
if (isImeCompositionKeyDown(e)) {
|
||||
return
|
||||
}
|
||||
// Why the detector still sees an IME-owned event instead of an early return: withholding
|
||||
// the press leaves the gesture armed, and a later real press then completes a tap the user
|
||||
// never made. Passing ownership in lets the detector refuse *and* reset in one place.
|
||||
const imeOwned = isImeCompositionKeyDown(e)
|
||||
const detected = doubleTapDetector.process(
|
||||
toModifierDoubleTapEvent({
|
||||
type: 'keyDown',
|
||||
@@ -1925,6 +1924,7 @@ function App(): React.JSX.Element {
|
||||
control: e.ctrlKey,
|
||||
alt: e.altKey,
|
||||
meta: e.metaKey,
|
||||
isComposing: imeOwned,
|
||||
isAutoRepeat: e.repeat
|
||||
}),
|
||||
Date.now()
|
||||
@@ -1941,11 +1941,8 @@ function App(): React.JSX.Element {
|
||||
}
|
||||
|
||||
const onKeyUp = (e: KeyboardEvent): void => {
|
||||
// The detector is stateful across keydown/keyup, so a composing release must not
|
||||
// complete a tap pair the matching press was already withheld from.
|
||||
if (isImeCompositionKeyDown(e)) {
|
||||
return
|
||||
}
|
||||
// Fed rather than withheld, for the same reason as the keydown path.
|
||||
const imeOwned = isImeCompositionKeyDown(e)
|
||||
doubleTapDetector.process(
|
||||
toModifierDoubleTapEvent({
|
||||
type: 'keyUp',
|
||||
@@ -1954,7 +1951,8 @@ function App(): React.JSX.Element {
|
||||
shift: e.shiftKey,
|
||||
control: e.ctrlKey,
|
||||
alt: e.altKey,
|
||||
meta: e.metaKey
|
||||
meta: e.metaKey,
|
||||
isComposing: imeOwned
|
||||
}),
|
||||
Date.now()
|
||||
)
|
||||
|
||||
@@ -60,6 +60,7 @@ import {
|
||||
} from '@/lib/floating-workspace-guest-bridge'
|
||||
import { closeTerminalTab } from '@/components/terminal/terminal-tab-actions'
|
||||
import { guardPinnedTabClose, resolvePinnedTabLabel } from '@/store/pinned-tab-close-guard'
|
||||
import { isImeCompositionKeyDown } from '@/lib/ime-composition-keyboard-event'
|
||||
import { extractIpcErrorMessage } from '@/lib/ipc-error'
|
||||
import { getShortcutPlatform } from '@/lib/shortcut-platform'
|
||||
import {
|
||||
@@ -1409,6 +1410,10 @@ export function FloatingTerminalPanel({
|
||||
return
|
||||
}
|
||||
|
||||
// Why the detector still sees an IME-owned event instead of an early return: withholding
|
||||
// the press leaves the gesture armed, and a later real press then completes a tap the user
|
||||
// never made. Passing ownership in lets the detector refuse *and* reset in one place.
|
||||
const imeOwned = isImeCompositionKeyDown(event)
|
||||
const detected = doubleTapDetectorRef.current?.process(
|
||||
toModifierDoubleTapEvent({
|
||||
type: 'keyDown',
|
||||
@@ -1418,6 +1423,7 @@ export function FloatingTerminalPanel({
|
||||
control: event.ctrlKey,
|
||||
alt: event.altKey,
|
||||
meta: event.metaKey,
|
||||
isComposing: imeOwned,
|
||||
isAutoRepeat: event.repeat
|
||||
}),
|
||||
Date.now()
|
||||
@@ -1495,6 +1501,8 @@ export function FloatingTerminalPanel({
|
||||
doubleTapDetectorRef.current?.reset()
|
||||
return
|
||||
}
|
||||
// Fed rather than withheld, for the same reason as the keydown path.
|
||||
const imeOwned = isImeCompositionKeyDown(event)
|
||||
doubleTapDetectorRef.current?.process(
|
||||
toModifierDoubleTapEvent({
|
||||
type: 'keyUp',
|
||||
@@ -1503,7 +1511,8 @@ export function FloatingTerminalPanel({
|
||||
shift: event.shiftKey,
|
||||
control: event.ctrlKey,
|
||||
alt: event.altKey,
|
||||
meta: event.metaKey
|
||||
meta: event.metaKey,
|
||||
isComposing: imeOwned
|
||||
}),
|
||||
Date.now()
|
||||
)
|
||||
|
||||
@@ -54,7 +54,10 @@ vi.mock('react', async () => {
|
||||
}
|
||||
})
|
||||
|
||||
vi.mock('../../../../shared/keybindings', () => ({
|
||||
// Only the matcher is stubbed. `keybindingInputIsImeOwned` stays real so the IME refusal in this
|
||||
// dispatcher runs for real rather than being mocked into always-false.
|
||||
vi.mock(import('../../../../shared/keybindings'), async (importOriginal) => ({
|
||||
...(await importOriginal()),
|
||||
keybindingMatchesAction: keybindingsMock.matchAction
|
||||
}))
|
||||
|
||||
@@ -367,6 +370,71 @@ describe('TabBarQuickCommandsMenu keyboard shortcut', () => {
|
||||
expect(secondDown.stopImmediatePropagation).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// These run against the real listener rather than the shared matcher, which is the gap that let
|
||||
// a composing chord through after the matcher was already guarded: the matcher is forced to
|
||||
// match here, so a toggle can only be stopped by the dispatcher's own refusal.
|
||||
describe('while an IME owns the keystroke', () => {
|
||||
const composingChord = (): KeyboardEvent =>
|
||||
// The real key shape a composing chord has — not `key: 'Process'`, which no binding
|
||||
// resolves to and which would therefore pass with the guard deleted.
|
||||
makeKeyEvent({ keyCode: 229, isComposing: true } as Partial<KeyboardEvent>)
|
||||
|
||||
const shiftTap = (over: Partial<KeyboardEvent> = {}): KeyboardEvent =>
|
||||
makeKeyEvent({ key: 'Shift', code: 'ShiftLeft', metaKey: false, shiftKey: true, ...over })
|
||||
|
||||
async function mountListeners(): Promise<void> {
|
||||
reactRuntime.index = 0
|
||||
const { TabBarQuickCommandsMenu } = await import('./TabBarQuickCommandsMenu')
|
||||
TabBarQuickCommandsMenu(makeProps())
|
||||
reactRuntime.effects[0]()
|
||||
}
|
||||
|
||||
// Bound to the gesture only, so the bare Shift presses that build it are not themselves
|
||||
// chords that toggle — the toggle can only come from a completed double-tap.
|
||||
const matchDoubleTapOnly = (): void => {
|
||||
keybindingsMock.matchAction.mockImplementation(
|
||||
(_actionId, input: { doubleTapModifier?: string }) => input.doubleTapModifier === 'Shift'
|
||||
)
|
||||
}
|
||||
|
||||
it('does not toggle on a marked chord', async () => {
|
||||
await mountListeners()
|
||||
keybindingsMock.matchAction.mockReturnValue(true)
|
||||
const event = composingChord()
|
||||
|
||||
windowListeners.get('keydown')!(event)
|
||||
|
||||
expect(reactRuntime.states[0]).toBe(false)
|
||||
expect(event.preventDefault).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not complete a double-tap whose second press is marked', async () => {
|
||||
await mountListeners()
|
||||
matchDoubleTapOnly()
|
||||
const keyDown = windowListeners.get('keydown')!
|
||||
|
||||
keyDown(shiftTap())
|
||||
windowListeners.get('keyup')!(shiftTap())
|
||||
keyDown(shiftTap({ keyCode: 229, isComposing: true } as Partial<KeyboardEvent>))
|
||||
|
||||
expect(reactRuntime.states[0]).toBe(false)
|
||||
})
|
||||
|
||||
it('does not let a marked release arm the second half of a double-tap', async () => {
|
||||
await mountListeners()
|
||||
matchDoubleTapOnly()
|
||||
const keyDown = windowListeners.get('keydown')!
|
||||
|
||||
keyDown(shiftTap())
|
||||
windowListeners.get('keyup')!(
|
||||
shiftTap({ keyCode: 229, isComposing: true } as Partial<KeyboardEvent>)
|
||||
)
|
||||
keyDown(shiftTap())
|
||||
|
||||
expect(reactRuntime.states[0]).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
it('removes the listener when the effect is cleaned up', async () => {
|
||||
reactRuntime.index = 0
|
||||
const { TabBarQuickCommandsMenu } = await import('./TabBarQuickCommandsMenu')
|
||||
|
||||
@@ -9,6 +9,8 @@ import {
|
||||
toModifierDoubleTapEvent
|
||||
} from '../../../../shared/modifier-double-tap-detector'
|
||||
import { getShortcutPlatform } from '@/lib/shortcut-platform'
|
||||
import { isImeCompositionKeyDown } from '@/lib/ime-composition-keyboard-event'
|
||||
import { toShortcutDispatchInput } from '@/lib/app-shortcut-dispatch-input'
|
||||
import { TOGGLE_QUICK_COMMANDS_MENU_EVENT } from '@/lib/quick-commands-menu-events'
|
||||
import { useAppStore } from '@/store'
|
||||
|
||||
@@ -67,6 +69,10 @@ export function useTabBarQuickCommandsShortcut({
|
||||
doubleTapDetector.reset()
|
||||
return
|
||||
}
|
||||
// Why the detector still sees an IME-owned event instead of an early return: withholding
|
||||
// the press leaves the gesture armed, and a later real press then completes a tap the user
|
||||
// never made. Passing ownership in lets the detector refuse *and* reset in one place.
|
||||
const imeOwned = isImeCompositionKeyDown(e)
|
||||
const detected = doubleTapDetector.process(
|
||||
toModifierDoubleTapEvent({
|
||||
type: 'keyDown',
|
||||
@@ -76,6 +82,7 @@ export function useTabBarQuickCommandsShortcut({
|
||||
control: e.ctrlKey,
|
||||
alt: e.altKey,
|
||||
meta: e.metaKey,
|
||||
isComposing: imeOwned,
|
||||
isAutoRepeat: e.repeat
|
||||
}),
|
||||
Date.now()
|
||||
@@ -89,19 +96,11 @@ export function useTabBarQuickCommandsShortcut({
|
||||
if (e.repeat) {
|
||||
return
|
||||
}
|
||||
if (
|
||||
!matchesShortcut(
|
||||
{
|
||||
key: e.key,
|
||||
code: e.code,
|
||||
altKey: e.altKey,
|
||||
metaKey: e.metaKey,
|
||||
ctrlKey: e.ctrlKey,
|
||||
shiftKey: e.shiftKey
|
||||
},
|
||||
e.target
|
||||
)
|
||||
) {
|
||||
// Built by the shared adapter rather than field-by-field: an adapter that forgets the
|
||||
// IME markers leaves the matcher nothing to refuse on, and that omission is invisible
|
||||
// here at the call site.
|
||||
const input = toShortcutDispatchInput(e)
|
||||
if (!input || !matchesShortcut(input, e.target)) {
|
||||
return
|
||||
}
|
||||
toggleMenu(e)
|
||||
@@ -111,6 +110,8 @@ export function useTabBarQuickCommandsShortcut({
|
||||
doubleTapDetector.reset()
|
||||
return
|
||||
}
|
||||
// Fed rather than withheld, for the same reason as the keydown path.
|
||||
const imeOwned = isImeCompositionKeyDown(e)
|
||||
doubleTapDetector.process(
|
||||
toModifierDoubleTapEvent({
|
||||
type: 'keyUp',
|
||||
@@ -119,7 +120,8 @@ export function useTabBarQuickCommandsShortcut({
|
||||
shift: e.shiftKey,
|
||||
control: e.ctrlKey,
|
||||
alt: e.altKey,
|
||||
meta: e.metaKey
|
||||
meta: e.metaKey,
|
||||
isComposing: imeOwned
|
||||
}),
|
||||
Date.now()
|
||||
)
|
||||
|
||||
@@ -3,7 +3,8 @@ import {
|
||||
ModifierDoubleTapDetector,
|
||||
modifierFromKeyEvent,
|
||||
toModifierDoubleTapEvent,
|
||||
type ModifierDoubleTapEvent
|
||||
type ModifierDoubleTapEvent,
|
||||
type ModifierKeyEventLike
|
||||
} from './modifier-double-tap-detector'
|
||||
|
||||
function down(
|
||||
@@ -134,3 +135,62 @@ describe('ModifierDoubleTapDetector', () => {
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
// A detected tap is dispatched as `{ doubleTapModifier }` with no key or modifier flags, so once
|
||||
// one is armed there is nothing left for the shortcut matcher to refuse on. That makes this the
|
||||
// only place the refusal can happen — and normalizing it here means a listener that forgets the
|
||||
// check still cannot arm a gesture from a keystroke the IME owns.
|
||||
describe('toModifierDoubleTapEvent refuses input an IME owns', () => {
|
||||
const shiftTap = (over: Partial<ModifierKeyEventLike> = {}): ModifierKeyEventLike => ({
|
||||
type: 'keyDown',
|
||||
code: 'ShiftLeft',
|
||||
key: 'Shift',
|
||||
shift: true,
|
||||
...over
|
||||
})
|
||||
|
||||
it('reads a bare modifier press as a tap when nothing marks it', () => {
|
||||
expect(toModifierDoubleTapEvent(shiftTap())).toMatchObject({
|
||||
modifier: 'Shift',
|
||||
isModifierOnly: true
|
||||
})
|
||||
})
|
||||
|
||||
it.each([
|
||||
['isComposing', { isComposing: true }],
|
||||
['keyCode 229', { keyCode: 229 }],
|
||||
['key Process', { key: 'Process' }]
|
||||
])('drops the modifier when the event is marked by %s', (_marker, over) => {
|
||||
expect(toModifierDoubleTapEvent(shiftTap(over))).toMatchObject({
|
||||
modifier: null,
|
||||
isModifierOnly: false
|
||||
})
|
||||
})
|
||||
|
||||
it('leaves callers that pass no marker fields unchanged', () => {
|
||||
// Electron's before-input-event carries neither marker; it must keep working.
|
||||
expect(
|
||||
toModifierDoubleTapEvent({ type: 'keyDown', code: 'MetaLeft', meta: true })
|
||||
).toMatchObject({ modifier: 'Cmd' })
|
||||
})
|
||||
|
||||
it('breaks a gesture whose second press the IME owns, rather than emitting', () => {
|
||||
const d = new ModifierDoubleTapDetector()
|
||||
d.process(toModifierDoubleTapEvent(shiftTap()), 0)
|
||||
d.process(toModifierDoubleTapEvent(shiftTap({ type: 'keyUp' })), 10)
|
||||
|
||||
expect(d.process(toModifierDoubleTapEvent(shiftTap({ isComposing: true })), 20)).toBeNull()
|
||||
// And the armed state is gone, so the next unmarked press starts over instead of completing.
|
||||
expect(d.process(toModifierDoubleTapEvent(shiftTap()), 30)).toBeNull()
|
||||
})
|
||||
|
||||
// The release is what arms the second half of the gesture, so it needs the same refusal as the
|
||||
// press — otherwise a composition that starts mid-gesture still completes a tap.
|
||||
it('does not let a composing release arm the gesture', () => {
|
||||
const d = new ModifierDoubleTapDetector()
|
||||
d.process(toModifierDoubleTapEvent(shiftTap()), 0)
|
||||
d.process(toModifierDoubleTapEvent(shiftTap({ type: 'keyUp', isComposing: true })), 10)
|
||||
|
||||
expect(d.process(toModifierDoubleTapEvent(shiftTap()), 20)).toBeNull()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import type { PhysicalModifierToken } from './keybindings'
|
||||
import { keybindingInputIsImeOwned, type PhysicalModifierToken } from './keybindings'
|
||||
|
||||
// Why: max gap between the first release and the second press. Internal — not
|
||||
// user-configurable — and tight enough that normal fast typing never triggers.
|
||||
@@ -27,6 +27,9 @@ export type ModifierKeyEventLike = {
|
||||
alt?: boolean
|
||||
meta?: boolean
|
||||
isAutoRepeat?: boolean
|
||||
// Optional so non-DOM callers (Electron before-input-event) keep their current behavior.
|
||||
isComposing?: boolean
|
||||
keyCode?: number
|
||||
}
|
||||
|
||||
const MODIFIER_BY_CODE: Record<string, PhysicalModifierToken> = {
|
||||
@@ -77,7 +80,13 @@ function otherModifierHeld(event: ModifierKeyEventLike, modifier: PhysicalModifi
|
||||
|
||||
// Normalizes a platform key event (DOM or Electron) into the detector input.
|
||||
export function toModifierDoubleTapEvent(event: ModifierKeyEventLike): ModifierDoubleTapEvent {
|
||||
const modifier = modifierFromKeyEvent(event.code, event.key)
|
||||
// A null modifier resets the gesture, which is what an IME-owned keystroke should do: the
|
||||
// synthetic input a detection produces carries no key or modifier flags, so once a tap is
|
||||
// armed there is nothing left for the shortcut matcher to refuse on. Deciding it here
|
||||
// rather than at each listener means a site that forgets the check still cannot arm one.
|
||||
const modifier = keybindingInputIsImeOwned(event)
|
||||
? null
|
||||
: modifierFromKeyEvent(event.code, event.key)
|
||||
return {
|
||||
type: event.type,
|
||||
modifier,
|
||||
|
||||
Reference in New Issue
Block a user