fix(dialog): ensure scroll lock is cleared on unmount during navigation#3840
fix(dialog): ensure scroll lock is cleared on unmount during navigation#3840sujiu222 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
When a Link inside Dialog triggers navigation, the Dialog unmounts but the scroll lock (overflow: hidden on body) was not being cleaned up, leaving the page in a non-scrollable state. Root cause: react-remove-scroll manages data-scroll-locked via useEffect (asynchronous). In certain SPA router scenarios (TanStack Router, Next.js App Router) the old route's async effect cleanups may be deferred or entirely skipped, causing data-scroll-locked to persist on document.body. Fix: Add a useLayoutEffect cleanup in DialogOverlayImpl that synchronously decrements (or removes) the data-scroll-locked attribute when the overlay unmounts while the dialog is still open (i.e. a forced unmount caused by router navigation, not a normal user-initiated close). The guard (isOpenRef.current) prevents double-decrement in the normal close flow: when the user closes the dialog through the normal UI, context.open transitions to false before the overlay unmounts via Presence, so we defer cleanup to react-remove-scroll's own useEffect cleanup in that case. The synchronous cleanup only fires when the dialog was still open at the time of unmount, correctly handling the navigation/forced-unmount scenario. Fixes radix-ui#3797
🦋 Changeset detectedLatest commit: 9c5e0f3 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
This PR addresses a Dialog scroll-lock cleanup bug that can occur during SPA navigation when a Dialog unmounts while still open, leaving the page in a locked-scroll state (data-scroll-locked on document.body). It also improves the dev-only accessibility warnings to better support Shadow DOM contexts.
Changes:
- Add a synchronous unmount cleanup in
DialogOverlayImplto decrement/remove thedata-scroll-lockedattribute on forced unmounts. - Update
TitleWarning(and align lookup logic withDescriptionWarning) to resolve IDs via the component’s root node to support Shadow DOM. - Add a regression test that simulates a forced unmount via
ReactDOM.createRoot().unmount()and asserts scroll lock cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/react/dialog/src/dialog.tsx |
Adds forced-unmount scroll-lock cleanup and updates a11y warning element lookup for Shadow DOM. |
packages/react/dialog/src/dialog.test.tsx |
Adds regression coverage for scroll-lock cleanup when the Dialog tree is force-unmounted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/react/dialog/src/dialog.tsx
Outdated
| React.useLayoutEffect(() => { | ||
| return () => { | ||
| // Only perform synchronous cleanup for forced unmounts (navigation while dialog is open). | ||
| // When the dialog closes normally, `context.open` is already `false` before this | ||
| // component unmounts, so we leave cleanup to `react-remove-scroll` to avoid | ||
| // double-decrementing the scroll lock counter. | ||
| if (!isOpenRef.current) return; |
There was a problem hiding this comment.
DialogOverlayImpl uses React.useLayoutEffect, but this repo generally uses the SSR-safe useLayoutEffect from @radix-ui/react-use-layout-effect to avoid server-render warnings (e.g. packages/react/portal/src/portal.tsx:4). Consider switching to the shared hook for consistency and SSR compatibility.
| // 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 DocumentFragment when Dialog.Portal is rendered into a DocumentFragment container (supported by @radix-ui/react-portal). DocumentFragment doesn’t guarantee getElementById, so (rootNode as Document | ShadowRoot).getElementById(...) can throw at runtime. Please guard for rootNode having getElementById (or fall back to document / use querySelector) before calling it.
| // 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 issue as TitleWarning: contentRef.current?.getRootNode() may be a DocumentFragment (portal container type allows this), which may not implement getElementById. Calling getElementById on it can throw; add a runtime check/fallback before using it.
- 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 radix-ui#3840.
|
Thanks for the review! Addressed all three points:
|
Summary
Fixes #3797
Problem
When a
Linkinside aDialogtriggers route navigation, the Dialog unmounts but the scroll lock (overflow: hiddenonbody) is not properly cleaned up. This leaves the page in a non-scrollable state —data-scroll-locked="1"persists ondocument.body— even after navigating to a different page and back.Root Cause
react-remove-scrollmanages thedata-scroll-lockedattribute viauseEffect(asynchronous). In certain SPA router scenarios (TanStack Router, Next.js App Router with transitions), the old route's async effect cleanups may be deferred or skipped before the new route is painted. This causesdata-scroll-lockedto remain ondocument.body, making the page non-scrollable.Solution
Add a
useLayoutEffectcleanup inDialogOverlayImplthat synchronously decrements (or removes) thedata-scroll-lockedattribute when the overlay unmounts while the dialog is still open (i.e. a forced unmount caused by router navigation).Double-decrement protection
A
isOpenRefguard prevents interfering with the normal close flow:context.opentransitions tofalsebefore the overlay unmounts (viaPresence), soisOpenRef.current === false→ our cleanup skips, leavingreact-remove-scroll's own cleanup to handle the decrement normally ✅context.openis stilltrueat unmount time → ouruseLayoutEffectcleanup fires synchronously and decrements the counter ✅Nested dialogs
When multiple dialogs are stacked (counter > 1), our cleanup correctly decrements to the next value rather than blindly removing the attribute.
react-remove-scroll's subsequent async decrement reads 0 and callsremoveAttribute, which is a safe no-op on an already-absent attribute.Testing
Added a regression test (
scroll lock cleanup on forced unmount) that:open={true}react-remove-scroll'suseEffectto set the attributeroot.unmount()directly (simulating a router tearing down the page)data-scroll-lockedis no longer present ondocument.bodyThe test passes with this fix and fails without it.