From 2c268f23eab4b3311944eae8895d6eede46e81db Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 2 Aug 2026 02:51:43 -0700 Subject: [PATCH] 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 --- src/renderer/src/App.tsx | 20 +++--- .../FloatingTerminalPanel.tsx | 11 ++- .../TabBarQuickCommandsMenu.keyboard.test.ts | 70 ++++++++++++++++++- .../tab-bar-quick-commands-shortcut.ts | 30 ++++---- .../modifier-double-tap-detector.test.ts | 62 +++++++++++++++- src/shared/modifier-double-tap-detector.ts | 13 +++- 6 files changed, 176 insertions(+), 30 deletions(-) diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index 4c60ab11da6..660dd3b2090 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -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() ) diff --git a/src/renderer/src/components/floating-terminal/FloatingTerminalPanel.tsx b/src/renderer/src/components/floating-terminal/FloatingTerminalPanel.tsx index 66c83ac063b..26723663ad1 100644 --- a/src/renderer/src/components/floating-terminal/FloatingTerminalPanel.tsx +++ b/src/renderer/src/components/floating-terminal/FloatingTerminalPanel.tsx @@ -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() ) diff --git a/src/renderer/src/components/tab-bar/TabBarQuickCommandsMenu.keyboard.test.ts b/src/renderer/src/components/tab-bar/TabBarQuickCommandsMenu.keyboard.test.ts index 51b8758d2c9..7611faff0fc 100644 --- a/src/renderer/src/components/tab-bar/TabBarQuickCommandsMenu.keyboard.test.ts +++ b/src/renderer/src/components/tab-bar/TabBarQuickCommandsMenu.keyboard.test.ts @@ -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) + + const shiftTap = (over: Partial = {}): KeyboardEvent => + makeKeyEvent({ key: 'Shift', code: 'ShiftLeft', metaKey: false, shiftKey: true, ...over }) + + async function mountListeners(): Promise { + 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)) + + 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) + ) + 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') diff --git a/src/renderer/src/components/tab-bar/tab-bar-quick-commands-shortcut.ts b/src/renderer/src/components/tab-bar/tab-bar-quick-commands-shortcut.ts index 2ddac712c78..7a209c470c2 100644 --- a/src/renderer/src/components/tab-bar/tab-bar-quick-commands-shortcut.ts +++ b/src/renderer/src/components/tab-bar/tab-bar-quick-commands-shortcut.ts @@ -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() ) diff --git a/src/shared/modifier-double-tap-detector.test.ts b/src/shared/modifier-double-tap-detector.test.ts index 7a0446e9b7b..a01468fa3d9 100644 --- a/src/shared/modifier-double-tap-detector.test.ts +++ b/src/shared/modifier-double-tap-detector.test.ts @@ -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 => ({ + 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() + }) +}) diff --git a/src/shared/modifier-double-tap-detector.ts b/src/shared/modifier-double-tap-detector.ts index aea6b6527c7..a0d03db6d8d 100644 --- a/src/shared/modifier-double-tap-detector.ts +++ b/src/shared/modifier-double-tap-detector.ts @@ -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 = { @@ -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,