Material-ui: [Modal] html overflow hidden issue
- [x] The issue is present in the latest release.
- [x] I have searched the issues of this repository and believe that this is not a duplicate.
Current Behavior 馃槸
Our app uses Popover and Dialog components. They work correctly with v4.5.0. With 4.5.2 and 4.6.1 the exhibit strange behavior.
Popover: When opening, scrolls page to the top. When closing, scrolls down, but not quite to original position
Dialog: When opening, the page scroll is not removed, scrolls to top. When closing, page stays scrolled to top.
This uses the Popover for a custom dropdown.
Before opening, note how page is scrolled down:
Now opened, page is scrolled up, Popover attached to bottom of the screen:
Dialog not removing scroll when opened, and scrolled to top:
Expected Behavior 馃
- Popover/Dialog should remove the scroll when opened, maintain the scroll position, and reset it when closed.
This is what we have in prod right now with 4.5.0
Before opening, page is scrolled down:
When opened, page scroll is removed, Popover attached to the opening element:
As dialog is opened, scrollbar is removed from the page, scroll position stays:
Steps to Reproduce 馃暪
I don't see this happening with the examples, so I'm not sure yet how to repro this outside our codebase.
Context 馃敠
I want to update to the latest version of material-ui. This is blocking us.
Your Environment 馃寧
| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.5.2 v4.6.1 |
| React | 16.11.0 |
| Browser | Chrome 78 |
| TypeScript | N/A |
| etc. | |
andreasheim
All 13 comments
17972 related?
oliviertassinari
on 13 Nov 2019
There are a lot of changes in that PR. My guess would be possibly the changes to ModalManager.js. Based on what I'm experiencing L67 looks like maybe the culprit.
Well, it would potentially explain the scrollbar still showing. Not seeing anything there that would change the scroll position.
Also don't know if applying style changes right away instead of all at once at the end makes any difference.
L77 scrollContainer.style.overflow = 'hidden';
Could measuring getPaddingRight(container) be tainted by changing overflow first?
L88
container.style['padding-right'] = `${getPaddingRight(container) + scrollbarSize}px`;
Still I wouldn't expect that to affect scroll position.
andreasheim
on 13 Nov 2019
@andreasheim Do you have a reproduction we can have a look at? Alternatively, I would encourage you to disable the Modal features (with the disabled prefix) and see what prop helps, e.g. disableScrollLock.
oliviertassinari
on 13 Nov 2019
@oliviertassinari disableScrollLock does eliminate the jump to top.
Not a viable workaround for a dropdown, but hopefully that helps isolate the issue.
Unfortunately, I don't have a way to post this publicly.
andreasheim
on 13 Nov 2019
@andreasheim Arrf, could you try the following:
- Load your page.
- Scroll to the middle of the page.
- Open the dev tools and add overflow hidden on the
<html>element.
oliviertassinari
on 13 Nov 2019
@oliviertassinari That totally makes the page jump to the top.
Also does it in reverse
- scroll down again
- remove
overflow: hidden
andreasheim
on 13 Nov 2019
@andreasheim It's really strange. I suspect an issue with the top layout structure of the app.
oliviertassinari
on 13 Nov 2019
@oliviertassinari We have
body {
overflow-y: scroll;
}
When I remove that, changing it on html has no effect.
I'm sure we're not the only ones setting overflow on body. So I suppose a solution might be to check if overflow is set on body, and only mess with html if not?
andreasheim
on 14 Nov 2019
@andreasheim I like the idea. Actually, I would propose the opposite. We only handle the html element if we detect the Gatsby "patch" to scroll jumps. So by default, we handle body only. What do you think about it?
oliviertassinari
on 14 Nov 2019
Since this change was only made for improved Gatsby support, it makes sense to only use it when present. I don't really know Gatsby, but since it's a site generator, I expect it'll be rather unlikely for folks to then mess with overflow on body.
andreasheim
on 14 Nov 2019
@andreasheim What do you think of this diff?
diff --git a/packages/material-ui/src/Modal/ModalManager.js b/packages/material-ui/src/Modal/ModalManager.js
index 2cbf3c8c6..9d0e9982b 100644
--- a/packages/material-ui/src/Modal/ModalManager.js
+++ b/packages/material-ui/src/Modal/ModalManager.js
@@ -64,7 +64,10 @@ function handleContainer(containerInfo, props) {
// Improve Gatsby support
// https://css-tricks.com/snippets/css/force-vertical-scrollbar/
const parent = container.parentElement;
- const scrollContainer = parent.nodeName === 'HTML' ? parent : container;
+ const scrollContainer =
+ parent.nodeName === 'HTML' && window.getComputedStyle(parent)['overflow-y'] === 'scroll'
+ ? parent
+ : container;
restoreStyle.push({
value: scrollContainer.style.overflow,
I _think_ this should work. This will then only apply if overflow is changed on html.
const html = document.querySelector('html');
window.getComputedStyle(html)['overflow-y']
// "visible" -> default when nothing applied to html
Cool, do you want to handle it? I think that it would be fair to credit you for reporting the bug, proposing a solution and helping testing it.
oliviertassinari
on 18 Nov 2019
Related issues
finaiized
路
3Comments
mb-copart
路
3Comments
TimoRuetten
路
3Comments
rbozan
路
3Comments
FranBran
路
3Comments