fix(dialog): use getRootNode() for Shadow DOM accessibility check#3839
fix(dialog): use getRootNode() for Shadow DOM accessibility check#3839sujiu222 wants to merge 4 commits intoradix-ui:mainfrom
Conversation
The DialogTitle and DialogDescription accessibility warnings used document.getElementById to find elements for validation. When Dialog is rendered inside a Shadow DOM, elements are not accessible via document.getElementById, causing false-positive accessibility warnings in the console. Fix: Use contentRef.current.getRootNode() to obtain the correct root node (Document or ShadowRoot), then call getElementById on that root. This works in both regular DOM and Shadow DOM contexts, since ShadowRoot implements DocumentOrShadowRoot which defines getElementById. Fixes radix-ui#3814
🦋 Changeset detectedLatest commit: d0960ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Fixes false-positive Dialog accessibility warnings when Dialog is rendered inside a Shadow DOM by resolving title/description elements via the correct root node instead of the global document.
Changes:
- Pass
contentRefintoTitleWarningso it can resolve elements relative to Dialog content. - Use
contentRef.current.getRootNode()(withdocumentfallback) to look upDialogTitle/DialogDescriptionby id in both warning components.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use getRootNode() to support Shadow DOM contexts where document.getElementById | ||
| // would fail to find elements rendered inside a shadow root. | ||
| const rootNode = contentRef.current?.getRootNode() ?? document; | ||
| const hasTitle = (rootNode as Document | ShadowRoot).getElementById(titleId); | ||
| if (!hasTitle) console.error(MESSAGE); |
There was a problem hiding this comment.
getRootNode() can return a Node that is neither Document nor ShadowRoot (e.g. a detached subtree root element or a DocumentFragment). In those cases, the cast to Document | ShadowRoot can cause a runtime TypeError when calling getElementById. Consider guarding with a runtime check (e.g. verify rootNode has getElementById) and otherwise fall back to contentRef.current?.ownerDocument ?? document.
| // Use getRootNode() to support Shadow DOM contexts where document.getElementById | ||
| // would fail to find elements rendered inside a shadow root. | ||
| const rootNode = contentRef.current?.getRootNode() ?? document; | ||
| const hasDescription = (rootNode as Document | ShadowRoot).getElementById(descriptionId); | ||
| if (!hasDescription) console.warn(MESSAGE); |
There was a problem hiding this comment.
Same concern as in TitleWarning: contentRef.current?.getRootNode() may return a Node without getElementById, which would throw at runtime. Add a runtime guard (or resolve via ownerDocument unless the root is a ShadowRoot) before calling getElementById.
packages/react/dialog/src/dialog.tsx
Outdated
| // Use getRootNode() to support Shadow DOM contexts where document.getElementById | ||
| // would fail to find elements rendered inside a shadow root. | ||
| const rootNode = contentRef.current?.getRootNode() ?? document; | ||
| const hasTitle = (rootNode as Document | ShadowRoot).getElementById(titleId); |
There was a problem hiding this comment.
This change adds Shadow DOM-specific behavior, but the existing test suite for these warnings doesn’t cover rendering the dialog inside a ShadowRoot. Adding a regression test (e.g. mounting into an element with attachShadow({mode: 'open'}) and asserting the title/description warnings don’t fire) would help ensure this doesn’t break in future refactors.
getRootNode() may return a Node without getElementById (e.g. detached subtree or DocumentFragment). Add instanceof guard and fall back to ownerDocument ?? document in those cases. Addresses Copilot review feedback on radix-ui#3839.
|
Thanks for the review! Addressed all three points in the latest commit:
|
Adds a test that mounts Dialog inside a ShadowRoot (attachShadow) and asserts that no console.error accessibility warning is fired when DialogTitle is present, verifying the getRootNode() fix works correctly. Addresses Copilot review feedback on radix-ui#3839.
Summary
Fixes #3814
Problem
Dialogcomponent usesdocument.getElementByIdto find theDialogTitleandDialogDescriptionelements for accessibility validation. When Dialog is rendered inside a Shadow DOM, elements are not accessible viadocument.getElementById, causing false-positive accessibility warnings in the console:Root Cause
Both
TitleWarningandDescriptionWarningcomponents calleddocument.getElementById(id)to check for the presence of the title/description elements. In Shadow DOM, elements reside in aShadowRoot, not in the top-leveldocument, sodocument.getElementByIdalways returnsnull.Solution
Replace
document.getElementByIdwithcontentRef.current?.getRootNode()to obtain the correct root node, then callgetElementByIdon that root. This correctly handles both:getRootNode()returns theDocument, same behavior as beforegetRootNode()returns theShadowRoot, which has its owngetElementById(via theDocumentOrShadowRootinterface)The
TitleWarningcomponent was also updated to receivecontentRef(already available inDialogContentImpl) alongside the existingcontentRefalready passed toDescriptionWarning.Changes
packages/react/dialog/src/dialog.tsxcontentRefprop toTitleWarningPropsandTitleWarningcomponentcontentRefto<TitleWarning>inDialogContentImpldocument.getElementByIdwith(contentRef.current?.getRootNode() ?? document).getElementByIdin both warning componentsTesting
getRootNode()returnsdocumentin non-shadow context)