mantinedev / mantine

A fully featured React components library
https://mantine.dev
MIT License
26.87k stars 1.9k forks source link

Opening modal removes scrollbar and moves page content #296

Closed lukvermeulen closed 3 years ago

lukvermeulen commented 3 years ago

Link to the page: https://mantine.dev/core/modal/

Opening the modal removes the scrollbar (probably by setting "overflow: hidden" on the body) resulting in an (IMO) unwanted movement of the page. Everything moves horizontally to fill the now higher width. Maybe this is also related to the component, not the page(?)

rtivital commented 3 years ago

This is intentional, Modal disabled scroll on body to:

lukvermeulen commented 3 years ago

That's a good point, I didn't think of that! I still wonder though, is there a way to prevent the layout shift? React MUI for example also adds a padding on the body. Chakra UI appears to do the same. I am not sure if this is a solution that works on all browsers (different browser, different scrollbar width?), but maybe there is a good way?

rtivital commented 3 years ago

I've explored the issue and here are my findings:

All magic should happen in use-scroll-lock hook, this way we will automatically add this feature to Modal and Drawer components.

@EmilMalanczak may I interest you with this issue?

EmilMalanczak commented 3 years ago

Yes, I can do it

At my current project, I use the reach-ui library and they handle it pretty well.

Assign me to it, does this issue has prior over use-scroll-into-view?

rtivital commented 3 years ago

This will be a part of 3.1.0 release, no priorities

lukvermeulen commented 3 years ago

You guys are awesome, thanks for the quick actions! I noticed the same behavior with the use-scroll-lock hook, just wanted to add this in case this is related to the modal :)

kosciolek commented 3 years ago

Note that outside of applying padding to body, you'll have to apply the same padding to position: fixed elements, otherwise they'll jump just like body does now.

You can:

Then you can add/remove the .compensate-scroll-lock class from body as scroll lock is being locked/unlocked. It can even handle nested locks. Every lock just adds one .compensate-scroll-lock to body once it locks, and removes one once it unlocks.

rtivital commented 3 years ago

Here are some questions:

porkopek commented 3 years ago

A quick, dirty hack while this is not implemented is to add the margin yourself in the component that uses the modal, i.e., your custom modal component

 useEffect(() => {
    document.querySelector('body').style.marginRight = scrollBarWidth;
    return () => {
      document.querySelector('body').style.marginRight = '0px';
    };
  }, [opened]);

where scrollBarWidth is the width of your scroll bar That adds the margin compensating the absence of scroll bar, but is just a temporal hack for those who get nervous because of the flickering

EmilMalanczak commented 3 years ago

implemented in this PR

rtivital commented 3 years ago

Fix released in 3.1.0

lukvermeulen commented 3 years ago

Thanks for the fix! Works great and seamlessly in most cases. I just noted one minor issue with the modal: although the content of the site doesn't move, the modal itself moves upon closing it. It looks like the scroll-lock / padding functionality is executed before the animation of the modal closing has finished, resulting in the modal moving after I press the closing button. Is there a way of switching this?

EmilMalanczak commented 3 years ago

@lukvermeulen I'll try to cover that before 3.2 but its kinda tricky to do and might cause other issues due to operating on the modal element (probably by transformX)

rtivital commented 3 years ago

It should not be hard, we should just postpone scrollbars removal/adding back while animation is playing. In other words:

Should prevent flickering

rtivital commented 3 years ago

We've fixed flickering in 3.1.1 patch