Calling enableBodyScroll can disable body scroll depending on situation

[maeertin]

Hey there, thank you for this cool library.

So the issue can be reproduced with the code below:

disableBodyScroll(targetElement)
setTimeout(() => {
  document.body.removeAttribute('style')
}, 100)
setTimeout(() => {
  enableBodyScroll(targetElement) // this will disable scrolling
}, 200)

In my case, I'm using material-ui where it's Dialog component already handles setting/removing of overflow: hidden on document.body and therefore end up in a similar situation as above. Sadly this css trick alone does not cancel scrolling on iOS and was hoping to use this library.

[willmcpo]

So the problem is you don't want the original setting to be restored when you call enableBodyScroll? If that's the case, we could pass a flag to enableBodyScroll to instruct it to not restore settings saved when disabledBodyScroll was called. Thoughts? PR welcome as well!

…

On Sat, Nov 17, 2018, 6:14 AM Martin Barri ***@***.***> wrote: Hey there, thank you for this cool library. So the issue can be reproduced with the code below: disableBodyScroll(targetElement) setTimeout(() => { document.body.removeAttribute('style') }, 100) setTimeout(() => { enableBodyScroll(targetElement) // this will disable scrolling }, 200) In my case, I'm using material-ui where it's Dialog component already handles setting/removing of overflow: hidden on document.body and therefore end up in a similar situation as above. Sadly this css trick alone does not cancel scrolling on iOS and was hoping to use this library. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#74>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANLxOvuSd98k2h5piFVbOFm1k107s9Lfks5uvw6wgaJpZM4Ym0RF> .

[maeertin]

Hey, thanks @willmcpo for the quick response!

Elaborating on my scenario
So I did some more digging into this and there seems to have been more to it then I thought. So the problem starts already when I call disableBodyScroll as this will set the restore values that you mention. In my case these saved restore values are incorrect due to material-ui/Dialog setting overflow: hidden to document.body before disableBodyScroll is fired, and therefore calling enableBodyScroll does not work. Don't really know how to go about this issue but I somehow expected that enableBodyScroll would enable body scrolling regardless of when I might have called disableBodyScroll.

The issue
Incorrect restore values being saved and no way to correct them.

Suggestion
What if the library exposed a function to either update the previousBody... vars or one that set some similar initialBody... vars to be used instead if defined?

Demo
I recreated a condensed example of the issue in this CodeSandbox where I use your library within the ScrollLock component.
https://codesandbox.io/s/50nkpljo9l

[maeertin]

Since this is a compatibility issue with handling of overflow: hidden on document.body, another solution would be to let lib users opt in/out of the css/js solutions of scroll canceling.

[maeertin]

I see that you already forward the options object to the function setOverflowHidden. What if this function was updated to something like below to allow control of the restore values.

const setOverflowHidden = (options = {}) => {
  const {
    reserveScrollBarGap,
    restoreBodyPaddingRight: optionsBPR,
    restoreBodyOverflowSetting: optionsBOS,
  } = options

  // Setting overflow on body/documentElement synchronously in Desktop Safari slows down
  // the responsiveness for some reason. Setting within a setTimeout fixes this.
  setTimeout(() => {
    // If previousBodyPaddingRight is already set, don't set it again.
    if (previousBodyPaddingRight === undefined) {
      const scrollBarGap = window.innerWidth - document.documentElement.clientWidth

      if (reserveScrollBarGap && scrollBarGap > 0) {
        // type-check against undefined to allow falsy values
        previousBodyPaddingRight = optionsBPR !== undefined
            ? optionsBPR
            : document.body.style.paddingRight
        document.body.style.paddingRight = `${scrollBarGap}px`
      }
    }

    // If previousBodyOverflowSetting is already set, don't set it again.
    if (previousBodyOverflowSetting === undefined) {
      // type-check against undefined to allow falsy values
      previousBodyOverflowSetting = optionsBOS !== undefined
        ? optionsBOS
        : document.body.style.overflow
      document.body.style.overflow = 'hidden'
    }
  })
}

[willmcpo]

from a quick glimpse, seems ok and intuitive.

would you mind putting that in a PR ? =) 👍