From 10a410eff1ac559c0d9c31f24536c77250ae2656 Mon Sep 17 00:00:00 2001 From: Chance Strickland Date: Tue, 2 Jun 2026 18:05:25 -0700 Subject: [PATCH 1/2] Fix issues with nested modal/non-modal layers --- .changeset/huge-foxes-serve.md | 14 ++ apps/storybook/stories/dialog.stories.tsx | 156 +++++++++++++++++- packages/react/dialog/src/dialog.tsx | 38 ++++- .../focus-scope/src/focus-scope.test.tsx | 39 +++++ .../react/focus-scope/src/focus-scope.tsx | 93 ++++++++++- packages/react/focus-scope/src/index.ts | 5 +- packages/react/hover-card/package.json | 1 + packages/react/hover-card/src/hover-card.tsx | 11 +- packages/react/menu/src/menu.tsx | 45 ++++- packages/react/popover/src/popover.tsx | 61 ++++++- pnpm-lock.yaml | 3 + 11 files changed, 443 insertions(+), 23 deletions(-) create mode 100644 .changeset/huge-foxes-serve.md diff --git a/.changeset/huge-foxes-serve.md b/.changeset/huge-foxes-serve.md new file mode 100644 index 0000000000..5a3c780afc --- /dev/null +++ b/.changeset/huge-foxes-serve.md @@ -0,0 +1,14 @@ +--- +"@radix-ui/react-focus-scope": patch +"@radix-ui/react-dialog": patch +"@radix-ui/react-popover": patch +"@radix-ui/react-menu": patch +"@radix-ui/react-hover-card": patch +"radix-ui": patch +--- + +Fixed nested, portalled layers being unusable inside a modal layer. A non-modal popover rendered inside a modal Dialog previously broke some user interactions because the modal layer's trapped `FocusScope` reclaimed focus, and its `RemoveScroll` only allowed scrolling within the modal content. + +`FocusScope` now accepts branches that are treated as part of the scope, and modal layers register nested portalled layers as branches so focus and scroll work as expected. This works for any depth of nesting between modal containers. + +See: https://github.com/radix-ui/primitives/issues/3423 diff --git a/apps/storybook/stories/dialog.stories.tsx b/apps/storybook/stories/dialog.stories.tsx index e6fb384a74..f0a092b769 100644 --- a/apps/storybook/stories/dialog.stories.tsx +++ b/apps/storybook/stories/dialog.stories.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { Dialog } from 'radix-ui'; +import { Dialog, DropdownMenu, HoverCard, Popover } from 'radix-ui'; import styles from './dialog.stories.module.css'; export default { title: 'Components/Dialog' }; @@ -536,3 +536,157 @@ export const Cypress = () => { ); }; + +/** + * Verification for https://github.com/radix-ui/primitives/issues/3423 + * + * A `Popover` (mimicking a cmdk `Command` with an input + scrollable list) nested inside a *modal* + * `Dialog`. You should be able to type in the input and scroll the list with `Popover modal` either + * ON or OFF: + * + * - OFF (non-modal popover, the shadcn default): the popover registers its content as a branch of + * the Dialog, so the Dialog's trapped `FocusScope` no longer reclaims focus and its `RemoveScroll` + * treats the popover as a scroll shard. + * - ON (modal popover): the popover mounts its own `RemoveScroll` + trapped `FocusScope`, which + * pauses the Dialog's. + * + * Before the fix, the OFF case could neither be typed in nor scrolled. + */ +export const PopoverInModalDialog = () => { + const [popoverModal, setPopoverModal] = React.useState(false); + return ( + + open dialog + + + + Dialog with a nested Popover + + Try to type in the popover's input and scroll its list. + + + + + + open popover + + + +
+ {Array.from({ length: 40 }, (_, i) => ( +
+ Item {i + 1} +
+ ))} +
+ + + + +
+ close + + + + ); +}; + +/** + * Verification for https://github.com/radix-ui/primitives/issues/3423 (other primitives) + * + * A *non-modal* `DropdownMenu` with a scrollable list and a `HoverCard` with a focusable link, + * both nested inside a *modal* `Dialog`. Before the fix, the Dialog's trapped `FocusScope` would + * reclaim focus from these portalled layers (and `RemoveScroll` would block scrolling). They now + * register their content as branches of the Dialog, so keyboard navigation, focusing the link, and + * scrolling all work. + */ +export const NestedLayersInModalDialog = () => { + return ( + + open dialog + + + + Dialog with nested menu / hover card + + Open the menu and arrow-key through / scroll it, and hover the card to focus its link. + + +
+ + open menu + + + {Array.from({ length: 30 }, (_, i) => ( + + Item {i + 1} + + ))} + + + + + + + hover me + + + + This card has a{' '} + + focusable link + + . + + + +
+ + close + + + + ); +}; diff --git a/packages/react/dialog/src/dialog.tsx b/packages/react/dialog/src/dialog.tsx index 72eaf92a2b..578ad99c52 100644 --- a/packages/react/dialog/src/dialog.tsx +++ b/packages/react/dialog/src/dialog.tsx @@ -5,7 +5,11 @@ import { createContext, createContextScope } from '@radix-ui/react-context'; import { useId } from '@radix-ui/react-id'; import { useControllableState } from '@radix-ui/react-use-controllable-state'; import { DismissableLayer } from '@radix-ui/react-dismissable-layer'; -import { FocusScope } from '@radix-ui/react-focus-scope'; +import { + FocusScope, + FocusScopeBranchProvider, + useFocusScopeBranchRegistry, +} from '@radix-ui/react-focus-scope'; import { Portal as PortalPrimitive } from '@radix-ui/react-portal'; import { Presence } from '@radix-ui/react-presence'; import { Primitive } from '@radix-ui/react-primitive'; @@ -15,6 +19,7 @@ import { hideOthers } from 'aria-hidden'; import { createSlot } from '@radix-ui/react-slot'; import type { Scope } from '@radix-ui/react-context'; +import type { FocusScopeBranchRegistry } from '@radix-ui/react-focus-scope'; /* ------------------------------------------------------------------------------------------------- * Dialog @@ -35,6 +40,11 @@ type DialogContextValue = { onOpenChange(open: boolean): void; onOpenToggle(): void; modal: boolean; + // Nodes of nested, portalled layers (eg. a non-modal `Popover`) that should + // be treated as part of this Dialog for focus trapping and scroll locking. + // See https://github.com/radix-ui/primitives/issues/3423 + branchNodes: HTMLElement[]; + branchRegistry: FocusScopeBranchRegistry; }; const [DialogProvider, useDialogContext] = createDialogContext(DIALOG_NAME); @@ -58,6 +68,7 @@ const Dialog: React.FC = (props: ScopedProps) => { } = props; const triggerRef = React.useRef(null); const contentRef = React.useRef(null); + const { nodes: branchNodes, registry: branchRegistry } = useFocusScopeBranchRegistry(); const [open, setOpen] = useControllableState({ prop: openProp, defaultProp: defaultOpen ?? false, @@ -77,6 +88,8 @@ const Dialog: React.FC = (props: ScopedProps) => { onOpenChange={setOpen} onOpenToggle={React.useCallback(() => setOpen((prevOpen) => !prevOpen), [setOpen])} modal={modal} + branchNodes={branchNodes} + branchRegistry={branchRegistry} > {children} @@ -201,9 +214,15 @@ const DialogOverlayImpl = React.forwardRef + // Make sure `Content` is scrollable even when it doesn't live inside + // `RemoveScroll` (eg. when `Overlay` and `Content` are siblings). Nested + // layers are registered as branches and added as shards so they remain + // scrollable too. See https://github.com/radix-ui/primitives/issues/3423 + ({ current: node }))]} + > ( (props: ScopedProps, forwardedRef) => { const { __scopeDialog, trapFocus, onOpenAutoFocus, onCloseAutoFocus, ...contentProps } = props; + const { children, ...layerProps } = contentProps; const context = useDialogContext(CONTENT_NAME, __scopeDialog); const contentRef = React.useRef(null); const composedRefs = useComposedRefs(forwardedRef, contentRef); @@ -400,6 +420,7 @@ const DialogContentImpl = React.forwardRef @@ -409,10 +430,15 @@ const DialogContentImpl = React.forwardRef context.onOpenChange(false)} - /> + > + {/* Lets nested, portalled layers register themselves as branches of this Dialog. */} + + {children} + + {process.env.NODE_ENV !== 'production' && ( <> diff --git a/packages/react/focus-scope/src/focus-scope.test.tsx b/packages/react/focus-scope/src/focus-scope.test.tsx index 10c448843f..1d51a57b5a 100644 --- a/packages/react/focus-scope/src/focus-scope.test.tsx +++ b/packages/react/focus-scope/src/focus-scope.test.tsx @@ -92,6 +92,45 @@ describe('FocusScope', () => { }); }); + describe('given a trapped FocusScope with branches', () => { + function BranchHarness({ withBranch }: { withBranch: boolean }) { + const [branch, setBranch] = React.useState(null); + return ( +
+ +
+ + + +
+ {/* Simulates portalled content of a nested layer living outside the scope's subtree */} +
+ +
+
+ ); + } + + it('reclaims focus moved into an unregistered outside node', async () => { + const rendered = render(); + const inner = rendered.getByLabelText(INNER_NAME_INPUT_LABEL) as HTMLInputElement; + const branchInput = rendered.getByLabelText('branch-input') as HTMLInputElement; + inner.focus(); + branchInput.focus(); + await waitFor(() => expect(inner).toHaveFocus()); + expect(branchInput).not.toHaveFocus(); + }); + + it('keeps focus when it moves into a registered branch', async () => { + const rendered = render(); + const inner = rendered.getByLabelText(INNER_NAME_INPUT_LABEL) as HTMLInputElement; + const branchInput = rendered.getByLabelText('branch-input') as HTMLInputElement; + inner.focus(); + branchInput.focus(); + await waitFor(() => expect(branchInput).toHaveFocus()); + }); + }); + describe('given a FocusScope with internal focus handlers', () => { const handleLastFocusableElementBlur = vi.fn(); let rendered: RenderResult; diff --git a/packages/react/focus-scope/src/focus-scope.tsx b/packages/react/focus-scope/src/focus-scope.tsx index 605c1e79a0..4812a8c556 100644 --- a/packages/react/focus-scope/src/focus-scope.tsx +++ b/packages/react/focus-scope/src/focus-scope.tsx @@ -32,6 +32,16 @@ interface FocusScopeProps extends PrimitiveDivProps { */ trapped?: boolean; + /** + * A list of nodes that should be treated as part of the focus scope even + * though they don't live within the scope's DOM subtree (eg. portalled + * content of a nested, non-modal layer). When the scope is `trapped`, focus + * is allowed to move into these branches without being reclaimed. + * + * See: https://github.com/radix-ui/primitives/issues/3423 + */ + branches?: HTMLElement[]; + /** * Event handler called when auto-focusing on mount. * Can be prevented. @@ -49,6 +59,7 @@ const FocusScope = React.forwardRef((props, const { loop = false, trapped = false, + branches, onMountAutoFocus: onMountAutoFocusProp, onUnmountAutoFocus: onUnmountAutoFocusProp, ...scopeProps @@ -59,6 +70,24 @@ const FocusScope = React.forwardRef((props, const lastFocusedElementRef = React.useRef(null); const composedRefs = useComposedRefs(forwardedRef, (node) => setContainer(node)); + // Keep the latest branches in a ref so the trap effect below doesn't need to resubscribe its + // listeners whenever the branch list updates. We sync it in the commit phase (not during render) + // to stay safe under concurrent rendering, where a render can be discarded or replayed. The trap's + // focus event handlers only run in response to user interaction (well after commit), so reading + // the ref from them always sees the committed branch list. + const branchesRef = React.useRef(branches); + React.useEffect(() => { + branchesRef.current = branches; + }); + const isTargetInScope = React.useCallback( + (target: HTMLElement | null) => { + if (!target) return false; + if (container?.contains(target)) return true; + return Boolean(branchesRef.current?.some((branch) => branch.contains(target))); + }, + [container], + ); + const focusScope = React.useRef({ paused: false, pause() { @@ -75,7 +104,7 @@ const FocusScope = React.forwardRef((props, function handleFocusIn(event: FocusEvent) { if (focusScope.paused || !container) return; const target = event.target as HTMLElement | null; - if (container.contains(target)) { + if (isTargetInScope(target)) { lastFocusedElementRef.current = target; } else { focus(lastFocusedElementRef.current, { select: true }); @@ -99,8 +128,9 @@ const FocusScope = React.forwardRef((props, if (relatedTarget === null) return; // If the focus has moved to an actual legitimate element (`relatedTarget !== null`) - // that is outside the container, we move focus to the last valid focused element inside. - if (!container.contains(relatedTarget)) { + // that is outside the container (and any registered branches), we move focus to the last + // valid focused element inside. + if (!isTargetInScope(relatedTarget)) { focus(lastFocusedElementRef.current, { select: true }); } } @@ -127,7 +157,7 @@ const FocusScope = React.forwardRef((props, mutationObserver.disconnect(); }; } - }, [trapped, container, focusScope.paused]); + }, [trapped, container, focusScope.paused, isTargetInScope]); React.useEffect(() => { if (container) { @@ -207,6 +237,56 @@ const FocusScope = React.forwardRef((props, FocusScope.displayName = FOCUS_SCOPE_NAME; +/* ------------------------------------------------------------------------------------------------- + * FocusScope branch registry + * -----------------------------------------------------------------------------------------------*/ + +/** + * Lets portalled content that is a React descendant of a (modal) layer — but rendered outside of + * that layer's DOM subtree — register itself as a "branch" of the layer's focus scope. + * + * A modal container (eg. `Dialog`) creates a registry via `useFocusScopeBranchRegistry`, renders a + * `FocusScopeBranchProvider` around its children, and passes the collected `nodes` to its trapped + * `FocusScope` (so focus isn't reclaimed from the branch) and, if applicable, to its `RemoveScroll` + * `shards` (so the branch remains scrollable). Nested layers (eg. a non-modal `Popover`) register + * their content node with `useFocusScopeBranch`. + * + * See: https://github.com/radix-ui/primitives/issues/3423 + */ +interface FocusScopeBranchRegistry { + add: (node: HTMLElement) => void; + remove: (node: HTMLElement) => void; +} + +const FocusScopeBranchContext = React.createContext(null); + +const FocusScopeBranchProvider = FocusScopeBranchContext.Provider; + +function useFocusScopeBranchRegistry() { + const [nodes, setNodes] = React.useState([]); + const registry = React.useMemo( + () => ({ + add: (node) => setNodes((prev) => (prev.includes(node) ? prev : [...prev, node])), + remove: (node) => setNodes((prev) => prev.filter((current) => current !== node)), + }), + [], + ); + return { nodes, registry } as const; +} + +/** + * Registers `node` as a branch of the nearest ancestor `FocusScopeBranchProvider`. No-ops when + * there is no ancestor registry (eg. the layer isn't nested inside a modal layer). + */ +function useFocusScopeBranch(node: HTMLElement | null) { + const registry = React.useContext(FocusScopeBranchContext); + React.useEffect(() => { + if (!node || !registry) return; + registry.add(node); + return () => registry.remove(node); + }, [node, registry]); +} + /* ------------------------------------------------------------------------------------------------- * Utils * -----------------------------------------------------------------------------------------------*/ @@ -346,7 +426,10 @@ const Root = FocusScope; export { FocusScope, + FocusScopeBranchProvider, + useFocusScopeBranchRegistry, + useFocusScopeBranch, // Root, }; -export type { FocusScopeProps }; +export type { FocusScopeProps, FocusScopeBranchRegistry }; diff --git a/packages/react/focus-scope/src/index.ts b/packages/react/focus-scope/src/index.ts index 4d6601a8a1..cc7ec37816 100644 --- a/packages/react/focus-scope/src/index.ts +++ b/packages/react/focus-scope/src/index.ts @@ -1,7 +1,10 @@ 'use client'; export { FocusScope, + FocusScopeBranchProvider, + useFocusScopeBranchRegistry, + useFocusScopeBranch, // Root, } from './focus-scope'; -export type { FocusScopeProps } from './focus-scope'; +export type { FocusScopeProps, FocusScopeBranchRegistry } from './focus-scope'; diff --git a/packages/react/hover-card/package.json b/packages/react/hover-card/package.json index e98d6e4db5..b82cb3d9e7 100644 --- a/packages/react/hover-card/package.json +++ b/packages/react/hover-card/package.json @@ -39,6 +39,7 @@ "@radix-ui/react-compose-refs": "workspace:*", "@radix-ui/react-context": "workspace:*", "@radix-ui/react-dismissable-layer": "workspace:*", + "@radix-ui/react-focus-scope": "workspace:*", "@radix-ui/react-popper": "workspace:*", "@radix-ui/react-portal": "workspace:*", "@radix-ui/react-presence": "workspace:*", diff --git a/packages/react/hover-card/src/hover-card.tsx b/packages/react/hover-card/src/hover-card.tsx index 2af243eb90..921479fa30 100644 --- a/packages/react/hover-card/src/hover-card.tsx +++ b/packages/react/hover-card/src/hover-card.tsx @@ -9,6 +9,7 @@ import { Portal as PortalPrimitive } from '@radix-ui/react-portal'; import { Presence } from '@radix-ui/react-presence'; import { Primitive } from '@radix-ui/react-primitive'; import { DismissableLayer } from '@radix-ui/react-dismissable-layer'; +import { useFocusScopeBranch } from '@radix-ui/react-focus-scope'; import type { Scope } from '@radix-ui/react-context'; @@ -269,7 +270,15 @@ const HoverCardContentImpl = React.forwardRef< const context = useHoverCardContext(CONTENT_NAME, __scopeHoverCard); const popperScope = usePopperScope(__scopeHoverCard); const ref = React.useRef(null); - const composedRefs = useComposedRefs(forwardedRef, ref); + + // When this `HoverCard` is nested inside a modal layer (eg. a `Dialog`) but portalled outside of + // it, register its content with the ancestor layer so focus isn't reclaimed and scroll isn't + // locked for any interactive content within it. No-ops when there is no ancestor layer. + // See: https://github.com/radix-ui/primitives/issues/3423 + const [branchNode, setBranchNode] = React.useState(null); + const composedRefs = useComposedRefs(forwardedRef, ref, setBranchNode); + useFocusScopeBranch(branchNode); + const [containSelection, setContainSelection] = React.useState(false); React.useEffect(() => { diff --git a/packages/react/menu/src/menu.tsx b/packages/react/menu/src/menu.tsx index dff0c1e9e0..accc6ab28a 100644 --- a/packages/react/menu/src/menu.tsx +++ b/packages/react/menu/src/menu.tsx @@ -6,7 +6,12 @@ import { createContextScope } from '@radix-ui/react-context'; import { useDirection } from '@radix-ui/react-direction'; import { DismissableLayer } from '@radix-ui/react-dismissable-layer'; import { useFocusGuards } from '@radix-ui/react-focus-guards'; -import { FocusScope } from '@radix-ui/react-focus-scope'; +import { + FocusScope, + FocusScopeBranchProvider, + useFocusScopeBranch, + useFocusScopeBranchRegistry, +} from '@radix-ui/react-focus-scope'; import { useId } from '@radix-ui/react-id'; import * as PopperPrimitive from '@radix-ui/react-popper'; import { createPopperScope } from '@radix-ui/react-popper'; @@ -373,6 +378,7 @@ const MenuContentImpl = React.forwardRef(null); const contentRef = React.useRef(null); - const composedRefs = useComposedRefs(forwardedRef, contentRef, context.onContentChange); + + // When this `Menu` is nested inside a modal layer (eg. a `Dialog`) but portalled outside of it, + // register its content with the ancestor layer so focus isn't reclaimed and scroll isn't locked + // for it (only relevant for non-modal menus, which don't trap focus / lock scroll themselves). + // It also exposes its own registry so nested, portalled layers can do the same against it. + // See: https://github.com/radix-ui/primitives/issues/3423 + const [branchNode, setBranchNode] = React.useState(null); + useFocusScopeBranch(branchNode); + const { nodes: branchNodes, registry: branchRegistry } = useFocusScopeBranchRegistry(); + // Only modal menus trap focus / lock scroll, so only they need to host branches. Non-modal + // menus stay transparent and let nested layers register against the next modal ancestor. + const isModal = Boolean(trapFocus || disableOutsideScroll); + const composedRefs = useComposedRefs( + forwardedRef, + contentRef, + context.onContentChange, + setBranchNode, + ); const timerRef = React.useRef(0); const searchRef = React.useRef(''); const pointerGraceTimerRef = React.useRef(0); @@ -392,7 +415,11 @@ const MenuContentImpl = React.forwardRef ({ current: node }))], + } : undefined; const handleTypeaheadSearch = (key: string) => { @@ -466,6 +493,7 @@ const MenuContentImpl = React.forwardRef { // when opening, explicitly focus the content area only and leave // `onEntryFocus` in control of focusing first item @@ -551,7 +579,16 @@ const MenuContentImpl = React.forwardRef + > + {/* Lets nested, portalled layers register themselves as branches of this menu. */} + {isModal ? ( + + {children} + + ) : ( + children + )} + diff --git a/packages/react/popover/src/popover.tsx b/packages/react/popover/src/popover.tsx index 8f89bccdc4..cc57de3974 100644 --- a/packages/react/popover/src/popover.tsx +++ b/packages/react/popover/src/popover.tsx @@ -4,7 +4,13 @@ import { useComposedRefs } from '@radix-ui/react-compose-refs'; import { createContextScope } from '@radix-ui/react-context'; import { DismissableLayer } from '@radix-ui/react-dismissable-layer'; import { useFocusGuards } from '@radix-ui/react-focus-guards'; -import { FocusScope } from '@radix-ui/react-focus-scope'; +import { + FocusScope, + FocusScopeBranchProvider, + useFocusScopeBranch, + useFocusScopeBranchRegistry, +} from '@radix-ui/react-focus-scope'; +import type { FocusScopeBranchRegistry } from '@radix-ui/react-focus-scope'; import { useId } from '@radix-ui/react-id'; import * as PopperPrimitive from '@radix-ui/react-popper'; import { createPopperScope } from '@radix-ui/react-popper'; @@ -245,7 +251,7 @@ const Slot = createSlot('PopoverContent.RemoveScroll'); type PopoverContentTypeElement = PopoverContentImplElement; interface PopoverContentTypeProps extends Omit< PopoverContentImplProps, - 'trapFocus' | 'disableOutsidePointerEvents' + 'trapFocus' | 'disableOutsidePointerEvents' | 'branchRegistry' | 'branchNodes' > {} const PopoverContentModal = React.forwardRef( @@ -255,6 +261,11 @@ const PopoverContentModal = React.forwardRef { const content = contentRef.current; @@ -262,10 +273,16 @@ const PopoverContentModal = React.forwardRef + ({ current: node }))]} + > ( @@ -391,11 +423,23 @@ const PopoverContentImpl = React.forwardRef(null); + const composedRefs = useComposedRefs(forwardedRef, setContentNode); + useFocusScopeBranch(contentNode); + // Make sure the whole tree has focus guards as our `Popover` may be // the last element in the DOM (because of the `Portal`) useFocusGuards(); @@ -405,6 +449,7 @@ const PopoverContentImpl = React.forwardRef @@ -423,7 +468,7 @@ const PopoverContentImpl = React.forwardRef + > + {branchRegistry ? ( + {children} + ) : ( + children + )} + ); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5715fa5193..45e0e437f0 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1037,6 +1037,9 @@ importers: '@radix-ui/react-dismissable-layer': specifier: workspace:* version: link:../dismissable-layer + '@radix-ui/react-focus-scope': + specifier: workspace:* + version: link:../focus-scope '@radix-ui/react-popper': specifier: workspace:* version: link:../popper From 0bf8f0e5b74476bcda79e80eba17ae753633528c Mon Sep 17 00:00:00 2001 From: Chance Strickland Date: Wed, 3 Jun 2026 08:21:30 -0700 Subject: [PATCH 2/2] revert to fragment for displayed select value --- packages/react/select/src/select.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/select/src/select.tsx b/packages/react/select/src/select.tsx index e66b77fbfa..478ebcdbb6 100644 --- a/packages/react/select/src/select.tsx +++ b/packages/react/select/src/select.tsx @@ -387,7 +387,7 @@ const SelectValue = React.forwardRef( // through the item they came from style={{ pointerEvents: 'none' }} > - {showPlaceholder ? placeholder : children} + {showPlaceholder ? <>{placeholder} : children} ); },