[Modal] Opening Modal causes the page to scroll

[cvara]

Rendering any component that internally relies on Modal, causes the page to jump/scroll.

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate ([Popover] On mobile Chrome while zoomed, opening popover causes page to jump #17636 might be related though 🤔)

Current Behavior 😯

Rendering a component that is based on Modal (i.e. Popover) causes the page to scroll. This happens as long as:

• the scroll container has already scrolled
• the contents of the scroll container affect its total height

Expected Behavior 🤔

Scroll position should not change.

Steps to Reproduce 🕹

This example showcases the issue: https://codesandbox.io/s/zen-visvesvaraya-w7pp4

Steps:

1. scroll to find the button
2. click the button to render the Popover
3. observe scroll position

Your Environment 🌎

Tech	Version
Material-UI	v4.7.2
React	16.12
Browser	Chrome 78, Firefox 71

Cause

The issue seems to be caused by the fact that the browser has the chance to render at least one frame after the scrollbar has been hidden - thus increasing the overall page width - but before the missing scrollbar width has been compensated by added padding, thus the content has been re-arranged to fit in a larger container, thus reducing the overall page height and causing the jump.

Grouping those 2 actions together seems to solve the issue, since both changes are applied on the same frame (happy to submit a PR with the fix).

[Amagon96]

Hi @cvara! I followed the steps as you said but I couldn't reproduce the issue with the codesandbox that you provided.

[cvara]

Huh, indeed this is not reproducible on all screen sizes. I tried editing the codesandbox a bit, but it seems that it has to do with how the content happens to be re-arranged by the browser on certain sizes.

@Amagon96 I can consistently reproduce it here https://w7pp4.csb.app/ by fixing the window width to certain values (like 720 or 1200 px) on Chrome 78.0.3904.108 & Chrome 79.0.3945.79 (MacOS 10.15), Chrome 78.0.3904.108 (Ubuntu 18.04). On Firefox I can't consistently reproduce, I have to play around by resizing the window or sometimes scrolling & re-opening the popover. Also I can reproduce it without css columns, albeit not as easily.

[oliviertassinari]

I have no idea how we could actually solve this problem. I fail to reproduce it on getbootstrap.com, so maybe there is something to explore.

[cvara]

@oliviertassinari can you reproduce it on https://w7pp4.csb.app/ though?

I can consistently solve this by editing the ModalManager's handleContainer, so that scrollbar disabling & adding of padding happen in succession, without the call to getScrollbarSize() in-between:

    if (overflowing) {
      // 👇 this must be the culprit - browser gets the chance to render a frame
      const scrollbarSize = getScrollbarSize(); 
      restoreStyle.push({
        value: container.style.paddingRight,
        key: 'padding-right',
        el: container,
      });
      // 👇 scroll disabling and added padding processed on the same frame
      scrollContainer.style.overflow = 'hidden';
      // Use computed style, here to get the real padding to add our scrollbar width.
      container.style['padding-right'] = `${getPaddingRight(container) + scrollbarSize}px`;

      // .mui-fixed is a global helper.
      fixedNodes = ownerDocument(container).querySelectorAll('.mui-fixed');
      [].forEach.call(fixedNodes, node => {
        restorePaddings.push(node.style.paddingRight);
        node.style.paddingRight = `${getPaddingRight(node) + scrollbarSize}px`;
      });
    }
  }

[oliviertassinari]

@cvara Thanks for the lead. Yes, I can reproduce and confirm your analysis. We used to do it correctly (2 months ago?), I would propose the following diff. Do you confirm it solves the problem? Do you want to work on it? :)

diff --git a/packages/material-ui/src/Modal/ModalManager.js b/packages/material-ui/src/Modal/ModalManager.js
index 9d0e9982b..2fdad7524 100644
--- a/packages/material-ui/src/Modal/ModalManager.js
+++ b/packages/material-ui/src/Modal/ModalManager.js
@@ -59,27 +59,8 @@ function handleContainer(containerInfo, props) {
   let fixedNodes;

   if (!props.disableScrollLock) {
-    const overflowing = isOverflowing(container);
-
-    // Improve Gatsby support
-    // https://css-tricks.com/snippets/css/force-vertical-scrollbar/
-    const parent = container.parentElement;
-    const scrollContainer =
-      parent.nodeName === 'HTML' && window.getComputedStyle(parent)['overflow-y'] === 'scroll'
-        ? parent
-        : container;
-
-    restoreStyle.push({
-      value: scrollContainer.style.overflow,
-      key: 'overflow',
-      el: scrollContainer,
-    });
-
-    // Block the scroll even if no scrollbar is visible to account for mobile keyboard
-    // screensize shrink.
-    scrollContainer.style.overflow = 'hidden';
-
-    if (overflowing) {
+    if (isOverflowing(container)) {
+      // Compute the size before applying overflow hidden to avoid any scroll jumps.
       const scrollbarSize = getScrollbarSize();

       restoreStyle.push({
@@ -97,6 +78,23 @@ function handleContainer(containerInfo, props) {
         node.style.paddingRight = `${getPaddingRight(node) + scrollbarSize}px`;
       });
     }
+
+    // Improve Gatsby support
+    // https://css-tricks.com/snippets/css/force-vertical-scrollbar/
+    const parent = container.parentElement;
+    const scrollContainer =
+      parent.nodeName === 'HTML' && window.getComputedStyle(parent)['overflow-y'] === 'scroll'
+        ? parent
+        : container;
+
+    // Block the scroll even if no scrollbar is visible to account for mobile keyboard
+    // screensize shrink.
+    restoreStyle.push({
+      value: scrollContainer.style.overflow,
+      key: 'overflow',
+      el: scrollContainer,
+    });
+    scrollContainer.style.overflow = 'hidden';
   }

   const restore = () => {

[cvara]

@oliviertassinari Yep, the diff you suggested solves the issue indeed. Happy to work on it, I will put together a PR later today.

[oliviertassinari]

Awesome