diff --git a/frontend/taskdeck-web/src/components/shell/AppShell.vue b/frontend/taskdeck-web/src/components/shell/AppShell.vue index 2c07d71a7..3066951eb 100644 --- a/frontend/taskdeck-web/src/components/shell/AppShell.vue +++ b/frontend/taskdeck-web/src/components/shell/AppShell.vue @@ -184,6 +184,34 @@ function activeKeyboardOwningSurfaces(): HTMLElement[] { }) } +/** + * One surface scan per keydown, shared by the two guards below (#2636). + * + * The capture-phase listener always runs before the bubble-phase one for the + * same event, so whichever asks first pays for the `querySelectorAll` plus + * `getComputedStyle` sweep and the other reads the answer. Without the memo the + * bubble guard would double the per-keystroke cost #1968 went out of its way to + * make lazy. + * + * The answer is deliberately pinned to the event rather than re-read on the + * bubble: a surface handler may have closed its own surface on the way up (a + * palette option activating, say), and the key still belonged to the surface + * that was open when it was pressed. + * + * Per shell instance, not per module: these live in this `setup()` closure, so + * two shells (as tests mount them) never share a memo. + */ +let scannedEvent: KeyboardEvent | null = null +let scannedSurfaces: HTMLElement[] = [] + +function keyboardOwningSurfacesFor(event: KeyboardEvent): HTMLElement[] { + if (scannedEvent !== event) { + scannedEvent = event + scannedSurfaces = activeKeyboardOwningSurfaces() + } + return scannedSurfaces +} + /** * True when this action's own surface is among the active ones and every active * surface belongs to the shell. That is what makes `?` and `mod+k` toggles @@ -278,6 +306,63 @@ function guardSurfaceFromPageShortcuts(event: KeyboardEvent, surfaces: readonly event.stopPropagation() } +/** + * The other half of the same guard, for keys pressed from INSIDE the surface + * (#2636). + * + * `guardSurfaceFromPageShortcuts` above runs in the capture phase and has to + * stand aside when the target is inside the surface, because at that point the + * surface's own handlers have not run yet. That carve-out was the leak PR #2635 + * recorded: Tab into the open help dialog on a Legacy board and press `f` or + * `n`, and the event ran the dialog's handlers and then kept bubbling out to + * `BoardView`'s `useKeyboardShortcuts` window listener -- the filter panel + * toggled and the add-card composer pulled focus out of the dialog. + * + * This listener sits on `document` in the bubble phase. The invariant that + * makes that safe is narrower than "the surfaces handle their own keys first", + * so state it exactly: + * + * A surface keeps a key only if it handles that key AT OR BELOW `document` + * -- an element-level handler, anywhere from the event target up to and + * including `document` -- or if the key is Escape, which is carved out below. + * Anything a surface binds on `window` is one hop OUTSIDE this guard and, + * unless it is Escape, will be silenced while a surface is active. + * + * Every surface in the tree satisfies that today by binding element-level + * handlers: the palettes' `@keydown.down/up/enter`, `CaptureModal`'s + * `@keydown`, the review dialogs' listeners on their own dialog elements. The + * one surface that does NOT is `PaperShortcutsOverlay`, whose Escape handler is + * a `window` bubble listener; it survives purely on the Escape carve-out, not + * on the premise above. So a new `window`-level NON-Escape handler belonging to + * a modal is a forbidden shape here -- it would be silenced -- and there is a + * spec pinning that from the Paper help overlay. + * + * Note the reach: this is not scoped to the four shell surfaces. It fires for + * every `dialog[open]`, `[role="alertdialog"]` or `[aria-modal="true"]` in the + * app -- `CardModal`, `TdDialog` and the review dialogs built on it, + * `ProvenanceDrawer`, `PaperBoardDialogShell`, the board modals, + * `WorkspaceSetupModal`, `MfaChallengeModal` -- which is the point, since the + * page-level listeners it has to silence all bind on `window`. + * + * Two carve-outs, for the same reasons the capture half has them: + * - Escape is never stopped. `useEscapeStack` listens in the capture phase so + * it is already past, but `BoardView.closeOpenUi` and + * `PaperShortcutsOverlay` both take Escape on the window bubble, and + * stopping it here would strand their surfaces open. + * - Text-entry targets are left alone. Typing is not a page shortcut: + * `useKeyboardShortcuts` ignores text entry outright, and honouring the + * early-out keeps the #1968 promise that an ordinary keystroke in a field + * never pays for the surface scan. + */ +function guardPageListenersFromSurfaceKeys(event: KeyboardEvent) { + if (event.key === 'Escape') return + if (event.isComposing) return + if (isTextEntryTarget(event.target)) return + if (keyboardOwningSurfacesFor(event).length === 0) return + + event.stopPropagation() +} + function handleKeydown(event: KeyboardEvent) { if (event.isComposing) { clearPendingChord() @@ -288,9 +373,9 @@ function handleKeydown(event: KeyboardEvent) { // Scanned at most once per event, and only once something actually needs the // answer, so an ordinary keystroke typed into a field never pays for the - // `querySelectorAll` plus `getComputedStyle` sweep (#1968). - let surfaces: HTMLElement[] | null = null - const keyboardOwningSurfaces = () => (surfaces ??= activeKeyboardOwningSurfaces()) + // `querySelectorAll` plus `getComputedStyle` sweep (#1968). The memo is shared + // with the bubble-phase guard so the pair still scans only once (#2636). + const keyboardOwningSurfaces = () => keyboardOwningSurfacesFor(event) if (pendingChord) { const chord = pendingChord @@ -366,6 +451,9 @@ function handleLogout() { onMounted(() => { window.addEventListener('keydown', handleKeydown, true) + // Bubble phase on `document`: after the surface's own handlers, before the + // page-level `window` listeners (#2636). + document.addEventListener('keydown', guardPageListenersFromSurfaceKeys) }) function hydratePreferencesIfNeeded() { @@ -403,6 +491,12 @@ watch( onUnmounted(() => { clearPendingChord() window.removeEventListener('keydown', handleKeydown, true) + document.removeEventListener('keydown', guardPageListenersFromSurfaceKeys) + // Belt-and-braces. The memo lives in this instance's `setup()` closure, not at + // module scope, so it is already unreachable once the instance is gone; this + // just drops the last event and surface references at a known point. + scannedEvent = null + scannedSurfaces = [] }) diff --git a/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts b/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts index e1082a6e1..7b7bc39ff 100644 --- a/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts +++ b/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts @@ -203,6 +203,16 @@ vi.mock('../../composables/useCaptureQueueSync', () => ({ useCaptureQueueSync: () => ({ pendingCount: { value: 0 }, syncing: { value: false }, replayQueue: vi.fn(), registerBackgroundSync: vi.fn(), refreshCount: vi.fn() }), })) +// The Paper sidebar reads the product version on mount, so the one Paper-skin +// spec below would otherwise make a real request and log an ECONNREFUSED per +// run. Stubbed the same way `AppShell.paperVariant.spec.ts` stubs it; this +// suite is about keyboard routing, not the version transport. +vi.mock('../../api/versionApi', () => ({ + versionApi: { + getProductVersion: vi.fn(async () => null), + }, +})) + /** * `attachTo` is load-bearing for anything that touches the modal-ownership * guard. Vue Test Utils only puts the component into the real document when it @@ -657,6 +667,234 @@ describe('AppShell workspace navigation and command palette', () => { expect(helpDialog()).toBeNull() }) + /** + * The residual PR #2635 recorded rather than fixed (#2636). + * + * The capture-phase guard stands aside when the keydown target is inside the + * surface: it runs ahead of every handler the surface owns, so stopping there + * would break typing and arrow navigation inside modals. That carve-out is + * the leak. Once the surface's own handlers have run, the same event keeps + * bubbling out of the surface and reaches page-level `window` listeners. + * + * `bubbles: true` from a node INSIDE the surface is load-bearing. Dispatching + * on `window` would put the event AT_TARGET, where capture and bubble + * listeners both run whatever propagation says, and every assertion below + * would pass against the unguarded source. + */ + function pressFrom(node: Element, key: string, init: KeyboardEventInit = {}) { + node.dispatchEvent(new KeyboardEvent('keydown', { key, bubbles: true, ...init })) + } + + /** + * Stands in for any page-level `window` keydown listener behind the surface -- + * the board keymap, `PaperHomeView`, `PaperInboxView`. All three bind on the + * bubble, which is the phase this probe watches. + */ + function trackPageLevelKeys() { + const keys: string[] = [] + const listener = (event: KeyboardEvent) => { + keys.push(event.key) + } + window.addEventListener('keydown', listener) + return { + keys, + stop: () => window.removeEventListener('keydown', listener), + } + } + + it('keeps board keys off the board when focus is inside the help dialog', async () => { + mountedWrapper = mountShell(document.body, { RouterView: BoardKeyProbe }) + await waitForUi() + + pressFromBody('?') + await waitForUi() + const dialog = helpDialog() + expect(dialog).not.toBeNull() + + // Both real paths into the dialog leave focus outside it and neither twin + // traps focus, so reaching this state takes a deliberate Tab or click. + const dialogClose = dialog!.querySelector('button') as HTMLButtonElement + dialogClose.focus() + expect(dialog!.contains(document.activeElement)).toBe(true) + + pressFrom(dialogClose, 'f') + pressFrom(dialogClose, 'n') + await waitForUi() + + expect(boardProbe.filterToggles).toBe(0) + expect(boardProbe.addCardClicks).toBe(0) + expect(dialog!.contains(document.activeElement)).toBe(true) + }) + + it('keeps the bare-letter navigation set and the g-chord off page listeners from inside a surface', async () => { + mountedWrapper = mountShell(document.body) + await waitForUi() + + pressFromBody('?') + await waitForUi() + const dialogClose = helpDialog()!.querySelector('button') as HTMLButtonElement + dialogClose.focus() + + const page = trackPageLevelKeys() + for (const key of ['h', 't', 'b', 'i', 'r', 'g']) { + pressFrom(dialogClose, key) + } + await waitForUi() + page.stop() + + expect(page.keys).toEqual([]) + expect(mockRouter.push).not.toHaveBeenCalled() + }) + + it('keeps board keys off the board with focus inside a capture modal over the board', async () => { + mountedWrapper = mountShell(document.body, { RouterView: BoardKeyProbe }) + const wrapper = mountedWrapper + await waitForUi() + + pressFromBody('C', { ctrlKey: true, shiftKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Capture modal"]').exists()).toBe(true) + + // Tab off the textarea onto a button and the target stops being text entry, + // which is what puts the board keymap back in range. + const captureButton = document.querySelector('.capture-close') as HTMLButtonElement + captureButton.focus() + + pressFrom(captureButton, 'n') + pressFrom(captureButton, 'f') + await waitForUi() + + expect(boardProbe.filterToggles).toBe(0) + expect(boardProbe.addCardClicks).toBe(0) + }) + + it('keeps board keys off the board from a focused option inside the command palette', async () => { + mountedWrapper = mountShell(document.body, { RouterView: BoardKeyProbe }) + await waitForUi() + + pressFromBody('k', { ctrlKey: true }) + await waitForUi() + const option = document.querySelector('[data-palette-index="0"]') as HTMLElement + expect(option).not.toBeNull() + option.focus() + + pressFrom(option, 'f') + pressFrom(option, 'n') + await waitForUi() + + expect(boardProbe.filterToggles).toBe(0) + expect(boardProbe.addCardClicks).toBe(0) + }) + + it('still lets Escape out of a surface to the page close paths', async () => { + // The capture modal is not on the escape stack, so its Escape is exactly the + // `BoardView.closeOpenUi` class of path: a dialog the stack does not carry + // still has to close, which means Escape must keep bubbling to the page. + mountedWrapper = mountShell(document.body, { RouterView: BoardKeyProbe }) + await waitForUi() + + pressFromBody('C', { ctrlKey: true, shiftKey: true }) + await waitForUi() + const captureButton = document.querySelector('.capture-close') as HTMLButtonElement + captureButton.focus() + + const page = trackPageLevelKeys() + pressFrom(captureButton, 'Escape') + await waitForUi() + page.stop() + + expect(page.keys).toEqual(['Escape']) + }) + + it('leaves each surface its own keys with focus inside it', async () => { + mountedWrapper = mountShell(document.body) + const wrapper = mountedWrapper + await waitForUi() + + // `?` still toggles the help dialog shut from a control inside it. + pressFromBody('?') + await waitForUi() + const dialogClose = helpDialog()!.querySelector('button') as HTMLButtonElement + dialogClose.focus() + pressFrom(dialogClose, '?') + await waitForUi() + expect(helpDialog()).toBeNull() + + pressFromBody('k', { ctrlKey: true }) + await waitForUi() + const input = document.querySelector('[aria-label="Command palette search"]') as HTMLInputElement + expect(input).not.toBeNull() + expect(document.querySelectorAll('[data-palette-index]').length).toBeGreaterThan(1) + + // Arrow navigation inside the palette input still moves the selection. + pressFrom(input, 'ArrowDown') + await waitForUi() + expect(document.querySelector('[aria-selected="true"]')?.getAttribute('data-palette-index')).toBe('1') + + // Re-query rather than reuse `input`. Measured under this mount (jsdom, + // Teleport stubbed): after the ArrowDown the node captured above reports + // `isConnected === false` and no longer matches the selector, so the element + // the test is holding is detached. Why Vue drops it here is not pinned down; + // what matters is the consequence. A key dispatched on a detached node never + // enters the tree, so no window listener runs and no surface state changes: + // the palette would simply stay open and the `exists()` assertion below + // would go red. So the re-query cannot mask a defect -- it removes the one + // way this assertion could pass without the guard being exercised at all -- + // and the `isConnected` check makes that precondition explicit. + const reboundInput = document.querySelector('[aria-label="Command palette search"]') as HTMLInputElement + expect(reboundInput.isConnected).toBe(true) + + // And mod+k still closes the palette from its own input. + pressFrom(reboundInput, 'k', { ctrlKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(false) + }) + + /** + * The Paper help twin, which is the one surface in the tree that does NOT + * satisfy the guard's stated invariant on its own. + * + * `PaperShortcutsOverlay` binds its Escape handler on `window` in the bubble + * phase -- one hop OUTSIDE the document-level guard -- so it survives only + * because Escape is carved out, not because its handler runs first. This pins + * both halves of that on the skin where it actually renders: a bare board key + * is still stopped, and Escape still gets through to close the overlay. A + * `window`-level NON-Escape handler on a modal is the shape this forbids, and + * this test is what would go red if one were added. + */ + it('stops board keys but not Escape from inside the Paper help overlay', async () => { + mockPaperTheme.isOn = true + mountedWrapper = mountShell(document.body, { RouterView: BoardKeyProbe }) + await waitForUi() + + pressFromBody('?') + await waitForUi() + const overlay = document.querySelector('[data-shell-surface="keyboard-help"]') + expect(overlay).not.toBeNull() + expect(overlay!.getAttribute('aria-modal')).toBe('true') + + const overlayClose = overlay!.querySelector('[aria-label="Close keyboard shortcuts"]') as HTMLButtonElement + expect(overlayClose).not.toBeNull() + overlayClose.focus() + + const page = trackPageLevelKeys() + pressFrom(overlayClose, 'f') + pressFrom(overlayClose, 'n') + await waitForUi() + + expect(page.keys).toEqual([]) + expect(boardProbe.filterToggles).toBe(0) + expect(boardProbe.addCardClicks).toBe(0) + + // Escape is the carve-out, so it must still leave the surface -- and it + // still closes the overlay. + pressFrom(overlayClose, 'Escape') + await waitForUi() + page.stop() + + expect(document.querySelector('[data-shell-surface="keyboard-help"]')).toBeNull() + }) + it('does not open the command palette over a modal it does not own', async () => { mountedWrapper = mountShell(document.body) const wrapper = mountedWrapper