diff --git a/.changeset/dialog-restore-focus.md b/.changeset/dialog-restore-focus.md new file mode 100644 index 0000000..cc42aaa --- /dev/null +++ b/.changeset/dialog-restore-focus.md @@ -0,0 +1,27 @@ +--- +'@dunky.dev/dom-dialog': minor +'@dunky.dev/react-dialog': minor +'@dunky.dev/solid-dialog': minor +--- + +`Dialog.Content` gains `restoreFocus` — the close-side counterpart to +`initialFocus`. Closing still returns focus to whatever held it before the +dialog opened; `restoreFocus` names where it goes when that holder can't +meaningfully take focus back: focus sat on the body (a pointer press can +leave it there), or on an element removed from the document since. +Typically the dialog's trigger. + +```tsx +// React — a ref, read at close time +… + +// Solid — an element or accessor, resolved at close time + trigger}>… +``` + +Before, those two cases silently dropped focus: restoring to the body goes +nowhere, and focusing a disconnected element is a no-op, leaving focus +stranded on the closing layer. The element focused before opening still +always wins when it is meaningful — the fallback never overrides it. At the +DOM layer, `openDialogLayer` takes `restoreFocus?: () => HTMLElement | null`, +resolved at close. diff --git a/packages/dom/components/dialog/SPEC.md b/packages/dom/components/dialog/SPEC.md index a2321b1..89c39f4 100644 --- a/packages/dom/components/dialog/SPEC.md +++ b/packages/dom/components/dialog/SPEC.md @@ -73,6 +73,12 @@ 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. +The restore prefers what step 1 remembered — but only when that element can +meaningfully take focus back. Focus that sat on the body (a pointer press +can leave it there) or on an element since removed from the document +restores to the consumer's `restoreFocus` target instead, typically the +layer's trigger; with neither, focus stays where it is. + Every focus move passes `preventScroll` — the scroll lock has already frozen the surface, so scrolling it would jump the view on open and again on close. diff --git a/packages/dom/components/dialog/src/open-layer.ts b/packages/dom/components/dialog/src/open-layer.ts index eba0860..8028e48 100644 --- a/packages/dom/components/dialog/src/open-layer.ts +++ b/packages/dom/components/dialog/src/open-layer.ts @@ -12,6 +12,11 @@ export interface OpenDialogLayerOptions { /** Closes this dialog when a layer above unwinds the whole stack; see * `Layer.dismiss` in dom-overlay. */ dismiss?: () => void + /** Resolves the fallback restore target for the close: used when nothing + * meaningful was focused before opening — the body (a pointer press can + * leave focus there), or an element since removed from the document. + * Typically the layer's trigger. */ + restoreFocus?: () => HTMLElement | null } /** @@ -60,6 +65,14 @@ export function openDialogLayer(content: HTMLElement, options: OpenDialogLayerOp return () => { unregister() - if (previous instanceof HTMLElement) previous.focus({ preventScroll: true }) + // The element focused before opening wins; when it can't meaningfully + // take focus back — the body, or gone from the document — the restore + // falls back to the consumer-designated target (normally the trigger). + const meaningful = + previous instanceof HTMLElement && + previous !== previous.ownerDocument.body && + previous.isConnected + const restoreTarget = meaningful ? previous : (options.restoreFocus?.() ?? null) + if (restoreTarget?.isConnected === true) restoreTarget.focus({ preventScroll: true }) } } diff --git a/packages/dom/components/dialog/tests/dialog.test.ts b/packages/dom/components/dialog/tests/dialog.test.ts index 94377e0..984516d 100644 --- a/packages/dom/components/dialog/tests/dialog.test.ts +++ b/packages/dom/components/dialog/tests/dialog.test.ts @@ -133,7 +133,10 @@ describe('openDialogLayer', () => { // never calls it still leaves the stack clean. Closing twice is a no-op. const open = ( html: string, - extra: Partial & { initialFocus?: HTMLElement | null } = {}, + extra: Partial & { + initialFocus?: HTMLElement | null + restoreFocus?: () => HTMLElement | null + } = {}, ): { content: HTMLElement; close: () => void } => { const content = document.createElement('div') content.tabIndex = -1 @@ -211,6 +214,51 @@ describe('openDialogLayer', () => { expect(document.activeElement).toBe(trigger) }) + it('prefers the element that held focus over the designated restore target', () => { + const trigger = document.createElement('button') + const designated = document.createElement('button') + document.body.append(trigger, designated) + trigger.focus() + + open('', { restoreFocus: () => designated }).close() + + expect(document.activeElement).toBe(trigger) + }) + + it('falls back to the restore target when focus sat on the body before opening', () => { + // A pointer press can leave focus on the body — nothing meaningful to + // restore to. + const designated = document.createElement('button') + document.body.append(designated) + ;(document.activeElement as HTMLElement | null)?.blur() + + open('', { restoreFocus: () => designated }).close() + + expect(document.activeElement).toBe(designated) + }) + + it('falls back to the restore target when the previous holder left the document', () => { + const trigger = document.createElement('button') + const designated = document.createElement('button') + document.body.append(trigger, designated) + trigger.focus() + + const { close } = open('', { restoreFocus: () => designated }) + trigger.remove() + close() + + expect(document.activeElement).toBe(designated) + }) + + it('leaves focus where it is when nothing meaningful preceded and no target is designated', () => { + ;(document.activeElement as HTMLElement | null)?.blur() + + const { content, close } = open('') + close() + + expect(document.activeElement).toBe(content) + }) + it('releases the layer beneath before focus returns to it', () => { // The ordering contract. jsdom doesn't enforce `inert`, so a focus // assertion wouldn't discriminate — observe the order directly instead: diff --git a/packages/react/dialog/SPEC.md b/packages/react/dialog/SPEC.md index 1b39064..f874feb 100644 --- a/packages/react/dialog/SPEC.md +++ b/packages/react/dialog/SPEC.md @@ -152,6 +152,7 @@ The dialog window; renders a `
` with the `dialog` role. | Prop | Type | Default | Description | | -------------- | -------------------------------- | ----------------- | ------------------------------------------- | | `initialFocus` | `RefObject` | the dialog window | The element to focus when the dialog opens. | +| `restoreFocus` | `RefObject` | — | Focused on close when nothing meaningful held focus before opening (the body, or an element since removed). Typically the trigger. | | `...props` | `ComponentProps<'div'>` | — | Forwarded to the rendered `
`. | ### `Dialog.Title` diff --git a/packages/react/dialog/src/dialog.tsx b/packages/react/dialog/src/dialog.tsx index 18d1136..ad2d978 100644 --- a/packages/react/dialog/src/dialog.tsx +++ b/packages/react/dialog/src/dialog.tsx @@ -190,17 +190,23 @@ export const Viewport: PartComponent = forw export interface DialogContentProps extends ComponentPropsWithoutRef<'div'> { /** The element to focus when the dialog opens. @default the dialog window */ initialFocus?: RefObject + /** Focused on close when nothing meaningful held focus before opening — it + * sat on the body (a pointer press leaves it there) or on an element since + * removed. Typically the dialog's trigger. */ + restoreFocus?: RefObject } export const Content: PartComponent = forwardRef< HTMLDivElement, DialogContentProps ->(({ initialFocus, ...props }, forwardedRef) => { +>(({ initialFocus, restoreFocus, ...props }, forwardedRef) => { const { api, machine, depth, container, backdropRef } = useDialogContext() const contentRef = useRef(null) useImperativeHandle(forwardedRef, () => contentRef.current as HTMLDivElement) const initialFocusRef = useRef(initialFocus) initialFocusRef.current = initialFocus + const restoreFocusRef = useRef(restoreFocus) + restoreFocusRef.current = restoreFocus // The machine's `open` state is the edge, not mount/unmount — an animated // dialog stays mounted through `closing`. The sequence and its inverse are @@ -215,6 +221,7 @@ export const Content: PartComponent = forwar modal: machine.context.modal, backdrop: () => backdropRef.current, initialFocus: initialFocusRef.current?.current, + restoreFocus: () => restoreFocusRef.current?.current ?? null, dismiss: () => machine.send({ type: 'close' }), }) }, [api.open, machine, depth, backdropRef]) diff --git a/packages/react/dialog/tests/dialog.test.tsx b/packages/react/dialog/tests/dialog.test.tsx index bac6a27..05979f0 100644 --- a/packages/react/dialog/tests/dialog.test.tsx +++ b/packages/react/dialog/tests/dialog.test.tsx @@ -314,6 +314,34 @@ describe('Dialog', () => { expect(document.activeElement).toBe(trigger) }) + it('falls back to restoreFocus when nothing meaningful held focus before opening', () => { + const Harness = () => { + const fallbackRef = useRef(null) + return ( + <> + + + Trigger + + + Close + + + + + ) + } + render() + // A click without a focus move leaves the body focused — nothing + // meaningful for the close to restore to. + openDialog() + + act(() => screen.getByText('Close').click()) + expect(document.activeElement).toBe(screen.getByText('Fallback')) + }) + // jsdom does no layout, so the scroll jump can't be reproduced — assert the // mechanism that prevents it: focus never scrolls the locked surface. it('moves focus without scrolling the locked surface', () => { diff --git a/packages/solid/dialog/SPEC.md b/packages/solid/dialog/SPEC.md index d9f9200..f0f98ea 100644 --- a/packages/solid/dialog/SPEC.md +++ b/packages/solid/dialog/SPEC.md @@ -59,7 +59,8 @@ Solid-specific notes on top of the core contract: break. - **`Content`'s `initialFocus`** accepts an element or an accessor resolved at open time — the Solid idiom for a ref variable that fills during render: - pass `initialFocus={() => cancelButton}`. + pass `initialFocus={() => cancelButton}`. `restoreFocus` takes the same + shape, resolved at close time. - **`Backdrop`** renders nothing when the dialog is non-modal (`modal={false}`), per the core parts contract. - **Exit animation** (`animated`): style the exit on the parts' @@ -157,6 +158,7 @@ The dialog window; renders a `
` with the `dialog` role. | Prop | Type | Default | Description | | -------------- | --------------------------------------------------------- | ----------------- | ------------------------------------------------------------------- | | `initialFocus` | `HTMLElement \| (() => HTMLElement \| null \| undefined)` | the dialog window | The element to focus when the dialog opens — resolved at open time. | +| `restoreFocus` | `HTMLElement \| (() => HTMLElement \| null \| undefined)` | — | Focused on close when nothing meaningful held focus before opening (the body, or an element since removed) — resolved at close time. Typically the trigger. | | `...props` | `ComponentProps<'div'>` | — | Forwarded to the rendered `
`. | ### `Dialog.Title` diff --git a/packages/solid/dialog/src/dialog.tsx b/packages/solid/dialog/src/dialog.tsx index c6c982b..6be3851 100644 --- a/packages/solid/dialog/src/dialog.tsx +++ b/packages/solid/dialog/src/dialog.tsx @@ -211,14 +211,20 @@ export interface DialogContentProps extends ComponentProps<'div'> { /** The element to focus when the dialog opens — an element, or an accessor * resolved at open time. @default the dialog window */ initialFocus?: HTMLElement | (() => HTMLElement | null | undefined) + /** Focused on close when nothing meaningful held focus before opening — it + * sat on the body (a pointer press leaves it there) or on an element since + * removed. An element, or an accessor resolved at close time. Typically the + * dialog's trigger. */ + restoreFocus?: HTMLElement | (() => HTMLElement | null | undefined) } -const resolveInitialFocus = (value: DialogContentProps['initialFocus']): HTMLElement | null => - (typeof value === 'function' ? value() : value) ?? null +const resolveFocusTarget = ( + value: HTMLElement | (() => HTMLElement | null | undefined) | undefined, +): HTMLElement | null => (typeof value === 'function' ? value() : value) ?? null export const Content: Component = props => { const { api, machine, depth, container, backdropRef } = useDialogContext() - const rest = omit(props, 'ref', 'initialFocus', 'children') + const rest = omit(props, 'ref', 'initialFocus', 'restoreFocus', 'children') let contentEl: HTMLDivElement | undefined // The `open` state is the edge, not mount/unmount: an animated dialog stays @@ -235,7 +241,8 @@ export const Content: Component = props => { depth, modal: machine.context.modal, backdrop: () => backdropRef.current, - initialFocus: untrack(() => resolveInitialFocus(props.initialFocus)), + initialFocus: untrack(() => resolveFocusTarget(props.initialFocus)), + restoreFocus: () => untrack(() => resolveFocusTarget(props.restoreFocus)), dismiss: () => machine.send({ type: 'close' }), }) }, diff --git a/packages/solid/dialog/tests/dialog.test.tsx b/packages/solid/dialog/tests/dialog.test.tsx index 5d0a7f3..0abaff0 100644 --- a/packages/solid/dialog/tests/dialog.test.tsx +++ b/packages/solid/dialog/tests/dialog.test.tsx @@ -329,6 +329,31 @@ describe('Dialog', () => { expect(document.activeElement).toBe(trigger) }) + it('falls back to restoreFocus when nothing meaningful held focus before opening', () => { + let fallback: HTMLButtonElement | undefined + render(() => ( + <> + + + Trigger + + fallback}> + Close + + + + + )) + // A click without a focus move leaves the body focused — nothing + // meaningful for the close to restore to. + openDialog() + + press(screen.getByText('Close')) + expect(document.activeElement).toBe(fallback) + }) + // jsdom does no layout, so the scroll jump can't be reproduced — assert the // mechanism that prevents it: focus never scrolls the locked surface. it('moves focus without scrolling the locked surface', () => {