Repository navigation
fix(Popover): cancel the auto-focus frame when the popover closes first - #8276
Conversation
usePopover focuses its content in the frame after it opens, and never cancelled that frame. A popover closed within it (Escape right after opening) had already left focus on the trigger, since focus never entered the trap; then the frame ran and focused the first control in the closed popover's content. On a loaded runner jsdom's frame timer runs late enough for SideNav's "returns focus to the trigger when the collapsed submenu closes" to land there and fail. The effect now cancels the frame on close or unmount. A Popover test and a SideNav test hold the frame until after Escape and expect the trigger to keep focus; the SideNav focus-return test drives its frame explicitly instead of waiting on a timer.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
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.
Recommended for approval: cancelling the auto-focus frame in the Effect's cleanup gives that frame the lifecycle owner the React runtime architecture requires, normal openings still focus their first content control, and both new regression tests fail without the fix and pass with it. One optional follow-up: in Chromium and WebKit the closed popover is display: none and already refused focus before this change, so the changeset's line about Escape now leaving focus on the trigger describes the jsdom race rather than something browser users will notice; consider rewording it to say the stale auto-focus frame is now cancelled.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] usePopover: a popover that closes or unmounts before its first frame no longer moves focus into its hidden content. Auto-focus waits a frame for the content to mount, and that frame is now cancelled with the open, so pressing Escape right after opening a Popover or a collapsed SideNav submenu leaves focus on the trigger. |
There was a problem hiding this comment.
In Chromium and WebKit the closed popover is display: none and already refused focus before this change: with the late frame run after Escape, focus stayed on the trigger on the base too. This line therefore describes the jsdom race rather than something browser users notice. Consider: "usePopover now cancels its auto-focus frame when the popover closes or unmounts before that frame runs, so a stale frame can no longer act on a closed popover."
User impact
When a Popover, or anything built on
usePopoversuch as a collapsed SideNav submenu, is dismissed within a frame of opening, focus jumps into the hidden popover's content. A quick Escape after a click or Enter is enough. Keyboard and screen-reader users are left on a control they can't see. The same race makes SideNav's "returns focus to the trigger when the collapsed submenu closes" test flaky on loaded runners, as seen on #8179: it took 1112 ms and ended with focus on the submenu's "General" link.Problem and solution fit
usePopoverfocuses its content in arequestAnimationFrameafter it opens and never cancels that frame.useFocusTrapleaves focus on the trigger because focus never entered the trap. Then the frame runs and focuses the first control in content that is no longer shown.The fix ties the frame to the open: closing or unmounting cancels it.
Expected behavior and authority
component:Popover(current):Popover.test.tsxcovers "returns focus to the trigger when closed via Escape".An opening that has already closed has no content to focus.
Smallest restoration
usePopover's auto-focus effect returns() => cancelAnimationFrame(frame).Evidence
Reproduction before the change: hold animation frames, open the collapsed SideNav submenu, press Escape, then run the held frame. Focus lands on the "General" link inside the closed submenu. The Popover equivalent lands on its content button.
Result after the change: in both cases the trigger keeps focus.
Representative unchanged path: opening still focuses the first content control in the next frame. Escape after that still returns focus to the trigger, now asserted without a timing wait.
Regression tests:
Both fail with the fix reverted.
Scope
Testing
pnpm vitest run --project uion Popover, SideNav, and every directory with ausePopoverconsumer: Breadcrumbs, Chat, ComplexSelector, DateInput, DateRangeInput, DateTimeInput, DropdownMenu, hooks, PowerSearch, Selector, TabList, TopNav, Typeahead, lab Schedule. 115 files, 3183 tests passed.usePopover.tsxreverted, the two new tests fail and the reworked SideNav focus-return test still passes. That test is now deterministic: it flushes the open frame, asserts focus is on "General", presses Escape, and asserts the trigger has focus immediately, with nowaitFor.pnpm -F @astryxdesign/core typecheck, eslint and prettier on the touched files,check:changesets, and pre-commitcheck:repo.