search

segmentio / evergreen

🌲 Evergreen React UI Framework by Segment
https://evergreen.segment.com
MIT License
12.39k stars 832 forks source link

[Fix] Prevent Scrolling gets stuck when other packages/script also change document.body.style.overflow #1383

Closed nicksmider closed 2 years ago

nicksmider commented 2 years ago

Overview

When another package/script OR multiple complements use prevent-body-scroll together previousOverflow and previousPaddingRight get stuck.

Scenario observed:

  1. React app creates fullscreen overlay(not evergreen), manually sets document.body.style.overflow = 'hidden'
  2. React app opens Evergreen overlay with preventBodyScrolling set overtop the fullscreen. previousOverflow is set to hidden
  3. Evergreen overlay is closed -> document.body.style.overflow = previousOverflow keeps it at hidden. Now only the fullscreen overlay(not evergreen) is up
  4. The fullscreen module is closed and the React app manually sets document.body.style.overflow = ''
  5. At this point document.body.style.overflow is not set, however, the next Evergreen overlay that does not use preventBodyScrolling will default to using the old previousOverflow which was never cleared, disabling all scrolling on the overlay, when it was not intended to

The proposed change will create a history of the overlay changes, and once restored, remove it from the history. This will allow multiple overlays to have preventBodyScrolling up at a time without having to figure out if another is already up, and it will allow it to play nicely with outside scripts that are also dealing with setting document.body.style.overflow

Screenshots (if applicable)

Documentation (n/a to this change)

netlify[bot] commented 2 years ago

✔️ Deploy Preview for evergreen-storybook ready!

🔨 Explore the source changes: c8e59e2644555464c108e9e1fcc5d6968744e444

🔍 Inspect the deploy log: https://app.netlify.com/sites/evergreen-storybook/deploys/61cca5343811e500078af6fb

😎 Browse the preview: https://deploy-preview-1383--evergreen-storybook.netlify.app

nicksmider commented 2 years ago

Hmm, makes sense to me. Is there an easy way to demo this change in Storybook or the docs or would it be easier to demo in the app?

Additionally, while I know there's no existing unit tests for this utility, it would be awesome to whip up a few at least to cover the new behavior 👍 Food for thought - non-blocking, as this seems like a pretty jarring bug as-is

Demoing in the app would definitely be easiest. I can whip up some tests 👍