Skip to content

fix(react-dialog): respect autoFocus element over first focusable on dialog open#36038

Open
Copilot wants to merge 5 commits intomasterfrom
copilot/fix-autofocus-dialog-issue
Open

fix(react-dialog): respect autoFocus element over first focusable on dialog open#36038
Copilot wants to merge 5 commits intomasterfrom
copilot/fix-autofocus-dialog-issue

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 24, 2026

When a Dialog/Drawer contained an element with autoFocus={true} preceded by any other focusable element, the autofocus was silently ignored — findFirstFocusable always won by DOM order.

Changes

  • useFocusFirstElement.ts: Before calling findFirstFocusable, query the dialog root for [autofocus]. React lowers autoFocus={true} to the DOM autofocus attribute on mount, so querySelector('[autofocus]') reliably finds it. Falls back to findFirstFocusable (and the existing dev warning) when none is present.
// Before
const element = dialogRef.current && findFirstFocusable(dialogRef.current);

// After
const autoFocusElement = dialogRef.current?.querySelector<HTMLElement>('[autofocus]');
const element = autoFocusElement || (dialogRef.current && findFirstFocusable(dialogRef.current));
  • useFocusFirstElement.test.ts (new): Unit tests covering the autofocus-wins path and the no-autofocus fallback path, with mocked useFocusFinders and useFluent_unstable.

Since OverlayDrawer uses DialogSurface internally, this fix covers both Dialog and Drawer without any changes to drawer code.

Copilot AI changed the title [WIP] Fix autofocus in dialog when focusable elements precede fix(react-dialog): respect autoFocus element over first focusable on dialog open Apr 24, 2026
Copilot AI requested a review from tudorpopams April 24, 2026 13:08
@tudorpopams
Copy link
Copy Markdown
Contributor

@copilot fix CI

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 24, 2026

@copilot fix CI

Fixed in c91b181. The react-dialog:lint failure was caused by two issues in the new test file: MutableRefObject (deprecated, replaced with RefObject) and a missing import type * as React from 'react' (needed for the type cast).

@tudorpopams tudorpopams marked this pull request as ready for review April 24, 2026 14:58
@tudorpopams tudorpopams requested a review from a team as a code owner April 24, 2026 14:58
@tudorpopams tudorpopams requested a review from dmytrokirpa April 24, 2026 14:58
@tudorpopams
Copy link
Copy Markdown
Contributor

PR Review: #36038 — fix(react-dialog): respect autoFocus element over first focusable on dialog open

Author: app/copilot-swe-agent
Type: bug-fix
Packages affected: @fluentui/react-dialog (covers Drawer transitively — OverlayDrawer uses DialogSurface internally)
CI Status: passing (all 15 checks green, including e2e, VR screenshots, CodeQL)

Confidence Score: 100/100

Single-line source change (querySelector('[autofocus]') before falling back to findFirstFocusable) with both-paths unit coverage. Correctly scoped to the autofocus preference — doesn't touch FocusTrap/tabster — matching the guardrail in the Copilot brief.

Findings

Blockers (must fix before merge)

none

Warnings (should address)

none

Info (consider)

  • dialogRef.current?.querySelector('[autofocus]') walks the full subtree. In the rare case of a nested dialog rendered inside another dialog's surface, the outer effect could pick an [autofocus] from the nested dialog. Not a realistic flow for the Dialog/Drawer contract and not worth guarding against in this PR, but worth knowing if someone reports a regression — a scoped :scope > ... [autofocus]:not([role="dialog"] [autofocus]) query could tighten it later if needed.
  • Independently verified in a prior session: the full Dialog suite passes 124/124 including the new tests; baseline (no-autofocus) behavior preserved — default story still focuses the first focusable on mount. Drawer coverage comes transitively because OverlayDrawer uses DialogSurface.

Category Breakdown

Category Status Notes
Change file PASS @fluentui-react-dialog-890df3cd-...json, type patch. Description correctly cites #35749 (the real issue — avoiding a prior Copilot session's wrong-issue-reference mistake).
V9 patterns PASS Hook is an internal utility; no React.FC, no styles touched, existing useFluent_unstable() targetDocument usage preserved for the dev-warning branch.
Dep layers PASS No package.json or new-import changes.
SSR safety PASS querySelector is called on dialogRef.current (a ref to a real DOM node that only exists after mount); no global document/window access. The existing no-focusable-element fallback path and its dev warning are unchanged.
Testing PASS New useFocusFirstElement.test.ts covers both paths — the no-autofocus case asserts findFirstFocusable is called and the first button is focused; the autofocus case asserts both that the autofocus input is focused and that findFirstFocusable is NOT called and the first button is NOT focused. The negative assertions are the ones that matter — they're what actually proves the preference order.
API surface PASS Internal hook; no public API change. etc/react-dialog.api.md untouched (correctly).
Accessibility PASS Makes React's autoFocus={true} behave per its documented contract inside dialogs. The existing empty-dialog fallback (dialogRef.current?.focus() with the dev warning for #25150) is preserved.
Security/Quality PASS No eval/dangerouslySetInnerHTML/console.log/@ts-ignore/any.

Recommendation

APPROVE

Clean minimal fix. The reshape from a single assignment to two lines preserves the existing "no focusable element at all" warning path exactly, and the test file explicitly asserts the wrong element is NOT focused in the autofocus case — which is the assertion that proves the fix, not just the positive path. Drawer coverage is handled automatically via DialogSurface composition, avoiding a duplicate fix on the drawer side.


Posted via the /review-pr skill.

@tudorpopams tudorpopams enabled auto-merge (squash) April 24, 2026 16:28
@github-actions
Copy link
Copy Markdown

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: entire library
1.302 MB
325.338 kB
1.302 MB
325.374 kB
80 B
36 B
react-dialog
Dialog (including children components)
101.984 kB
30.317 kB
102.064 kB
30.335 kB
80 B
18 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
70.415 kB
19.963 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
237.29 kB
68.851 kB
react-components
react-components: FluentProvider & webLightTheme
43.63 kB
14.026 kB
react-headless-components-preview
react-headless-components-preview: entire library
84.122 kB
24.635 kB
react-portal-compat
PortalCompatProvider
8.386 kB
2.624 kB
react-timepicker-compat
TimePicker
109.674 kB
36.193 kB
🤖 This report was generated against 0392152eacf970d6d7063c12af84b20b7d45c8e1

@github-actions
Copy link
Copy Markdown

Pull request demo site: URL

@@ -0,0 +1,7 @@
{
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕵🏾‍♀️ visual changes to review in the Visual Change Report

vr-tests-react-components/Charts-DonutChart 2 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests-react-components/Charts-DonutChart.Dynamic - Dark Mode.default.chromium.png 7530 Changed
vr-tests-react-components/Charts-DonutChart.Dynamic.default.chromium.png 5581 Changed
vr-tests-react-components/Menu Converged - submenuIndicator slotted content 2 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests-react-components/Menu Converged - submenuIndicator slotted content.default - RTL.submenus open.chromium.png 404 Changed
vr-tests-react-components/Menu Converged - submenuIndicator slotted content.default.submenus open.chromium.png 413 Changed
vr-tests-react-components/Positioning 2 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png 852 Changed
vr-tests-react-components/Positioning.Positioning end.chromium.png 778 Changed
vr-tests-react-components/ProgressBar converged 2 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests-react-components/ProgressBar converged.Indeterminate + thickness - High Contrast.default.chromium.png 32 Changed
vr-tests-react-components/ProgressBar converged.Indeterminate + thickness.default.chromium.png 26 Changed
vr-tests-react-components/TagPicker 1 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests-react-components/TagPicker.disabled.chromium.png 677 Changed

There were 1 duplicate changes discarded. Check the build logs for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Autofocus does not work in dialog/drawer if any focusable element precedes the autofocus element

2 participants