Fix dialog background layout shift

[cyeet]

Closes https://github.com/github/copilot-productivity/issues/2608

Background

Original PR: Prevents body scroll when Dialog is open #3547

This issue is caused by the overscroll: hidden behaviour which removes the scrollbar when the dialog is shown.
As you can see, the layout noticeably shifts in the background.

current-behaviour.mov

I have explored several solutions, each with its own tradeoffs:

1. overscroll-behavior
   I tried adding overscroll-behavior: contain on the dialog and ::backdrop element but that didn’t seem to work.

2. Always showing the scrollbars
   Using overscroll-gutter: stable or overflow-y: scroll will always show the scrollbar, which leaves a blank strip down the side of the page even when the scrollbar isn’t there, which can look awkward. But this is the most stable solution, likely to work on untested layouts.

scrollbar-gutter.mov

3. Compensate for missing scrollbar
   Another avenue I tried was to add a padding or margin to the body to compensate for the missing scrollbar ala Bootstrap. It works but any fixed position components will still see a jitter as the top/right/bottom/left positioning doesn't respect padding or margin. It could be fixed programmatically by finding all fixed position elements and adding an offset calc(100vw - 100%).

Notice the background layout is unchanged.

padding-right.mov

4. Position fixed
   Finally I followed the suggestion from StackOverflow.
   It kinda works but could likely fail with other layouts.

position-fixed-solution.mov

5. Positioning inside a 100vw width parent
   I haven't tried this: https://dev.to/rashidshamloo/preventing-the-layout-shift-caused-by-scrollbars-2flp

6. Leave it
   I'm not sure if the solutions listed above is worth the problems they might caused.

References:
https://frontendmasters.com/blog/scroll-locked-dialogs/

Changelog

New

Changed

Removed

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan
• None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

[changeset-bot]

⚠️ No Changeset found

Latest commit: 1b132d7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

[github-actions]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	103.83 KB (+0.13% 🔺)
packages/react/dist/browser.umd.js	104.14 KB (-0.01% 🔽)

[github-actions]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.