From 5ff56db9ab6cae359ae565c19f86d317cca5cae9 Mon Sep 17 00:00:00 2001 From: Le Ha Quang Date: Sun, 23 Aug 2026 17:13:02 +0200 Subject: [PATCH 1/3] fix(webui): broaden escape overlay guard + restore two-step cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two regressions from the escape-to-select handler (#244, #245): - #244: the focused-overlay DOM guard only matched menu/dialog/alertdialog, so dismissing a Radix Select/combobox/listbox/popover with Escape also yanked the tool to select. Broaden it to [role='listbox'] and the generic [data-radix-popper-content-wrapper] that wraps all Radix popper content. - #245: the capture-phase handler switched the tool without stopPropagation, so canvas-harness's bubble-phase Escape also ran and cleared the selection / aborted the draw on the same press — no two-step parity. The lib reads the tool via a ref with no reset on change and couples abort+deselect, so on the first Escape we now switch to select, abort the in-progress draft ourselves via resetInteractionState(), and stopPropagation to suppress the lib's coupled deselect (selection preserved). A second Escape (tool already select) falls through to the lib and deselects. Fixes #244 Fixes #245 --- .../harness/canvas/use-board-keyboard.ts | 36 ++++++++++++++----- 1 file changed, 27 insertions(+), 9 deletions(-) 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..6cc0f1a0 100644 --- a/webui/src/features/board/harness/canvas/use-board-keyboard.ts +++ b/webui/src/features/board/harness/canvas/use-board-keyboard.ts @@ -48,7 +48,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,16 +122,19 @@ 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. + // Escape implements the tldraw/excalidraw two-step cancel on the board: + // 1st press — put an active create tool (rect / note / arrow / …) away + // (→ select) and abort any in-progress draft, KEEPING the + // current selection. + // 2nd press — with the tool already `select`, deselect. + // // 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. + // leave the tool untouched. // // "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 @@ -154,15 +158,29 @@ export const useBoardKeyboard = (store: CanvasStore): void => { 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. + // Store-less Radix overlays (dropdown / context menu, dialog, alert, and + // popper-based Select / combobox / listbox / popover) keep open-state + // locally — skip when the Escape is focused inside one so it closes the + // overlay instead of resetting the tool. `[data-radix-popper-content-wrapper]` + // is the generic wrapper Radix renders around all popper content. const target = e.target if ( target instanceof HTMLElement && - target.closest("[role='menu'],[role='dialog'],[role='alertdialog']") + target.closest( + "[role='menu'],[role='dialog'],[role='alertdialog'],[role='listbox'],[data-radix-popper-content-wrapper]", + ) ) return - if (app.tool !== "select") app.setTool("select") + // Step 1: put the create tool away. canvas-harness couples abort + deselect + // into its own (bubble-phase) Escape handler, so to keep the selection we + // abort the in-progress draft ourselves and stopPropagation to suppress it. + // With the tool already `select` we fall through — the harness handles the + // 2nd Escape as the deselect step. + if (app.tool !== "select") { + app.setTool("select") + store.resetInteractionState() + e.stopPropagation() + } } window.addEventListener("keydown", onKey) From df37cd39d66eb050c3901ca91b0ac2048fc12110 Mon Sep 17 00:00:00 2001 From: Le Ha Quang Date: Sun, 23 Aug 2026 17:21:11 +0200 Subject: [PATCH 2/3] test(webui): extract decideBoardEscape as a pure fn and unit-test it The Escape branch logic had two fix rounds while untested. Pull the decision (no-op / defer-to-overlay / switch-to-select) out of the window listener into a pure decideBoardEscape(e, app, {isTyping, insideOverlayDom}); the handler now just gathers the two DOM facts and applies the outcome. Add use-board-keyboard.test.ts covering the core switch, the two-step 'already select' no-op, modifier/typing/ off-board guards, and every overlay-defer path (store flags + focused-overlay DOM). --- .../harness/canvas/use-board-keyboard.test.ts | 92 ++++++++++++ .../harness/canvas/use-board-keyboard.ts | 137 ++++++++++-------- 2 files changed, 172 insertions(+), 57 deletions(-) create mode 100644 webui/src/features/board/harness/canvas/use-board-keyboard.test.ts 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 6cc0f1a0..a477aa42 100644 --- a/webui/src/features/board/harness/canvas/use-board-keyboard.ts +++ b/webui/src/features/board/harness/canvas/use-board-keyboard.ts @@ -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 testable subset of the store). */ +export type EscapeAppState = { + viewMode: string + chromeDialog: unknown + activeNodeSurface: unknown + presentationMode: boolean + chatSheetOpen: boolean + tool: string +} + + +/** 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: @@ -122,65 +182,28 @@ export const useBoardKeyboard = (store: CanvasStore): void => { } } - // Escape implements the tldraw/excalidraw two-step cancel on the board: - // 1st press — put an active create tool (rect / note / arrow / …) away - // (→ select) and abort any in-progress draft, KEEPING the - // current selection. - // 2nd press — with the tool already `select`, deselect. - // - // 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. - // - // "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 => { - 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 (dropdown / context menu, dialog, alert, and - // popper-based Select / combobox / listbox / popover) keep open-state - // locally — skip when the Escape is focused inside one so it closes the - // overlay instead of resetting the tool. `[data-radix-popper-content-wrapper]` - // is the generic wrapper Radix renders around all popper content. const target = e.target - if ( - target instanceof HTMLElement && - target.closest( - "[role='menu'],[role='dialog'],[role='alertdialog'],[role='listbox'],[data-radix-popper-content-wrapper]", - ) - ) - return - // Step 1: put the create tool away. canvas-harness couples abort + deselect - // into its own (bubble-phase) Escape handler, so to keep the selection we - // abort the in-progress draft ourselves and stopPropagation to suppress it. - // With the tool already `select` we fall through — the harness handles the - // 2nd Escape as the deselect step. - if (app.tool !== "select") { - app.setTool("select") - store.resetInteractionState() - e.stopPropagation() - } + 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) From a6c44ea79c74df0d570d116f7a6a33b6f9f6d0f9 Mon Sep 17 00:00:00 2001 From: Le Ha Quang Date: Sun, 23 Aug 2026 17:26:42 +0200 Subject: [PATCH 3/3] fix(webui): keep escape short-circuit in the listener + tighten EscapeAppState Review follow-ups on the capture-phase Escape listener: - Restore the early `if (e.key !== 'Escape') return` in onEscape. The refactor moved the key check into decideBoardEscape, so the window-wide capture listener paid a 5-selector `closest()` walk + getState() on every keystroke. Bail first. - Type EscapeAppState as Pick instead of a hand-rolled loose subset, so `viewMode !== 'board'` / `tool !== 'select'` stay checked against the store's real unions and can't silently drift. --- .../harness/canvas/use-board-keyboard.ts | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) 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 a477aa42..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" /** @@ -43,15 +43,15 @@ 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 testable subset of the store). */ -export type EscapeAppState = { - viewMode: string - chromeDialog: unknown - activeNodeSurface: unknown - presentationMode: boolean - chatSheetOpen: boolean - tool: string -} +/** + * 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. */ @@ -189,6 +189,9 @@ export const useBoardKeyboard = (store: CanvasStore): void => { // 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 const target = e.target const insideOverlayDom = target instanceof HTMLElement && target.closest(OVERLAY_ROLE_SELECTOR) !== null