Repository navigation
fix(Dialog): capture the opener before descendant autofocus effects - #6607
Conversation
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
community
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression38 of 478 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 18 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks — the focus fix itself looks right and matches the Dialog contract: capturing the opener in a layout effect means a DialogHeader that mounts in the open commit can no longer replace the return target, and suppressing header autofocus while the dialog is closed stops focus landing on an invisible title. The new regressions cover both shapes, and the Dialog and dependent suites pass.
One change is needed before this lands:
packages/core/src/Dialog/__tests__/Dialog.a11y.known-failures.tsstill recordsmodal-dialog.focus.restoredfor theconditional-content-focus-restorationstate as a current failure ("closing the modal dialog did not return focus to its still-available invoker"). This PR makes that outcome pass, and the Chromium accessibility contract requires every known-failure record to match exactly one executed result — an unmatched record failsDialog.a11y.chromium.spec.tswhen it runs at release-check (it does not run in PR CI, which is why checks are green). Please delete just that record, keeping theenters-after-modalrecord, whose focus-entry ordering this PR does not change and which remains accurate. Running the Dialog Chromium accessibility spec after the removal, where a browser is available, would confirm it.
No other blocking issues were found; the pre-existing focus-entry ordering issue stays recorded and is out of scope here.
[Automated review]
# Conflicts: # packages/core/src/Dialog/Dialog.tsx
There was a problem hiding this comment.
Requesting changes: the layout-effect opener capture fixes #5637 (in Chromium, closing now returns focus to the opener, and the known-failure removal is correct), but the DialogHeader isOpen gate makes the header focus its title when the dialog opens, before Dialog calls showModal(), so with an always-mounted DialogHeader focus now enters the dialog before it is modal. Dialog.a11y.chromium.spec.ts, which runs at release-check rather than in PR CI, now fails modal-dialog.focus.enters-after-modal for labelled-described-default-title and explicit-descendant-focus, and passes again when only the DialogHeader.tsx change is reverted. Please keep the capture and either drop the open-edge refocus (Chromium already gives no focus to a header under a closed dialog, since the closed dialog isn't rendered) or have Dialog focus the title after showModal(), then update the changeset and rerun that spec.
[Automated review]
| const shouldAutoFocus = | ||
| dialogContext?.isInline !== true && dialogContext?.isOpen !== false; |
There was a problem hiding this comment.
This makes the focus effect below re-run when the Dialog opens. It runs as a child passive effect, before Dialog's passive effect calls showModal(). So for a header that stays mounted, the title gets focus while the dialog is not yet modal, which breaks component:Dialog FR1. Dialog.a11y.chromium.spec.ts fails modal-dialog.focus.enters-after-modal for labelled-described-default-title and explicit-descendant-focus with this change, and passes without it. A closed dialog is display: none, so in Chromium the old mount-time focus() call already moved nothing.
… refocus Re-running the header's focus effect when isOpen flips focused the title before Dialog calls showModal(), so focus entered the dialog before it was modal (component:Dialog FR1; modal-dialog.focus.enters-after-modal). A header under a closed dialog gets no focus in a browser anyway, since the closed dialog is not rendered. Revert the isOpen context gate and its jsdom-only test; the layout-effect capture (FR4) stays.
There was a problem hiding this comment.
This looks ready to merge: saving the opener in a layout effect fixes the #5637 focus return for DialogHeader-style content, the Dialog Chromium accessibility contract now passes focus restoration for content that mounts on open, and focus-entry order is unchanged. A few non-blocking follow-ups:
- Please update the PR description, which still describes the reverted
DialogHeader/DialogContextchange and its second regression test. - Content that takes focus during the opening commit itself, such as a
TextInputwithhasAutoFocus, still returns focus to the page body on close as before, so consider narrowing the changeset's first sentence and keeping #5637 or a follow-up open for that case. - Optionally, saving the opener only once per open (skipping the save while one is held) would also keep it under React StrictMode for a Dialog that mounts already open.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Restore focus to the element that opened a Dialog when its content mounts in the open commit. The opener was captured in the Dialog's passive effect, which runs after a freshly mounted DialogHeader's autofocus effect had already moved focus to the dialog's own title — closing then restored focus to that detached title and the user's focus landed on `<body>`. The capture now runs in a layout effect, before any descendant's passive effect. (#5637) |
There was a problem hiding this comment.
Non-blocking: content that takes focus during the opening commit itself, such as a TextInput with hasAutoFocus (React applies autoFocus before this layout effect runs), is still saved as the opener, so this first sentence reads broader than the fix. Consider scoping it to focus moved from a passive effect, such as DialogHeader's title.
There was a problem hiding this comment.
Optional hardening: when a Dialog mounts with isOpen already true, React StrictMode re-runs this effect after the first pass's passive effects, so the saved element becomes the dialog's own title (development only, and the same outcome as before this change). Saving once per open keeps the opener, and the close branch already clears the ref:
if (isInline || !isOpen || triggerElementRef.current !== null) {
return;
}
github-actions
Bot
removed
the
needs:code-review
Fixes #5637
Summary
A controlled Dialog whose content mounts in the open commit restored focus to
<body>on close. Two defects, both fixed:document.activeElementin its passive effect, but React runs child passive effects before the parent's — so a freshly mounted DialogHeader focused its own title first, and the Dialog captured its own heading. When closing unmounts that content, focus restoration targeted a detached node and the user's focus stranded on<body>. The capture now runs in auseLayoutEffecton the open edge: all layout effects run before any passive effect, so the capture always sees the element the user was on before the dialog's descendants moved focus.shouldAutoFocuscheck only excluded inline mode, and the standard path renders children (hidden) while closed, so a header mounted underisOpen={false}stole focus onto an invisible element.DialogContextnow carriesisOpen, and the header suppresses autofocus while closed — its effect re-runs when the dialog opens, so the title still becomes the screen reader anchor on open, exactly as before for content that mounts in the open commit.Tests
document.activeElementis the opener. Failed on main (focus landed on<body>), passes here.DialogHeadermounted underisOpen={false}leaves focus on<body>. Failed on main (title stolen focus), passes here.Verification
tsc --noEmitclean (core); eslint 0 problems on touched files; prettier clean;check:changesetsclean. Changeset included.