Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/dialog-restore-focus.md
Original file line number Diff line number Diff line change
@@ -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
<Dialog.Content restoreFocus={triggerRef}>…</Dialog.Content>

// Solid — an element or accessor, resolved at close time
<Dialog.Content restoreFocus={() => trigger}>…</Dialog.Content>
```

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.
6 changes: 6 additions & 0 deletions packages/dom/components/dialog/SPEC.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
15 changes: 14 additions & 1 deletion packages/dom/components/dialog/src/open-layer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

/**
Expand Down Expand Up @@ -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 })
}
}
50 changes: 49 additions & 1 deletion packages/dom/components/dialog/tests/dialog.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof options> & { initialFocus?: HTMLElement | null } = {},
extra: Partial<typeof options> & {
initialFocus?: HTMLElement | null
restoreFocus?: () => HTMLElement | null
} = {},
): { content: HTMLElement; close: () => void } => {
const content = document.createElement('div')
content.tabIndex = -1
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions packages/react/dialog/SPEC.md
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ The dialog window; renders a `<div>` with the `dialog` role.
| Prop | Type | Default | Description |
| -------------- | -------------------------------- | ----------------- | ------------------------------------------- |
| `initialFocus` | `RefObject<HTMLElement \| null>` | the dialog window | The element to focus when the dialog opens. |
| `restoreFocus` | `RefObject<HTMLElement \| null>` | — | 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 `<div>`. |

### `Dialog.Title`
Expand Down
9 changes: 8 additions & 1 deletion packages/react/dialog/src/dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -190,17 +190,23 @@ export const Viewport: PartComponent<DialogViewportProps, HTMLDivElement> = forw
export interface DialogContentProps extends ComponentPropsWithoutRef<'div'> {
/** The element to focus when the dialog opens. @default the dialog window */
initialFocus?: RefObject<HTMLElement | null>
/** 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<HTMLElement | null>
}

export const Content: PartComponent<DialogContentProps, HTMLDivElement> = forwardRef<
HTMLDivElement,
DialogContentProps
>(({ initialFocus, ...props }, forwardedRef) => {
>(({ initialFocus, restoreFocus, ...props }, forwardedRef) => {
const { api, machine, depth, container, backdropRef } = useDialogContext()
const contentRef = useRef<HTMLDivElement>(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
Expand All @@ -215,6 +221,7 @@ export const Content: PartComponent<DialogContentProps, HTMLDivElement> = 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])
Expand Down
28 changes: 28 additions & 0 deletions packages/react/dialog/tests/dialog.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<HTMLButtonElement | null>(null)
return (
<>
<button type='button' ref={fallbackRef}>
Fallback
</button>
<Dialog>
<Dialog.Trigger>Trigger</Dialog.Trigger>
<Dialog.Portal>
<Dialog.Content aria-label='Settings' restoreFocus={fallbackRef}>
<Dialog.Close>Close</Dialog.Close>
</Dialog.Content>
</Dialog.Portal>
</Dialog>
</>
)
}
render(<Harness />)
// 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', () => {
Expand Down
4 changes: 3 additions & 1 deletion packages/solid/dialog/SPEC.md
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -157,6 +158,7 @@ The dialog window; renders a `<div>` 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 `<div>`. |

### `Dialog.Title`
Expand Down
15 changes: 11 additions & 4 deletions packages/solid/dialog/src/dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<DialogContentProps> = 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
Expand All @@ -235,7 +241,8 @@ export const Content: Component<DialogContentProps> = 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' }),
})
},
Expand Down
25 changes: 25 additions & 0 deletions packages/solid/dialog/tests/dialog.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => (
<>
<button type='button' ref={element => (fallback = element)}>
Fallback
</button>
<Dialog>
<Dialog.Trigger>Trigger</Dialog.Trigger>
<Dialog.Portal>
<Dialog.Content aria-label='Settings' restoreFocus={() => fallback}>
<Dialog.Close>Close</Dialog.Close>
</Dialog.Content>
</Dialog.Portal>
</Dialog>
</>
))
// 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', () => {
Expand Down
Loading