Repository navigation
fix(core): one focus return for Popover, Dialog and DropdownMenu that never strands focus (AST-113 FR17–FR21) - #11121
Conversation
|
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.
Needs an owner decision on the spec, plus three fixes.
returnFocus adds a caller-chosen focus target that no current record covers yet: the Popover contract returns focus to the trigger, and spec:AST-113 FR17 is still draft with the input name open. An engineering owner needs to approve that direction, by promoting FR17 or by commenting /approve-spec <full head SHA> here (the author can do this as an engineering owner).
Blocking either way:
- In Chromium, Escape and the fallback close button still put focus on the opener with
returnFocus="anchor"becausehidePopover()restores focus before the focus trap reads the target, which the jsdom tests cannot show, so please make the decision in the hide path and add a browser interaction test for Escape, the close button and an outside press. - With
presentation="bottom-sheet"(or"adaptive"on a phone),returnFocusnever reaches the sheet and focus goes back to the trigger, so please pass the target to the sheet (for example through BottomSheet'sfinalFocusRef) or document the exception in the JSDoc, the docs and the changeset. focusableWithinre-implements the focus trap'sgetFocusableElementswithout its visibility filter, so when the anchor's first control is hidden, focus ends on the page body; please reuse the shared helper and remove the copy.
[Automated review]
88e86f0 to
b905360
Compare
45a09cf to
ad85352
Compare
nynexman4464
changed the title
There was a problem hiding this comment.
Needs an engineering owner's decision on the spec, plus three fixes.
The shared focus return (the stand-in chain after the opener, the parent-to-child handoff and the rule that nothing turns it off) is defined only in draft spec:AST-113 FR17–FR21 and DEC-6, while the current Popover, Dialog and DropdownMenu contracts return focus only to the trigger or invoker. An engineering owner records the decision by approving and promoting AST-113 or by commenting /approve-spec <full head SHA> on this pull request (the author can do this as an engineering owner), and should also say whether the public useFocusTrap hook and Drawer, which now use the same chain, are in scope.
Blocking either way:
- The Dialog docs and the changeset tell callers to focus their target in
onOpenChange, but Dialog calls it while the modal still makes the page inert, so the call does nothing and focus goes to the opener; please document a route that works afterclose(), or say there is none yet. - The description and changeset present the menu-row Dialog and the manual (
hasLightDismiss={false}) popover as falling to the page body, yet both already return focus to their trigger on the base branch in Chromium and WebKit (the new menu-row test also passes there), and the "one rule" does not reach the opt-in bottom-sheet presentations, so please narrow the changeset to the popover-presentation cases this change fixes. utils/focusReturn.tsadds its own reachability test (tabStopsIn,isRendered,canTakeReturnedFocus) beside the focus trap'sgetFocusableElements, without itsaria-hiddenrule, so returned focus can land on a control screen readers skip; please share one helper and remove the copy.
| { | ||
| guidance: false, | ||
| description: | ||
| "Don't add your own focus return or try to turn it off. When the dialog closes, focus goes back to the opener, or to the nearest control that stands in for it (a dialog opened from a menu row returns focus to the menu's trigger), never to the page. To send focus somewhere specific, focus that element in onOpenChange.", |
There was a problem hiding this comment.
Dialog calls onOpenChange(false) while the modal is still open (the dismissal stack, a backdrop press, cancel), and the page outside it is inert until close(), as the comment in Dialog.tsx says. A caller who focuses another element here gets no error and no move: in Chromium, focusing "Next row" in onOpenChange and pressing Escape leaves focus on the opener. This advice works for Popover, which calls onOpenChange after the return, but Dialog needs a route that runs after close(), or the docs should say there is none yet. The same applies to line 225 and the changeset.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Popover, Dialog and DropdownMenu return focus through one rule when they close, and focus no longer falls to the page when the control that opened them is gone. Focus goes to the opener; if the opener is gone, it goes to its replacement (a trigger that remounted, or the control that took its place in the same group), then the anchor, then the nearest surviving ancestor, then the layer that opened this one, then the page's `main` region. A Dialog opened from a menu row returns focus to the menu's trigger. Focus that you or the person already moved outside the closing layer stays where it is. There is no option to turn focus return off; to send focus somewhere specific, focus it in `onOpenChange` or `onHide`. Focus returns before the browser's own restore, so the trigger gets a focus ring only after keyboard input. |
There was a problem hiding this comment.
"A Dialog opened from a menu row returns focus to the menu's trigger" already holds on the base branch: keyboard and mouse picks in Chromium and a mouse pick in WebKit end on the trigger, and the new menu-row test passes there too. A manual popover also returns to its trigger on the base. The "one rule" does not reach presentation="bottom-sheet", which keeps BottomSheet's own return. Suggest: "In their popover presentation, focus no longer falls to the page when the opener remounted, was removed, or the layer was opened by code", without the menu-row sentence.
| function canTakeReturnedFocus(element: Element | null): element is HTMLElement { | ||
| return ( | ||
| isHTMLElement(element) && | ||
| element.isConnected && | ||
| !isPageRoot(element) && | ||
| !element.matches(':disabled') && | ||
| !element.closest('[inert]') && | ||
| !isInsideClosingLayer(element) && | ||
| isRendered(element) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is a second reachability test beside isVisiblyFocusable/getFocusableElements in hooks/useFocusTrap.ts, which is exported so callers judge reachability exactly as the trap does. It has no aria-hidden check: when the opener is replaced by a control inside aria-hidden="true", returned focus lands on that control, which screen readers skip and the trap's own list leaves out. Moving both helpers to a shared module (for example hooks/focusableSelector.ts, which already holds FOCUSABLE_SELECTOR) avoids the import cycle and lets tabStopsIn and this check call one implementation.
ad85352 to
244aa61
Compare
There was a problem hiding this comment.
Needs an engineering owner's decision on the spec, plus four fixes.
The shared focus return's stand-in chain after the opener, the parent-to-child handoff and the rule that nothing turns it off are defined only in draft spec:AST-113 FR17–FR21 and DEC-6, while the current Popover, Dialog and DropdownMenu contracts return focus only to the trigger or invoker. An engineering owner records the decision by approving and promoting AST-113 or by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and should also say whether the public useFocusTrap hook and Drawer, which now use the same chain, are in scope.
Blocking either way:
- The Dialog docs and the changeset tell callers to focus their target in
onOpenChange, but Dialog calls it while the modal still makes the page inert, so the call does nothing and focus goes to the opener; please document a route that works afterclose(), or say there is none yet. - The description and changeset present the menu-row Dialog and the manual (
hasLightDismiss={false}) popover as falling to the page body, yet both already return focus to their trigger on the base branch (the new menu-row test passes there), and the "one rule" does not reach the bottom-sheet presentations, so please narrow the changeset to the cases this change fixes. utils/focusReturn.tsadds its own reachability test (tabStopsIn,isRendered,canTakeReturnedFocus) beside the focus trap'sgetFocusableElements, without itsaria-hiddenrule, so returned focus can land on a control screen readers skip; please share one helper and remove the copy.- Miss: DropdownMenu's
restoreFocusTomoves focus without the temporary tab stop the default move adds, so the ancestor, parent-layer andmainstand-ins cannot take focus and a menu removed while open still leaves focus on the page body in Chromium and WebKit; please add the tab stop for caller-supplied moves and cover DropdownMenu in themaintest.
| if (element === buttonRef.current) { | ||
| prepareFocusReturn(); | ||
| } | ||
| returnFocusForLastInput(element); |
There was a problem hiding this comment.
Miss: this move adds no temporary tab stop (returnFocusForLastInput → focusForLastInput), and the trap uses it in place of the default focusReturnTarget. So the nearest-ancestor, parent-layer and main stand-ins cannot take focus: a DropdownMenu removed while open (for example with its row) ends on the page body in Chromium and WebKit, where a Popover in the same place hands focus to main. That contradicts "never strands focus". Calling focusForLastInputTemporarily(element, {preventScroll: true}) here, or adding the tab stop in restoreLayerFocus around any caller's move, fixes it; a DropdownMenu case in the main test would guard it.
244aa61 to
18cc3b1
Compare
There was a problem hiding this comment.
Needs an engineering owner's decision on the spec, plus five fixes.
The shared focus return's stand-in chain after the opener, the parent-to-child handoff, the development warning and the rule that nothing turns the return off are defined only in draft spec:AST-113 FR17–FR21 and DEC-6, while the current Popover, Dialog and DropdownMenu contracts return focus only to the trigger or invoker. An engineering owner records the decision by approving and promoting AST-113 or by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and should also say whether the public useFocusTrap hook and Drawer, which now use the same chain, are in scope.
Blocking either way:
- Miss: DropdownMenu's
restoreFocusTomoves focus without the temporary tab stop the default move adds, so the ancestor, parent-layer andmainstand-ins cannot take focus and a menu removed while open still leaves focus on the page body; please add the tab stop for caller-supplied moves and cover DropdownMenu in themaintest. - The Dialog docs and the changeset tell callers to focus their target in
onOpenChange, but Dialog calls it while the modal still makes the page inert, so the call does nothing and focus goes to the opener; please document a route that works afterclose(), or say there is none yet. - Miss: a scrim Drawer's trap deactivates while its dialog is still modal for the slide-out, so every stand-in is inert and each close logs "no element could take focus … focus is left on the page body" although Drawer then returns focus to its opener; please skip the trap's return while its container is an open modal dialog, or keep Drawer off the chain until it is in scope.
utils/focusReturn.tsadds its own reachability test (tabStopsIn,isRendered,canTakeReturnedFocus) beside the focus trap'sgetFocusableElements, without itsaria-hiddenrule, so returned focus can land on a control screen readers skip; please share one helper and remove the copy.- The description and changeset present the menu-row Dialog and the manual (
hasLightDismiss={false}) popover as falling to the page body, yet both already return focus to their trigger on the base branch (the new menu-row and nested manual-popover tests pass there), and the "one rule" does not reach the bottom-sheet presentations, so please narrow the changeset to the cases this change fixes.
|
|
||
| if (!isFocusLost(container, doc)) { | ||
| if (container == null || container.isConnected) { | ||
| restoreLayerFocus(record); |
There was a problem hiding this comment.
Miss: for Drawer this cleanup runs while the drawer's <dialog> is still modal: isActive turns false at once (Drawer.tsx:477-479), but close() waits for the slide-out (useDrawerDialogPresence.ts:166-179). Every candidate outside the drawer is then inert, isInertUnderModalDialog skips them all, and restoreLayerFocus warns "no element could take focus when a layer closed; focus is left on the page body" on every close. In Chromium focus in fact returns to the opener through Drawer's own return; the base logs nothing. Skipping the return here while container is an open modal dialog (its host returns focus after close(), as Dialog does) would remove the false warning.
… never strands focus (spec:AST-113 FR17–FR21)
18cc3b1 to
923cd1e
Compare
There was a problem hiding this comment.
Needs an engineering owner's decision on the spec, plus six fixes.
The shared focus return's stand-in chain after the opener, the parent-to-child handoff, the development warning and the rule that nothing turns the return off are defined only in draft spec:AST-113 FR17–FR21 and DEC-6, while the current Popover, Dialog and DropdownMenu contracts return focus only to the trigger or invoker. An engineering owner records the decision by approving and promoting AST-113 or by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and should also say whether the public useFocusTrap hook and Drawer, which now use the same chain, are in scope.
Blocking either way:
- The new closed-dialog check in
canTakeReturnedFocus(utils/focusReturn.ts) skips everything inside a non-modal Drawer or BottomSheet, which is a<dialog>shown withshowPopover()that never hasopen, so closing a Popover opened from one sends focus tomaininstead of its trigger; please keep the focus trap's popover-open exception and add a test for it. - DropdownMenu's
restoreFocusTomoves focus without the temporary tab stop the default move adds, so the ancestor, parent-layer andmainstand-ins cannot take focus and a menu removed while open still leaves focus on the page body; please add the tab stop for caller-supplied moves and cover DropdownMenu in themaintest. - The Dialog docs and the changeset tell callers to focus their target in
onOpenChange, but Dialog calls it while the modal still makes the page inert, so the call does nothing and focus goes to the opener; please document a route that works afterclose(), or say there is none yet. - A scrim Drawer's trap deactivates while its dialog is still modal for the slide-out, so every stand-in is inert and each close logs "no element could take focus … focus is left on the page body" although Drawer then returns focus to its opener; please skip the trap's return while its container is an open modal dialog, or keep Drawer off the chain until it is in scope.
utils/focusReturn.tsadds its own reachability test (tabStopsIn,isRendered,canTakeReturnedFocus) beside the focus trap'sgetFocusableElements, and it has already drifted (noaria-hiddenrule, no popover-open exception), so please share one helper and remove the copy.- The description and changeset present the menu-row Dialog and the manual (
hasLightDismiss={false}) popover as falling to the page body, yet both already return focus to their trigger on the base branch (the new menu-row and nested manual-popover tests pass there), and the "one rule" does not reach the bottom-sheet presentations, so please narrow the changeset to the cases this change fixes.
| !element.closest('[inert]') && | ||
| // A closed dialog may still be laid out (a display utility class beats | ||
| // its closed state); nothing inside it is a place to return to. | ||
| element.closest('dialog:not([open])') == null && |
There was a problem hiding this comment.
This test also rejects every element inside a non-modal Drawer or BottomSheet: both render <dialog popover="manual"> and open with showPopover(), which never sets open (Drawer.tsx:610, BottomSheet.tsx:965). Closing a Popover opened from such a panel then skips the trigger, its slot, its ancestors and the panel, and focus goes to main: in Chromium the base returns focus to the trigger, and this head moves it behind the drawer. The focus trap's own test keeps a popover-open dialog as shown (hooks/useFocusTrap.ts:109, !dialog.open && !isPopoverOpen(dialog)). Reuse it here, ideally through one shared helper, and add a popover-open <dialog> case to the test.
User need
Losing focus strands keyboard and screen reader users. Today each layer returns focus its own way, and each one falls to the page body in a common case:
The browser's own restore does not cover these. In Chromium 149, Firefox 151 and WebKit 26.5:
<dialog>whose opener still exists;Problem and solution fit
One shared return in
utils/focusReturn.ts(trackLayerFocus,isLayerFocusStranded,restoreLayerFocus), used by Popover (through the focus trap), Dialog and DropdownMenu (AST-113 FR17–FR21, DEC-6).When it acts. Only when focus entered the layer and is now inside it, inside another closing layer, or on nothing. Focus already outside stays put, including focus a caller moved in
onHideoronOpenChange.Handoff. The runtime records which layer opened which. A closing layer with a child that is still open moves nothing, and the child inherits its chain, so menu → dialog → the menu trigger works.
Chain. Focus goes to the first candidate that is connected, rendered, focusable (with a temporary tab stop if needed), not inert (including under an open modal dialog), not inside a closed dialog, and not in a closing layer:
aria-controlsnames the layer;main.It never lands on the body; a development warning fires if nothing qualifies.
Removal. A layer removed while open (with its owner, or its whole view) returns focus to its opener at once when the opener is still on the page. Otherwise it waits until the removal is done, so it never lands on an element the same removal is taking away, and it does not warn if nothing survives.
Timing. A popover returns focus in its
beforetoggleto closed, while it still holds focus, so the browser's restore has nothing to do and the ring follows the last input. A modal<dialog>keeps the page inert untilclose(), so Dialog reads whether focus is stranded first, closes, then returns focus.Move. Popover moves focus with a plain focus, as released, adding a temporary tab stop only when the element has none. Dialog and DropdownMenu keep their ring-follows-last-input moves.
No off switch. There is no option, and the types accept no
false. The Popover and Dialog docs say so.Current specification
spec:AST-113FR17–FR21, DEC-6 (draft, #11112). It supersedes the earlierreturnFocusoption this PR carried.Public delta and caller burden
restoreFocusTo.useFocusTrapInternalgainsgetTrigger,getAnchorandlayerId, and returnsreturnFocus;useLayer's close-start hook drives it for Popover.Design and alternatives
Evidence
layerFocusReturn.test.tsx(9):main, never the body;inertattribute for it);focus-drive.mjsfor a macOS run.showModal()does in a browser.useFocusTrap.test.tsx.Scope
Components: Popover, usePopover, Dialog, DropdownMenu, useFocusTrap (internal), utils/focusReturn (internal)
Testing
pnpm exec vitest run --project ui packages/core/src/utils/layerFocusReturn.test.tsx packages/core/src/Dialog packages/core/src/DropdownMenu packages/core/src/Popover packages/core/src/PowerSearch packages/core/src/Typeahead packages/core/src/hooks/useFocusTrap.test.tsxpnpm exec vitest run --project ui --maxWorkers=6 packages/core: 824 files, 21,891 of 21,892 tests passed. The one failure, Selector "shows the reason tooltip on keyboard focus", passes on its own on this head (twice) and on the base; it is a timing flake under full-suite load.tsc --noEmit(core), eslint: clean