[popover][menu] De-duplicate focus guard handlers for popover and menu triggers#4522
[popover][menu] De-duplicate focus guard handlers for popover and menu triggers#4522sai6855 merged 5 commits intomui:masterfrom
Conversation
sai6855
added
component: menu
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
sai6855
requested review from
LukasTy,
atomiks,
colmtuite,
flaviendelangle and
michaldudak
as code owners
There was a problem hiding this comment.
Pull request overview
This PR refactors duplicated focus-guard tab-out handling in PopoverTrigger and MenuTrigger into a shared useTriggerFocusGuards hook under utils/popups, aiming to reduce duplication and centralize the focus-management behavior.
Changes:
- Added
useTriggerFocusGuardshook that encapsulates the pre/post focus guard handlers and refs. - Updated
PopoverTriggerandMenuTriggerto use the shared hook instead of local duplicated logic. - Re-exported the new hook from
utils/popupsbarrel.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/utils/popups/useTriggerFocusGuards.ts | Introduces shared focus guard behavior for popup triggers. |
| packages/react/src/utils/popups/index.ts | Exposes the new hook via the popups barrel export. |
| packages/react/src/popover/trigger/PopoverTrigger.tsx | Replaces inline focus-guard logic with the shared hook. |
| packages/react/src/menu/trigger/MenuTrigger.tsx | Replaces inline focus-guard logic with the shared hook. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export * from './popupStoreUtils'; | ||
| export * from './popupTriggerMap'; | ||
| export * from './store'; | ||
| export * from './useTriggerFocusGuards'; |
There was a problem hiding this comment.
Re-exporting useTriggerFocusGuards from this barrel will cause every import ... from '../../utils/popups' to pull in a 'use client' module (and react-dom), which can unintentionally turn otherwise non-client modules into client-only in React Server Components setups. Since many non-'use client' files import from ../../utils/popups (e.g. stores/utilities), consider not exporting this hook from the barrel and instead importing it directly from ./useTriggerFocusGuards in PopoverTrigger/MenuTrigger, or split out a dedicated client-only barrel.
| export * from './useTriggerFocusGuards'; |
| /** | ||
| * Minimal store interface required by the focus guard hook. | ||
| * Both PopoverStore and MenuStore satisfy this interface. | ||
| */ | ||
| interface TriggerFocusGuardStore { | ||
| setOpen(open: boolean, eventDetails: any): void; | ||
| select(key: 'positionerElement', ...args: any[]): HTMLElement | null; | ||
| context: { | ||
| readonly beforeContentFocusGuardRef: React.RefObject<HTMLElement | null>; | ||
| readonly triggerFocusTargetRef: React.RefObject<HTMLElement | null>; | ||
| }; |
There was a problem hiding this comment.
TriggerFocusGuardStore currently uses any for setOpen's eventDetails and select args, which weakens type-safety for a shared hook that’s meant to be reused across popup stores. Consider typing eventDetails as Parameters<Store['setOpen']>[1] via a generic store parameter (or otherwise referencing the store’s ChangeEventDetails type), and narrowing select to just the 'positionerElement' selector signature used here.
atomiks
left a comment
atomiks
left a comment
There was a problem hiding this comment.
Codex Review (GPT-5.4)
Dominant type: refactor (secondary: tests, react-ui). I reviewed the full branch diff against a freshly fetched upstream/master; the extraction looks behavior-preserving overall, but it leaves one menu-side regression path unpinned.
1. Bugs / Issues
1. 🟡 Missing menu regression for the newly shared trigger focus guards (non-blocking)
packages/react/src/utils/popups/useTriggerFocusGuards.ts
const handlePreFocusGuardFocus = useStableCallback((event: React.FocusEvent) => {
ReactDOM.flushSync(() => {
store.setOpen(false, createChangeEventDetails(REASONS.focusOut, event.nativeEvent, event.currentTarget as HTMLElement));
});
const previousTabbable: FocusableElement | null = getTabbableBeforeElement(preFocusGuardRef.current);
previousTabbable?.focus();
});
const handleFocusTargetFocus = useStableCallback((event: React.FocusEvent) => {
const positionerElement = store.select('positionerElement');
if (positionerElement && isOutsideEvent(event, positionerElement)) {
store.context.beforeContentFocusGuardRef.current?.focus();
} else {
// closes and advances focus past the trigger/popup
}
});packages/react/src/menu/root/MenuRoot.test.tsx
it('closes submenus when focus is lost by shift-tabbing from a nested menu', async () => {
// ...
await user.keyboard('{Shift>}{Tab}{/Shift}');
// ...
});This hook now owns MenuTrigger's root-trigger tab-out behavior too, but the menu suites still only cover nested submenu/dialog Shift+Tab scenarios and never directly assert the root trigger guards themselves. Popover already has explicit forward/backward tab coverage for the same extracted logic, so a future menu-only regression in this shared hook could slip through without a failing test.
Recommendation: Add at least one root-menu regression that tabs forward from an open menu to the element after the trigger, and one backward-tab case that returns focus to the trigger while preserving the expected close behavior.
Behavior Preservation Assessment
The extraction is a straight lift of the previous PopoverTrigger/MenuTrigger handlers: the flushSync timing, focusOut event details, and popup-skipping tabbable loop all match the pre-refactor code. Importing the hook directly also avoids the client-only barrel issue that would have been riskier.
Test Coverage Assessment
I ran these targeted validations and they passed:
pnpm test:jsdom PopoverTrigger --no-watchpnpm test:jsdom PopoverRoot --no-watchpnpm test:jsdom MenuRoot --no-watchpnpm test:chromium PopoverRoot --no-watchpnpm test:chromium MenuRoot --no-watch
Those runs make me comfortable with the refactor itself; the remaining concern is specifically the missing menu regression around the newly shared root-trigger focus-guard path.
Recommendation
Approve after nits are addressed 🟡
I do not see a behavior regression in the branch as-is, but I would add the missing menu coverage before merging so this shared hook is protected on both consumers rather than only implicitly through popover-oriented tests.
|
@atomiks added tests recommended by codex |
This PR refactors the focus guard logic used in MenuTrigger and PopoverTrigger by extracting it into a shared hook (useTriggerFocusGuards) to reduce duplication and improve maintainability.