diff --git a/src/main/browser/browser-guest-ui.test.ts b/src/main/browser/browser-guest-ui.test.ts index ce7871e7d98..84d9218a3ab 100644 --- a/src/main/browser/browser-guest-ui.test.ts +++ b/src/main/browser/browser-guest-ui.test.ts @@ -420,6 +420,47 @@ describe('setupGuestShortcutForwarding', () => { guestOffMock = vi.fn() }) + it('commits Ctrl+Tab switching from focused guest pages on generic release events', () => { + setupGuestShortcutForwarding({ + browserTabId, + guest: makeGuest(), + resolveRenderer: () => makeRenderer() + }) + + const ctrlTabInput = { code: 'Tab', key: 'Tab', control: true, meta: false } + const releaseInputs: Partial[] = [ + { + type: 'keyUp', + code: 'Control', + key: 'Control', + control: false, + meta: false + }, + { + type: 'keyUp', + code: 'Tab', + key: 'Tab', + control: false, + meta: false + } + ] + + for (const releaseInput of releaseInputs) { + rendererSendMock.mockClear() + const keyDownPreventDefault = triggerBeforeInput(ctrlTabInput) + const tabReleasePreventDefault = triggerBeforeInput({ ...ctrlTabInput, type: 'keyUp' }) + const keyUpPreventDefault = triggerBeforeInput(releaseInput) + + expect(keyDownPreventDefault).toHaveBeenCalledTimes(1) + expect(tabReleasePreventDefault).not.toHaveBeenCalled() + expect(keyUpPreventDefault).toHaveBeenCalledTimes(1) + expect(rendererSendMock).toHaveBeenNthCalledWith(1, 'ui:ctrlTabKeyDown', { + shiftKey: false + }) + expect(rendererSendMock).toHaveBeenNthCalledWith(2, 'ui:ctrlTabKeyUp') + } + }) + it('forwards browser page zoom shortcuts from focused guest pages', () => { setupGuestShortcutForwarding({ browserTabId, diff --git a/src/main/browser/browser-guest-ui.ts b/src/main/browser/browser-guest-ui.ts index 6d2e253dcb7..6272c4ba0e6 100644 --- a/src/main/browser/browser-guest-ui.ts +++ b/src/main/browser/browser-guest-ui.ts @@ -9,6 +9,7 @@ import { redactKagiSessionToken } from '../../shared/browser-url' import { + isRecentTabSwitcherCommitRelease, matchesRecentTabSwitcherChord, resolveWindowShortcutAction } from '../../shared/window-shortcut-policy' @@ -51,10 +52,6 @@ export function resolveGuestMouseWheelZoomDirection( return deltaY < 0 ? 'in' : 'out' } -function isControlKeyRelease(input: Electron.Input): boolean { - return input.type === 'keyUp' && (input.code === 'ControlLeft' || input.code === 'ControlRight') -} - export function setupGuestContextMenu(args: { browserTabId: string guest: Electron.WebContents @@ -268,17 +265,18 @@ export function setupGuestShortcutForwarding(args: { let ctrlTabSwitching = false const handler = (event: Electron.Event, input: Electron.Input): void => { const keybindings = getKeybindings?.() - if (matchesRecentTabSwitcherChord(input, process.platform, keybindings)) { + if ( + input.type === 'keyDown' && + matchesRecentTabSwitcherChord(input, process.platform, keybindings) + ) { event.preventDefault() - if (input.type === 'keyDown') { - ctrlTabSwitching = true - const renderer = resolveRenderer(browserTabId) - renderer?.send('ui:ctrlTabKeyDown', { shiftKey: input.shift === true }) - } + ctrlTabSwitching = true + const renderer = resolveRenderer(browserTabId) + renderer?.send('ui:ctrlTabKeyDown', { shiftKey: input.shift === true }) return } - if (ctrlTabSwitching && isControlKeyRelease(input)) { + if (ctrlTabSwitching && isRecentTabSwitcherCommitRelease(input)) { event.preventDefault() ctrlTabSwitching = false const renderer = resolveRenderer(browserTabId) diff --git a/src/main/window/createMainWindow.test.ts b/src/main/window/createMainWindow.test.ts index 2a5cb6a7390..a2f764e8872 100644 --- a/src/main/window/createMainWindow.test.ts +++ b/src/main/window/createMainWindow.test.ts @@ -456,7 +456,7 @@ describe('createMainWindow', () => { expect(webContents.send).toHaveBeenCalledWith('ui:jumpToTabIndex', 4) }) - it('forwards Ctrl+Tab keydown and Ctrl release to the renderer switcher', () => { + it('lets main-window Ctrl+Tab flow to the renderer held switcher', () => { const windowHandlers: Record void> = {} const webContents = { on: vi.fn((event, handler) => { @@ -491,52 +491,30 @@ describe('createMainWindow', () => { createMainWindow(null) const beforeInputEvent = windowHandlers['before-input-event'] - const firstPreventDefault = vi.fn() - beforeInputEvent( - { preventDefault: firstPreventDefault } as never, - { - type: 'keyDown', - code: 'Tab', - key: 'Tab', - control: true, - meta: false, - alt: false, - shift: false - } as never - ) - const secondPreventDefault = vi.fn() - beforeInputEvent( - { preventDefault: secondPreventDefault } as never, - { - type: 'keyDown', - code: 'Tab', - key: 'Tab', - control: true, - meta: false, - alt: false, - shift: true - } as never - ) - const releasePreventDefault = vi.fn() - beforeInputEvent( - { preventDefault: releasePreventDefault } as never, - { - type: 'keyUp', - code: 'ControlLeft', - key: 'Control', - control: false, - meta: false, - alt: false, - shift: false - } as never - ) + const dispatchInput = (input: Electron.Input): ReturnType => { + const preventDefault = vi.fn() + beforeInputEvent({ preventDefault } as never, input as never) + return preventDefault + } + const ctrlTabInput = { + code: 'Tab', + key: 'Tab', + control: true, + meta: false, + alt: false + } + const preventDefaults = [ + { type: 'keyDown', shift: false }, + { type: 'keyDown', shift: true }, + { type: 'keyUp', shift: true }, + { type: 'keyUp', code: 'ControlLeft', key: 'Control', control: false, shift: false } + ].map((input) => dispatchInput({ ...ctrlTabInput, ...input } as Electron.Input)) - expect(firstPreventDefault).toHaveBeenCalledTimes(1) - expect(secondPreventDefault).toHaveBeenCalledTimes(1) - expect(releasePreventDefault).toHaveBeenCalledTimes(1) - expect(webContents.send).toHaveBeenNthCalledWith(1, 'ui:ctrlTabKeyDown', { shiftKey: false }) - expect(webContents.send).toHaveBeenNthCalledWith(2, 'ui:ctrlTabKeyDown', { shiftKey: true }) - expect(webContents.send).toHaveBeenNthCalledWith(3, 'ui:ctrlTabKeyUp') + for (const preventDefault of preventDefaults) { + expect(preventDefault).not.toHaveBeenCalled() + } + expect(webContents.send).not.toHaveBeenCalledWith('ui:ctrlTabKeyDown', expect.anything()) + expect(webContents.send).not.toHaveBeenCalledWith('ui:ctrlTabKeyUp') }) it('does not hardcode Ctrl+Tab when the recent-tab binding is disabled', () => { diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index a4d4d43f5cd..e6151e7c3e8 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -43,10 +43,6 @@ function forceRepaint(window: BrowserWindow): void { }, 32) } -function isControlKeyRelease(input: Electron.Input): boolean { - return input.type === 'keyUp' && (input.code === 'ControlLeft' || input.code === 'ControlRight') -} - function nativeZoomCommandMatchesKeybindings( direction: 'in' | 'out', platform: NodeJS.Platform, @@ -655,7 +651,6 @@ export function createMainWindow( clearRendererRecoveryTimer() }) - let ctrlTabSwitching = false mainWindow.webContents.on('before-input-event', (event, input) => { if (shortcutRecorderFocused) { return @@ -679,23 +674,11 @@ export function createMainWindow( ) } if ( + input.type === 'keyDown' && matchesRecentTabSwitcherChord(input, process.platform, keybindings, terminalShortcutContext) ) { - // Why: Ctrl+Tab is a held-key interaction. Route both press and release - // through IPC so renderer keyup suppression from preventDefault cannot - // leave the switcher overlay stranded. - event.preventDefault() - if (input.type === 'keyDown') { - ctrlTabSwitching = true - mainWindow.webContents.send('ui:ctrlTabKeyDown', { shiftKey: input.shift === true }) - } - return - } - - if (ctrlTabSwitching && isControlKeyRelease(input)) { - event.preventDefault() - ctrlTabSwitching = false - mainWindow.webContents.send('ui:ctrlTabKeyUp') + // Why: the held switcher commits on modifier keyup. If main prevents the + // keydown, Electron can suppress the renderer keyup and strand the overlay. return } diff --git a/src/renderer/src/components/tab-bar/RecentTabSwitcher.test.tsx b/src/renderer/src/components/tab-bar/RecentTabSwitcher.test.tsx new file mode 100644 index 00000000000..e0a0ab9f7f0 --- /dev/null +++ b/src/renderer/src/components/tab-bar/RecentTabSwitcher.test.tsx @@ -0,0 +1,217 @@ +// @vitest-environment happy-dom + +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Tab } from '../../../../shared/types' +import type { AppState } from '../../store/types' + +const { activateCyclableTabMock, getStateMock } = vi.hoisted(() => ({ + activateCyclableTabMock: vi.fn(), + getStateMock: vi.fn() +})) + +vi.mock('../../store', () => ({ + useAppStore: { + getState: getStateMock + } +})) + +vi.mock('../../hooks/ipc-tab-switch', () => ({ + activateCyclableTab: activateCyclableTabMock +})) + +vi.mock('@/i18n/i18n', () => ({ + translate: (_key: string, fallback: string) => fallback +})) + +import RecentTabSwitcher from './RecentTabSwitcher' + +const WORKTREE_ID = 'wt-1' +const GROUP_ID = 'group-1' + +type CtrlTabKeyDownCallback = (data: { shiftKey: boolean }) => void +type CtrlTabKeyUpCallback = () => void + +let ctrlTabKeyDownCallback: CtrlTabKeyDownCallback | null = null + +function makeTab(id: string, entityId: string, label: string): Tab { + return { + id, + entityId, + groupId: GROUP_ID, + worktreeId: WORKTREE_ID, + contentType: 'editor', + label, + customLabel: null, + color: null, + sortOrder: 0, + createdAt: 1 + } +} + +function makeStore(): AppState { + const tabs = [ + makeTab('tab-a', 'file-a', 'A'), + makeTab('tab-b', 'file-b', 'B'), + makeTab('tab-c', 'file-c', 'C') + ] + return { + activeView: 'terminal', + activeWorktreeId: WORKTREE_ID, + activeBrowserTabId: null, + activeFileId: 'file-a', + activeGroupIdByWorktree: { [WORKTREE_ID]: GROUP_ID }, + activeTabId: null, + activeTabType: 'editor', + browserTabsByWorktree: {}, + groupsByWorktree: { + [WORKTREE_ID]: [ + { + id: GROUP_ID, + worktreeId: WORKTREE_ID, + activeTabId: 'tab-a', + tabOrder: ['tab-a', 'tab-b', 'tab-c'], + recentTabIds: ['tab-c', 'tab-b', 'tab-a'] + } + ] + }, + openFiles: tabs.map((tab) => ({ + id: tab.entityId, + worktreeId: WORKTREE_ID, + isDirty: false + })), + settings: { ctrlTabOrderMode: 'mru' }, + tabBarOrderByWorktree: {}, + tabsByWorktree: {}, + unifiedTabsByWorktree: { [WORKTREE_ID]: tabs } + } as unknown as AppState +} + +function installWindowApi(): void { + Object.defineProperty(window, 'api', { + configurable: true, + value: { + ui: { + onCtrlTabKeyDown: vi.fn((callback: CtrlTabKeyDownCallback) => { + ctrlTabKeyDownCallback = callback + return vi.fn() + }), + onCtrlTabKeyUp: vi.fn((_callback: CtrlTabKeyUpCallback) => vi.fn()) + } + } + }) +} + +async function renderSwitcher(): Promise<{ container: HTMLDivElement; root: Root }> { + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + await act(async () => { + root.render() + }) + return { container, root } +} + +function appendTerminalTextarea(): { + input: HTMLTextAreaElement + keyDown: ReturnType + keyUp: ReturnType +} { + const input = document.createElement('textarea') + input.className = 'xterm-helper-textarea' + const keyDown = vi.fn() + const keyUp = vi.fn() + input.addEventListener('keydown', keyDown) + input.addEventListener('keyup', keyUp) + document.body.appendChild(input) + return { input, keyDown, keyUp } +} + +async function dispatchKeyboard( + target: HTMLElement, + type: 'keydown' | 'keyup', + init: KeyboardEventInit +): Promise { + const event = new KeyboardEvent(type, { + bubbles: true, + cancelable: true, + ...init + }) + await act(async () => { + target.dispatchEvent(event) + }) + return event +} + +function expectCommittedToTabB(): void { + expect(activateCyclableTabMock).toHaveBeenCalledTimes(1) + expect(activateCyclableTabMock.mock.calls[0][1]).toMatchObject({ key: 'tab-b', label: 'B' }) + expect(document.body.querySelector('[role="listbox"]')).toBeNull() +} + +describe('RecentTabSwitcher', () => { + beforeEach(() => { + ctrlTabKeyDownCallback = null + activateCyclableTabMock.mockReset() + getStateMock.mockReturnValue(makeStore()) + installWindowApi() + }) + + afterEach(() => { + document.body.innerHTML = '' + vi.unstubAllGlobals() + }) + + it('commits the selected tab on modifier release before terminal input sees it', async () => { + const { root } = await renderSwitcher() + + await act(async () => { + ctrlTabKeyDownCallback?.({ shiftKey: false }) + }) + + const terminal = appendTerminalTextarea() + const event = await dispatchKeyboard(terminal.input, 'keyup', { + key: 'Control', + code: 'ControlLeft', + ctrlKey: false + }) + + expect(event.defaultPrevented).toBe(true) + expect(terminal.keyUp).not.toHaveBeenCalled() + expectCommittedToTabB() + + await act(async () => { + root.unmount() + }) + }) + + it('opens from DOM Ctrl+Tab and commits on DOM Ctrl release', async () => { + const { root } = await renderSwitcher() + const terminal = appendTerminalTextarea() + + const keyDown = await dispatchKeyboard(terminal.input, 'keydown', { + key: 'Tab', + code: 'Tab', + ctrlKey: true + }) + + expect(keyDown.defaultPrevented).toBe(true) + expect(terminal.keyDown).not.toHaveBeenCalled() + expect(document.body.querySelector('[role="listbox"]')).not.toBeNull() + + const keyUp = await dispatchKeyboard(terminal.input, 'keyup', { + key: 'Control', + code: 'ControlLeft', + ctrlKey: false + }) + + expect(keyUp.defaultPrevented).toBe(true) + expect(terminal.keyUp).not.toHaveBeenCalled() + expectCommittedToTabB() + + await act(async () => { + root.unmount() + }) + }) +}) diff --git a/src/renderer/src/components/tab-bar/RecentTabSwitcher.tsx b/src/renderer/src/components/tab-bar/RecentTabSwitcher.tsx index 85e47fe9fe2..b4f16f31a97 100644 --- a/src/renderer/src/components/tab-bar/RecentTabSwitcher.tsx +++ b/src/renderer/src/components/tab-bar/RecentTabSwitcher.tsx @@ -4,7 +4,10 @@ import { FileText, GitCompare, Globe2, TerminalSquare } from 'lucide-react' import { useAppStore } from '../../store' import { activateCyclableTab } from '../../hooks/ipc-tab-switch' import { getShortcutPlatform } from '../../hooks/useShortcutLabel' -import { matchesRecentTabSwitcherChord } from '../../../../shared/window-shortcut-policy' +import { + isRecentTabSwitcherCommitRelease, + matchesRecentTabSwitcherChord +} from '../../../../shared/window-shortcut-policy' import { buildRecentTabSwitcherModel, getNextRecentTabSwitcherIndex, @@ -18,6 +21,11 @@ type SwitcherState = { selectedIndex: number } +function consumeKeyboardEvent(event: KeyboardEvent): void { + event.preventDefault() + event.stopPropagation() +} + function TabIcon({ item }: { item: RecentTabSwitcherItem }): React.JSX.Element { const className = 'size-4 shrink-0 text-muted-foreground' if (item.type === 'terminal') { @@ -105,36 +113,32 @@ export default function RecentTabSwitcher(): React.JSX.Element | null { // Why: Electron's native before-input-event path is authoritative, but // CDP/test-dispatched keys can reach the renderer directly. Respect the // keybinding registry here too so tests do not bypass user customization. - event.preventDefault() - event.stopPropagation() + consumeKeyboardEvent(event) openOrAdvance(event.shiftKey ? -1 : 1) return } - if (!switcherRef.current || event.key !== 'Escape') { + if (!switcherRef.current) { return } - event.preventDefault() - cancel() + if (event.key === 'Escape') { + consumeKeyboardEvent(event) + cancel() + } } const onKeyUp = (event: KeyboardEvent): void => { - if ( - !switcherRef.current || - (event.code !== 'ControlLeft' && event.code !== 'ControlRight' && event.key !== 'Control') - ) { + if (!switcherRef.current || !isRecentTabSwitcherCommitRelease(event)) { return } - event.preventDefault() - event.stopPropagation() + consumeKeyboardEvent(event) commit() } - const onBlur = (): void => cancel() window.addEventListener('keydown', onKeyDown, { capture: true }) window.addEventListener('keyup', onKeyUp, { capture: true }) - window.addEventListener('blur', onBlur) + window.addEventListener('blur', cancel) return () => { window.removeEventListener('keydown', onKeyDown, { capture: true }) window.removeEventListener('keyup', onKeyUp, { capture: true }) - window.removeEventListener('blur', onBlur) + window.removeEventListener('blur', cancel) } }, [cancel, commit, openOrAdvance]) diff --git a/src/shared/window-shortcut-policy.test.ts b/src/shared/window-shortcut-policy.test.ts index e220ebff9ae..82ac6a21f51 100644 --- a/src/shared/window-shortcut-policy.test.ts +++ b/src/shared/window-shortcut-policy.test.ts @@ -4,6 +4,7 @@ navigation, new-workspace tab routing). Splitting across files would fragment the test of a single pure function. */ import { describe, expect, it } from 'vitest' import { + isRecentTabSwitcherCommitRelease, isWindowShortcutModifierChord, matchesRecentTabSwitcherChord, resolveWindowShortcutAction, @@ -329,6 +330,49 @@ describe('resolveWindowShortcutAction', () => { expect(matchesRecentTabSwitcherChord(eventInput, 'linux')).toBe(true) }) + it('recognizes Ctrl+Tab commit releases across Electron surfaces', () => { + expect( + isRecentTabSwitcherCommitRelease({ + type: 'keyUp', + code: 'ControlLeft', + key: 'Control', + control: false + }) + ).toBe(true) + expect( + isRecentTabSwitcherCommitRelease({ + type: 'keyUp', + code: 'Control', + key: 'Control', + control: false + }) + ).toBe(true) + expect( + isRecentTabSwitcherCommitRelease({ + type: 'keyUp', + code: 'Tab', + key: 'Tab', + control: false + }) + ).toBe(true) + expect( + isRecentTabSwitcherCommitRelease({ + type: 'keyUp', + code: 'Tab', + key: 'Tab', + control: true + }) + ).toBe(false) + expect( + isRecentTabSwitcherCommitRelease({ + type: 'keyup', + code: 'ControlLeft', + key: 'Control', + ctrlKey: false + }) + ).toBe(true) + }) + it('accepts all supported zoom key variants', () => { const zoomInCases: WindowShortcutInput[] = [ { key: '=', meta: true, control: false, alt: false, shift: false }, diff --git a/src/shared/window-shortcut-policy.ts b/src/shared/window-shortcut-policy.ts index 95fb96092e5..2d2647ace65 100644 --- a/src/shared/window-shortcut-policy.ts +++ b/src/shared/window-shortcut-policy.ts @@ -92,6 +92,32 @@ export function matchesRecentTabSwitcherChord( ) } +function isControlKey(input: WindowShortcutInput): boolean { + return ( + input.code === 'ControlLeft' || + input.code === 'ControlRight' || + input.code === 'Control' || + input.key === 'Control' + ) +} + +function isTabKey(input: WindowShortcutInput): boolean { + return input.code === 'Tab' || input.key === 'Tab' +} + +export function isRecentTabSwitcherCommitRelease(input: WindowShortcutInput): boolean { + if (input.type !== 'keyUp' && input.type !== 'keyup') { + return false + } + if (isControlKey(input)) { + return true + } + const control = input.control ?? input.ctrlKey + // Why: some Electron surfaces report the final Ctrl+Tab release as Tab + // keyup after Control is already up, so commit instead of stranding the UI. + return isTabKey(input) && control === false +} + function actionMatches( actionId: KeybindingActionId, input: WindowShortcutInput,