[Modal] html overflow hidden issue #18336

andreasheim · 2019-11-12T19:52:41Z

The issue is present in the latest release.
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.

oliviertassinari · 2019-11-12T23:06:35Z

#17972 related?

andreasheim · 2019-11-13T07:01:50Z

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.

oliviertassinari · 2019-11-13T11:05:39Z

@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.

andreasheim · 2019-11-13T20:19:05Z

@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.

oliviertassinari · 2019-11-13T20:23:50Z

@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.

andreasheim · 2019-11-13T22:52:47Z

@oliviertassinari That totally makes the page jump to the top.

Also does it in reverse
4. scroll down again
5. remove overflow: hidden

oliviertassinari · 2019-11-13T22:59:46Z

@andreasheim It's really strange. I suspect an issue with the top layout structure of the app.

andreasheim · 2019-11-13T23:08:02Z

@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?

oliviertassinari · 2019-11-13T23:12:31Z

@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?

andreasheim · 2019-11-13T23:16:25Z

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.

oliviertassinari · 2019-11-14T10:46:01Z

@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,

andreasheim · 2019-11-18T02:50:17Z

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

oliviertassinari · 2019-11-18T10:02:08Z

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 added the status: waiting for author Issue with insufficient information label Nov 13, 2019

oliviertassinari added bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! and removed status: waiting for author Issue with insufficient information labels Nov 13, 2019

oliviertassinari changed the title ~~[Popover] Opening changes page scroll pos since 4.5.2~~ [Modal] html overflow hidden issue Nov 13, 2019

oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Nov 14, 2019

andreasheim mentioned this issue Nov 19, 2019

[Modal] Prefer to lock scroll on body than html element #18445

Merged

1 task

oliviertassinari closed this as completed in #18445 Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Modal] html overflow hidden issue #18336

[Modal] html overflow hidden issue #18336

andreasheim commented Nov 12, 2019 •

edited

Loading

oliviertassinari commented Nov 12, 2019

andreasheim commented Nov 13, 2019 •

edited

Loading

oliviertassinari commented Nov 13, 2019

andreasheim commented Nov 13, 2019

oliviertassinari commented Nov 13, 2019

andreasheim commented Nov 13, 2019

oliviertassinari commented Nov 13, 2019 •

edited

Loading

andreasheim commented Nov 13, 2019

oliviertassinari commented Nov 13, 2019 •

edited

Loading

andreasheim commented Nov 13, 2019

oliviertassinari commented Nov 14, 2019

andreasheim commented Nov 18, 2019

oliviertassinari commented Nov 18, 2019

[Modal] html overflow hidden issue #18336

[Modal] html overflow hidden issue #18336

Comments

andreasheim commented Nov 12, 2019 • edited Loading

Current Behavior 😯

Expected Behavior 🤔

Steps to Reproduce 🕹

Context 🔦

Your Environment 🌎

oliviertassinari commented Nov 12, 2019

andreasheim commented Nov 13, 2019 • edited Loading

oliviertassinari commented Nov 13, 2019

andreasheim commented Nov 13, 2019

oliviertassinari commented Nov 13, 2019

andreasheim commented Nov 13, 2019

oliviertassinari commented Nov 13, 2019 • edited Loading

andreasheim commented Nov 13, 2019

oliviertassinari commented Nov 13, 2019 • edited Loading

andreasheim commented Nov 13, 2019

oliviertassinari commented Nov 14, 2019

andreasheim commented Nov 18, 2019

oliviertassinari commented Nov 18, 2019

andreasheim commented Nov 12, 2019 •

edited

Loading

andreasheim commented Nov 13, 2019 •

edited

Loading

oliviertassinari commented Nov 13, 2019 •

edited

Loading

oliviertassinari commented Nov 13, 2019 •

edited

Loading