willmcpo / body-scroll-lock

Body scroll locking that just works with everything 😏
MIT License
4.06k stars 339 forks source link

Jiggling scrollbar on position fixed scrollable menu #173

Open Ariane-B opened 4 years ago

Ariane-B commented 4 years ago

This script works great with our full-screen scrollable menus. The body is indeed not scrollable. But we'd ideally want to completely sync the body's scrollbar's disappearance with the menu's scrollbar's appearance so as to make the process seamless. In other words, it'd be nice if it only appeared as though the scrollbar suddenly changed lengths, instead of disappearing/appearing.

However, even when the disableBodyScroll and the element.classList statement that gives my element its overflow-y value are right next to one another, a weird scrollbar jiggle happens.

My hamburger button, which is also position: fixed, noticeably shifts to the right as well. Although I could very well improve it by measuring the scrollbar myself into a CSS variable and adding a margin to my button when appropriate, I would still not be able to fully sync that with the scrollbar. So instead of permanently shifting, it would jiggle like the scrollbars do. See the result in video:

https://imgur.com/a/N36a5yL

The option reserveScrollBarGap helps with part of the problem (notably, body's width noticeably shifting if my menu isn't opaque), but since my menu is position: fixed, the padding on body doesn't affect it.

I can only see one solution to the issue. Someone may have other ideas though.

You could provide custom events (for instance, bodyscrollenabled and bodyscrolldisabled) that fire on the target element and bubble up all the way to window the very moment the scrollbar disappears. Then we might be able to completely sync with the scrollbar by listening to that event.

I'm not even sure it would really solve the problem; completely syncing things is always difficult in JavaScript. Or maybe there's a way to "prepare" the body scroll lock changes AND any user listeners, and only once they are all complete, push all changes to the actual DOM? Anyway, maybe it would work.

I'm feeling relatively confident I could add this in. If you think it's a good idea and would like me to create a pull request, just say the word.

Ariane-B commented 4 years ago

In case someone runs into the issue, I've figured out a fix that doesn't require modifying body-scroll-lock. Instead of applying my own class to decide whether the element scrolls or not, I just rely on the body's style attribute.

// Using custom classes causes a jiggle no matter when you apply them:
.mobile-menu.open:not(.opening) {
  .mobile-menu-scrollable-el { overflow-y: scroll; }
}

// This does NOT cause a jiggle, since it's always synced with the actual CSS change:
body[style*="overflow"] .mobile-menu {
  .mobile-menu-scrollable-el { overflow-y: scroll; }
}

It would be much cleaner if the script applied a class to the body, which I could then target instead of the style attribute, but it works.

diachedelic commented 4 years ago

I think assigning a class to body is a great idea.

dyelax commented 4 years ago

I ended up resolving this by using react-remove-scroll-bar instead, which doesn't have this issue

malte1989 commented 4 years ago

In case someone runs into the issue, I've figured out a fix that doesn't require modifying body-scroll-lock. Instead of applying my own class to decide whether the element scrolls or not, I just rely on the body's style attribute.

// Using custom classes causes a jiggle no matter when you apply them:
.mobile-menu.open:not(.opening) {
  .mobile-menu-scrollable-el { overflow-y: scroll; }
}

// This does NOT cause a jiggle, since it's always synced with the actual CSS change:
body[style*="overflow"] .mobile-menu {
  .mobile-menu-scrollable-el { overflow-y: scroll; }
}

It would be much cleaner if the script applied a class to the body, which I could then target instead of the style attribute, but it works.

Thank you very much for the idea via the css selector! I needed the idea with the css selector because I had a jiggling sticky tab because of the reserveScrollbar option (which works fine) but leads to transforming the fixed sticky. With additional css selector it works smooth.

body[style*='overflow'] #sticky-container { padding-right: 15px; }

Explicit class name would be better, clearer and less error driven.

Ariane-B commented 4 years ago

I completely agree. I'm honestly a little disappointed the author wouldn't fix this. Almost makes me want to fork the project.

kalnode commented 4 years ago

body[style*='overflow'] #sticky-container { padding-right: 15px; }

Any way to not have hard-coded pixel values here? Surely scroll bar widths differ across browsers/OS's/devices.

Ariane-B commented 4 years ago

@TheMangoTrain Absolutely! It was just an example and I would actually not recommend using a flat pixel value at all!

Take a look at this Stack Overflow question: https://stackoverflow.com/questions/13382516/getting-scroll-bar-width-using-javascript

Personally I like to save that value soon in my page's lifecycle into both a global variable (property of the window object) and a CSS variable (custom property of the html element), so that I can access it anywhere if needed. Then you can just use padding-right: var( --scrollbar-width );.

RoelN commented 2 years ago

Until this is fixed, I'm using @malte1989's solution to apply the proper padding to the fixed element without it being hardcoded to 15px. It's clunky, but works for the probable range of padding:

body[style*='padding-right: 14px'] .my-fixed-container {
    padding-right: 14px;
}

body[style*='padding-right: 15px'] .my-fixed-container {
    padding-right: 15px;
}

body[style*='padding-right: 16px'] .my-fixed-container {
    padding-right: 16px;
}

body[style*='padding-right: 17px'] .my-fixed-container {
    padding-right: 17px;
}

body[style*='padding-right: 18px'] .my-fixed-container {
    padding-right: 18px;
}
jakec-dev commented 1 year ago

Still no progress with this? The jiggling is extremely annoying.