-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(dialog): ensure scroll lock is cleared on unmount during navigation #3840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
31b74eb
31932f5
196487e
9c5e0f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@radix-ui/react-dialog': patch | ||
| --- | ||
|
|
||
| Fix scroll lock not being released when Dialog is forcefully unmounted during SPA navigation. Added a `useLayoutEffect` cleanup in `DialogOverlayImpl` that synchronously removes `data-scroll-locked` from `document.body` when the overlay unmounts while the dialog is still open (e.g. route change triggered by a Link inside the Dialog). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -196,10 +196,69 @@ interface DialogOverlayImplProps extends PrimitiveDivProps {} | |
|
|
||
| const Slot = createSlot('DialogOverlay.RemoveScroll'); | ||
|
|
||
| /** | ||
| * The attribute set on `document.body` by `react-remove-scroll-bar` to lock scroll. | ||
| * We reference it here to ensure synchronous cleanup on forced unmount. | ||
| */ | ||
| const SCROLL_LOCK_ATTRIBUTE = 'data-scroll-locked'; | ||
|
|
||
| const DialogOverlayImpl = React.forwardRef<DialogOverlayImplElement, DialogOverlayImplProps>( | ||
| (props: ScopedProps<DialogOverlayImplProps>, forwardedRef) => { | ||
| const { __scopeDialog, ...overlayProps } = props; | ||
| const context = useDialogContext(OVERLAY_NAME, __scopeDialog); | ||
|
|
||
| /** | ||
| * Ensure the scroll lock is released synchronously when the overlay is forcefully | ||
| * unmounted (e.g. when a router Link inside the Dialog triggers navigation while | ||
| * the Dialog is still open). | ||
| * | ||
| * `react-remove-scroll` manages `data-scroll-locked` via `useEffect`, which is | ||
| * asynchronous. In certain SPA router scenarios the old route's async effect | ||
| * cleanups may be deferred or skipped, leaving `overflow: hidden` on the body | ||
| * and preventing scrolling on the destination or return page. | ||
| * | ||
| * By using `useLayoutEffect` (synchronous, fires during the commit phase) we | ||
| * guarantee the attribute is decremented and removed as soon as the overlay | ||
| * leaves the React tree when the Dialog was still open at the time of unmount | ||
| * (i.e. a forced unmount, not a user-initiated close). | ||
| * | ||
| * We only run the cleanup when `context.open` is still `true` at unmount time. | ||
| * When the Dialog is closed normally, `context.open` transitions to `false` | ||
| * before the overlay unmounts (via Presence), so we defer to `react-remove-scroll`'s | ||
| * own cleanup to avoid double-decrementing the lock counter. | ||
| * | ||
| * Counter coordination: `react-remove-scroll-bar` stores the number of active | ||
| * locks as an integer in the attribute value. We read and write that same | ||
| * counter so nested / stacked dialogs continue to work correctly. Because our | ||
| * `useLayoutEffect` cleanup runs *before* `react-remove-scroll`'s `useEffect` | ||
| * cleanup, the subsequent async decrement will read 0 and call | ||
| * `removeAttribute`, which is a safe no-op on an already-absent attribute. | ||
| */ | ||
| // Track the latest `open` state in a ref so the cleanup function below can | ||
| // read it without needing to be in the effect's dependency array. | ||
| const isOpenRef = React.useRef(context.open); | ||
| isOpenRef.current = context.open; | ||
|
|
||
| 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; | ||
|
|
||
| const raw = document.body.getAttribute(SCROLL_LOCK_ATTRIBUTE); | ||
| if (raw === null) return; // already cleaned up | ||
| const current = parseInt(raw, 10); | ||
| const next = isFinite(current) ? current - 1 : 0; | ||
| if (next <= 0) { | ||
| document.body.removeAttribute(SCROLL_LOCK_ATTRIBUTE); | ||
| } else { | ||
| document.body.setAttribute(SCROLL_LOCK_ATTRIBUTE, String(next)); | ||
| } | ||
| }; | ||
| }, []); | ||
|
|
||
| return ( | ||
| // Make sure `Content` is scrollable even when it doesn't live inside `RemoveScroll` | ||
| // ie. when `Overlay` and `Content` are siblings | ||
|
|
@@ -414,7 +473,7 @@ const DialogContentImpl = React.forwardRef<DialogContentImplElement, DialogConte | |
| </FocusScope> | ||
| {process.env.NODE_ENV !== 'production' && ( | ||
| <> | ||
| <TitleWarning titleId={context.titleId} /> | ||
| <TitleWarning contentRef={contentRef} titleId={context.titleId} /> | ||
| <DescriptionWarning contentRef={contentRef} descriptionId={context.descriptionId} /> | ||
| </> | ||
| )} | ||
|
|
@@ -503,9 +562,12 @@ const [WarningProvider, useWarningContext] = createContext(TITLE_WARNING_NAME, { | |
| docsSlug: 'dialog', | ||
| }); | ||
|
|
||
| type TitleWarningProps = { titleId?: string }; | ||
| type TitleWarningProps = { | ||
| contentRef: React.RefObject<DialogContentElement | null>; | ||
| titleId?: string; | ||
| }; | ||
|
|
||
| const TitleWarning: React.FC<TitleWarningProps> = ({ titleId }) => { | ||
| const TitleWarning: React.FC<TitleWarningProps> = ({ contentRef, titleId }) => { | ||
| const titleWarningContext = useWarningContext(TITLE_WARNING_NAME); | ||
|
|
||
| const MESSAGE = `\`${titleWarningContext.contentName}\` requires a \`${titleWarningContext.titleName}\` for the component to be accessible for screen reader users. | ||
|
|
@@ -516,10 +578,13 @@ For more information, see https://radix-ui.com/primitives/docs/components/${titl | |
|
|
||
| React.useEffect(() => { | ||
| if (titleId) { | ||
| const hasTitle = document.getElementById(titleId); | ||
| // 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); | ||
|
Comment on lines
+582
to
593
|
||
| } | ||
| }, [MESSAGE, titleId]); | ||
| }, [MESSAGE, contentRef, titleId]); | ||
|
|
||
| return null; | ||
| }; | ||
|
|
@@ -539,7 +604,10 @@ const DescriptionWarning: React.FC<DescriptionWarningProps> = ({ contentRef, des | |
| const describedById = contentRef.current?.getAttribute('aria-describedby'); | ||
| // if we have an id and the user hasn't set aria-describedby={undefined} | ||
| if (descriptionId && describedById) { | ||
| const hasDescription = document.getElementById(descriptionId); | ||
| // 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); | ||
|
Comment on lines
+615
to
626
|
||
| } | ||
| }, [MESSAGE, contentRef, descriptionId]); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DialogOverlayImplusesReact.useLayoutEffect, but this repo generally uses the SSR-safeuseLayoutEffectfrom@radix-ui/react-use-layout-effectto avoid server-render warnings (e.g.packages/react/portal/src/portal.tsx:4). Consider switching to the shared hook for consistency and SSR compatibility.