fix(shell): stop keys pressed inside a shell surface from reaching page-level listeners

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 = []
 })
 </script>
 

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<HTMLElement>('[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