diff --git a/.changeset/dialog-escape-once-per-press.md b/.changeset/dialog-escape-once-per-press.md new file mode 100644 index 0000000..3036e8a --- /dev/null +++ b/.changeset/dialog-escape-once-per-press.md @@ -0,0 +1,34 @@ +--- +'@dunky.dev/dom-dialog': patch +'@dunky.dev/react-dialog': patch +'@dunky.dev/solid-dialog': patch +--- + +One Escape press closes at most one dialog — in a real browser too. + +Each open dialog listens for Escape on the document and answers only while it +is the topmost layer. But a browser runs a microtask checkpoint between the +listeners of a user's key press, and that is where React, Solid, and Svelte +flush state: the dialog that answered had already left the stack when the next +dialog's listener asked whether it was topmost now — and it answered the same +press. One Escape closed a nested dialog together with the one beneath it, or +ran the outer dialog's `onEscapeKeyDown` guard for the inner dialog's press. +It took a lower dialog's listener running after the upper one's: a nested +stack that mounts open in one render (React runs a child's effects first), a +sibling dialog opened from inside another, or an `onEscapeKeyDown` that +changes identity after a nested dialog opened. Script-dispatched events run no +such checkpoint, so tests never saw it. + +The dialog that takes a press now marks it answered, and every other dialog +stands down for it, whatever the answer: a close, a veto from +`onEscapeKeyDown`, a `closeOnEscape={false}` gate, or a controlled dialog's +intent that its consumer follows on the next render. A stack-scoped Escape +(`escapeScope: 'stack'`) still unwinds every layer, once. Copies of the +package loaded side by side on one page honor each other's answers, from this +version on. The dialog never calls `preventDefault()` on the event — that +remains the consumer's veto. + +A press another handler already consumed — its default prevented, as popup +libraries do with the Escape they close on — no longer reaches +`onEscapeKeyDown` either. It could never close the dialog, but it still ran the +consumer's guard for a press that belonged to the popup. diff --git a/packages/dom/components/dialog/SPEC.md b/packages/dom/components/dialog/SPEC.md index 95cacc2..9a3c355 100644 --- a/packages/dom/components/dialog/SPEC.md +++ b/packages/dom/components/dialog/SPEC.md @@ -44,6 +44,18 @@ answer wherever focus is. Only the topmost layer answers, and it offers the consumer's `onEscapeKeyDown` a veto through `preventDefault` before it moves the machine. +A press is answered once, by the dialog that is topmost when it arrives. Each +dialog has its own listener and asks the stack only when that listener runs — +and a browser runs a microtask checkpoint between the listeners of a user's key +press, where the binding flushes the close the previous listener made, so the +dialog beneath may already be topmost. The dialog that takes a press therefore +marks it answered, and every other dialog's listener stands down for it, +whatever the answer turns out to be: a close, a veto, a `closeOnEscape` gate, or +an intent a controlled consumer has yet to follow. A press whose default another +handler already prevented — what popup libraries do with the Escape they close +on — counts as answered too. Copies of this package loaded side by side share +the mark, from this version on. + A popup inside the dialog that never joined the stack — a listbox or menu from another library, a control whose popup is expanded — answers before the dialog while it holds focus: the listener stands down until focus is back in the @@ -177,4 +189,6 @@ still in the window, and Tab is how such a popup is left. | Press gating takes a structural `{ target, currentTarget }` | React's synthetic event and Solid's native one share only that shape; requiring either would drag a framework type into this layer. | | The back guard reports state instead of returning a disposer | Its life spans a Back-close, so no host's "while open" scope fits it. Reporting the open state keeps the arm/park/release decision here rather than in each host. | | A stack-scoped Escape reads the stack before it moves the machine | Closing the layer releases it from the stack, and the answer to "what was beneath me" goes with it. Dismissing only after the machine actually left `open` is what makes a veto leave the stack standing. | +| A press is marked answered when a dialog takes it, not on outcome | A controlled dialog's close lands only when its consumer re-renders — on the very checkpoint that lets the next listener in — so waiting for the outcome would hand that dialog's press to the one beneath. | +| The mark is a realm-global `WeakSet`, not `preventDefault` | The overlay store's idiom: duplicate copies of the package rendezvous on one record. `preventDefault` stays the consumer's veto, and the dialog has no reason to cancel the browser's own handling of the key. | | A returning dialog is recognized by its nesting depth, not its id | The auto-generated id does not survive the remount (React's `useId` mints a fresh one), and requiring an explicit id would make the reopen an opt-in. Depth is what genuinely survives — at the cost of the same-depth ambiguity, resolved by reopening nobody. | diff --git a/packages/dom/components/dialog/src/effects.ts b/packages/dom/components/dialog/src/effects.ts index f225449..f8addd5 100644 --- a/packages/dom/components/dialog/src/effects.ts +++ b/packages/dom/components/dialog/src/effects.ts @@ -6,12 +6,35 @@ import { layersBelow, } from '@dunky.dev/dom-overlay' +// The Escape presses a dialog already answered: one press, one answer. Each +// dialog asks the stack only when its own listener runs, and a user's key +// press runs a microtask checkpoint between listeners — where the binding +// flushes the close the previous listener made, so the dialog beneath can be +// topmost by then. Realm-global, like the overlay store, so a duplicate copy +// of this package honors it too. +const ANSWERED_KEY = Symbol.for('@dunky.dev/dom-dialog#answered-escapes') + +function answeredEscapes(): WeakSet { + const scope = globalThis as unknown as Record | undefined> + let answered = scope[ANSWERED_KEY] + if (answered === undefined) { + answered = new WeakSet() + scope[ANSWERED_KEY] = answered + } + return answered +} + // Escape is a document-level concern, not a part's — it must work wherever // focus is. const trackEscape: DialogEffect = [ (machine, props) => { const onKeyDown = (event: KeyboardEvent): void => { if (event.key !== 'Escape' || !machine.matches('open')) return + // Another dialog took this press, or another handler already consumed + // it — preventing its default is what popup libraries do with the + // Escape they close on. + const answered = answeredEscapes() + if (answered.has(event) || event.defaultPrevented) return // Only the topmost dialog answers Escape — a nested stack closes one // layer at a time, unless this dialog's scope is the whole stack. const { id } = machine.context @@ -20,6 +43,9 @@ const trackEscape: DialogEffect = [ // while it holds focus — this listener runs in the capture phase, so // it would otherwise close the dialog under the popup. if (foreignPopupHoldsFocus(id) || expandedPopupControlHoldsFocus(id)) return + // The press is this dialog's now, whatever comes of it: a close, a veto, + // a gate, or an intent a controlled consumer has yet to follow. + answered.add(event) props.onEscapeKeyDown?.(event) if (event.defaultPrevented) return // Read the stack before the send: closing this layer releases it, and diff --git a/packages/dom/components/dialog/tests/dialog.test.ts b/packages/dom/components/dialog/tests/dialog.test.ts index 81755f8..da1f67b 100644 --- a/packages/dom/components/dialog/tests/dialog.test.ts +++ b/packages/dom/components/dialog/tests/dialog.test.ts @@ -1,6 +1,6 @@ // @vitest-environment jsdom // The DOM half of the Dialog, driven directly — no substrate, no framework. -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { machine, type Machine } from '@dunky.dev/state-machine' import { dialogMachine } from '@dunky.dev/dialog' import type { @@ -33,10 +33,12 @@ const build = (options: DialogOptions = {}): DialogService => { // disposed after the test: the listener is document-level, so one left over // — a vetoing one above all — would answer the next test's Escape too. const armed: (() => void)[] = [] -const armEscape = (service: DialogService, props: DialogOptions = {}): (() => void) => { - const [effect] = domDialogEffects[ - domDialogEffects.length - 1 - ] as (typeof domDialogEffects)[number] +const armEscape = ( + service: DialogService, + props: DialogOptions = {}, + effects = domDialogEffects, +): (() => void) => { + const [effect] = effects[effects.length - 1] as (typeof effects)[number] const dispose = effect(service, props) ?? ((): void => {}) armed.push(dispose) return dispose @@ -45,10 +47,82 @@ const armEscape = (service: DialogService, props: DialogOptions = {}): (() => vo const pressEscape = (): boolean => document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', cancelable: true })) +// A user's key press, as a browser delivers it: one listener at a time, with a +// microtask checkpoint after each, where a binding flushes the close the +// listener made. jsdom's dispatchEvent runs listeners back to back, so the +// press drives the document's keydown listeners by hand, in the order added. +const keydownListeners: EventListenerOrEventListenerObject[] = [] +const captureKeydownListeners = (): void => { + keydownListeners.length = 0 + const add = document.addEventListener.bind(document) + const remove = document.removeEventListener.bind(document) + vi.spyOn(document, 'addEventListener').mockImplementation((type, listener, options) => { + if (type === 'keydown') keydownListeners.push(listener) + add(type, listener, options) + }) + vi.spyOn(document, 'removeEventListener').mockImplementation((type, listener, options) => { + const index = keydownListeners.indexOf(listener) + if (type === 'keydown' && index !== -1) keydownListeners.splice(index, 1) + remove(type, listener, options) + }) +} + +const pressEscapeAsUser = async (): Promise => { + const event = new KeyboardEvent('keydown', { key: 'Escape', cancelable: true }) + for (const listener of keydownListeners.slice()) { + // A listener removed mid-press is skipped, as in a real dispatch. + if (!keydownListeners.includes(listener)) continue + if (typeof listener === 'function') listener(event) + else listener.handleEvent(event) + // The checkpoint drains every microtask, those queued while draining too + // (a re-render queuing the release); a task boundary is how a test gets + // that far. + await new Promise(resolve => setTimeout(resolve)) + } +} + // The layer stack is a realm-global that outlives a test — every registration // has to be undone or the next test inherits a stale topmost. const registered: (() => void)[] = [] +// A dialog run the way a binding runs it: open, its window in the stack — and +// out of it on the microtask after the machine leaves `open`, when the +// framework flushes the change, never synchronously. +const openLayer = (options: DialogOptions & { id: string }, depth: number): DialogService => { + const service = build({ defaultOpen: true, ...options }) + const content = document.createElement('div') + document.body.append(content) + const release = registerLayer({ + id: options.id, + depth, + element: content, + modal: true, + backdrop: () => null, + dismiss: () => service.send({ type: 'close' }), + }) + registered.push(release) + const stop = service.subscribe(() => { + if (service.matches('open')) return + stop() + queueMicrotask(release) + }) + return service +} + +// A stack of dialogs, bottom first, whose Escape listeners are armed in +// `order` — the order a browser runs them in. +type ListenerOrder = 'top-first' | 'bottom-first' +const openStack = ( + order: ListenerOrder, + ...dialogs: Array +): DialogService[] => { + const stack = dialogs.map((options, index) => openLayer(options, index + 1)) + const arming = [...stack.keys()] + if (order === 'top-first') arming.reverse() + for (const index of arming) armEscape(stack[index], dialogs[index]) + return stack +} + // A layer, mounted and registered, standing in for a rendered dialog window. const mountLayer = (id: string, depth: number, html = '', dismiss?: () => void): HTMLElement => { const content = document.createElement('div') @@ -88,38 +162,33 @@ describe('domDialogEffects — Escape', () => { expect(service.matches('open')).toBe(true) }) - it('is ignored by a dialog that is not topmost — one layer per press', () => { - const service = build({ defaultOpen: true }) - mountLayer('dlg', 1) - mountLayer('above', 2) - armEscape(service) - - pressEscape() - expect(service.matches('open')).toBe(true) - }) - - // escapeScope: 'stack' — the receiving dialog gates and vetoes, then the - // layers beneath get a plain close, top-down. - it('unwinds the whole stack when the topmost dialog scopes Escape to it', () => { + // escapeScope: 'stack' — the receiving dialog gates and vetoes, and only an + // Escape it allowed gives the layers beneath their plain close. + it('leaves the stack alone when the topmost dialog vetoes its stack-scoped Escape', () => { const lower = build({ defaultOpen: true, id: 'lower' }) const upper = build({ defaultOpen: true, id: 'upper', escapeScope: 'stack' }) mountLayer('lower', 1, '', () => lower.send({ type: 'close' })) mountLayer('upper', 2, '', () => upper.send({ type: 'close' })) - armEscape(upper) + armEscape(upper, { onEscapeKeyDown: event => event.preventDefault?.() }) pressEscape() - expect([upper.matches('open'), lower.matches('open')]).toEqual([false, false]) + expect([upper.matches('open'), lower.matches('open')]).toEqual([true, true]) }) - it('leaves the stack alone when the topmost dialog vetoes its stack-scoped Escape', () => { - const lower = build({ defaultOpen: true, id: 'lower' }) - const upper = build({ defaultOpen: true, id: 'upper', escapeScope: 'stack' }) - mountLayer('lower', 1, '', () => lower.send({ type: 'close' })) - mountLayer('upper', 2, '', () => upper.send({ type: 'close' })) - armEscape(upper, { onEscapeKeyDown: event => event.preventDefault?.() }) + // A popup library prevents the default of the Escape it closes on; the press + // was the popup's, so it never reaches the consumer's guard. + it('stands down for a press another handler already consumed', () => { + const onEscapeKeyDown = vi.fn() + const service = build({ defaultOpen: true }) + mountLayer('dlg', 1) + const consume = (event: KeyboardEvent): void => event.preventDefault() + window.addEventListener('keydown', consume, true) + armed.push(() => window.removeEventListener('keydown', consume, true)) + armEscape(service, { onEscapeKeyDown }) pressEscape() - expect([upper.matches('open'), lower.matches('open')]).toEqual([true, true]) + expect(service.matches('open')).toBe(true) + expect(onEscapeKeyDown).not.toHaveBeenCalled() }) it('detaches its listener on dispose', () => { @@ -181,6 +250,110 @@ describe('domDialogEffects — Escape', () => { }) }) +// One press, one answer — from the dialog topmost when it arrived, however +// soon a closed dialog leaves the stack. +describe('domDialogEffects — one answer per Escape press', () => { + beforeEach(captureKeydownListeners) + + // Top-first from here on: the topmost dialog's listener runs first, so its + // close has flushed by the time the one beneath asks who is topmost. + it('closes only the topmost dialog — the one beneath never hears the press', async () => { + const lowerEscape = vi.fn() + const [lower, upper] = openStack( + 'top-first', + { id: 'lower', onEscapeKeyDown: lowerEscape }, + { id: 'upper' }, + ) + + await pressEscapeAsUser() + expect([upper.matches('open'), lower.matches('open')]).toEqual([false, true]) + expect(lowerEscape).not.toHaveBeenCalled() + }) + + // A confirm-then-close guard: it vetoes the dismissal and closes its dialog + // itself, which leaves the stack all the same. + it('ends a vetoed press at the topmost dialog', async () => { + const lowerEscape = vi.fn() + const [lower, upper] = openStack( + 'top-first', + { id: 'lower', onEscapeKeyDown: lowerEscape }, + { + id: 'upper', + onEscapeKeyDown: event => { + event.preventDefault?.() + upper.send({ type: 'close' }) + }, + }, + ) + + await pressEscapeAsUser() + expect(lower.matches('open')).toBe(true) + expect(lowerEscape).not.toHaveBeenCalled() + }) + + // A controlled dialog only records the intent; it closes when its consumer + // echoes `open={false}` back — on the checkpoint, as a re-render would. + it('ends the press at a controlled topmost dialog, before its consumer follows', async () => { + const lowerEscape = vi.fn() + const [lower, upper] = openStack( + 'top-first', + { id: 'lower', onEscapeKeyDown: lowerEscape }, + { + id: 'upper', + open: true, + onEscapeKeyDown: () => + queueMicrotask(() => upper.send({ type: 'controlled.sync', value: false })), + }, + ) + + await pressEscapeAsUser() + expect([upper.matches('open'), lower.matches('open')]).toEqual([false, true]) + expect(lowerEscape).not.toHaveBeenCalled() + }) + + // The layers beneath get a plain close, never the press itself. + it('unwinds a stack-scoped press through every layer, once', async () => { + const beneathEscape = vi.fn() + const stack = openStack( + 'top-first', + { id: 'bottom', onEscapeKeyDown: beneathEscape }, + { id: 'middle', onEscapeKeyDown: beneathEscape }, + { id: 'top', escapeScope: 'stack' }, + ) + + await pressEscapeAsUser() + expect(stack.map(dialog => dialog.matches('open'))).toEqual([false, false, false]) + expect(beneathEscape).not.toHaveBeenCalled() + }) + + // Only the dialog that takes the press marks it: a lower dialog that hears + // it first, before anything closed, must leave it to the topmost. + it('leaves the press to the topmost dialog when the one beneath hears it first', async () => { + const [lower, upper] = openStack('bottom-first', { id: 'lower' }, { id: 'upper' }) + + await pressEscapeAsUser() + expect([upper.matches('open'), lower.matches('open')]).toEqual([false, true]) + }) +}) + +describe('domDialogEffects — a duplicate copy of the package', () => { + beforeEach(captureKeydownListeners) + + // A monorepo or micro-frontend can load the package twice: a press one copy + // answered must count as answered for the other too. + it('honors a press another copy of the package already answered', async () => { + vi.resetModules() + const copy = await import('@dunky.dev/dom-dialog') + const lower = openLayer({ id: 'lower' }, 1) + const upper = openLayer({ id: 'upper' }, 2) + armEscape(upper) + armEscape(lower, {}, copy.domDialogEffects) + + await pressEscapeAsUser() + expect([upper.matches('open'), lower.matches('open')]).toEqual([false, true]) + }) +}) + describe('openDialogLayer', () => { const options = { id: 'dlg', depth: 1, modal: true, backdrop: () => null } diff --git a/packages/react/dialog/tests/dialog.test.tsx b/packages/react/dialog/tests/dialog.test.tsx index 05979f0..0c7d5d2 100644 --- a/packages/react/dialog/tests/dialog.test.tsx +++ b/packages/react/dialog/tests/dialog.test.tsx @@ -3,7 +3,7 @@ // covered in @dunky.dev/dialog's tests. import { useRef, useState } from 'react' import { act, cleanup, fireEvent, render, screen } from '@testing-library/react' -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, onTestFinished, vi } from 'vitest' import { Dialog, type DialogProps } from '@dunky.dev/react-dialog' const DefaultDialog = (props: DialogProps) => ( @@ -790,6 +790,39 @@ describe('Dialog', () => { expect(screen.queryByText('Outer')).toBeNull() }) + // A browser runs a microtask checkpoint after each listener of a user's + // key press, and React flushes there; act() per listener stands in for it. + // Mounted in one commit, the inner dialog's listener runs first (child + // effects run first), so the outer one's runs after the inner left the stack. + it('one Escape closes one layer even when React flushes between listeners', () => { + const add = document.addEventListener.bind(document) + const remove = document.removeEventListener.bind(document) + const wrappers = new Map() + vi.spyOn(document, 'addEventListener').mockImplementation((type, listener, options) => { + if (type !== 'keydown') return add(type, listener, options) + const wrapper: EventListener = event => + act(() => + typeof listener === 'function' ? listener(event) : listener.handleEvent(event), + ) + wrappers.set(listener, wrapper) + add(type, wrapper, options) + }) + vi.spyOn(document, 'removeEventListener').mockImplementation((type, listener, options) => + remove(type, wrappers.get(listener) ?? listener, options), + ) + // After RTL's cleanup, which removes the wrapped listeners through the spy. + onTestFinished(() => { + vi.restoreAllMocks() + }) + + render() + // Not fireEvent: it wraps the dispatch in act(), and a nested act() + // defers its flush to the outermost one. + document.body.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })) + expect(screen.queryByText('Inner')).toBeNull() + expect(screen.queryByText('Outer')).not.toBeNull() + }) + it('a stack-scoped Escape on the topmost dialog unwinds every layer', () => { render(