Skip to content

fix(dom-dialog): one Escape press, one answer - #60

Open
ivanbanov wants to merge 2 commits into
mainfrom
fix/dom-dialog-escape-once
Open

ivanbanov wants to merge 2 commits into
mainfrom
fix/dom-dialog-escape-once

Conversation

@ivanbanov

Copy link
Copy Markdown
Member

fix(dom-dialog): one Escape press, one answer

Problem

In a real browser, one Escape press could close two stacked dialogs, or run the outer dialog's onEscapeKeyDown guard for the inner dialog's press.

Every open dialog installs its own capture-phase keydown listener on the document (trackEscape) and checks isTopmostLayer(id) when that listener runs. For a trusted (user) event the browser runs a microtask checkpoint after each listener, and React, Solid 2, and Svelte flush state there. So the dialog that just answered has already released its layer when the next dialog's listener asks whether it is topmost, and it answers the same press. jsdom's script-dispatched events run no checkpoint, so the suite never saw it.

It happens whenever a lower dialog's listener runs after the topmost one's:

  • a nested stack that mounts open in one commit: React runs child effects first, so the inner listener registers first (the existing NestedDialog fixture reproduces it);
  • a sibling dialog rendered earlier but opened later, from inside another (reproduces in React and Solid);
  • an outer onEscapeKeyDown whose identity changes after the inner opened (dirty ? confirmDiscard : undefined), which re-registers the outer listener last.

Fix

trackEscape now decides once per press. The dialog that takes an Escape (open, topmost, no inner popup holding focus) adds the KeyboardEvent to a realm-global WeakSet before it calls onEscapeKeyDown. Every other dialog's listener returns early for a press in that set, and for a press whose default another handler already prevented.

  • Marked when taken, not on outcome. A controlled dialog only records the intent; its close lands when the consumer re-renders, on the same checkpoint that lets the next listener in. Marking only on close or veto would hand that press to the dialog beneath.
  • Realm-global (Symbol.for('@dunky.dev/dom-dialog#answered-escapes'), resolved lazily, the overlay store's idiom), so copies of the package loaded side by side honor each other's answers.
  • No preventDefault() of our own. It stays the consumer's veto, and the dialog does not cancel the browser's handling of the key.
  • defaultPrevented on entry. Popup libraries (Radix, React Aria, Headless UI) prevent the default of the Escape they close on. Such a press already could not close the dialog; now it no longer reaches the consumer's guard either.
  • One shared document listener was the alternative. It needs cross-copy ownership of the listener itself, for the same result.

Tests

packages/dom/components/dialog/tests/dialog.test.ts (framework-free) delivers presses the way a browser delivers a user's: the captured document keydown listeners run one at a time, in registration order, with the microtask queue drained after each. Dialogs leave the stack on the microtask after their machine closes, as a binding does.

  • Top-first (topmost listener runs first): only the topmost dialog closes and the one beneath never hears the press; a veto ends the press at the topmost dialog, even when its guard then closes the dialog; a controlled topmost dialog ends the press before its consumer follows; a stack-scoped press unwinds every layer once.
  • Bottom-first guard: the press still goes to the topmost dialog when the one beneath hears it first.
  • A duplicate copy of the package (vi.resetModules() + dynamic import) honors a press the first copy answered.
  • A press another handler already consumed never reaches onEscapeKeyDown.
  • Two synchronous tests that the per-press tests supersede are removed ("is ignored by a dialog that is not topmost", "unwinds the whole stack when the topmost dialog scopes Escape to it").

packages/react/dialog/tests/dialog.test.tsx: the existing nested fixture, with act() around each document keydown listener and a real dispatchEvent.

Red before the fix: origin/main's effects.ts fails 6 of the new tests (5 dom-dialog, 1 React). Wrong variants fail too: marking only on close or veto fails the controlled test, marking on first sight fails the bottom-first guard, and a module-local WeakSet fails the duplicate-copy test. The stack-scope test is a non-regression guard.

Audit: other per-instance listeners that read the stack

  • trapFocus (dom-focus-trap, through dialogTrapOptions): same shape, a per-dialog document capture keydown listener that reads isTopmostLayer live. Handling Tab only moves focus inside the topmost window, and nothing in the library releases a layer on a focus move, so the stack cannot change between two traps' listeners. Only consumer code that closes a dialog on focus loss could double-step one Tab. Not changed.
  • Backdrop and viewport presses: element handlers run by the framework's single root or delegated listener; lower layers are inert, and the viewport requires target === currentTarget. Not affected.
  • dom-overlay popup-focus queries: pure reads, no listeners. Not affected.
  • browser-navigation: one shared popstate listener that resolves one entry per Back and does not read the layer stack. Not affected.

Verification

pnpm build, pnpm test:ci (vitest 302 tests, 296 on origin/main; native jest 13), pnpm typecheck, pnpm lint, pnpm format:check: pass. pnpm changeset status --since=origin/main: patch for @dunky.dev/dom-dialog, @dunky.dev/react-dialog, @dunky.dev/solid-dialog. pnpm knip: same native-only report as origin/main.

🤖 Generated with Claude Code

ivanbanov and others added 2 commits October 6, 2026 23:18
Every open dialog has its own capture-phase document keydown listener
that asks isTopmostLayer(id) when it runs. A browser runs a microtask
checkpoint between the listeners of a user's key press, and React, Solid,
and Svelte flush there: the dialog that answered has already released its
layer when the next dialog's listener asks whether it is topmost now, so
it answers the same press. One Escape closed a nested dialog and the one
beneath it, or ran the outer dialog's onEscapeKeyDown guard for the inner
dialog's press. jsdom's script-dispatched events run no checkpoint.

- trackEscape decides once per press: the dialog that takes it (open,
  topmost, no inner popup holding focus) adds the event to a realm-global
  WeakSet (Symbol.for, the overlay store's idiom, so duplicate copies of
  the package share it) before the consumer's callback runs, whatever the
  outcome: a controlled dialog's close only lands on its consumer's
  re-render. Every other dialog's listener returns early for the press.
- A press whose default another handler already prevented (what popup
  libraries do with the Escape they close on) is not answered either.
- No preventDefault() of our own: that stays the consumer's veto.
- Tests deliver presses the way a browser does (listeners one at a time,
  the microtask queue drained between them, layers released on a
  microtask like a binding): the top-first race cases (close, veto,
  controlled, stack scope), a bottom-first guard, a duplicate package
  copy, and a consumed press. Two synchronous tests the new ones
  supersede are removed.
- SPEC: the once-per-press rule. Changeset: patch for dom-dialog and the
  React and Solid bindings that inherit it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tween listeners

The nested fixture mounts both dialogs in one commit, and React runs
child effects first, so the inner dialog's Escape listener comes first.
With act() around each document keydown listener, standing in for the
browser's microtask checkpoint that jsdom's dispatch never runs, the
inner dialog's release flushes before the outer listener asks who is
topmost. Red on the previous dom-dialog (both dialogs closed), green
with the fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant