Fix inconsistent modal scroll behavior#48789
Conversation
Adds a body.modal-open class, toggled in the shared Modal component, that sets overflow: hidden on the page while any modal is mounted. The modal's own background overlay keeps overflow: auto so its content can still scroll. Fixes fleetdm#48788
A component's own .modal__background node is already detached from the DOM by the time its unmount cleanup runs, so the unlock check must be === 0 remaining backgrounds, not <= 1 — the latter unlocked scroll one modal too early when two modals were stacked.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48789 +/- ##
=======================================
Coverage 68.02% 68.03%
=======================================
Files 3682 3682
Lines 233995 234000 +5
Branches 12308 12309 +1
=======================================
+ Hits 159186 159191 +5
Misses 60490 60490
Partials 14319 14319
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
__content had max-height: 600px; overflow-y: auto, which clipped only the content area while the Close button footer stayed fixed below it, reading as a sticky footer. Removed so the whole modal scrolls via the outer overlay, matching EditColumnsModal and other Fleet modals.
WalkthroughThis PR adds body scroll locking for open modals. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/styles/global/_global.scss (1)
25-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo scrollbar-gutter compensation — possible layout shift on modal open.
Setting
overflow: hiddenonbodyremoves the vertical scrollbar on pages that have one, shifting page content horizontally by the scrollbar width while the modal is open (and shifting back on close). Consider reserving the gutter (e.g.scrollbar-gutter: stableonhtml, or compensating withpadding-rightequal to the scrollbar width) to avoid the visible content jump.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/styles/global/_global.scss` around lines 25 - 31, The body lock in the modal-open rule currently hides overflow without preserving the scrollbar gutter, which can cause a horizontal content jump when the modal opens and closes. Update the global modal scrolling styles in the modal-open block to reserve space for the scrollbar, either by applying a stable gutter on the root element or by adding equivalent right-side compensation when overflow is hidden, so the layout stays fixed while the modal is mounted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/styles/global/_global.scss`:
- Around line 25-31: The body lock in the modal-open rule currently hides
overflow without preserving the scrollbar gutter, which can cause a horizontal
content jump when the modal opens and closes. Update the global modal scrolling
styles in the modal-open block to reserve space for the scrollbar, either by
applying a stable gutter on the root element or by adding equivalent right-side
compensation when overflow is hidden, so the layout stays fixed while the modal
is mounted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 942d9b40-f4b4-44b8-b54f-10817c242711
📒 Files selected for processing (4)
changes/48788-modal-scroll-lockfrontend/components/Modal/Modal.tests.tsxfrontend/components/Modal/Modal.tsxfrontend/styles/global/_global.scss
noahtalerman
changed the title
1 participant
Related issue: Resolves #48788
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary
Two fixes for inconsistent modal scroll behavior:
Page scroll lock. Adds a
modal-openclass ondocument.body, toggled from the sharedModalcomponent (frontend/components/Modal/Modal.tsx), that setsoverflow: hiddenon the page while any modal is mounted. The modal's own background overlay is unaffected (overflow: auto, unchanged) so the modal's own content still scrolls. The unlock-on-unmount check counts remaining.modal__backgroundnodes in the DOM rather than a module-level counter, so stacked/hidden modals (e.g. via theisHiddenprop) keep the page locked until the last modal closes. Covered by two new Jest tests inModal.tests.tsx, including a stacked-modal regression case.Certificate details modal.
CertificateDetailsModal's content area hadmax-height: 600px; overflow-y: auto, so only the content scrolled while the "Close" button footer stayed fixed below it — a "sticky footer" look inconsistent with other Fleet modals (e.g. "Edit columns" on the Hosts page), which scroll as a single unit via the outer modal overlay. Removed the internal height/scroll constraint so this modal now behaves the same way.Note: I wasn't able to manually QA either fix in a running browser in this environment. The scroll-lock fix has automated test coverage exercising the DOM class toggling directly; the certificate modal fix is a 2-line SCSS-only change. Someone should confirm both visually via
make generate-devbefore merge — particularly the certificate modal with enough fields to overflow the viewport.