diff --git a/webui/src/features/board/harness/canvas/use-board-keyboard.test.ts b/webui/src/features/board/harness/canvas/use-board-keyboard.test.ts new file mode 100644 index 00000000..aaa0dd7e --- /dev/null +++ b/webui/src/features/board/harness/canvas/use-board-keyboard.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, it } from "vitest" +import { decideBoardEscape, type EscapeAppState } from "./use-board-keyboard" + + +type EscKey = Pick + + +/** A bare Escape press (no modifiers). */ +const escape = (over: Partial = {}): EscKey => ({ + key: "Escape", + metaKey: false, + ctrlKey: false, + altKey: false, + shiftKey: false, + ...over, +}) + + +/** Board state with a create tool active on the canvas and no overlay open. */ +const appState = (over: Partial = {}): EscapeAppState => ({ + viewMode: "board", + chromeDialog: null, + activeNodeSurface: null, + presentationMode: false, + chatSheetOpen: false, + tool: "rect", + ...over, +}) + + +const dom = (over: Partial<{ isTyping: boolean; insideOverlayDom: boolean }> = {}) => ({ + isTyping: false, + insideOverlayDom: false, + ...over, +}) + + +describe("decideBoardEscape", () => { + it("switches a create tool back to select (the core feature)", () => { + expect(decideBoardEscape(escape(), appState({ tool: "rect" }), dom())).toBe("switch-to-select") + expect(decideBoardEscape(escape(), appState({ tool: "arrow" }), dom())).toBe("switch-to-select") + }) + + it("is a no-op when the tool is already select (library owns the 2nd-press deselect)", () => { + // Two-step parity: with nothing to put away we don't consume, so the + // harness's own Escape handler runs and deselects. + expect(decideBoardEscape(escape(), appState({ tool: "select" }), dom())).toBe("no-op") + }) + + it("ignores non-Escape keys and modified Escape presses", () => { + expect(decideBoardEscape(escape({ key: "a" }), appState(), dom())).toBe("no-op") + expect(decideBoardEscape(escape({ metaKey: true }), appState(), dom())).toBe("no-op") + expect(decideBoardEscape(escape({ ctrlKey: true }), appState(), dom())).toBe("no-op") + expect(decideBoardEscape(escape({ altKey: true }), appState(), dom())).toBe("no-op") + expect(decideBoardEscape(escape({ shiftKey: true }), appState(), dom())).toBe("no-op") + }) + + it("ignores Escape while typing", () => { + expect(decideBoardEscape(escape(), appState(), dom({ isTyping: true }))).toBe("no-op") + }) + + it("is a no-op off the board canvas (files / list views)", () => { + expect(decideBoardEscape(escape(), appState({ viewMode: "files" }), dom())).toBe("no-op") + expect(decideBoardEscape(escape(), appState({ viewMode: "list" }), dom())).toBe("no-op") + }) + + it("defers to store-tracked overlays instead of resetting the tool (#244/#245)", () => { + const cases: Partial[] = [ + { chromeDialog: "shape-menu" }, + { activeNodeSurface: { nodeId: "n1", kind: "sheet" } }, + { presentationMode: true }, + { chatSheetOpen: true }, // non-modal → focus on canvas, needs the flag + ] + for (const over of cases) { + expect(decideBoardEscape(escape(), appState(over), dom())).toBe("defer-to-overlay") + } + }) + + it("defers when the Escape is focused inside a store-less Radix overlay (#244)", () => { + // Covers Select / combobox / listbox / popover — matched via the DOM guard, + // surfaced here as `insideOverlayDom`. + expect(decideBoardEscape(escape(), appState(), dom({ insideOverlayDom: true }))).toBe( + "defer-to-overlay", + ) + }) + + it("checks typing before overlay state (a focused input always wins)", () => { + expect( + decideBoardEscape(escape(), appState({ chromeDialog: "shape-menu" }), dom({ isTyping: true })), + ).toBe("no-op") + }) +}) diff --git a/webui/src/features/board/harness/canvas/use-board-keyboard.ts b/webui/src/features/board/harness/canvas/use-board-keyboard.ts index a8610270..10145905 100644 --- a/webui/src/features/board/harness/canvas/use-board-keyboard.ts +++ b/webui/src/features/board/harness/canvas/use-board-keyboard.ts @@ -1,7 +1,7 @@ import { useEffect } from "react" import type { CanvasStore } from "@canvas-harness/core" import { isTypingTarget } from "@/lib/dom/is-typing-target" -import { useBoardAppStore } from "../store/board-app-store" +import { useBoardAppStore, type BoardAppState } from "../store/board-app-store" /** @@ -32,6 +32,66 @@ const TOOL_SHORTCUTS: Record = { } +/** + * Focused-overlay selector: store-less Radix overlays (dropdown / context menu, + * dialog, alert, and popper-based Select / combobox / listbox / popover). Escape + * focused inside one should close the overlay, not reset the tool. + * `[data-radix-popper-content-wrapper]` is the generic wrapper Radix renders + * around all popper content. + */ +const OVERLAY_ROLE_SELECTOR = + "[role='menu'],[role='dialog'],[role='alertdialog'],[role='listbox'],[data-radix-popper-content-wrapper]" + + +/** + * Board-app state the Escape decision reads — a `Pick` of the real store type so + * the literal comparisons (`viewMode !== "board"`, `tool !== "select"`) stay + * checked against the store's precise unions and can't drift out of sync. + */ +export type EscapeAppState = Pick< + BoardAppState, + "viewMode" | "chromeDialog" | "activeNodeSurface" | "presentationMode" | "chatSheetOpen" | "tool" +> + + +/** Outcome of an Escape press on the board. */ +export type EscapeDecision = "no-op" | "defer-to-overlay" | "switch-to-select" + + +/** + * Decide what a board Escape press does — pure, so the branch logic is unit- + * tested without a DOM / React mount. The caller supplies the two DOM-derived + * facts: `isTyping` (focus in input/textarea/contentEditable) and + * `insideOverlayDom` (focus inside a store-less Radix overlay). + * - "switch-to-select" — consume: put an active create tool away. The caller + * also aborts the in-progress draft + stopPropagation so the selection is + * kept (canvas-harness couples abort + deselect into its own Escape). + * - "defer-to-overlay" — an overlay owns this Escape; leave the tool alone. + * - "no-op" — not Escape, modified, typing, off the board canvas, or the tool + * is already `select` (the library handles the 2nd-press deselect). + */ +export const decideBoardEscape = ( + e: Pick, + app: EscapeAppState, + ctx: { isTyping: boolean; insideOverlayDom: boolean }, +): EscapeDecision => { + if (e.key !== "Escape") return "no-op" + if (e.metaKey || e.ctrlKey || e.altKey || e.shiftKey) return "no-op" + if (ctx.isTyping) return "no-op" + // The tool only exists on the board canvas — no-op in files / list views. + if (app.viewMode !== "board") return "no-op" + // Store-tracked overlays own the Escape (their own handlers close them). + // `chatSheetOpen` needs the explicit flag: the CopilotSheet is non-modal, so + // focus stays on the canvas and the `insideOverlayDom` DOM check misses it. + if (app.chromeDialog || app.activeNodeSurface || app.presentationMode || app.chatSheetOpen) + return "defer-to-overlay" + if (ctx.insideOverlayDom) return "defer-to-overlay" + // Tool already `select` → fall through so the library handles the deselect. + if (app.tool !== "select") return "switch-to-select" + return "no-op" +} + + /** * Global keyboard bindings for the canvas-harness board. Mirrors * prod's `use-board-shortcuts` keymap so muscle memory carries over: @@ -48,7 +108,8 @@ const TOOL_SHORTCUTS: Record = { * - M → toggle Slides panel * - G → open Icons search dialog * - I → open Images search dialog - * - Escape → return to select tool (unless an overlay owns it) + * - Escape → put the active create tool away (→ select), + * then deselect on a 2nd press (overlays win) * * Skipped when focus is in an input / textarea / contentEditable so * inline editing keeps the native shortcuts. canvas-harness already @@ -121,48 +182,31 @@ export const useBoardKeyboard = (store: CanvasStore): void => { } } - // Escape returns the canvas to the `select` tool (matches tldraw/excalidraw), - // so a create tool (rect / note / arrow / …) isn't left stuck after one shape. - // Registered in the CAPTURE phase on purpose: it must read overlay state - // BEFORE the same Escape is consumed by a handler that clears it — Radix - // dialogs/menus close on a document-level capture handler (clearing - // `chromeDialog`), and the presentation / node-surface handlers are window - // bubble listeners that clear their own flags. Reading in bubble phase would - // race all of them. When an overlay owns the Escape we defer to its close and - // leave the tool untouched; canvas-harness's own Escape handler still clears - // the selection + aborts an in-progress drag / marquee / draft edge. - // - // "Is an overlay open?" is a hand-maintained enumeration (store flags below + - // the focused-overlay DOM guard). A shared open-overlay signal would be less - // fragile — new overlays must remember to opt in here — but that's a broader - // refactor; this list covers every dismissable the board mounts today. + // Escape implements the tldraw/excalidraw two-step cancel (see + // `decideBoardEscape`). Registered in the CAPTURE phase on purpose: it reads + // the store-tracked overlay flags, which must be read BEFORE the same Escape + // is consumed by a handler that clears them — Radix dialogs/menus close on a + // document-capture handler, and the presentation / node-surface handlers are + // window bubble listeners; a bubble-phase read would race all of them. const onEscape = (e: KeyboardEvent): void => { + // Fires for every keydown (capture, window-wide) — bail before the DOM walk + // + store read so only actual Escape presses pay for them. if (e.key !== "Escape") return - if (e.metaKey || e.ctrlKey || e.altKey || e.shiftKey) return - if (isTypingTarget(e.target)) return - const app = useBoardAppStore.getState() - // The tool only exists on the board canvas — no-op in files / list views. - if (app.viewMode !== "board") return - // Overlays whose open-state lives in the store own the Escape; their own - // handlers close them, so don't also steal the tool switch. `chatSheetOpen` - // needs the explicit flag because the CopilotSheet is non-modal — focus - // stays on the canvas, so the focused-overlay DOM guard below misses it. - if ( - app.chromeDialog || - app.activeNodeSurface || - app.presentationMode || - app.chatSheetOpen - ) - return - // Store-less Radix overlays (view menu, context menu, delete-confirm alert) - // keep open-state locally — skip when the Escape is focused inside one. const target = e.target - if ( - target instanceof HTMLElement && - target.closest("[role='menu'],[role='dialog'],[role='alertdialog']") - ) - return - if (app.tool !== "select") app.setTool("select") + const insideOverlayDom = + target instanceof HTMLElement && target.closest(OVERLAY_ROLE_SELECTOR) !== null + const app = useBoardAppStore.getState() + const decision = decideBoardEscape(e, app, { + isTyping: isTypingTarget(target), + insideOverlayDom, + }) + if (decision !== "switch-to-select") return + // Consume: put the create tool away. canvas-harness couples abort + deselect + // into its own (bubble-phase) Escape, so we abort the draft ourselves and + // stopPropagation to suppress the lib's deselect — keeping the selection. + app.setTool("select") + store.resetInteractionState() + e.stopPropagation() } window.addEventListener("keydown", onKey)