diff --git a/frontend/taskdeck-web/src/components/paper/PaperCommandPalette.vue b/frontend/taskdeck-web/src/components/paper/PaperCommandPalette.vue index 75e5c983a..e5beb4cf5 100644 --- a/frontend/taskdeck-web/src/components/paper/PaperCommandPalette.vue +++ b/frontend/taskdeck-web/src/components/paper/PaperCommandPalette.vue @@ -231,6 +231,7 @@ watch(orderedItems, (items) => { role="dialog" aria-modal="true" aria-label="Command palette" + data-shell-surface="command-palette" @click.self="handleClose" @keydown.escape.prevent="handleClose" > diff --git a/frontend/taskdeck-web/src/components/paper/PaperShortcutsOverlay.vue b/frontend/taskdeck-web/src/components/paper/PaperShortcutsOverlay.vue index a35a25458..222935ab4 100644 --- a/frontend/taskdeck-web/src/components/paper/PaperShortcutsOverlay.vue +++ b/frontend/taskdeck-web/src/components/paper/PaperShortcutsOverlay.vue @@ -56,6 +56,7 @@ function onBackdropClick() { role="dialog" aria-modal="true" aria-labelledby="paper-shortcuts-title" + data-shell-surface="keyboard-help" @click.self="onBackdropClick" >
diff --git a/frontend/taskdeck-web/src/components/shell/AppShell.vue b/frontend/taskdeck-web/src/components/shell/AppShell.vue index ea64b183c..bdd04d8cb 100644 --- a/frontend/taskdeck-web/src/components/shell/AppShell.vue +++ b/frontend/taskdeck-web/src/components/shell/AppShell.vue @@ -11,8 +11,9 @@ import { useViewportMode } from '../../composables/useViewportMode' import { provideShellKeyboardHelp } from '../../composables/useShellKeyboardHelp' import { APP_SHELL_SHORTCUT_BINDINGS, + strokeMatches, + type AppShellShortcutAction, type AppShellShortcutBinding, - type ShortcutStroke, } from '../../utils/keyboardShortcuts' import CaptureModal from '../common/CaptureModal.vue' import OfflineBanner from './OfflineBanner.vue' @@ -148,18 +149,31 @@ function isTextEntryTarget(target: EventTarget | null): boolean { return target.matches(selector) || target.closest(selector) !== null } +/** + * Only a surface that declares itself MODAL owns the keyboard (#1968). + * + * A bare `[role="dialog"]` is not enough, and matching it was a live defect: + * `CardModal` keeps `role="dialog"` in both presentations but sets + * `aria-modal` only outside the inspector, so the Paper desktop card inspector + * -- a sticky side panel that traps nothing and leaves the board usable -- + * counted as a keyboard-owning surface. That made `?`, `mod+k` and + * `mod+shift+c` dead for as long as a card was open for reading, and stopped + * every non-Escape key pressed outside the panel. + * + * `dialog[open]` and `[role="alertdialog"]` stay: a native open `` is + * modal when shown as one and an alertdialog is modal by definition. + */ const KEYBOARD_OWNING_SURFACE_SELECTOR = [ 'dialog[open]', - '[role="dialog"]', '[role="alertdialog"]', '[aria-modal="true"]', ].join(', ') -function hasActiveKeyboardOwningSurface(): boolean { - if (typeof document === 'undefined') return false +function activeKeyboardOwningSurfaces(): HTMLElement[] { + if (typeof document === 'undefined') return [] return Array.from(document.querySelectorAll(KEYBOARD_OWNING_SURFACE_SELECTOR)) - .some((surface) => { + .filter((surface) => { if (!surface.isConnected) return false if (surface.closest('[hidden], [aria-hidden="true"], [inert]')) return false @@ -168,6 +182,31 @@ function hasActiveKeyboardOwningSurface(): boolean { }) } +/** + * 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 + * rather than one-way openers: the help dialog owns `?`, the command palette + * owns `mod+k`, and neither opens over the other or over anything else (#1968). + * + * Deliberately not "the topmost surface owns it". Stack order is not readable + * here: both help twins and both palettes teleport to `body`, and a `` + * places its anchor when the SHELL mounts, not when the surface opens, so + * document order is AppShell's template order whatever the user opened first. + * Asking every surface instead would deadlock a stack -- open the help dialog, + * then the topbar Search control, and neither key could close its own surface + * again. + * + * `navigate` and `quick-capture` name no surface, so an active surface always + * wins over them: nothing behind a modal should move the route, and quick + * capture would stack a second modal on the first (the #1959 class). + */ +function shellSurfaceOwnsAction(surfaces: readonly HTMLElement[], action: AppShellShortcutAction): boolean { + if (surfaces.length === 0) return true + + return surfaces.some((surface) => surface.dataset.shellSurface === action.type) && + surfaces.every((surface) => surface.dataset.shellSurface !== undefined) +} + const CHORD_TIMEOUT_MS = 1_000 let pendingChord: AppShellShortcutBinding | null = null let chordTimer: ReturnType | null = null @@ -180,15 +219,6 @@ function clearPendingChord() { } } -function strokeMatches(event: KeyboardEvent, stroke: ShortcutStroke): boolean { - const modPressed = event.ctrlKey || event.metaKey - const shiftMatches = stroke.shift === undefined || stroke.shift === event.shiftKey - return event.key.toLowerCase() === stroke.key.toLowerCase() && - modPressed === Boolean(stroke.mod) && - event.altKey === Boolean(stroke.alt) && - shiftMatches -} - function consumeShortcut(event: KeyboardEvent) { event.preventDefault() // AppShell owns the workspace-level keys. Capture-phase handling means a @@ -217,6 +247,35 @@ function runAppShellShortcut(binding: AppShellShortcutBinding) { } } +/** + * Keep a key an active surface does not own from reaching the page behind it + * (#2621). + * + * With the help dialog open over a Legacy board, a plain `f` or `n` used to + * bubble past this listener to `BoardView`'s `useKeyboardShortcuts` window + * listener: `f` toggled the filter panel behind the modal and `n` clicked the + * column's add-card button and pulled focus out of the dialog. + * + * Two carve-outs keep this from taking more than it should: + * - Escape is never stopped. Board dialogs the escape stack does not carry + * (label manager, board settings, filter panel, column form) are closed by + * `BoardView.closeOpenUi` on the bubble, and stopping Escape here would + * strand them open. + * - A target inside the surface is left alone, because this listener runs in + * the capture phase, ahead of every handler the surface owns. Stopping + * there would break typing and arrow navigation inside modals. + * Text-entry targets never reach this, and never triggered board shortcuts in + * the first place -- `useKeyboardShortcuts` ignores them. + */ +function guardSurfaceFromPageShortcuts(event: KeyboardEvent, surfaces: readonly HTMLElement[]) { + if (event.key === 'Escape') return + + const target = event.target instanceof Node ? event.target : null + if (target && surfaces.some((surface) => surface.contains(target))) return + + event.stopPropagation() +} + function handleKeydown(event: KeyboardEvent) { if (event.isComposing) { clearPendingChord() @@ -224,15 +283,23 @@ function handleKeydown(event: KeyboardEvent) { } const textEntryTarget = isTextEntryTarget(event.target) - const keyboardOwningSurfaceActive = hasActiveKeyboardOwningSurface() - if (keyboardOwningSurfaceActive) clearPendingChord() + // 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()) if (pendingChord) { const chord = pendingChord clearPendingChord() const nextStroke = chord.sequence[1] - if (!textEntryTarget && nextStroke && strokeMatches(event, nextStroke)) { + if ( + !textEntryTarget && + nextStroke && + strokeMatches(event, nextStroke) && + keyboardOwningSurfaces().length === 0 + ) { consumeShortcut(event) runAppShellShortcut(chord) return @@ -242,8 +309,8 @@ function handleKeydown(event: KeyboardEvent) { const direct = APP_SHELL_SHORTCUT_BINDINGS.find((binding) => binding.sequence.length === 1 && strokeMatches(event, binding.sequence[0]!) && - (binding.action.type !== 'navigate' || !keyboardOwningSurfaceActive) && - (!textEntryTarget || binding.allowInTextEntry === true), + (!textEntryTarget || binding.allowInTextEntry === true) && + shellSurfaceOwnsAction(keyboardOwningSurfaces(), binding.action), ) if (direct) { consumeShortcut(event) @@ -251,7 +318,12 @@ function handleKeydown(event: KeyboardEvent) { return } - if (textEntryTarget || keyboardOwningSurfaceActive) return + if (textEntryTarget) return + + if (keyboardOwningSurfaces().length > 0) { + guardSurfaceFromPageShortcuts(event, keyboardOwningSurfaces()) + return + } const chord = APP_SHELL_SHORTCUT_BINDINGS.find((binding) => binding.sequence.length > 1 && strokeMatches(event, binding.sequence[0]!), diff --git a/frontend/taskdeck-web/src/components/shell/ShellCommandPalette.vue b/frontend/taskdeck-web/src/components/shell/ShellCommandPalette.vue index bed68f612..7218a2074 100644 --- a/frontend/taskdeck-web/src/components/shell/ShellCommandPalette.vue +++ b/frontend/taskdeck-web/src/components/shell/ShellCommandPalette.vue @@ -208,6 +208,7 @@ watch(allPaletteItems, (items) => { role="dialog" aria-label="Command palette" aria-modal="true" + data-shell-surface="command-palette" @click.self="handleClose" @keydown.escape="handleClose" > diff --git a/frontend/taskdeck-web/src/components/shell/ShellKeyboardHelp.vue b/frontend/taskdeck-web/src/components/shell/ShellKeyboardHelp.vue index 9b9b4e563..a4b7c9f4d 100644 --- a/frontend/taskdeck-web/src/components/shell/ShellKeyboardHelp.vue +++ b/frontend/taskdeck-web/src/components/shell/ShellKeyboardHelp.vue @@ -40,6 +40,7 @@ const emit = defineEmits<{ role="dialog" aria-label="Keyboard shortcuts" aria-modal="true" + data-shell-surface="keyboard-help" @click.self="emit('close')" @keydown.escape="emit('close')" > diff --git a/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts b/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts index 7e722bb71..ceac98dd0 100644 --- a/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts +++ b/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts @@ -6,6 +6,8 @@ import { useShellKeyboardHelp, type ShellKeyboardHelpControl, } from '../../composables/useShellKeyboardHelp' +import { useKeyboardShortcuts } from '../../composables/useKeyboardShortcuts' +import { APP_SHELL_SHORTCUT_BINDINGS } from '../../utils/keyboardShortcuts' import type { FeatureFlags } from '../../types/feature-flags' /** @@ -22,6 +24,92 @@ const ShellHelpProbe = defineComponent({ }, }) +const boardProbe = reactive({ filterToggles: 0, addCardClicks: 0 }) + +/** + * The routed board's keyboard seam, at composable level (#2621). + * + * It installs the REAL `useKeyboardShortcuts` with BoardView's own `f` and `n` + * bindings and the same `[data-action="toggle-add-card"]` / + * `[data-action="add-card-input"]` DOM contract `createCardInSelectedColumn` + * drives, so the routing under test — AppShell's capture-phase window listener + * versus the board's bubble-phase one — is the shipped mechanism, not a mock. + * + * Fidelity limit: BoardView passes `enabled` predicates these bindings omit. + * Both predicates are true in the state this file exercises (a routed Legacy + * board with no Paper dialog open), which is exactly why the shell guard is + * the only thing standing between the modal and the board. + */ +function installBoardShortcuts() { + useKeyboardShortcuts([ + { + key: 'f', + description: 'Toggle filter panel', + action: () => { + boardProbe.filterToggles += 1 + }, + }, + { + key: 'n', + description: 'New card in current column', + action: () => { + const column = document.querySelector('[data-column-id="column-1"]') + column?.querySelector('[data-action="toggle-add-card"]')?.click() + column?.querySelector('[data-action="add-card-input"]')?.focus() + }, + }, + ]) +} + +function boardColumnNode() { + return h('div', { 'data-column-id': 'column-1' }, [ + h( + 'button', + { + 'data-action': 'toggle-add-card', + onClick: () => { + boardProbe.addCardClicks += 1 + }, + }, + 'Add card', + ), + h('textarea', { 'data-action': 'add-card-input' }), + ]) +} + +const BoardKeyProbe = defineComponent({ + setup() { + installBoardShortcuts() + return () => boardColumnNode() + }, +}) + +/** + * The Paper desktop card inspector over its board (#2635 round 2). + * + * `CardModal.vue` keeps `role="dialog"` in both presentations and sets + * `aria-modal` only outside the inspector, so at desktop widths the open card + * is a sticky side panel that traps nothing: the board behind it stays usable + * and the shell keys have to keep working. + */ +const InspectorBoardProbe = defineComponent({ + setup() { + installBoardShortcuts() + return () => h('div', [ + h( + 'div', + { + role: 'dialog', + 'aria-label': 'Edit Card', + 'data-testid': 'card-inspector', + }, + [h('button', { 'data-testid': 'inspector-close' }, 'Close')], + ), + boardColumnNode(), + ]) + }, +}) + const mockRouter = { push: vi.fn(), } @@ -107,6 +195,17 @@ vi.mock('../../composables/useCaptureQueueSync', () => ({ useCaptureQueueSync: () => ({ pendingCount: { value: 0 }, syncing: { value: false }, replayQueue: vi.fn(), registerBackgroundSync: vi.fn(), refreshCount: vi.fn() }), })) +/** + * `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 + * is given, and the guard answers `document.querySelectorAll`, so an unattached + * mount reports no active surface and defangs the gate entirely. Every spec + * below that opens a surface and then presses a key passes `document.body`. + * + * Teleport stays stubbed throughout. The guard does not read stack order -- a + * `` places its anchor when the shell mounts, not when the surface + * opens, so document order is AppShell's template order either way. + */ function mountShell(attachTo?: HTMLElement, extraStubs: Record = {}) { return mount(AppShell, { attachTo, @@ -159,6 +258,8 @@ describe('AppShell workspace navigation and command palette', () => { mockPaperTheme.isOn = false mockSession.isAuthenticated = true injectedShellHelp = null + boardProbe.filterToggles = 0 + boardProbe.addCardClicks = 0 }) afterEach(() => { @@ -452,6 +553,273 @@ describe('AppShell workspace navigation and command palette', () => { expect(mockRouter.push).not.toHaveBeenCalled() }) + /** + * The real routing path. A key pressed with focus outside a modal targets + * `document.body`, so the event runs AppShell's capture-phase window listener + * first and only then bubbles back to the board's window listener. Dispatching + * on `window` instead would put the event AT_TARGET, where capture and bubble + * listeners both run whatever propagation says, and prove nothing about the + * order the browser actually uses. + */ + function pressFromBody(key: string, init: KeyboardEventInit = {}) { + document.body.dispatchEvent(new KeyboardEvent('keydown', { key, bubbles: true, ...init })) + } + + function helpDialog(): HTMLElement | null { + return document.querySelector('[aria-label="Keyboard shortcuts"]') + } + + it('keeps board bare-letter keys away from the board while the help dialog is open', async () => { + mountedWrapper = mountShell(document.body, { RouterView: BoardKeyProbe }) + await waitForUi() + + pressFromBody('?') + await waitForUi() + const dialog = helpDialog() + expect(dialog).not.toBeNull() + + const dialogClose = dialog!.querySelector('button') as HTMLButtonElement + dialogClose.focus() + expect(dialog!.contains(document.activeElement)).toBe(true) + + pressFromBody('f') + pressFromBody('n') + await waitForUi() + + // `f` toggled the filter panel behind the modal and `n` clicked the column's + // add-card button and pulled focus into the composer, both from behind an + // open `aria-modal` dialog (#2621). + expect(boardProbe.filterToggles).toBe(0) + expect(boardProbe.addCardClicks).toBe(0) + expect(dialog!.contains(document.activeElement)).toBe(true) + }) + + it('keeps every shell key live while a non-modal card inspector is open', async () => { + mountedWrapper = mountShell(document.body, { RouterView: InspectorBoardProbe }) + const wrapper = mountedWrapper + await waitForUi() + expect(document.querySelector('[data-testid="card-inspector"]')).not.toBeNull() + + pressFromBody('?') + await waitForUi() + expect(helpDialog()).not.toBeNull() + pressFromBody('?') + await waitForUi() + expect(helpDialog()).toBeNull() + + pressFromBody('k', { ctrlKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(true) + pressFromBody('k', { ctrlKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(false) + + pressFromBody('C', { ctrlKey: true, shiftKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Capture modal"]').exists()).toBe(true) + await wrapper.get('.capture-close').trigger('click') + await waitForUi() + + // And the board behind the panel is still the board: a side panel that + // traps nothing does not take the board's keys away either. + pressFromBody('f') + await waitForUi() + expect(boardProbe.filterToggles).toBe(1) + }) + + it('keeps ? and Escape working on the help dialog with the board listener mounted', async () => { + mountedWrapper = mountShell(document.body, { RouterView: BoardKeyProbe }) + await waitForUi() + + pressFromBody('?') + await waitForUi() + expect(helpDialog()).not.toBeNull() + + // The surface that owns `?` still gets it, so the key stays a toggle. + pressFromBody('?') + await waitForUi() + expect(helpDialog()).toBeNull() + + pressFromBody('?') + await waitForUi() + expect(helpDialog()).not.toBeNull() + + pressFromBody('Escape') + await waitForUi() + expect(helpDialog()).toBeNull() + }) + + it('does not open the command palette over a modal it does not own', async () => { + mountedWrapper = mountShell(document.body) + const wrapper = mountedWrapper + + pressFromBody('C', { ctrlKey: true, shiftKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Capture modal"]').exists()).toBe(true) + + pressFromBody('k', { ctrlKey: true }) + await waitForUi() + + expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(false) + }) + + it('still toggles the command palette closed with the key that opened it', async () => { + mountedWrapper = mountShell(document.body) + const wrapper = mountedWrapper + + pressFromBody('k', { ctrlKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(true) + + pressFromBody('k', { ctrlKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(false) + }) + + it('does not open quick capture over an active modal', async () => { + mountedWrapper = mountShell(document.body) + const wrapper = mountedWrapper + + pressFromBody('?') + await waitForUi() + expect(helpDialog()).not.toBeNull() + + pressFromBody('C', { ctrlKey: true, shiftKey: true }) + await waitForUi() + + expect(wrapper.find('[aria-label="Capture modal"]').exists()).toBe(false) + }) + + it('does not toggle the help dialog over a modal that owns the keyboard', async () => { + mountedWrapper = mountShell(document.body) + const wrapper = mountedWrapper + + pressFromBody('C', { ctrlKey: true, shiftKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Capture modal"]').exists()).toBe(true) + + pressFromBody('?') + await waitForUi() + + expect(helpDialog()).toBeNull() + }) + + it('does not scan the DOM for modal surfaces on an ordinary keystroke in a field', async () => { + mountedWrapper = mountShell(document.body) + const field = document.createElement('input') + document.body.appendChild(field) + + const querySelectorAll = vi.spyOn(document, 'querySelectorAll') + const surfaceScans = () => querySelectorAll.mock.calls + .filter(([selector]) => String(selector).includes('aria-modal')) + + field.dispatchEvent(new KeyboardEvent('keydown', { key: 'a', bubbles: true })) + field.dispatchEvent(new KeyboardEvent('keydown', { key: 'h', bubbles: true })) + await waitForUi() + + // Typing pays for `isTextEntryTarget` alone; the `querySelectorAll` plus + // `getComputedStyle` sweep is only reached once an answer is needed (#1968). + expect(surfaceScans()).toHaveLength(0) + + // mod+k is allowed inside text entry, so there the surface state decides + // whether it may open and the scan is the point. + field.dispatchEvent(new KeyboardEvent('keydown', { key: 'k', ctrlKey: true, bubbles: true })) + await waitForUi() + expect(surfaceScans().length).toBeGreaterThan(0) + + querySelectorAll.mockRestore() + field.remove() + }) + + it('lets each surface of a shell stack close itself with its own key', async () => { + // Both shell surfaces open at once, which only a mouse can reach now. Each + // key still closes the surface that owns it; requiring every active surface + // to be this action's surface would deadlock the stack instead. + mountedWrapper = mountShell(document.body) + const wrapper = mountedWrapper + + await wrapper.get('[aria-label="Keyboard shortcuts help"]').trigger('click') + await wrapper.get('[aria-label^="Open command palette"]').trigger('click') + await waitForUi() + expect(helpDialog()).not.toBeNull() + expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(true) + + pressFromBody('k', { ctrlKey: true }) + await waitForUi() + expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(false) + expect(helpDialog()).not.toBeNull() + + pressFromBody('?') + await waitForUi() + expect(helpDialog()).toBeNull() + }) + + it.each(APP_SHELL_SHORTCUT_BINDINGS.map((binding) => [binding.id, binding] as const))( + 'runs the ledger action the %s row advertises', + async (_id, binding) => { + mountedWrapper = mountShell(document.body) + const wrapper = mountedWrapper + + for (const stroke of binding.sequence) { + pressFromBody(stroke.key, { + ctrlKey: stroke.mod === true, + shiftKey: stroke.shift === true, + altKey: stroke.alt === true, + }) + } + await waitForUi() + + switch (binding.action.type) { + case 'navigate': + expect(mockRouter.push).toHaveBeenCalledWith(binding.action.path) + break + case 'command-palette': + expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(true) + break + case 'quick-capture': + expect(wrapper.find('[aria-label="Capture modal"]').exists()).toBe(true) + break + case 'keyboard-help': + expect(helpDialog()).not.toBeNull() + break + } + }, + ) + + it('does not navigate when Shift is held with a bare-letter binding', async () => { + mountedWrapper = mountShell() + + // `strokeMatches` compares keys case-insensitively, so `Shift+H` arrives as + // `H` and used to navigate Home (#1968). + window.dispatchEvent(new KeyboardEvent('keydown', { key: 'H', shiftKey: true })) + window.dispatchEvent(new KeyboardEvent('keydown', { key: 'T', shiftKey: true })) + await waitForUi() + + expect(mockRouter.push).not.toHaveBeenCalled() + }) + + it('toggles the keyboard map when ? needs AltGr and leaves Alt letter combos alone', async () => { + // Attached: the second press has to close the help dialog THROUGH the + // surface gate, which only sees surfaces that are really in the document. + mountedWrapper = mountShell(document.body) + const wrapper = mountedWrapper + + // AltGr reports `altKey: true`, and on Windows `ctrlKey: true` as well, so + // the strict modifier comparison made `?` unreachable on those layouts. + window.dispatchEvent(new KeyboardEvent('keydown', { key: '?', altKey: true })) + await waitForUi() + expect(wrapper.find('[aria-label="Keyboard shortcuts"]').exists()).toBe(true) + + window.dispatchEvent(new KeyboardEvent('keydown', { key: '?', altKey: true, ctrlKey: true })) + await waitForUi() + expect(wrapper.find('[aria-label="Keyboard shortcuts"]').exists()).toBe(false) + + // Alt over a letter binding is still a different stroke, not Home. + window.dispatchEvent(new KeyboardEvent('keydown', { key: 'h', altKey: true })) + await waitForUi() + expect(mockRouter.push).not.toHaveBeenCalled() + }) + it.each(['input', 'textarea', 'select', 'contenteditable'])( 'suppresses bare and chord navigation inside %s targets', async (kind) => { @@ -520,14 +888,19 @@ describe('AppShell workspace navigation and command palette', () => { }) it('closes only the top-most escape surface first', async () => { - mountedWrapper = mountShell() + // Attached, because the surface gate answers `document.querySelectorAll`, + // and stacked by MOUSE: the gate deliberately refuses to open the palette + // over a modal it does not own, so a keyboard-built stack is no longer a + // reachable state. Clicking a background control still is, and the escape + // stack's ordering is what this asserts. + mountedWrapper = mountShell(document.body) const wrapper = mountedWrapper - window.dispatchEvent(new KeyboardEvent('keydown', { key: '?' })) + await wrapper.get('[aria-label="Keyboard shortcuts help"]').trigger('click') await waitForUi() expect(wrapper.find('[aria-label="Keyboard shortcuts"]').exists()).toBe(true) - window.dispatchEvent(new KeyboardEvent('keydown', { key: 'k', ctrlKey: true })) + await wrapper.get('[aria-label^="Open command palette"]').trigger('click') await waitForUi() expect(wrapper.find('[aria-label="Command palette"]').exists()).toBe(true) @@ -542,7 +915,9 @@ describe('AppShell workspace navigation and command palette', () => { }) it('opens the same help surface from the routed-view seam that ? opens', async () => { - mountedWrapper = mountShell(undefined, { RouterView: ShellHelpProbe }) + // Attached: every `?` after the first has to pass the surface gate, which + // only sees surfaces that are really in the document. + mountedWrapper = mountShell(document.body, { RouterView: ShellHelpProbe }) const wrapper = mountedWrapper await waitForUi() diff --git a/frontend/taskdeck-web/src/tests/components/paper/PaperShortcutsOverlay.spec.ts b/frontend/taskdeck-web/src/tests/components/paper/PaperShortcutsOverlay.spec.ts index b40594640..08d330e0a 100644 --- a/frontend/taskdeck-web/src/tests/components/paper/PaperShortcutsOverlay.spec.ts +++ b/frontend/taskdeck-web/src/tests/components/paper/PaperShortcutsOverlay.spec.ts @@ -8,8 +8,10 @@ import boardViewSource from '../../../views/BoardView.vue?raw' import { APP_SHELL_SHORTCUT_BINDINGS, formatShortcut, + KEYBOARD_HELP_SHORTCUT, PAPER_SHORTCUT_GROUPS, SHORTCUT_HANDLER_CONTRACTS, + strokeMatches, type ShortcutHandlerOwner, } from '../../../utils/keyboardShortcuts' @@ -100,7 +102,49 @@ describe('PaperShortcutsOverlay', () => { expect(expectedRows.filter((row) => row.handlerOwner === 'app-shell').every( (row) => appShellIds.has(row.id), )).toBe(true) - expect(appShellSource).toContain('APP_SHELL_SHORTCUT_BINDINGS.find') + }) + + /** + * Replaces a `toContain('APP_SHELL_SHORTCUT_BINDINGS.find')` scan of the + * AppShell source (#1968). What that line was reaching for is that an + * app-shell row on this surface is a key the shell really dispatches, and a + * grep cannot see that. Running the shipped matcher over the stroke the row + * advertises can: a row whose declared stroke its own matcher rejects is + * exactly the dead affordance the ledger exists to prevent. + */ + it('prints each app-shell row as the key the shell dispatches it from', () => { + wrapper = mount(PaperShortcutsOverlay, { props: { visible: true }, attachTo: document.body }) + + const printed = new Map(Array.from( + teleportContent().querySelectorAll('[data-shortcut-id]'), + ).map((row) => [ + row.dataset.shortcutId, + row.querySelector('.paper-shortcuts-overlay__row-kbd')?.textContent?.trim(), + ])) + + const displayedAppShellBindings = APP_SHELL_SHORTCUT_BINDINGS + .filter((binding) => printed.has(binding.id)) + expect(displayedAppShellBindings.length).toBeGreaterThan(0) + + // Couples the chip the user reads to the ledger row the shell dispatches: + // change one side and this reddens. Asserting the stroke against an event + // built from that same stroke could not. + for (const binding of displayedAppShellBindings) { + expect({ id: binding.id, chip: printed.get(binding.id) }) + .toEqual({ id: binding.id, chip: formatShortcut(binding.descriptor) }) + } + + // The overlay prints the help key in its footer rather than as a grouped + // row, and that key is the one the shell toggles this surface on. It has to + // survive the layouts that need Shift or AltGr to type it (#1968). + const root = teleportContent().querySelector('[data-paper-shortcuts]') as HTMLElement + expect(root.textContent).toContain(formatShortcut(KEYBOARD_HELP_SHORTCUT.descriptor)) + + const helpStroke = KEYBOARD_HELP_SHORTCUT.sequence[0]! + expect(strokeMatches(new KeyboardEvent('keydown', { key: '?', shiftKey: true }), helpStroke)) + .toBe(true) + expect(strokeMatches(new KeyboardEvent('keydown', { key: '?', altKey: true }), helpStroke)) + .toBe(true) }) it('documents the implemented Paper Board navigation and movement commands', () => { diff --git a/frontend/taskdeck-web/src/tests/utils/keyboardShortcuts.spec.ts b/frontend/taskdeck-web/src/tests/utils/keyboardShortcuts.spec.ts index 22a405e31..4d7e2f33e 100644 --- a/frontend/taskdeck-web/src/tests/utils/keyboardShortcuts.spec.ts +++ b/frontend/taskdeck-web/src/tests/utils/keyboardShortcuts.spec.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from 'vitest' -import { formatShortcut } from '../../utils/keyboardShortcuts' +import { + APP_SHELL_SHORTCUT_BINDINGS, + formatShortcut, + KEYBOARD_HELP_SHORTCUT, + strokeMatches, +} from '../../utils/keyboardShortcuts' describe('formatShortcut', () => { it('prefers userAgentData and renders Apple modifier glyphs', () => { @@ -62,3 +67,104 @@ describe('formatShortcut', () => { expect(formatShortcut('⌘⏎', { userAgentData: { platform: 'macOS' } })).toBe('⌘⏎') }) }) + +describe('strokeMatches', () => { + function keydown(init: KeyboardEventInit): KeyboardEvent { + return new KeyboardEvent('keydown', init) + } + + const homeStroke = { key: 'h' } as const + const paletteStroke = { key: 'k', mod: true } as const + const captureStroke = { key: 'c', mod: true, shift: true } as const + const helpStroke = KEYBOARD_HELP_SHORTCUT.sequence[0]! + + it('matches a bare letter stroke with no modifiers held', () => { + expect(strokeMatches(keydown({ key: 'h' }), homeStroke)).toBe(true) + }) + + it('does not match a bare letter stroke while Shift is held', () => { + // The comparison is case-insensitive, so `Shift+H` arrives as `H` and used + // to navigate Home (#1968). + expect(strokeMatches(keydown({ key: 'H', shiftKey: true }), homeStroke)).toBe(false) + expect(strokeMatches(keydown({ key: 'h', shiftKey: true }), homeStroke)).toBe(false) + expect(strokeMatches(keydown({ key: 'K', ctrlKey: true, shiftKey: true }), paletteStroke)) + .toBe(false) + }) + + it('keeps a declared shift exact in both directions', () => { + expect(strokeMatches(keydown({ key: 'C', ctrlKey: true, shiftKey: true }), captureStroke)) + .toBe(true) + expect(strokeMatches(keydown({ key: 'c', ctrlKey: true }), captureStroke)).toBe(false) + }) + + it('keeps mod and alt exact over letter strokes', () => { + expect(strokeMatches(keydown({ key: 'h', altKey: true }), homeStroke)).toBe(false) + expect(strokeMatches(keydown({ key: 'h', ctrlKey: true }), homeStroke)).toBe(false) + expect(strokeMatches(keydown({ key: 'k', ctrlKey: true }), paletteStroke)).toBe(true) + expect(strokeMatches(keydown({ key: 'k', metaKey: true }), paletteStroke)).toBe(true) + expect(strokeMatches(keydown({ key: 'k' }), paletteStroke)).toBe(false) + expect(strokeMatches(keydown({ key: 'k', ctrlKey: true, altKey: true }), paletteStroke)) + .toBe(false) + }) + + it('reaches the ? help stroke on layouts that need Shift or AltGr', () => { + expect(strokeMatches(keydown({ key: '?' }), helpStroke)).toBe(true) + // The common case: `?` is the shifted character, so Shift is always down. + expect(strokeMatches(keydown({ key: '?', shiftKey: true }), helpStroke)).toBe(true) + // AltGr reports altKey, and on Windows ctrlKey with it (#1968). + expect(strokeMatches(keydown({ key: '?', altKey: true }), helpStroke)).toBe(true) + expect(strokeMatches(keydown({ key: '?', altKey: true, ctrlKey: true }), helpStroke)).toBe(true) + }) + + it('still refuses a real Ctrl or Command chord over the ? help stroke', () => { + expect(strokeMatches(keydown({ key: '?', ctrlKey: true }), helpStroke)).toBe(false) + expect(strokeMatches(keydown({ key: '?', metaKey: true }), helpStroke)).toBe(false) + }) + + /** + * Building the event out of the stroke's own fields would make this pass for + * any stroke at all. The descriptor is the independent side: it is the string + * the help surfaces PRINT, so pressing what the user is told to press has to + * reach the stroke the shell listens for. A row whose descriptor and sequence + * drift apart is the dead affordance the ledger exists to prevent. + */ + function strokesFromDescriptor(descriptor: string) { + return descriptor.trim().split(/\s+/).map((chord) => { + const tokens = chord.split('+').map((token) => token.toLowerCase()) + const modifiers = ['mod', 'shift', 'alt'] + return { + key: tokens.filter((token) => !modifiers.includes(token)).join('+'), + ...(tokens.includes('mod') ? { mod: true } : {}), + ...(tokens.includes('shift') ? { shift: true } : {}), + ...(tokens.includes('alt') ? { alt: true } : {}), + } + }) + } + + it('presses what each app-shell row prints and reaches the stroke it dispatches', () => { + expect(APP_SHELL_SHORTCUT_BINDINGS.length).toBeGreaterThan(0) + + for (const binding of APP_SHELL_SHORTCUT_BINDINGS) { + const printed = strokesFromDescriptor(binding.descriptor) + const declared = binding.sequence.map((stroke) => ({ + key: stroke.key.toLowerCase(), + ...(stroke.mod ? { mod: true } : {}), + ...(stroke.shift ? { shift: true } : {}), + ...(stroke.alt ? { alt: true } : {}), + })) + expect({ id: binding.id, strokes: declared }).toEqual({ id: binding.id, strokes: printed }) + + binding.sequence.forEach((stroke, index) => { + const asPrinted = printed[index]! + const event = keydown({ + key: asPrinted.key, + ctrlKey: asPrinted.mod === true, + shiftKey: asPrinted.shift === true, + altKey: asPrinted.alt === true, + }) + expect({ id: binding.id, step: index, reaches: strokeMatches(event, stroke) }) + .toEqual({ id: binding.id, step: index, reaches: true }) + }) + } + }) +}) diff --git a/frontend/taskdeck-web/src/utils/keyboardShortcuts.ts b/frontend/taskdeck-web/src/utils/keyboardShortcuts.ts index ac9601758..88a18d939 100644 --- a/frontend/taskdeck-web/src/utils/keyboardShortcuts.ts +++ b/frontend/taskdeck-web/src/utils/keyboardShortcuts.ts @@ -180,6 +180,56 @@ export function formatShortcut( .join(' / ') } +/** + * A stroke key the keyboard produces the same way whichever modifiers reach it. + * Letters are the case that needs the guard below: `Shift+H` arrives as `H`, + * which a case-insensitive comparison cannot tell apart from `h`. + */ +const LETTER_KEY = /^[a-z]$/i + +/** + * A printable character the LAYOUT produces, `?` being the one in the ledger. + * Which modifiers were held to reach it is a property of the layout, not of the + * shortcut: on a layout where `?` needs AltGr the browser reports `altKey`, and + * on Windows `ctrlKey` too, because AltGr is Ctrl+Alt there. + */ +function isLayoutProducedCharacter(key: string): boolean { + return key.length === 1 && !/[a-z0-9]/i.test(key) +} + +/** + * Match one keydown against one canonical stroke. + * + * Shift: a stroke that declares `shift` is exact. A stroke that does not + * declare it requires Shift to be UP over a letter, so `Shift+H` no longer + * navigates Home (#1968); over a layout-produced character it stays permissive, + * because Shift is usually how you type the character at all. + * + * Alt and mod: exact, except over a layout-produced character that declares + * neither. There Alt is ignored, and Ctrl is ignored while Alt is also down -- + * the AltGr signature -- so the `?` help key stays reachable on those layouts + * without loosening any ordinary Ctrl or Alt combination. + */ +export function strokeMatches(event: KeyboardEvent, stroke: ShortcutStroke): boolean { + if (event.key.toLowerCase() !== stroke.key.toLowerCase()) return false + + const layoutCharacter = !stroke.mod && !stroke.alt && isLayoutProducedCharacter(stroke.key) + + if (stroke.shift !== undefined) { + if (event.shiftKey !== stroke.shift) return false + } else if (LETTER_KEY.test(stroke.key) && event.shiftKey) { + return false + } + + if (!layoutCharacter) { + return (event.ctrlKey || event.metaKey) === Boolean(stroke.mod) && + event.altKey === Boolean(stroke.alt) + } + + const altGrPressed = event.ctrlKey && event.altKey + return altGrPressed || !(event.ctrlKey || event.metaKey) +} + /** * The handler owner is part of every displayed row. Adding an overlay entry * therefore requires naming the concrete runtime that owns it rather than