fix(react-headless-components-preview): reserve the scrollbar gutter while a dialog locks scroll - #36666
Open
Ray Knight (ArrayKnight) wants to merge 2 commits into
Conversation
…while a dialog locks scroll lockDocumentScroll sets body.style.overflow = 'visible clip' and nothing else, so the page scrollbar disappears without its space being reserved and every fixed or centred element on the page shifts sideways by half the scrollbar width when a modal opens. Measured on a scrolling 1280px page: a centred marker sits at x 332.5 with the dialog closed and jumps to 340 when it opens, where the Griffel equivalent stays at 332.5 because react-dialog's useDisableBodyScroll also reserves the gutter. The reservation goes on the document element, not on body. `overflow` propagates from body to the viewport — which is why the lock works at all with `<html>` left `visible` — but `scrollbar-gutter` does not: measured, `scrollbar-gutter: stable` on body alongside the clip reserves nothing and leaves the same 7.5px jump. The docblock's promise to leave `<html>` untouched cannot be kept and still fix this; padding body instead was measured too and does not help, because it narrows body rather than the initial containing block a top-layer surface resolves against. `stable` reserves a gutter whether or not the page had a scrollbar, so writing it unconditionally would introduce the mirror-image defect: a 7.5px shift on a page that never scrolled. It is written only when the scrollbar is actually taking layout width, read before the lock removes it. That reads 0 under overlay scrollbars as well, where nothing needs reserving and Griffel's own height-based guard over-reserves. Unlock restores the previous inline gutter next to the previous overflow, so a host application that had already set one gets it back, and the existing lockCount reference count scopes both to the outermost dialog. Non-modal dialogs never take the lock and are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013wmpBCYJpDJCLXcScCWz1i
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj9uA3rCVgosnh2zNn8qkc
| const { body, documentElement } = targetDocument; | ||
| // Read the scrollbar's layout width before the lock takes it away. Overlay | ||
| // scrollbars and unscrollable pages both measure 0, and both want no gutter. | ||
| const scrollbarWidth = (targetDocument.defaultView?.innerWidth ?? 0) - documentElement.clientWidth; |
Contributor
There was a problem hiding this comment.
could we use useApplyScrollbarWidth or useScrollbarWidth hooks from @fluentui/react-utilities?
Contributor
Author
There was a problem hiding this comment.
I looked at both hooks before settling on this shape — they solve a different half of the problem, so I kept the native gutter, but happy to switch if you prefer the tradeoff:
- This fix reserves the gutter natively (
scrollbar-gutter: stableon<html>) instead of compensating with a measured pixel width. Its only measurement,innerWidth - documentElement.clientWidth, asks whether this page, right now has a layout-consuming scrollbar. useScrollbarWidth/useApplyScrollbarWidthmeasure a probe element (measureScrollbarWidth), which reports the UA's classic scrollbar width even when the page itself doesn't scroll — so on an unscrollable page a width/padding compensation would introduce the very shift this PR removes, and we'd still need the current-viewport check on top.useApplyScrollbarWidthis a mount-only ref callback (it writes${width}pxon attach and early-returns on detach), so it can't express the lock's restore semantics — previous inline values plus the refcount for nested modals — and there's no natural ref to hand it fordocument.documentElement.
Adopting them would replace only the measurement while keeping all the lock/restore code, and would trade native reservation for pixel compensation. If you'd rather standardize on the shared utility regardless, I'm glad to rework it that way.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The headless Dialog's
lockDocumentScrollclipsbodyoverflow and reserves nothing in the scrollbar's place, so on a page that scrolls, opening a modal dialog removes the scrollbar's layout width and every fixed and centred element shifts sideways for as long as the dialog is open (measured at a 7.5px jump with classic scrollbars; the Griffel Dialog on the same page does not move).The fix reserves the gutter while the lock is held, with three details that were measured rather than assumed: the
scrollbar-gutterwrite must go ondocumentElement(overflow propagates from body to the viewport,scrollbar-gutterdoes not); it must be guarded on the scrollbar actually taking layout width, read before the lock removes it, so pages that never scroll — and overlay-scrollbar environments — do not get the mirror-image shift; and unlock restores the previous inline gutter alongside the previous overflow. Non-modal dialogs never take the lock and are unaffected. Four regression tests are added; the two behavioural ones were verified to fail without the fix.Note for verification: Puppeteer passes
--hide-scrollbarsby default, under which this defect cannot reproduce at all — measurements were taken withignoreDefaultArgs: ['--hide-scrollbars'].Fixes #36648.
Extracted from #36656 per maintainer request — each in-tree fix from that PR as an isolated change.