fix(core): restore focus when a focus trap deactivates#3732
Open
bhamodi wants to merge 1 commit into
Open
Conversation
bhamodi
requested review from
cixzhang,
ejhammond,
imdreamrunner,
josephfarina,
marie-lucas,
nynexman4464 and
thedjpetersen
as code owners
|
@bhamodi is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
meta-cla
Bot
added
the
CLA Signed
bhamodi
force-pushed
the
a11y/focustrap-restore-focus
branch
from
c30ba23 to
a7b661a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useFocusTraptrapped focus but never restored it. The hook tracked focus inside the container (lastFocusRef,useFocusTrap.ts:207) purely for Tab-wrap logic, and its effect cleanups only removed listeners (useFocusTrap.ts:261-263,339-344) -- it never captureddocument.activeElementon activation nor returned focus to it on deactivation.The one caller of the hook is
usePopover(usePopover.tsx:335-- the onlyuseFocusTrap()call site in the package;Dialogmerely imports the helper utilitieshasActiveFocusTrapEscape/isImeKeyEventand does its own capture/restore atDialog.tsx:376/405). Astryx opens layers imperatively viashowPopover()(useLayer.tsx:333), so the browser's declarativepopovertargetauto-restore never applies. The result: closing a Popover via Escape or light dismiss dropped keyboard focus to<body>.usePopovereven declared atriggerElementRef(usePopover.tsx:319, assigned at357) intended for this, but it was never read -- dead code standing in for a missing restore.This fixes it in the primitive so every trap consumer benefits, via a single effect keyed on
isActive:document.activeElementas the restore target.Guard design (why it can't steal focus from consumers that self-restore)
The cleanup restores focus only when
document.activeElementisnull,document.body,document.documentElement, or still inside the (possibly already-unmounted) trap container. If focus has moved to some other element outside the trap, the restore is a no-op. This is safe becauseuseLayer.hide()fires the consumer'sonHidesynchronously (useLayer.tsx:357) before React re-renders and runs the trap cleanup -- so a consumer that refocuses its trigger inonHide(e.g.DropdownMenu.tsx:220) has already moved focus outside the trap by the time cleanup runs, and the primitive stands down. The restore is also guarded against a captured element that was removed from the DOM (isConnected+ focusable check), so it never throws.Changes
hooks/useFocusTrap.ts: add a capture-on-activate / restore-on-deactivate effect with the guard above; update the file header and hook JSDoc to document the restore.hooks/useFocusTrap.doc.mjs: document the restore behavior and add a best-practice note (rely on built-in restoration; add your ownonHideonly to send focus elsewhere).hooks/useFocusTrap.test.tsx: 4 restore tests (below).Popover/Popover.test.tsx: 2 integration tests (Escape + light dismiss return focus to the trigger).Consumer trace
useFocusTrap()is called in exactly one place --usePopover-- so the entire blast radius flows through Popover. Across the 16usePopoverconsumers in the package the guard produces three behaviors, all correct:DropdownMenu,Selector,BaseTypeahead,DateInput,DateTimeInput,DateRangeInput,MultiSelector,PowerSearch,TopNavMegaMenu): theironHidemoves focus to the trigger first -> primitive no-ops.role: "none"popups (comboboxes / listboxes / mention menu --Selector,Chat/useTriggerMenu, etc.): the trigger keeps DOM focus the whole time, so on closeactiveElementis already outside the trap -> primitive no-ops.Popover,TabMenu,SideNavItem/SideNavHeading,TopNavHeading/TopNavMenu): previously dropped focus to<body>; now focus returns to the trigger -- this is the fix.ContextMenuandCommandPaletteuseuseLayerdirectly (notusePopover) and are unaffected. No opt-out flag was needed -- no consumer test broke, so the default-on behavior with the guard is sufficient.Test plan
node_modules/.bin/vitest run --root . packages/core/src/hooks/useFocusTrap.test.tsx-- 10 pass. Four new tests underfocus restoration:node_modules/.bin/vitest run --root . packages/core/src/Popover/Popover.test.tsx-- 21 pass (2 new: Escape and light-dismiss both return focus to the trigger).node_modules/.bin/vitest run --root . packages/core/src-- 3851 pass across 182 files. No consumer test broke.node_modules/.bin/tsc --project packages/core/tsconfig.json --noEmit-- clean.node_modules/.bin/eslinton changed files -- clean.Notes
Found during a broader a11y audit of core components; scoped to one fix per PR. No existing test encoded the focus-drop-to-
<body>behavior, so nothing had to be weakened. The pre-existing unusedtriggerElementRefinusePopoveris left untouched to keep this change focused on the primitive; it is now redundant and can be removed in a separate cleanup. Real light dismiss is not simulable in jsdom, so the light-dismiss test drives the same code path the browser uses -- thetoggle(newState: "closed") event thatuseLayerlistens for.