Skip to content

Commit 9c5e0f3

Browse files
committed
fix(dialog): address Copilot review feedback
- Use @radix-ui/react-use-layout-effect instead of React.useLayoutEffect for SSR safety and consistency with other Radix packages - Add instanceof Document || instanceof ShadowRoot guard before calling getElementById; fall back to ownerDocument ?? document for DocumentFragment containers (e.g. portal into DocumentFragment) Both issues apply to TitleWarning and DescriptionWarning. Addresses Copilot review on #3840.
1 parent 196487e commit 9c5e0f3

File tree

2 files changed

+19
-3
lines changed

2 files changed

+19
-3
lines changed

packages/react/dialog/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
"@radix-ui/react-primitive": "workspace:*",
4848
"@radix-ui/react-slot": "workspace:*",
4949
"@radix-ui/react-use-controllable-state": "workspace:*",
50+
"@radix-ui/react-use-layout-effect": "workspace:*",
5051
"aria-hidden": "^1.2.4",
5152
"react-remove-scroll": "^2.6.3"
5253
},

packages/react/dialog/src/dialog.tsx

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { composeEventHandlers } from '@radix-ui/primitive';
33
import { useComposedRefs } from '@radix-ui/react-compose-refs';
44
import { createContext, createContextScope } from '@radix-ui/react-context';
55
import { useId } from '@radix-ui/react-id';
6+
import { useLayoutEffect } from '@radix-ui/react-use-layout-effect';
67
import { useControllableState } from '@radix-ui/react-use-controllable-state';
78
import { DismissableLayer } from '@radix-ui/react-dismissable-layer';
89
import { FocusScope } from '@radix-ui/react-focus-scope';
@@ -239,7 +240,7 @@ const DialogOverlayImpl = React.forwardRef<DialogOverlayImplElement, DialogOverl
239240
const isOpenRef = React.useRef(context.open);
240241
isOpenRef.current = context.open;
241242

242-
React.useLayoutEffect(() => {
243+
useLayoutEffect(() => {
243244
return () => {
244245
// Only perform synchronous cleanup for forced unmounts (navigation while dialog is open).
245246
// When the dialog closes normally, `context.open` is already `false` before this
@@ -580,8 +581,15 @@ For more information, see https://radix-ui.com/primitives/docs/components/${titl
580581
if (titleId) {
581582
// Use getRootNode() to support Shadow DOM contexts where document.getElementById
582583
// would fail to find elements rendered inside a shadow root.
584+
// Guard: getRootNode() may return a DocumentFragment or other Node without
585+
// getElementById (e.g. portal into a DocumentFragment). Fall back to
586+
// ownerDocument ?? document in those cases.
583587
const rootNode = contentRef.current?.getRootNode() ?? document;
584-
const hasTitle = (rootNode as Document | ShadowRoot).getElementById(titleId);
588+
const searchRoot =
589+
rootNode instanceof Document || rootNode instanceof ShadowRoot
590+
? rootNode
591+
: (contentRef.current?.ownerDocument ?? document);
592+
const hasTitle = searchRoot.getElementById(titleId);
585593
if (!hasTitle) console.error(MESSAGE);
586594
}
587595
}, [MESSAGE, contentRef, titleId]);
@@ -606,8 +614,15 @@ const DescriptionWarning: React.FC<DescriptionWarningProps> = ({ contentRef, des
606614
if (descriptionId && describedById) {
607615
// Use getRootNode() to support Shadow DOM contexts where document.getElementById
608616
// would fail to find elements rendered inside a shadow root.
617+
// Guard: getRootNode() may return a DocumentFragment or other Node without
618+
// getElementById (e.g. portal into a DocumentFragment). Fall back to
619+
// ownerDocument ?? document in those cases.
609620
const rootNode = contentRef.current?.getRootNode() ?? document;
610-
const hasDescription = (rootNode as Document | ShadowRoot).getElementById(descriptionId);
621+
const searchRoot =
622+
rootNode instanceof Document || rootNode instanceof ShadowRoot
623+
? rootNode
624+
: (contentRef.current?.ownerDocument ?? document);
625+
const hasDescription = searchRoot.getElementById(descriptionId);
611626
if (!hasDescription) console.warn(MESSAGE);
612627
}
613628
}, [MESSAGE, contentRef, descriptionId]);

0 commit comments

Comments
 (0)