fix(webui): broaden escape overlay guard + restore two-step cancel - #246
Merged
Conversation
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
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).
…eAppState 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<BoardAppState, ...> 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two regressions in the board Escape handler introduced by #238, reported as #244 and #245.
#244 — Escape on non-menu/dialog overlays also switches tool to select
The focused-overlay DOM guard only matched
[role='menu'],[role='dialog'],[role='alertdialog']. Radix Select / combobox / listbox / popover content isn't any of those, so dismissing one with Escape (while a create tool was active) also reset the tool.Fix: broaden the guard to
[role='listbox']and[data-radix-popper-content-wrapper]— the generic wrapper Radix renders around all popper-based content (Select, Combobox, Popover, Tooltip, Dropdown). This also makes #245'sstopPropagationsafe: an overlay can't slip past the guard and be denied its close.#245 — single Escape both switches tool and clears selection
The capture-phase handler called
setTool('select')withoutstopPropagation(), so canvas-harness's own bubble-phase Escape handler still ran and cleared the selection / aborted the draw on the same press. No two-step parity, despite the comment claiming it.The library reads
toolvia a ref (no reset on tool change) and couples abort + deselect into its one Escape handler, so the switch alone can't cancel a draw and I can't ask the library to abort-without-deselecting.Fix — restore the tldraw/excalidraw two-step:
select, abort the in-progress draft ourselves viastore.resetInteractionState(), andstopPropagation()to suppress the library's coupled deselect → selection preserved.select): fall through to the library → deselect.Refactor + tests
The Escape branch logic had two fix rounds while untested. Extracted the decision into a pure
decideBoardEscape(e, app, { isTyping, insideOverlayDom })returningno-op | defer-to-overlay | switch-to-select; the window listener now just gathers the two DOM facts and applies the outcome. Addeduse-board-keyboard.test.ts(8 cases): core switch, the two-step "already select" no-op, modifier/typing/off-board guards, and every overlay-defer path.Testing
npm run check-all(type-check + eslint) + the new unit tests pass clean.Notes
Closes #244
Closes #245