From 5327f27ccccff0fa5336dede353b170d3b6ed671 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 2 Aug 2026 03:22:37 -0700 Subject: [PATCH] fix(ime): carry the composition markers through the last two rebuilt inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more places rebuilt a keybinding input field-by-field and dropped the markers the matcher refuses on. `matchesRecentTabSwitcherChord` is the harder one: its callers pass the raw event, so a call-site sweep reads clean, but the helper normalizes Shift away by hand-building a literal one level down. Ctrl+Tab therefore still resolved mid-composition. The two `before-input-event` handlers omitted `isComposing` when feeding the double-tap detector. `Electron.Input` does carry it — a completed gesture dispatches `{ doubleTapModifier }` with no key or modifier flags, so once one is armed the matcher has nothing left to refuse on. Also covers the floating terminal panel, whose product fix already landed but whose suite passed unchanged when both ownership decisions were mutated away. Every guard here is mutation-checked: disabling one fails exactly the tests that name it, and no others. Co-authored-by: Orca --- src/main/browser/browser-guest-ui.test.ts | 39 +++++++++ src/main/browser/browser-guest-ui.ts | 3 + src/main/window/createMainWindow.test.ts | 35 ++++++++ src/main/window/createMainWindow.ts | 3 + .../FloatingTerminalPanel.test.tsx | 86 +++++++++++++++++++ ...ndow-shortcut-policy-ime-ownership.test.ts | 44 ++++++++++ src/shared/window-shortcut-policy.ts | 10 ++- 7 files changed, 219 insertions(+), 1 deletion(-) create mode 100644 src/shared/window-shortcut-policy-ime-ownership.test.ts diff --git a/src/main/browser/browser-guest-ui.test.ts b/src/main/browser/browser-guest-ui.test.ts index 946ed38a7ef..b5edb09691f 100644 --- a/src/main/browser/browser-guest-ui.test.ts +++ b/src/main/browser/browser-guest-ui.test.ts @@ -801,6 +801,45 @@ describe('setupGuestShortcutForwarding', () => { expect(rendererSendMock).not.toHaveBeenCalledWith('ui:openQuickOpen') }) + // `Electron.Input` carries `isComposing`, so the guest path can refuse an IME-owned press the + // same way the renderer does. A completed gesture dispatches `{ doubleTapModifier }` with no + // key or modifier flags, leaving the matcher nothing to refuse on — it has to stop here. + it.each([ + ['a marked second press', 1], + ['a marked release', 0] + ])('does not complete a guest double-tap from %s', (_case, markedIndex) => { + setupGuestShortcutForwarding({ + browserTabId, + guest: makeGuest(), + resolveRenderer: () => makeRenderer(), + getKeybindings: () => ({ + 'worktree.quickOpen': ['DoubleTap+Shift'] + }) + }) + + const modifierInput = { + code: 'ShiftLeft', + key: 'Shift', + shift: true, + meta: false, + control: false, + alt: false + } + const sequence = [ + { ...modifierInput, type: 'keyUp' as const }, + { ...modifierInput, type: 'keyDown' as const } + ] + triggerBeforeInput(modifierInput) + const preventDefaults = sequence.map((step, index) => + triggerBeforeInput(index === markedIndex ? { ...step, isComposing: true } : step) + ) + + for (const preventDefault of preventDefaults) { + expect(preventDefault).not.toHaveBeenCalled() + } + expect(rendererSendMock).not.toHaveBeenCalledWith('ui:openQuickOpen') + }) + // A floating-workspace-owned guest routes close/index chords to the floating-scoped // IPC *carrying its source id*; a main-owned guest keeps main routing. describe('source-aware close/index routing', () => { diff --git a/src/main/browser/browser-guest-ui.ts b/src/main/browser/browser-guest-ui.ts index b9c52b39aa7..0c6aed69c41 100644 --- a/src/main/browser/browser-guest-ui.ts +++ b/src/main/browser/browser-guest-ui.ts @@ -500,6 +500,9 @@ export function setupGuestShortcutForwarding(args: { control: input.control, alt: input.alt, meta: input.meta, + // Electron.Input carries isComposing (no numeric keyCode); without it the detector + // could arm a gesture from a keystroke the IME owns. + isComposing: input.isComposing, isAutoRepeat: input.isAutoRepeat }), Date.now() diff --git a/src/main/window/createMainWindow.test.ts b/src/main/window/createMainWindow.test.ts index 8a61f157584..775e07a6380 100644 --- a/src/main/window/createMainWindow.test.ts +++ b/src/main/window/createMainWindow.test.ts @@ -1315,6 +1315,41 @@ describe('createMainWindow', () => { const togglePreventDefault = triggerDoubleTapShift() expect(togglePreventDefault).toHaveBeenCalledTimes(1) expect(webContents.send).toHaveBeenCalledWith('ui:dictationKeyDown') + + // `Electron.Input` carries `isComposing`. A completed gesture dispatches + // `{ doubleTapModifier }` with no key or modifier flags, so the shortcut matcher has nothing + // left to refuse on — the ownership check has to happen before the detector sees the press. + const modifierInput = { + code: 'ShiftLeft', + key: 'Shift', + shift: true, + meta: false, + control: false, + alt: false + } + for (const markedIndex of [1, 0]) { + webContents.send.mockClear() + const steps = [ + { ...modifierInput, type: 'keyUp' }, + { ...modifierInput, type: 'keyDown' } + ] + windowHandlers['before-input-event']( + { preventDefault: vi.fn() } as never, + { ...modifierInput, type: 'keyDown' } as never + ) + for (const [index, step] of steps.entries()) { + windowHandlers['before-input-event']( + { preventDefault: vi.fn() } as never, + (index === markedIndex ? { ...step, isComposing: true } : step) as never + ) + } + expect(webContents.send).not.toHaveBeenCalledWith('ui:dictationKeyDown') + // Clear the trailing armed state so the next case starts from idle. + windowHandlers['before-input-event']( + { preventDefault: vi.fn() } as never, + { code: 'KeyA', key: 'a', type: 'keyDown' } as never + ) + } }) it('forwards ctrl/cmd+j to the worktree palette toggle event', () => { diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index 49c3eb4c125..a8fe4f2ae73 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -834,6 +834,9 @@ export function createMainWindow( control: input.control, alt: input.alt, meta: input.meta, + // Electron.Input carries isComposing (no numeric keyCode); without it the detector + // could arm a gesture from a keystroke the IME owns. + isComposing: input.isComposing, isAutoRepeat: input.isAutoRepeat }), Date.now() diff --git a/src/renderer/src/components/floating-terminal/FloatingTerminalPanel.test.tsx b/src/renderer/src/components/floating-terminal/FloatingTerminalPanel.test.tsx index d9f846fbbda..3b03442891d 100644 --- a/src/renderer/src/components/floating-terminal/FloatingTerminalPanel.test.tsx +++ b/src/renderer/src/components/floating-terminal/FloatingTerminalPanel.test.tsx @@ -1713,6 +1713,92 @@ describe('FloatingTerminalPanel close behavior', () => { expect(mocks.createTab).not.toHaveBeenCalled() }) + // The double-tap gesture is dispatched as `{ doubleTapModifier }` with no key or modifier + // flags, so once it is armed the shortcut matcher has nothing left to refuse on. These drive + // the real window listeners rather than the detector, because the panel is where the ownership + // decision is read off the event and handed down. + describe('while an IME owns a focused floating terminal keystroke', () => { + async function bindDoubleTapListeners(): Promise<{ + keyDown: (event: unknown) => void + keyUp: (event: unknown) => void + shiftTap: (over?: Record) => Record + }> { + setFloatingTabs([makeTab({ id: 'tab-1' })]) + ;(storeBox.state as FloatingPanelStoreState).keybindings = { + 'tab.newTerminal': ['DoubleTap+Shift'] + } + const element = await renderPanel(true) + const panel = findByProp(element, 'data-floating-terminal-panel') + const panelElement = { contains: vi.fn().mockReturnValue(true), focus: vi.fn() } + const target = { + classList: { contains: vi.fn((token: string) => token === 'xterm-helper-textarea') }, + closest: vi.fn((selector: string) => + selector === '[data-floating-terminal-panel]' ? panelElement : null + ) + } + Object.setPrototypeOf(target, HTMLElement.prototype) + attachRef(panel.props.ref, panelElement) + vi.stubGlobal('document', { + activeElement: target, + addEventListener: vi.fn(), + removeEventListener: vi.fn() + }) + runEffects() + const listener = (type: string): ((event: unknown) => void) | undefined => + vi.mocked(window.addEventListener).mock.calls.find(([name]) => name === type)?.[1] as + | ((event: unknown) => void) + | undefined + const keyDown = listener('keydown') + const keyUp = listener('keyup') + if (!keyDown || !keyUp) { + throw new Error('keyboard listeners not registered') + } + return { + keyDown, + keyUp, + shiftTap: (over = {}) => ({ + altKey: false, + code: 'ShiftLeft', + ctrlKey: false, + defaultPrevented: false, + key: 'Shift', + metaKey: false, + preventDefault: vi.fn(), + repeat: false, + shiftKey: true, + stopImmediatePropagation: vi.fn(), + stopPropagation: vi.fn(), + target, + ...over + }) + } + } + + it('does not complete a double-tap whose second press is marked', async () => { + const { keyDown, keyUp, shiftTap } = await bindDoubleTapListeners() + + keyDown(shiftTap()) + keyUp(shiftTap()) + // The real shape a composing press has — not `key: 'Process'`, which resolves to no + // binding and so would pass even with the ownership check deleted. + keyDown(shiftTap({ isComposing: true, keyCode: 229 })) + await flushAsyncWork() + + expect(mocks.createTab).not.toHaveBeenCalled() + }) + + it('does not let a marked release arm the second half of the gesture', async () => { + const { keyDown, keyUp, shiftTap } = await bindDoubleTapListeners() + + keyDown(shiftTap()) + keyUp(shiftTap({ isComposing: true, keyCode: 229 })) + keyDown(shiftTap()) + await flushAsyncWork() + + expect(mocks.createTab).not.toHaveBeenCalled() + }) + }) + it('routes focused floating tab switch shortcuts to the floating workspace', async () => { setFloatingTabs([makeTab({ id: 'tab-1' }), makeTab({ id: 'tab-2' })]) const element = await renderPanel(true) diff --git a/src/shared/window-shortcut-policy-ime-ownership.test.ts b/src/shared/window-shortcut-policy-ime-ownership.test.ts new file mode 100644 index 00000000000..5269ea2fdea --- /dev/null +++ b/src/shared/window-shortcut-policy-ime-ownership.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from 'vitest' +import { + matchesRecentTabSwitcherChord, + resolveWindowShortcutAction, + type WindowShortcutInput +} from './window-shortcut-policy' +import type { KeybindingOverrides } from './keybindings' + +// `matchesRecentTabSwitcherChord` takes a whole event but hands the matcher a literal it rebuilds +// field-by-field, so the IME markers can go missing one level below a call site that looks +// perfectly safe. That is how it survived the sweep that fixed the four direct adapters. +describe('window shortcut resolution refuses input an IME owns', () => { + const recentTab: KeybindingOverrides = { 'tab.previousRecent': ['Mod+Tab'] } + + const ctrlTab = (over: Partial = {}): WindowShortcutInput => ({ + type: 'keyDown', + key: 'Tab', + code: 'Tab', + control: true, + ...over + }) + + it('matches a real Ctrl+Tab', () => { + expect(matchesRecentTabSwitcherChord(ctrlTab(), 'linux', recentTab)).toBe(true) + }) + + it.each([ + ['isComposing', { isComposing: true }], + ['keyCode 229', { keyCode: 229 }] + ])('refuses the switcher chord when marked by %s', (_marker, over) => { + expect(matchesRecentTabSwitcherChord(ctrlTab(over), 'linux', recentTab)).toBe(false) + }) + + // The resolver forwards its input untouched, so this pins that it keeps doing so. + it('resolves no action for a marked chord', () => { + const zoomIn: WindowShortcutInput = { key: '=', code: 'Equal', control: true } + + expect(resolveWindowShortcutAction(zoomIn, 'linux')).toEqual({ + type: 'zoom', + direction: 'in' + }) + expect(resolveWindowShortcutAction({ ...zoomIn, isComposing: true }, 'linux')).toBeNull() + }) +}) diff --git a/src/shared/window-shortcut-policy.ts b/src/shared/window-shortcut-policy.ts index 0099c7e88a0..553a4d85f60 100644 --- a/src/shared/window-shortcut-policy.ts +++ b/src/shared/window-shortcut-policy.ts @@ -22,6 +22,10 @@ export type WindowShortcutInput = { metaKey?: boolean ctrlKey?: boolean shiftKey?: boolean + // The matcher refuses a chord an IME owns by reading these, so anything that rebuilds this + // shape has to carry them. `Electron.Input` supplies isComposing; it has no numeric keyCode. + isComposing?: boolean + keyCode?: number // Set only by the double-tap detector; threads the synthetic input through // the main-process resolver so allowlisted actions can fire on double-tap. doubleTapModifier?: PhysicalModifierToken @@ -89,7 +93,11 @@ export function matchesRecentTabSwitcherChord( altKey: alt, metaKey: meta, ctrlKey: control, - shiftKey: false + shiftKey: false, + // Rebuilt rather than forwarded because Shift is deliberately normalized away, so the + // markers have to be carried across by hand or the matcher has nothing to refuse on. + isComposing: input.isComposing, + keyCode: input.keyCode }, platform, keybindings,