SelectPanel: Lock body scroll when modal variant is open#7757
SelectPanel: Lock body scroll when modal variant is open#7757liuliu-dev wants to merge 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: f5b44c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
liuliu-dev
added
the
Canary Release
|
Uh oh! @liuliu-dev, at least one image you shared is missing helpful alt text. Check your pull request body to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
github-actions
bot
added
the
integration-tests: recommended
|
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/18621 |
There was a problem hiding this comment.
Pull request overview
Fixes SelectPanel’s modal UX by preventing page scrolling behind the modal/backdrop, aligning modal behavior with typical overlay expectations.
Changes:
- Lock
<body>scrolling whenSelectPanelis open invariant="modal"(in addition to the existing narrow-screen fullscreen behavior). - Add unit tests verifying body scroll is locked on open and restored on close for the modal variant.
- Update the modal backdrop styling to use
position: fixedso it covers the viewport.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/SelectPanel/SelectPanel.tsx | Extends body scroll locking condition to include variant="modal" and updates effect deps. |
| packages/react/src/SelectPanel/SelectPanel.test.tsx | Adds tests asserting body overflow is hidden on modal open and restored on close. |
| packages/react/src/SelectPanel/SelectPanel.module.css | Changes .Backdrop positioning from absolute to fixed for full-viewport coverage. |
| .changeset/gold-humans-tap.md | Adds a patch changeset entry for the behavior change. |
Copilot's findings
Comments suppressed due to low confidence (1)
packages/react/src/SelectPanel/SelectPanel.tsx:374
- The body scroll lock logic bails out when
document.body.style.overflowis already'hidden', but that breaks the case where two SelectPanel modals are open at the same time: the second instance will skip registering a cleanup, and when the first unmounts/closes it will restore body scrolling even though the second is still open. Consider switching to a ref-counted approach (like Dialog’sdata-dialog-scroll-disabledpattern) so scroll is only restored when the last SelectPanel requiring the lock closes.
if (open && (variant === 'modal' || (isNarrowScreenSize && usingFullScreenOnNarrow))) {
const bodyOverflowStyle = document.body.style.overflow || ''
// If the body is already set to overflow: hidden, it likely means
// that there is already a modal open. In that case, we should bail
// so we don't re-enable scroll after the second dialog is closed.
if (bodyOverflowStyle === 'hidden') {
return
}
- Files reviewed: 4/4 changed files
- Comments generated: 1
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Closes https://github.com/github/primer/issues/6548
Changelog
SelectPanel with
variant="modal"did not lock the body's scrollbar, allowing users to scroll behind both the dialog and backdrop. Now body scroll is now locked whenvariant="modal"is open.Rollout strategy
Testing & Reviewing
Merge checklist