Repository navigation
Conversation
The scroll lock hides the overflow of the scroll container, which removes the scrollbar and widens the viewport. It compensates by adding the scrollbar width as padding-right to the container and to .mui-fixed elements, but that compensation is wrong when the page reserves the scrollbar space with scrollbar-gutter: stable — the gutter survives overflow: hidden, so nothing moves and the padding shifts the content. Read the computed scrollbar-gutter of the scroll container and skip the compensation when it is stable. Reading the computed value doubles as a feature detection: unsupported browsers resolve it to an empty string and keep the existing behaviour. The docs opt in, which fixes the cookie banner jumping to the right when a Select opens. Also neutralise the margin-right that DocSearch puts on the body on open: it compensates for a scrollbar that never disappears here, shifting the page to the left.
Janpot
added
type: regression
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
| // so there is no layout shift to compensate for. | ||
| // Reading the computed value doubles as a feature detection: browsers that don't support | ||
| // scrollbar-gutter resolve it to an empty string. | ||
| function hasStableScrollbarGutter(scrollContainer: HTMLElement): boolean { |
There was a problem hiding this comment.
Codex found one possible false positive in the implementation:
When a modal opens, MUI hides the page’s scrollbar to prevent scrolling. If that scrollbar occupied 15px, hiding it gives the page 15px more horizontal space. Centered content and fixed elements can move.
MUI normally adds padding to compensate for that extra space.
scrollbar-gutter: stable can keep the scrollbar’s space reserved even after the scrollbar disappears. In that case, adding padding would overcompensate, so the PR correctly tries to skip it.
The problem is where the gutter is declared:
html { scrollbar-gutter: stable; } /* Reserves viewport scrollbar space */
body { scrollbar-gutter: stable; } /* Does not reserve viewport scrollbar space */For an ordinary document, setting overflow: hidden on <body> can hide the viewport’s scrollbar. But setting scrollbar-gutter: stable on <body> does not reserve space for that scrollbar. Those two properties behave differently.
The PR currently accepts stable on either element. With only the body declaration, it skips padding, the viewport scrollbar disappears, and the page shifts.
The fix is to check the gutter on the element that controls the relevant scrollbar space. Within the current document-scrolling logic:
- When locking
<body>or<html>, check<html>. - When locking a custom container, check that container.
That means replacing the “either body or HTML” check with:
const gutterElement =
scrollContainer === doc.body ? doc.documentElement : scrollContainer;A body-only gutter then keeps the existing padding compensation. A root gutter still skips it, as the PR intends.
There was a problem hiding this comment.
Codex is right, fixed.
The gutter is now read from the element that actually reserves the space: documentElement when the scroll container is the document scroller, the container itself otherwise. A gutter declared on <body> reserves nothing for the page scrollbar, so it no longer counts as one.
Copilot found the mirror image of this below: a gutter on a container whose overflow is visible is equally not in effect, and blocking the scroll is what turns that container into a scroll container and makes the gutter real. Compensating there makes things worse rather than better, because padding adds to the same edge the gutter takes, so with border-box sizing the content moves by two scrollbar widths instead of none.
So the lock now pins an inert gutter to auto for its duration and restores it on close, and skips the compensation. ModalManager writes scrollbar-gutter now, but only on a custom container that is not yet a scroll container, never on the document scroller.
For the case you raised nothing changed: with <body> as the scroll container the gutter element resolves to documentElement, so the pinning branch is skipped and the compensation runs as your fix intended.
There was a problem hiding this comment.
🟡 Changes recommended
Custom containers using visible or clipped overflow can gain a gutter during locking and still shift.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds stable scrollbar-gutter support to prevent layout shifts when modals lock scrolling.
Changes:
- Skips scrollbar compensation for stable gutters.
- Adds browser tests for container and viewport gutters.
- Enables stable gutters and fixes DocSearch compensation in docs.
File summaries
| File | Description |
|---|---|
ModalManager.ts |
Detects stable gutters before compensation. |
ModalManager.test.ts |
Covers gutter-related scroll locking. |
brandingTheme.ts |
Enables stable gutters in docs. |
AppSearch.tsx |
Overrides unnecessary DocSearch body margin. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@claude review |
This comment was marked as resolved.
This comment was marked as resolved.
Only treat a scrollbar gutter as reserved when it is actually in effect: the declaration is inert on a container whose overflow computes to `visible` or `clip`, and blocking the scroll is what turns it into a scroll container, so the gutter it then creates still has to be compensated. Skip the compensation entirely when the scrollbar has zero width, matching the guard in MenuList: overlay scrollbars have nothing to compensate for, and writing the inline padding anyway freezes it for the lifetime of the modal. Pin the tests to the symptom rather than to the absence of padding alone, read the scrollbar size before the lock changes it, and document the gutter in the FAQ that until now offered only `.mui-fixed`.
Override the property DocSearch actually sets: it moved to `margin-inline-end` in v4, so the physical `margin-right` override silently stopped cancelling the shift, and missed RTL even before that. Share the document-scroller test between the two places that need it, group the gutter tests under one block, and record why the docs still declare `overflow-y: scroll` next to the gutter.
Define "the viewport scrollbar is visible" once, in terms of the size helper that already computes it, and flatten the gutter tests back to the file's usual depth now that the group titles only restated the test names. Cover the zero-width scrollbar path, which was the one new branch without a test, and say what the FAQ actually guarantees: the gutter belongs on the root element, and it is inert on a container that doesn't already scroll.
The gutter tests checked which padding was written but never that the width actually held across the lock, so an engine that dropped the gutter would keep them green while users saw the jump. Measure it. Skip the zero-width case on engines that render a classic scrollbar instead of asserting the size away: `--hide-scrollbars` is Chromium-only, and WebKit and Firefox run this suite too.
A container whose overflow is visible or clip reserves no gutter, and blocking the scroll turns it into a scroll container. Pin the gutter to auto for the duration so the width stays put, and skip the compensation, which adds to the same edge the gutter would take.
|
This pull request has been inactive for 30 days. Please remove the stale label or leave a comment to keep it open. Otherwise, it will be closed in 15 days. |
github-actions
Bot
added
the
stale
The docs set
html { overflow-y: scroll }. Opening a Select setsoverflow: hiddenon<html>, so the scrollbar vanishes and right-anchored fixed elements jump — most visibly the cookie banner.scrollbar-gutter: stablefixes that, but enabling it alone shifted the page the other way:isOverflowing()is still true while a gutter is reserved, so the scroll container got apadding-rightnothing cancelled out.ModalManagernow skips the compensation when the computed gutter is stable. Unsupported browsers resolve it to''and keep today's behaviour. Docs opt in.Separately, the search box shifted left because
@docsearch/reactsetsmargin-right: <scrollbar width>on the body to compensate for a scrollbar that never disappears here.Closes #40748 (was moved to base ui before)
try it out on https://deploy-preview-49079--material-ui.netlify.app/material-ui/react-select/