diff --git a/.changeset/dialog-accessible-name-warning.md b/.changeset/dialog-accessible-name-warning.md new file mode 100644 index 0000000..21845cc --- /dev/null +++ b/.changeset/dialog-accessible-name-warning.md @@ -0,0 +1,13 @@ +--- +'@dunky.dev/dom-dialog': patch +--- + +The dialog warns when it resolves no accessible name. The core contract — +a rendered Title, or an `aria-label` / `aria-labelledby` on Content, never +neither — was documented but unenforced, so a nameless dialog shipped +silently. `openDialogLayer` now checks the window's own attributes on open +and warns with the fix, the same loud treatment the stranded-focus miss +already gets. The check reads the DOM rather than the machine's +Title-presence flag (a label may arrive through a prop spread), defers a +macrotask so a rendered Title's registration commits first, and is cancelled +by the close. diff --git a/.changeset/dialog-drag-never-dismisses.md b/.changeset/dialog-drag-never-dismisses.md new file mode 100644 index 0000000..51dc7fd --- /dev/null +++ b/.changeset/dialog-drag-never-dismisses.md @@ -0,0 +1,21 @@ +--- +'@dunky.dev/dom-press-origin': minor +'@dunky.dev/dom-dialog': minor +'@dunky.dev/react-dialog': patch +'@dunky.dev/solid-dialog': patch +--- + +A text-selection drag no longer dismisses the dialog. A mousedown inside the +window with a mouseup outside it makes the browser fire `click` on their +common ancestor — the viewport or the backdrop — which was indistinguishable +from a genuine outside press by the click's target alone, so selecting text +across the window's edge closed the dialog and lost the form state with it. + +Where a press *began* now decides, not where it ended. New package: +`@dunky.dev/dom-press-origin` — `trackPressOrigin(element)` captures each +press's origin at `pointerdown`, before the browser collapses the click +target; it is its own util because every light-dismissable layer needs the +same guard. `@dunky.dev/dom-dialog`'s `acceptsBackdropPress` / +`acceptsViewportPress` take the answer as a new `startedInside` argument and +refuse the press when the gesture started inside the window. A press that +starts and ends outside still dismisses as before. diff --git a/.changeset/dialog-nonmodal-pointer-passthrough.md b/.changeset/dialog-nonmodal-pointer-passthrough.md new file mode 100644 index 0000000..b3dddca --- /dev/null +++ b/.changeset/dialog-nonmodal-pointer-passthrough.md @@ -0,0 +1,26 @@ +--- +'@dunky.dev/dom-overlay': minor +'@dunky.dev/dom-dialog': minor +'@dunky.dev/react-dialog': patch +'@dunky.dev/solid-dialog': patch +--- + +A non-modal dialog no longer blocks the page around it. `Dialog.Viewport` is +a full-coverage layer, so even with no backdrop rendered it swallowed every +press aimed at the page beneath — the page element never received it, and +the dialog dismissed instead. Now, while `modal={false}`, the Viewport +renders with `pointer-events: none` and the window with +`pointer-events: auto` (the consumer's own `style` wins over both), so +presses on the empty area reach the page and the window stays interactive. + +Outside-press detection moves with it: a transparent Viewport never receives +the press it used to detect, so `@dunky.dev/dom-overlay` gains +`watchOutsidePress` — a document-level watch (the only vantage point that +sees both the portaled layer and the page) with the same refusals as the +element path: topmost layer only, the layer's element excepted, and the +trigger excepted so its own press stays a plain toggle instead of a +close-and-reopen. It lives in the overlay util, not the dialog package, +because any layer whose surface lets presses fall through will need it. +`@dunky.dev/dom-dialog`'s `viewportPointerEvents(modal)` and +`contentPointerEvents` carry the two style values so every DOM substrate +applies the same pair. diff --git a/packages/core/dialog/SPEC.md b/packages/core/dialog/SPEC.md index 2ca4129..9a1b109 100644 --- a/packages/core/dialog/SPEC.md +++ b/packages/core/dialog/SPEC.md @@ -50,7 +50,15 @@ Using the dialog is a walkthrough of intent, not a prop list: - Pressing the **backdrop** — or the **viewport** area around the dialog window — counts as an "outside interaction"; whether that dismisses follows the dialog's dismissal settings, whichever layer was pressed. Presses inside the - content never count as outside. + content never count as outside — including a press that starts inside and + releases outside (a text-selection drag): where a press ends doesn't decide, + where it began does. The trigger's own press is never an outside interaction + either; it stays a plain toggle. +- A **non-modal** dialog (`modal={false}`) coexists with the page: no backdrop, + no focus trap, no scroll lock, and the page around it stays fully + interactive — a press on the empty area around the window reaches whatever + the page has there. That press still counts as the dialog's outside + interaction (the trigger excepted), following the same dismissal settings. - The **content** is the dialog window, labelled and described by the Title and Description parts when they are rendered. - **Title / Description** name and describe the dialog. The dialog's ARIA name diff --git a/packages/dom/components/dialog/SPEC.md b/packages/dom/components/dialog/SPEC.md index a2321b1..c1ffea0 100644 --- a/packages/dom/components/dialog/SPEC.md +++ b/packages/dom/components/dialog/SPEC.md @@ -69,6 +69,17 @@ typically because it lacks `tabindex="-1"` — focus is stranded outside the layer, against the modal dialog pattern. That miss is loud: a `console.warn` names the fix instead of failing silently. +The window's accessible name gets the same loud treatment: neither a +rendered Title (`aria-labelledby`) nor an explicit `aria-label` on the +window warns, rather than shipping a dialog assistive tech can't name. +Reading the window's own attributes — not the machine's Title-registered +flag — is deliberate: a consumer's label can arrive through a prop spread +rather than a typed prop, and the DOM is where it lands either way. The +check defers a macrotask so a rendered Title's registration commits first +(it arrives through the substrate's own re-render, not this call's turn), +and the close cancels it — a dialog gone before the check fires must not +warn about a window that no longer exists. + The disposer releases the stack **before** restoring focus. Both orders are load-bearing: the stack must exist before focus moves in, and the layers beneath must be un-inerted before focus can land on one of them. @@ -119,6 +130,29 @@ dialog's to answer: press must have started on the viewport itself, and then the same topmost rule applies. +Both also refuse a press whose gesture began inside the window. A +`click`'s own target can't answer that once the browser has collapsed it: a +mousedown inside the window and a mouseup outside it fires `click` on their +common ancestor — the backdrop or the viewport, not where the press +actually started — so a text-selection drag that starts inside and releases +outside would otherwise read as a genuine outside press and lose whatever +the user was selecting. The origin comes from +[`@dunky.dev/dom-press-origin`](../../utils/press-origin/SPEC.md)'s +`trackPressOrigin`, which captures it at `pointerdown`, before that +collapse, the only point it's still recoverable — a substrate tracks the +window and feeds `startedInside` into both predicates. + +A non-modal dialog has no Backdrop, and its Viewport lets a press on the +empty area fall through to the page rather than swallow it — +`viewportPointerEvents` is `none` there, `contentPointerEvents` on the +window stays `auto` regardless. That fall-through means Viewport never +receives those presses to detect them, so the substrate arms +[`@dunky.dev/dom-overlay`](../../utils/overlay/SPEC.md)'s +`watchOutsidePress` instead — the document-level outside-press watch for +layers whose own surfaces can't see one. It carries the same refusals as +the click-based path (topmost-only, the window excepted, the trigger +excepted, the started-inside guard); the dialog only supplies its pieces. + ### Focus trap `dialogTrapOptions` is the trap configuration the substrate hands to its @@ -127,15 +161,17 @@ part is the cycle's last stop wherever it renders. ## API -| Export | Description | -| ------------------------------------- | --------------------------------------------------------------------------- | -| `domDialogEffects` | Core effects + the document Escape listener, as `DialogEffect` tuples. | -| `openDialogLayer(content, options)` | The open sequence; returns the close sequence. | -| `startExitWindow(content, options)` | Hides and watches the still-painting layer; returns the undo. | -| `guardBackNavigation(options)` | The history guard: report the open state as it changes, release at the end. | -| `acceptsBackdropPress(id)` | Whether a backdrop press is this dialog's outside interaction. | -| `acceptsViewportPress(id, event)` | Same for the viewport, ignoring presses that bubbled from the content. | -| `dialogTrapOptions(machine, closeId)` | `TrapFocusOptions` for the dialog window. | +| Export | Description | +| ------------------------------------------------ | ----------------------------------------------------------------------------------- | +| `domDialogEffects` | Core effects + the document Escape listener, as `DialogEffect` tuples. | +| `openDialogLayer(content, options)` | The open sequence; returns the close sequence. | +| `startExitWindow(content, options)` | Hides and watches the still-painting layer; returns the undo. | +| `guardBackNavigation(options)` | The history guard: report the open state as it changes, release at the end. | +| `acceptsBackdropPress(id, startedInside)` | Whether a backdrop press is this dialog's outside interaction. | +| `acceptsViewportPress(id, event, startedInside)` | Same for the viewport, ignoring presses that bubbled from the content. | +| `viewportPointerEvents(modal)` | The Viewport's `pointer-events` value: `undefined` while modal, `'none'` otherwise. | +| `contentPointerEvents` | The window's `pointer-events` value — always `'auto'`. | +| `dialogTrapOptions(machine, closeId)` | `TrapFocusOptions` for the dialog window. | ## Constraints diff --git a/packages/dom/components/dialog/src/index.ts b/packages/dom/components/dialog/src/index.ts index cecf6bd..4ceaff6 100644 --- a/packages/dom/components/dialog/src/index.ts +++ b/packages/dom/components/dialog/src/index.ts @@ -7,4 +7,5 @@ export { type BackNavigationGuardOptions, } from './back-navigation' export { acceptsBackdropPress, acceptsViewportPress } from './press' +export { viewportPointerEvents, contentPointerEvents } from './viewport-style' export { dialogTrapOptions } from './focus-trap' diff --git a/packages/dom/components/dialog/src/open-layer.ts b/packages/dom/components/dialog/src/open-layer.ts index eba0860..25bd01d 100644 --- a/packages/dom/components/dialog/src/open-layer.ts +++ b/packages/dom/components/dialog/src/open-layer.ts @@ -58,7 +58,26 @@ export function openDialogLayer(content: HTMLElement, options: OpenDialogLayerOp } } + // The window's own attributes are the reliable read: a consumer's label may + // arrive through a prop spread rather than a typed prop, and the DOM is + // where it lands either way. Deferred a macrotask: a rendered Title + // registers presence through its own effect, and the canonical nesting + // (Title inside Content) settles before this call's own effect runs — but + // nothing requires that arrangement, so a Title rendered as a later + // sibling still gets a chance to register first. Cleared on close: a + // dialog that closes before the check fires must not warn about a window + // that no longer exists. + const nameCheck = setTimeout(() => { + if (!content.hasAttribute('aria-label') && !content.hasAttribute('aria-labelledby')) { + console.warn( + '[openDialogLayer] the dialog has no accessible name: render a , ' + + 'or pass aria-label / aria-labelledby to Content.', + ) + } + }) + return () => { + clearTimeout(nameCheck) unregister() if (previous instanceof HTMLElement) previous.focus({ preventScroll: true }) } diff --git a/packages/dom/components/dialog/src/press.ts b/packages/dom/components/dialog/src/press.ts index 5301984..e6f23fb 100644 --- a/packages/dom/components/dialog/src/press.ts +++ b/packages/dom/components/dialog/src/press.ts @@ -10,9 +10,13 @@ interface PressTarget { /** * Whether a backdrop press is this dialog's outside interaction. Only the * topmost dialog of a stack answers one — a nested stack dismisses one layer - * at a time, the same rule Escape follows. + * at a time, the same rule Escape follows. `startedInside` (see + * `trackPressOrigin` in `@dunky.dev/dom-press-origin`) refuses a press whose gesture began inside the + * window: a text-selection drag that starts inside and releases on the + * backdrop must not read as an outside press. */ -export function acceptsBackdropPress(id: string): boolean { +export function acceptsBackdropPress(id: string, startedInside: boolean): boolean { + if (startedInside) return false return isTopmostLayer(id) } @@ -20,8 +24,17 @@ export function acceptsBackdropPress(id: string): boolean { * Whether a viewport press is this dialog's outside interaction. Content * presses bubble up to the viewport, so only a press that started on the * viewport itself counts — and then only for the topmost dialog. + * `startedInside` catches what the bubble check can't: a text-selection drag + * starting inside the window and releasing on the viewport's own background + * collapses the click's target to the viewport, passing the bubble check — + * see `trackPressOrigin`. */ -export function acceptsViewportPress(id: string, event: PressTarget): boolean { +export function acceptsViewportPress( + id: string, + event: PressTarget, + startedInside: boolean, +): boolean { + if (startedInside) return false if (event.target !== event.currentTarget) return false return isTopmostLayer(id) } diff --git a/packages/dom/components/dialog/src/viewport-style.ts b/packages/dom/components/dialog/src/viewport-style.ts new file mode 100644 index 0000000..0b0ebd7 --- /dev/null +++ b/packages/dom/components/dialog/src/viewport-style.ts @@ -0,0 +1,15 @@ +/** + * The Viewport's pointer-events reach. Modal: unset — it is the outside- + * press surface for whatever gutter sits around the window. Non-modal: the + * page coexists with the dialog, so the Viewport's own full-coverage box + * must not swallow presses meant for whatever sits beneath it — `none` lets + * them fall through; the window stays reachable regardless (see + * `contentPointerEvents`). + */ +export function viewportPointerEvents(modal: boolean): 'none' | undefined { + return modal ? undefined : 'none' +} + +/** The dialog window always stays interactive, even where its Viewport + * disables pointer events to let non-modal presses fall through around it. */ +export const contentPointerEvents = 'auto' as const diff --git a/packages/dom/components/dialog/tests/dialog.test.ts b/packages/dom/components/dialog/tests/dialog.test.ts index 94377e0..6733bb9 100644 --- a/packages/dom/components/dialog/tests/dialog.test.ts +++ b/packages/dom/components/dialog/tests/dialog.test.ts @@ -48,6 +48,7 @@ const registered: (() => void)[] = [] const mountLayer = (id: string, depth: number, html = '', dismiss?: () => void): HTMLElement => { const content = document.createElement('div') content.tabIndex = -1 + content.setAttribute('aria-label', 'Layer') content.innerHTML = html document.body.append(content) registered.push( @@ -137,6 +138,7 @@ describe('openDialogLayer', () => { ): { content: HTMLElement; close: () => void } => { const content = document.createElement('div') content.tabIndex = -1 + content.setAttribute('aria-label', 'Dialog') content.innerHTML = html document.body.append(content) @@ -154,6 +156,7 @@ describe('openDialogLayer', () => { it('moves focus to the first form field, without scrolling the locked surface', () => { const content = document.createElement('div') content.tabIndex = -1 + content.setAttribute('aria-label', 'Dialog') content.innerHTML = '' document.body.append(content) const field = document.getElementById('field') as HTMLInputElement @@ -168,6 +171,7 @@ describe('openDialogLayer', () => { it('honors an explicit initialFocus over the overlay default', () => { const content = document.createElement('div') content.tabIndex = -1 + content.setAttribute('aria-label', 'Dialog') content.innerHTML = '' document.body.append(content) const pick = content.querySelector('#pick') as HTMLButtonElement @@ -180,6 +184,7 @@ describe('openDialogLayer', () => { it('falls back to the dialog window when the target refuses focus', () => { const content = document.createElement('div') content.tabIndex = -1 + content.setAttribute('aria-label', 'Dialog') content.innerHTML = '' document.body.append(content) const field = content.querySelector('#field') as HTMLInputElement @@ -201,6 +206,32 @@ describe('openDialogLayer', () => { expect(warn).toHaveBeenCalledWith(expect.stringContaining('tabindex="-1"')) }) + it('warns when the dialog has neither a Title nor an accessible label', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const content = document.createElement('div') + content.tabIndex = -1 + document.body.append(content) + + registered.push(openDialogLayer(content, options)) + await new Promise(resolve => setTimeout(resolve, 0)) + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('no accessible name')) + }) + + it('does not warn when the window carries aria-labelledby', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const content = document.createElement('div') + content.tabIndex = -1 + content.setAttribute('aria-labelledby', 'dlg-title') + content.innerHTML = '

Title

' + document.body.append(content) + + registered.push(openDialogLayer(content, options)) + await new Promise(resolve => setTimeout(resolve, 0)) + + expect(warn).not.toHaveBeenCalledWith(expect.stringContaining('no accessible name')) + }) + it('restores focus to whatever held it before the dialog opened', () => { const trigger = document.createElement('button') document.body.append(trigger) @@ -357,10 +388,10 @@ describe('guardBackNavigation', () => { describe('outside-press gating', () => { it('lets only the topmost dialog answer a backdrop press', () => { mountLayer('dlg', 1) - expect(acceptsBackdropPress('dlg')).toBe(true) + expect(acceptsBackdropPress('dlg', false)).toBe(true) mountLayer('above', 2) - expect(acceptsBackdropPress('dlg')).toBe(false) + expect(acceptsBackdropPress('dlg', false)).toBe(false) }) it('ignores a viewport press that bubbled up from the content', () => { @@ -368,8 +399,22 @@ describe('outside-press gating', () => { const viewport = document.createElement('div') const content = document.createElement('div') - expect(acceptsViewportPress('dlg', { target: viewport, currentTarget: viewport })).toBe(true) - expect(acceptsViewportPress('dlg', { target: content, currentTarget: viewport })).toBe(false) + expect(acceptsViewportPress('dlg', { target: viewport, currentTarget: viewport }, false)).toBe( + true, + ) + expect(acceptsViewportPress('dlg', { target: content, currentTarget: viewport }, false)).toBe( + false, + ) + }) + + it('refuses a backdrop or viewport press whose gesture started inside the window', () => { + mountLayer('dlg', 1) + const viewport = document.createElement('div') + + expect(acceptsBackdropPress('dlg', true)).toBe(false) + expect(acceptsViewportPress('dlg', { target: viewport, currentTarget: viewport }, true)).toBe( + false, + ) }) }) diff --git a/packages/dom/utils/overlay/SPEC.md b/packages/dom/utils/overlay/SPEC.md index 139b251..7a58bca 100644 --- a/packages/dom/utils/overlay/SPEC.md +++ b/packages/dom/utils/overlay/SPEC.md @@ -72,6 +72,25 @@ one; the predicates are [`@dunky.dev/dom-element`](../element/SPEC.md)'s `isRendered` and `isFocusable`, shared with the focus trap so the two can't disagree on what counts. +### Outside presses without a surface + +A layer's own elements usually detect an outside press — a backdrop click, a +press on the viewport gutter. A layer that coexists with the page has +neither: no backdrop renders, and its viewport lets presses fall through +(`pointer-events: none`) precisely so the page stays interactive — which +also means the viewport never receives the press it would have detected. +`watchOutsidePress` is the substitute: a document-level listener, because +the document is the only vantage point that sees both the layer (portaled +out of the page's own subtree) and the page. + +It refuses four ways: only the topmost layer of the stack answers; a press +inside the layer's element is never outside; the trigger that opened the +layer is excepted, so its own press stays a plain toggle rather than a +close-and-immediately-reopen; and a press whose gesture _began_ inside the +element is refused via the caller-supplied `startedInside` (see +[`@dunky.dev/dom-press-origin`](../press-origin/SPEC.md)) — a +text-selection drag ending outside is not an outside press. + ### The exit window A closing overlay has already left the stack — the page beneath is live @@ -94,6 +113,7 @@ again — but keeps painting until its exit visual finishes: | Export | Description | | ------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------- | | `registerLayer(layer)` | Joins the shared stack and syncs containment; returns the disposer that restores it. | +| `watchOutsidePress(id, options)` | Document-level outside-press watch for a layer whose own surfaces can't detect one. Returns the undo. | | `Layer` | `OverlayLayer` + `element`, `modal`, an optional `backdrop` getter, and an optional `dismiss`. | | `isTopmostLayer(id)` | Whether the layer owns Escape and the focus trap right now. | | `layersBelow(id)` | The layers stacked beneath, topmost first — the unwinding order for a stack-scoped dismissal. | diff --git a/packages/dom/utils/overlay/src/index.ts b/packages/dom/utils/overlay/src/index.ts index 1753d05..d62237f 100644 --- a/packages/dom/utils/overlay/src/index.ts +++ b/packages/dom/utils/overlay/src/index.ts @@ -1,4 +1,5 @@ export { registerLayer, isTopmostLayer, layersBelow, type Layer } from './stack' +export { watchOutsidePress, type WatchOutsidePressOptions } from './watch-outside-press' export { getInitialFocus } from './get-initial-focus' export { watchExitAnimation } from './watch-exit-animation' export { hideExitingLayer } from './hide-exiting-layer' diff --git a/packages/dom/utils/overlay/src/watch-outside-press.ts b/packages/dom/utils/overlay/src/watch-outside-press.ts new file mode 100644 index 0000000..a7c1dae --- /dev/null +++ b/packages/dom/utils/overlay/src/watch-outside-press.ts @@ -0,0 +1,37 @@ +import { isTopmostLayer } from './stack' + +export interface WatchOutsidePressOptions { + /** The layer's element; a press landing inside it is never outside. */ + element: HTMLElement + /** The trigger that opened this layer, excepted so its own press stays a + * plain toggle — counting it as outside would close and immediately + * reopen. */ + trigger: Element | null + /** Whether the gesture that produced this click began inside `element` — + * see `trackPressOrigin` in `@dunky.dev/dom-press-origin`. A + * text-selection drag starting inside and releasing outside must not read + * as an outside press either. */ + startedInside: () => boolean + onOutsidePress: (event: MouseEvent) => void +} + +/** + * The outside-press watch for a layer whose own surfaces can't detect one — + * a non-modal layer has no backdrop, and a viewport that lets presses fall + * through to the page (`pointer-events: none`) never receives them itself. + * The document is the only vantage point left that can see both the layer + * (portaled out of the page's own subtree) and the page. + */ +export function watchOutsidePress(id: string, options: WatchOutsidePressOptions): () => void { + const onClick = (event: MouseEvent): void => { + if (!isTopmostLayer(id)) return + if (options.startedInside()) return + const target = event.target + if (!(target instanceof Node)) return + if (options.element.contains(target)) return + if (options.trigger?.contains(target) === true) return + options.onOutsidePress(event) + } + document.addEventListener('click', onClick, true) + return () => document.removeEventListener('click', onClick, true) +} diff --git a/packages/dom/utils/overlay/tests/watch-outside-press.test.ts b/packages/dom/utils/overlay/tests/watch-outside-press.test.ts new file mode 100644 index 0000000..8cc1e75 --- /dev/null +++ b/packages/dom/utils/overlay/tests/watch-outside-press.test.ts @@ -0,0 +1,100 @@ +// @vitest-environment jsdom +import { afterEach, describe, expect, it, vi } from 'vitest' +import { registerLayer, watchOutsidePress } from '@dunky.dev/dom-overlay' + +const registered: Array<() => void> = [] + +const mountLayer = (id: string, depth: number): HTMLElement => { + const element = document.createElement('div') + document.body.append(element) + registered.push(registerLayer({ id, depth, element, modal: false, backdrop: () => null })) + return element +} + +const click = (target: Element): void => { + target.dispatchEvent(new MouseEvent('click', { bubbles: true })) +} + +afterEach(() => { + for (const undo of registered) undo() + registered.length = 0 + document.body.innerHTML = '' +}) + +describe('watchOutsidePress', () => { + const setup = ( + startedInside: () => boolean = () => false, + ): { + element: HTMLElement + trigger: HTMLElement + outside: HTMLElement + onOutsidePress: () => void + } => { + const element = mountLayer('layer', 1) + const trigger = document.createElement('button') + const outside = document.createElement('button') + document.body.append(trigger, outside) + const onOutsidePress = vi.fn() + registered.push(watchOutsidePress('layer', { element, trigger, startedInside, onOutsidePress })) + return { element, trigger, outside, onOutsidePress } + } + + it('fires for a press outside the layer and the trigger', () => { + const { outside, onOutsidePress } = setup() + + click(outside) + + expect(onOutsidePress).toHaveBeenCalledTimes(1) + }) + + it('ignores a press inside the layer', () => { + const { element, onOutsidePress } = setup() + + click(element) + + expect(onOutsidePress).not.toHaveBeenCalled() + }) + + it('ignores a press on the trigger — its own press stays a plain toggle', () => { + const { trigger, onOutsidePress } = setup() + + click(trigger) + + expect(onOutsidePress).not.toHaveBeenCalled() + }) + + it('ignores a press once this layer is no longer topmost', () => { + const { outside, onOutsidePress } = setup() + mountLayer('above', 2) + + click(outside) + + expect(onOutsidePress).not.toHaveBeenCalled() + }) + + it('ignores a press whose gesture started inside the layer', () => { + const { outside, onOutsidePress } = setup(() => true) + + click(outside) + + expect(onOutsidePress).not.toHaveBeenCalled() + }) + + it('stops watching once disposed', () => { + const element = mountLayer('layer', 1) + const outside = document.createElement('button') + document.body.append(outside) + const onOutsidePress = vi.fn() + + const dispose = watchOutsidePress('layer', { + element, + trigger: null, + startedInside: () => false, + onOutsidePress, + }) + dispose() + click(outside) + + expect(onOutsidePress).not.toHaveBeenCalled() + }) +}) diff --git a/packages/dom/utils/press-origin/README.md b/packages/dom/utils/press-origin/README.md new file mode 100644 index 0000000..ec68f01 --- /dev/null +++ b/packages/dom/utils/press-origin/README.md @@ -0,0 +1,34 @@ +# @dunky.dev/dom-press-origin + +Framework-free tracking of where a pointer press began — kept past the +browser's click-target collapse. + +A mousedown inside an element and a mouseup outside it fire `click` on their +common ancestor, not where the press started. Any "click outside dismisses" +surface misreads a text-selection drag because of that collapse; this util +captures the origin at `pointerdown`, the last moment it is still observable. + +## Install + +```sh +npm install @dunky.dev/dom-press-origin +``` + +## Usage + +```ts +import { trackPressOrigin } from '@dunky.dev/dom-press-origin' + +const tracker = trackPressOrigin(panel) + +document.addEventListener('click', event => { + // A drag that began inside the panel is not an outside press, + // wherever the click's own target ended up. + if (tracker.startedInside()) return + if (!(event.target instanceof Node) || panel.contains(event.target)) return + dismiss() +}) + +// later +tracker.dispose() +``` diff --git a/packages/dom/utils/press-origin/SPEC.md b/packages/dom/utils/press-origin/SPEC.md new file mode 100644 index 0000000..0259f77 --- /dev/null +++ b/packages/dom/utils/press-origin/SPEC.md @@ -0,0 +1,35 @@ +# SPEC / DOM / Press origin + +## Overview + +Framework-free tracking of where a pointer press began. The browser answers +"where did this press _end_" for free; where it _began_ is gone by the time +`click` fires — a mousedown in one place and a mouseup in another dispatch +`click` on their common ancestor, and nothing on the event says the gesture +started somewhere else. Any surface that treats a click as an "outside press" +misreads a text-selection drag because of exactly that collapse; this package +recovers the origin by capturing it at `pointerdown`, the last moment it is +still observable. + +## Behavior + +- `trackPressOrigin(element)` listens for `pointerdown` on the document + (capture phase) and remembers whether the press began inside `element`. +- `startedInside()` answers for the most recent press — it is a reading of + the latest gesture, not a log. +- Before any press is observed, the answer is `false`. +- `dispose()` detaches the listener; the last answer freezes. + +## API + +| Export | Description | +| --------------------------- | ----------------------------------------------------------------- | +| `trackPressOrigin(element)` | Starts tracking; returns `{ startedInside, dispose }`. | +| `PressOriginTracker` | The returned pair: `startedInside(): boolean`, `dispose(): void`. | + +## Constraints + +- No framework import, and nothing from this repo — the util sits at the + bottom of the DOM layer. +- One tracker per tracked element; consumers that need the answer in several + places share the `startedInside` reference rather than tracking twice. diff --git a/packages/dom/utils/press-origin/package.json b/packages/dom/utils/press-origin/package.json new file mode 100644 index 0000000..3ee035a --- /dev/null +++ b/packages/dom/utils/press-origin/package.json @@ -0,0 +1,38 @@ +{ + "name": "@dunky.dev/dom-press-origin", + "version": "0.0.0", + "description": "Framework-free press-origin tracking — where a pointer press began, kept past the click-target collapse.", + "license": "MIT", + "repository": { + "type": "git", + "url": "git+https://github.com/dunky-dev/ui.git", + "directory": "packages/dom/utils/press-origin" + }, + "files": [ + "dist", + "src", + "SPEC.md" + ], + "type": "module", + "sideEffects": false, + "main": "./src/index.ts", + "types": "./src/index.ts", + "exports": { + ".": "./src/index.ts" + }, + "publishConfig": { + "main": "./dist/index.js", + "module": "./dist/index.js", + "types": "./dist/index.d.ts", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.js" + } + }, + "access": "public" + }, + "scripts": { + "build": "tsdown" + } +} diff --git a/packages/dom/utils/press-origin/src/index.ts b/packages/dom/utils/press-origin/src/index.ts new file mode 100644 index 0000000..ef8f89c --- /dev/null +++ b/packages/dom/utils/press-origin/src/index.ts @@ -0,0 +1 @@ +export { trackPressOrigin, type PressOriginTracker } from './track-press-origin' diff --git a/packages/dom/utils/press-origin/src/track-press-origin.ts b/packages/dom/utils/press-origin/src/track-press-origin.ts new file mode 100644 index 0000000..d3e56d0 --- /dev/null +++ b/packages/dom/utils/press-origin/src/track-press-origin.ts @@ -0,0 +1,28 @@ +export interface PressOriginTracker { + /** Whether the most recently observed press began inside the tracked + * element. */ + startedInside: () => boolean + /** Stops tracking. */ + dispose: () => void +} + +/** + * Tracks whether the most recent pointer press began inside `element`. A + * click's own target can't answer that once the browser has collapsed it: a + * mousedown inside the element and a mouseup outside it fires `click` on + * their common ancestor, not on where the press actually started — a + * text-selection drag then reads as a fresh press on whatever sits above + * that ancestor. Capturing at pointerdown, before the collapse, is the only + * way to recover it. + */ +export function trackPressOrigin(element: HTMLElement): PressOriginTracker { + let insideElement = false + const onPointerDown = (event: PointerEvent): void => { + insideElement = event.target instanceof Node && element.contains(event.target) + } + document.addEventListener('pointerdown', onPointerDown, true) + return { + startedInside: () => insideElement, + dispose: () => document.removeEventListener('pointerdown', onPointerDown, true), + } +} diff --git a/packages/dom/utils/press-origin/tests/track-press-origin.test.ts b/packages/dom/utils/press-origin/tests/track-press-origin.test.ts new file mode 100644 index 0000000..84829e7 --- /dev/null +++ b/packages/dom/utils/press-origin/tests/track-press-origin.test.ts @@ -0,0 +1,45 @@ +// @vitest-environment jsdom +import { afterEach, describe, expect, it } from 'vitest' +import { trackPressOrigin } from '@dunky.dev/dom-press-origin' + +const dispatchPointerDown = (target: Element): void => { + target.dispatchEvent(new Event('pointerdown', { bubbles: true })) +} + +afterEach(() => { + document.body.innerHTML = '' +}) + +describe('trackPressOrigin', () => { + it('reports whether the most recent press began inside the tracked element', () => { + const element = document.createElement('div') + const inner = document.createElement('button') + element.append(inner) + const outer = document.createElement('button') + document.body.append(element, outer) + + const tracker = trackPressOrigin(element) + expect(tracker.startedInside()).toBe(false) + + dispatchPointerDown(inner) + expect(tracker.startedInside()).toBe(true) + + dispatchPointerDown(outer) + expect(tracker.startedInside()).toBe(false) + + tracker.dispose() + }) + + it('stops updating once disposed', () => { + const element = document.createElement('div') + const inner = document.createElement('button') + element.append(inner) + document.body.append(element) + + const tracker = trackPressOrigin(element) + tracker.dispose() + dispatchPointerDown(inner) + + expect(tracker.startedInside()).toBe(false) + }) +}) diff --git a/packages/react/dialog/SPEC.md b/packages/react/dialog/SPEC.md index 1b39064..07515d7 100644 --- a/packages/react/dialog/SPEC.md +++ b/packages/react/dialog/SPEC.md @@ -102,7 +102,7 @@ The root: owns open/close state, renders no DOM. Accepts the core | `role` | `'dialog' \| 'alertdialog'` | `'dialog'` | The ARIA pattern. | | `closeOnEscape` | `boolean` | `true` | Whether Escape closes the dialog. | | `escapeScope` | `'layer' \| 'stack'` | `'layer'` | How far an allowed Escape reaches: this dialog, or its whole stack. | -| `closeOnInteractOutside` | `boolean` | `true` — `false` for `role="alertdialog"` | Whether pressing the backdrop/viewport closes the dialog. | +| `closeOnInteractOutside` | `boolean` | `true` — `false` for `role="alertdialog"` | Whether a press outside the dialog window closes it. | | `animated` | `boolean` | `false` | Keeps the dialog mounted through `data-state="closing"` while its exit animation plays. | | `closeOnBack` | `boolean` | `false` | The browser's Back closes the open dialog instead of navigating (a guard entry in the session history), and Forward reopens what Back closed. | | `onBackNavigation` | `(event?) => void` | — | Fired before a back-navigation dismissal; `preventDefault()` vetoes. | @@ -139,7 +139,11 @@ The layer behind the dialog window; renders nothing when `modal={false}`. ### `Dialog.Viewport` -The positioning + scroll layer around the dialog window. +The positioning + scroll layer around the dialog window. While non-modal it +renders with `pointer-events: none` (the consumer's own `style` wins), so a +press on the empty area falls through to the page instead of being +swallowed; outside presses are then observed at the document level. While +modal it stays the outside-press surface itself. | Prop | Type | Default | Description | | ---------- | ----------------------- | ------- | ---------------------------------- | @@ -147,7 +151,9 @@ The positioning + scroll layer around the dialog window. ### `Dialog.Content` -The dialog window; renders a `
` with the `dialog` role. +The dialog window; renders a `
` with the `dialog` role and +`pointer-events: auto` (the consumer's own `style` wins), so it stays +interactive where a non-modal Viewport lets presses fall through around it. | Prop | Type | Default | Description | | -------------- | -------------------------------- | ----------------- | ------------------------------------------- | diff --git a/packages/react/dialog/package.json b/packages/react/dialog/package.json index 2d9d979..5fdd9e7 100644 --- a/packages/react/dialog/package.json +++ b/packages/react/dialog/package.json @@ -38,6 +38,8 @@ "dependencies": { "@dunky.dev/dialog": "workspace:*", "@dunky.dev/dom-dialog": "workspace:*", + "@dunky.dev/dom-overlay": "workspace:*", + "@dunky.dev/dom-press-origin": "workspace:*", "@dunky.dev/react-state-machine": "^0.3.4", "@dunky.dev/react-use-focus-trap": "workspace:*", "@dunky.dev/react-use-scroll-lock": "workspace:*" diff --git a/packages/react/dialog/src/context.ts b/packages/react/dialog/src/context.ts index 149635a..e21a77c 100644 --- a/packages/react/dialog/src/context.ts +++ b/packages/react/dialog/src/context.ts @@ -15,6 +15,18 @@ export interface DialogContextValue { // sibling parts: Content's stack entry excepts its own backdrop from the // containment so it stays pressable while its dialog is topmost. backdropRef: RefObject + // The rendered Content element, shared so Viewport's non-modal outside- + // press watcher (a document-level listener, since a `pointer-events: none` + // Viewport never receives those presses itself) knows what "inside" means. + contentRef: RefObject + // The rendered Trigger element, shared for the same watcher: its own press + // stays a plain toggle — counting it as outside would close and + // immediately reopen. + triggerRef: RefObject + // Whether the most recent press began inside Content — see + // `trackPressOrigin`. Read by Backdrop and Viewport to refuse a + // text-selection drag that starts inside and releases outside. + pressOriginRef: RefObject<(() => boolean) | null> } export const DialogContext: Context = createContext< diff --git a/packages/react/dialog/src/dialog.tsx b/packages/react/dialog/src/dialog.tsx index 18d1136..26b2760 100644 --- a/packages/react/dialog/src/dialog.tsx +++ b/packages/react/dialog/src/dialog.tsx @@ -16,13 +16,17 @@ import { useFocusTrap } from '@dunky.dev/react-use-focus-trap' import { useScrollLock } from '@dunky.dev/react-use-scroll-lock' import type { DialogOptions } from '@dunky.dev/dialog' +import { watchOutsidePress } from '@dunky.dev/dom-overlay' +import { trackPressOrigin } from '@dunky.dev/dom-press-origin' import { acceptsBackdropPress, acceptsViewportPress, + contentPointerEvents, dialogTrapOptions, guardBackNavigation, openDialogLayer, startExitWindow, + viewportPointerEvents, type BackNavigationGuard, } from '@dunky.dev/dom-dialog' import { mergeProps, normalize } from '@dunky.dev/react-state-machine' @@ -46,6 +50,9 @@ export const Dialog: ((props: DialogProps) => ReactNode) & Parts = ({ children, const depth = (useContext(DialogContext)?.depth ?? 0) + 1 const { api, machine } = useDialog(options) const backdropRef = useRef(null) + const contentRef = useRef(null) + const triggerRef = useRef(null) + const pressOriginRef = useRef<(() => boolean) | null>(null) // Read through a ref so the history guard's lifecycle follows the open // state alone: re-arming on every render (fresh callback identities) would @@ -79,7 +86,18 @@ export const Dialog: ((props: DialogProps) => ReactNode) & Parts = ({ children, ) return ( - + {children} ) @@ -95,12 +113,13 @@ export const Trigger: PartComponent = for HTMLButtonElement, DialogTriggerProps >((props, forwardedRef) => { - const { api } = useDialogContext() + const { api, triggerRef } = useDialogContext() + useImperativeHandle(forwardedRef, () => triggerRef.current as HTMLButtonElement) const merged = mergeProps( { type: 'button', ...props }, normalize(api.parts.trigger), ) - return + + Trigger + + + + + + + + + + ) + + // jsdom does no hit-testing, so the blocked-page-click bug itself can't + // be reproduced — assert the mechanism that prevents it: the empty + // viewport area is transparent to pointer events, and the window stays + // reachable regardless. + it('lets pointer events fall through the empty viewport area', () => { + render() + expect(screen.getByTestId('viewport').style.pointerEvents).toBe('none') + expect(screen.getByRole('dialog').style.pointerEvents).toBe('auto') + }) + + it('keeps the viewport itself pressable when modal — it is the outside-press surface there', () => { + render() + expect(screen.getByTestId('viewport').style.pointerEvents).toBe('') + }) + + // With the viewport transparent to pointer events, it never receives a + // press on the empty area to detect as outside — a document-level watch + // is the substitute. + it('dismisses on a press elsewhere in the document', () => { + render() + act(() => screen.getByText('Page button').click()) + expect(screen.queryByRole('dialog')).toBeNull() + }) + + it('does not dismiss on a press inside the content', () => { + render() + act(() => screen.getByText('Action').click()) + expect(screen.queryByRole('dialog')).not.toBeNull() + }) + + it('does not double-fire on the trigger’s own press — it stays a plain toggle', () => { + const onOpenChange = vi.fn() + render( + + Trigger + + + + + + , + ) + act(() => screen.getByText('Trigger').click()) + expect(onOpenChange).toHaveBeenCalledTimes(1) + expect(screen.queryByRole('dialog')).toBeNull() + }) + + it('does not dismiss on a drag that starts inside the content and releases in the document', () => { + render() + fireEvent.pointerDown(screen.getByText('Action')) + act(() => screen.getByText('Page button').click()) + expect(screen.queryByRole('dialog')).not.toBeNull() + }) }) describe('controlled open', () => { @@ -290,6 +389,26 @@ describe('Dialog', () => { expect(dialog.hasAttribute('aria-describedby')).toBe(false) }) + it('warns when the dialog resolves no accessible name', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + render( + + + content + + , + ) + await act(() => new Promise(resolve => setTimeout(resolve, 0))) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('no accessible name')) + }) + + it('does not warn when a Title is rendered', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + render() + await act(() => new Promise(resolve => setTimeout(resolve, 0))) + expect(warn).not.toHaveBeenCalledWith(expect.stringContaining('no accessible name')) + }) + it('renders role=alertdialog when requested', () => { render() expect(screen.queryByRole('alertdialog')).not.toBeNull() diff --git a/packages/solid/dialog/SPEC.md b/packages/solid/dialog/SPEC.md index d9f9200..5e38f0b 100644 --- a/packages/solid/dialog/SPEC.md +++ b/packages/solid/dialog/SPEC.md @@ -107,7 +107,7 @@ The root: owns open/close state, renders no DOM. Accepts the core | `role` | `'dialog' \| 'alertdialog'` | `'dialog'` | The ARIA pattern. | | `closeOnEscape` | `boolean` | `true` | Whether Escape closes the dialog. | | `escapeScope` | `'layer' \| 'stack'` | `'layer'` | How far an allowed Escape reaches: this dialog, or its whole stack. | -| `closeOnInteractOutside` | `boolean` | `true` — `false` for `role="alertdialog"` | Whether pressing the backdrop/viewport closes the dialog. | +| `closeOnInteractOutside` | `boolean` | `true` — `false` for `role="alertdialog"` | Whether a press outside the dialog window closes it. | | `animated` | `boolean` | `false` | Keeps the dialog mounted through `data-state="closing"` while its exit animation plays. | | `closeOnBack` | `boolean` | `false` | The browser's Back closes the open dialog instead of navigating (a guard entry in the session history), and Forward reopens what Back closed. | | `onBackNavigation` | `(event?) => void` | — | Fired before a back-navigation dismissal; `preventDefault()` vetoes. | @@ -144,7 +144,11 @@ The layer behind the dialog window; renders nothing when `modal={false}`. ### `Dialog.Viewport` -The positioning + scroll layer around the dialog window. +The positioning + scroll layer around the dialog window. While non-modal it +renders with `pointer-events: none` (the consumer's own `style` wins), so a +press on the empty area falls through to the page instead of being +swallowed; outside presses are then observed at the document level. While +modal it stays the outside-press surface itself. | Prop | Type | Default | Description | | ---------- | ----------------------- | ------- | ---------------------------------- | @@ -152,7 +156,9 @@ The positioning + scroll layer around the dialog window. ### `Dialog.Content` -The dialog window; renders a `
` with the `dialog` role. +The dialog window; renders a `
` with the `dialog` role and +`pointer-events: auto` (the consumer's own `style` wins), so it stays +interactive where a non-modal Viewport lets presses fall through around it. | Prop | Type | Default | Description | | -------------- | --------------------------------------------------------- | ----------------- | ------------------------------------------------------------------- | diff --git a/packages/solid/dialog/package.json b/packages/solid/dialog/package.json index c885d16..6b50e18 100644 --- a/packages/solid/dialog/package.json +++ b/packages/solid/dialog/package.json @@ -38,6 +38,8 @@ "dependencies": { "@dunky.dev/dialog": "workspace:*", "@dunky.dev/dom-dialog": "workspace:*", + "@dunky.dev/dom-overlay": "workspace:*", + "@dunky.dev/dom-press-origin": "workspace:*", "@dunky.dev/solid-state-machine": "^0.3.0", "@dunky.dev/solid-use-focus-trap": "workspace:*", "@dunky.dev/solid-use-scroll-lock": "workspace:*" diff --git a/packages/solid/dialog/src/context.ts b/packages/solid/dialog/src/context.ts index 2d1ebb5..3ce019a 100644 --- a/packages/solid/dialog/src/context.ts +++ b/packages/solid/dialog/src/context.ts @@ -13,6 +13,17 @@ export interface DialogContextValue { // the containment. A plain box, not a signal: the layer walk reads it in the // same settle that mounts the backdrop, before a signal write would commit. backdropRef: { current: HTMLDivElement | null } + // The rendered Content, shared so Viewport's non-modal outside-press + // watcher (a document-level listener, since a `pointer-events: none` + // Viewport never receives those presses itself) knows what "inside" means. + contentRef: { current: HTMLDivElement | null } + // The rendered Trigger, shared for the same watcher: its own press stays a + // plain toggle — counting it as outside would close and immediately reopen. + triggerRef: { current: HTMLButtonElement | null } + // Whether the most recent press began inside Content — see + // `trackPressOrigin`. Read by Backdrop and Viewport to refuse a + // text-selection drag that starts inside and releases outside. + pressOriginRef: { current: (() => boolean) | null } } // A `null` default: a default-less context throws on the root's optional diff --git a/packages/solid/dialog/src/dialog.tsx b/packages/solid/dialog/src/dialog.tsx index c6c982b..7a85175 100644 --- a/packages/solid/dialog/src/dialog.tsx +++ b/packages/solid/dialog/src/dialog.tsx @@ -14,13 +14,17 @@ import { useFocusTrap } from '@dunky.dev/solid-use-focus-trap' import { useScrollLock } from '@dunky.dev/solid-use-scroll-lock' import type { DialogOptions } from '@dunky.dev/dialog' +import { watchOutsidePress } from '@dunky.dev/dom-overlay' +import { trackPressOrigin } from '@dunky.dev/dom-press-origin' import { acceptsBackdropPress, acceptsViewportPress, + contentPointerEvents, dialogTrapOptions, guardBackNavigation, openDialogLayer, startExitWindow, + viewportPointerEvents, type BackNavigationGuard, } from '@dunky.dev/dom-dialog' import { mergeProps, normalize } from '@dunky.dev/solid-state-machine' @@ -53,6 +57,9 @@ export const Dialog: Component & Parts = props => { const depth = (useContext(DialogContext)?.depth ?? 0) + 1 const { api, machine } = useDialog(options) const backdropRef: { current: HTMLDivElement | null } = { current: null } + const contentRef: { current: HTMLDivElement | null } = { current: null } + const triggerRef: { current: HTMLButtonElement | null } = { current: null } + const pressOriginRef: { current: (() => boolean) | null } = { current: null } // The guard lives on the root — it concerns the dialog's openness, not any // rendered part. It spans more than the open state, so it can't be this @@ -80,7 +87,18 @@ export const Dialog: Component & Parts = props => { }) return ( - null, backdropRef }}> + null, + backdropRef, + contentRef, + triggerRef, + pressOriginRef, + }} + > {props.children} ) @@ -93,11 +111,16 @@ export const Dialog: Component & Parts = props => { export interface DialogTriggerProps extends ComponentProps<'button'> {} export const Trigger: Component = props => { - const { api } = useDialogContext() - const rest = omit(props, 'children') + const { api, triggerRef } = useDialogContext() + const rest = omit(props, 'ref', 'children') + onSettled(() => () => (triggerRef.current = null)) return ( @@ -145,7 +168,7 @@ export const Portal: Component = props => { export interface DialogBackdropProps extends ComponentProps<'div'> {} export const Backdrop: Component = props => { - const { api, machine, backdropRef } = useDialogContext() + const { api, machine, backdropRef, pressOriginRef } = useDialogContext() const rest = omit(props, 'ref', 'children') onSettled(() => () => (backdropRef.current = null)) @@ -156,7 +179,8 @@ export const Backdrop: Component = props => { return { ...attrs, onClick: (event: MouseEvent) => { - if (acceptsBackdropPress(machine.context.id)) onClick?.(event) + if (acceptsBackdropPress(machine.context.id, pressOriginRef.current?.() ?? false)) + onClick?.(event) }, } } @@ -184,8 +208,8 @@ export const Backdrop: Component = props => { export interface DialogViewportProps extends ComponentProps<'div'> {} export const Viewport: Component = props => { - const { api, machine } = useDialogContext() - const rest = omit(props, 'children') + const { api, machine, contentRef, triggerRef, pressOriginRef } = useDialogContext() + const rest = omit(props, 'style', 'children') const bindings = (): Record => { const { onClick, ...attrs } = normalize(api.parts.viewport) as { @@ -194,12 +218,47 @@ export const Viewport: Component = props => { return { ...attrs, onClick: (event: MouseEvent) => { - if (acceptsViewportPress(machine.context.id, event)) onClick?.(event) + if (acceptsViewportPress(machine.context.id, event, pressOriginRef.current?.() ?? false)) + onClick?.(event) }, } } - return
(rest, bindings())}>{props.children}
+ // Non-modal: the Viewport's own pointer-events are off (see the style + // below), so it never receives a press on the empty area to detect as + // outside — the document is the only vantage point left. + createEffect( + () => api.open, + open => { + if (!open || machine.context.modal) return + const content = contentRef.current + if (content === null) return + return watchOutsidePress(machine.context.id, { + element: content, + trigger: triggerRef.current, + startedInside: () => pressOriginRef.current?.() ?? false, + onOutsidePress: event => + untrack(() => { + const { onClick } = normalize(api.parts.viewport) as { + onClick?: (event: MouseEvent) => void + } + onClick?.(event) + }), + }) + }, + ) + + return ( +
(rest, bindings())} + // Non-modal: the page coexists with the dialog, so the empty area + // around the window must let presses fall through rather than swallow + // them. + style={withPointerEvents(props.style, viewportPointerEvents(machine.context.modal))} + > + {props.children} +
+ ) } // ============================================================================= @@ -216,10 +275,23 @@ export interface DialogContentProps extends ComponentProps<'div'> { const resolveInitialFocus = (value: DialogContentProps['initialFocus']): HTMLElement | null => (typeof value === 'function' ? value() : value) ?? null +// The consumer's own style wins over the pointer-events default; a string +// style keeps the default as the earlier (overridable) declaration. +const withPointerEvents = ( + style: ComponentProps<'div'>['style'], + value: 'none' | 'auto' | undefined, +): ComponentProps<'div'>['style'] => { + if (value === undefined) return style + if (typeof style === 'string') return `pointer-events:${value};${style}` + if (typeof style === 'object' && style !== null) return { 'pointer-events': value, ...style } + return { 'pointer-events': value } +} + export const Content: Component = props => { - const { api, machine, depth, container, backdropRef } = useDialogContext() - const rest = omit(props, 'ref', 'initialFocus', 'children') - let contentEl: HTMLDivElement | undefined + const { api, machine, depth, container, backdropRef, contentRef, pressOriginRef } = + useDialogContext() + const rest = omit(props, 'ref', 'initialFocus', 'style', 'children') + onSettled(() => () => (contentRef.current = null)) // The `open` state is the edge, not mount/unmount: an animated dialog stays // mounted through `closing`. The sequence and its inverse are the DOM @@ -227,10 +299,10 @@ export const Content: Component = props => { createEffect( () => api.open, open => { - const content = contentEl - if (!open || content === undefined) return + const content = contentRef.current + if (!open || content === null) return - return openDialogLayer(content, { + const releaseLayer = openDialogLayer(content, { id: machine.context.id, depth, modal: machine.context.modal, @@ -238,6 +310,17 @@ export const Content: Component = props => { initialFocus: untrack(() => resolveInitialFocus(props.initialFocus)), dismiss: () => machine.send({ type: 'close' }), }) + // A click's own target can't tell a text-selection drag from a genuine + // outside press once the browser has collapsed it — see + // `trackPressOrigin`. Shared via context: Backdrop and Viewport are + // separate parts, and both need the answer. + const pressOrigin = trackPressOrigin(content) + pressOriginRef.current = pressOrigin.startedInside + return () => { + pressOriginRef.current = null + pressOrigin.dispose() + releaseLayer() + } }, ) @@ -245,8 +328,8 @@ export const Content: Component = props => { createEffect( () => api.open, open => { - const content = contentEl - if (open || content === undefined) return + const content = contentRef.current + if (open || content === null) return return untrack(() => startExitWindow(content, { @@ -268,7 +351,7 @@ export const Content: Component = props => { ) useFocusTrap( - () => contentEl ?? null, + () => contentRef.current, dialogTrapOptions(machine, () => api.ids.close), ) @@ -278,8 +361,11 @@ export const Content: Component = props => { return (
(rest, normalize(api.parts.content))} + // Always interactive, even where a non-modal Viewport disables pointer + // events to let presses fall through around it. + style={withPointerEvents(props.style, contentPointerEvents)} ref={element => { - contentEl = element + contentRef.current = element applyConsumerRef(props.ref, element) }} > diff --git a/packages/solid/dialog/stories/dialog.stories.tsx b/packages/solid/dialog/stories/dialog.stories.tsx index a1c71ba..d39b85d 100644 --- a/packages/solid/dialog/stories/dialog.stories.tsx +++ b/packages/solid/dialog/stories/dialog.stories.tsx @@ -594,7 +594,8 @@ const ContainedPage = () => { A non-modal layer above the dialog, held out of the containment while its - neighbor article stays in it. Escape closes this layer first. + neighbor article stays in it. Escape — or a press outside it — closes this + layer first. diff --git a/packages/solid/dialog/tests/dialog.test.tsx b/packages/solid/dialog/tests/dialog.test.tsx index 5d0a7f3..ff0c50d 100644 --- a/packages/solid/dialog/tests/dialog.test.tsx +++ b/packages/solid/dialog/tests/dialog.test.tsx @@ -41,6 +41,9 @@ const pressEscape = (): void => { // Auto-cleanup needs vitest globals; this repo runs with globals: false. afterEach(cleanup) +// Spies on globals (console.warn) accumulate across tests otherwise — +// vi.spyOn on an already-spied method returns the same spy, history intact. +afterEach(() => vi.restoreAllMocks()) describe('Dialog', () => { describe('open / close', () => { @@ -150,6 +153,102 @@ describe('Dialog', () => { render(() => ) expect(screen.queryByTestId('backdrop')).toBeNull() }) + + // A text-selection drag starting inside the content and releasing on the + // viewport/backdrop collapses the click's target to whichever it lands + // on — indistinguishable from a genuine outside press by target alone. + it('does not close on a drag that starts inside the content and releases on the viewport', () => { + render(() => ) + fireEvent.pointerDown(screen.getByText('Action')) + press(screen.getByTestId('viewport')) + expect(screen.queryByRole('dialog')).not.toBeNull() + }) + + it('does not close on a drag that starts inside the content and releases on the backdrop', () => { + render(() => ) + fireEvent.pointerDown(screen.getByText('Action')) + press(screen.getByTestId('backdrop')) + expect(screen.queryByRole('dialog')).not.toBeNull() + }) + + it('still closes on a press that starts and ends on the viewport — not a drag', () => { + render(() => ) + fireEvent.pointerDown(screen.getByTestId('viewport')) + press(screen.getByTestId('viewport')) + expect(screen.queryByRole('dialog')).toBeNull() + }) + }) + + describe('non-modal outside interaction', () => { + const NonModalDialog = () => ( + <> + + + Trigger + + + + + + + + + + ) + + // jsdom does no hit-testing, so the blocked-page-click bug itself can't + // be reproduced — assert the mechanism that prevents it: the empty + // viewport area is transparent to pointer events, and the window stays + // reachable regardless. + it('lets pointer events fall through the empty viewport area', () => { + render(() => ) + expect(screen.getByTestId('viewport').style.pointerEvents).toBe('none') + expect(screen.getByRole('dialog').style.pointerEvents).toBe('auto') + }) + + it('keeps the viewport itself pressable when modal — it is the outside-press surface there', () => { + render(() => ) + expect(screen.getByTestId('viewport').style.pointerEvents).toBe('') + }) + + // With the viewport transparent to pointer events, it never receives a + // press on the empty area to detect as outside — a document-level watch + // is the substitute. + it('dismisses on a press elsewhere in the document', () => { + render(() => ) + press(screen.getByText('Page button')) + expect(screen.queryByRole('dialog')).toBeNull() + }) + + it('does not dismiss on a press inside the content', () => { + render(() => ) + press(screen.getByText('Action')) + expect(screen.queryByRole('dialog')).not.toBeNull() + }) + + it('does not double-fire on the trigger’s own press — it stays a plain toggle', () => { + const onOpenChange = vi.fn() + render(() => ( + + Trigger + + + + + + + )) + press(screen.getByText('Trigger')) + expect(onOpenChange).toHaveBeenCalledTimes(1) + expect(screen.queryByRole('dialog')).toBeNull() + }) + + it('does not dismiss on a drag that starts inside the content and releases in the document', () => { + render(() => ) + fireEvent.pointerDown(screen.getByText('Action')) + press(screen.getByText('Page button')) + expect(screen.queryByRole('dialog')).not.toBeNull() + }) }) describe('controlled open', () => { @@ -305,6 +404,28 @@ describe('Dialog', () => { expect(dialog.hasAttribute('aria-describedby')).toBe(false) }) + it('warns when the dialog resolves no accessible name', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + render(() => ( + + + content + + + )) + flush() + await new Promise(resolve => setTimeout(resolve, 0)) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('no accessible name')) + }) + + it('does not warn when a Title is rendered', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + render(() => ) + flush() + await new Promise(resolve => setTimeout(resolve, 0)) + expect(warn).not.toHaveBeenCalledWith(expect.stringContaining('no accessible name')) + }) + it('renders role=alertdialog when requested', () => { render(() => ) expect(screen.queryByRole('alertdialog')).not.toBeNull() diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2022777..24315c5 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -106,6 +106,8 @@ importers: specifier: workspace:* version: link:../../../core/utils/overlay + packages/dom/utils/press-origin: {} + packages/dom/utils/scroll-lock: {} packages/native: @@ -220,6 +222,12 @@ importers: '@dunky.dev/dom-dialog': specifier: workspace:* version: link:../../dom/components/dialog + '@dunky.dev/dom-overlay': + specifier: workspace:* + version: link:../../dom/utils/overlay + '@dunky.dev/dom-press-origin': + specifier: workspace:* + version: link:../../dom/utils/press-origin '@dunky.dev/react-state-machine': specifier: ^0.3.4 version: 0.3.4(react@19.2.7) @@ -322,6 +330,12 @@ importers: '@dunky.dev/dom-dialog': specifier: workspace:* version: link:../../dom/components/dialog + '@dunky.dev/dom-overlay': + specifier: workspace:* + version: link:../../dom/utils/overlay + '@dunky.dev/dom-press-origin': + specifier: workspace:* + version: link:../../dom/utils/press-origin '@dunky.dev/solid-state-machine': specifier: ^0.3.0 version: 0.3.0(solid-js@2.0.0-rc.1) diff --git a/tsconfig.json b/tsconfig.json index 4e39ec3..d0572cb 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -24,6 +24,7 @@ "@dunky.dev/dom-overlay": ["./packages/dom/utils/overlay/src"], "@dunky.dev/dom-focus-trap": ["./packages/dom/utils/focus-trap/src"], "@dunky.dev/dom-element": ["./packages/dom/utils/element/src"], + "@dunky.dev/dom-press-origin": ["./packages/dom/utils/press-origin/src"], "@dunky.dev/browser-navigation": ["./packages/dom/utils/navigation/src"], "@dunky.dev/dom-scroll-lock": ["./packages/dom/utils/scroll-lock/src"], "@dunky.dev/react-use-focus-trap": ["./packages/react/hooks/use-focus-trap/src"], diff --git a/tsdown.config.ts b/tsdown.config.ts index 2704eb7..38e3747 100644 --- a/tsdown.config.ts +++ b/tsdown.config.ts @@ -18,6 +18,7 @@ export default defineConfig({ 'packages/dom/utils/focus-trap', 'packages/dom/utils/overlay', 'packages/dom/utils/navigation', + 'packages/dom/utils/press-origin', 'packages/dom/utils/scroll-lock', 'packages/native/dialog', 'packages/react/dialog',