Skip to content

fix: restore default-open popovers to their trigger - #10696

Open
minwookshin wants to merge 3 commits into
adobe:mainfrom
minwookshin:fix/default-open-focus-restore
Open

minwookshin wants to merge 3 commits into
adobe:mainfrom
minwookshin:fix/default-open-focus-restore

Conversation

@minwookshin

@minwookshin minwookshin commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Closes #10661

An overlay opened at mount records document.body as its restore target, leaving focus there after Escape. Pass the closing scope with the existing restore-focus event so the matching trigger can handle that fallback. Existing focus targets retain priority, and the private FocusScope helper is removed.

AI-assisted implementation and verification with Codex, following AGENTS.md and the contribution guidance.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated tests: browser regressions for default-open DialogTrigger, Select and Modal overlays, existing focus targets and parent scopes.
  • Filled out test instructions.
  • Documentation checked; no public API changes.
  • Looked at the Accessibility Practices for this feature - Button Pattern.
  • I understand every change in this PR and can explain why it's there.
  • If AI-assisted, I followed our AI contribution guidance and pointed my assistant at AGENTS.md.

📝 Test Instructions:

Run yarn vitest run --config=vitest.browser.config.ts packages/react-aria-components/test/Popover.browser.test.tsx. Escape should return a default-open overlay to its trigger, including after reopening. A previously focused element should remain the restore target. All three default-open cases fail on the unchanged base.

All seven cases pass in Chromium, Firefox and WebKit. The final related unit tests, SSR, lint, formatting and type checks pass. Broader local runs hit an existing WebKit Modal failure reproduced on the base, an intermittent DateRangePicker render failure and one Jest worker crash; the affected tests passed separately. The first SSR run timed out at startup, and the second full run passed without changes.

Keyboard and mouse interactions are covered. Screen-reader interaction was not manually tested.

🧢 Your Project:

Independent open-source contribution.

@snowystinger snowystinger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We'll want to try and do this without adding any new props if possible

@minwookshin

Copy link
Copy Markdown
Contributor Author

Removed the public fallback props. usePopover now uses its existing trigger ref to fill the nearest restoring scope's fallback, while preserving existing focus targets; regression tests pass in Chromium, Firefox, and WebKit.

}

/** @private */
export function setFocusScopeRestoreTarget(element: Element, target: FocusableElement): void {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think a private export is quite the right solution yet, considering FocusScope is meant to eventually be upstreamed into React. We should try to find a different way to approach this problem.

I know there is precedent with isElementInChildOfActiveScope but I'm pretty sure that was a temporary measure.

@snowystinger

Copy link
Copy Markdown
Member

Thanks for exploring another avenue. Talked with @devongovett and we're more in favour of the first approach you took. A FocusScope prop could be useful in this case. PopoverTrigger and DialogTrigger could set it automatically.

@nwidynski

nwidynski commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

@snowystinger Actually, I think there is a way to avoid both prop and the private method if we take a similar approach to PreviewTrigger. An overlay trigger could intercept the focus restoration event, then check if its within scope by passing it as an event detail.

@snowystinger

Copy link
Copy Markdown
Member

Thanks for the idea @nwidynski , happy to have that tried out as well, it sounds promising

@minwookshin

Copy link
Copy Markdown
Contributor Author

Switched to the restore-focus event and removed the private export. The trigger only handles restoration from its own overlay when the saved target is body.

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.

A Popover or Select that is open at mount restores focus to body instead of its trigger

3 participants