From a124aadec190bdce73adf94fc9b638b045011d58 Mon Sep 17 00:00:00 2001 From: Christoforos Varakliotis Date: Thu, 12 Dec 2019 19:04:12 +0200 Subject: [PATCH 1/3] [test] Ensure ModalManager always disables scroll --- .../src/Modal/ModalManager.test.js | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/material-ui/src/Modal/ModalManager.test.js b/packages/material-ui/src/Modal/ModalManager.test.js index 9d3ef4915fde9f..cc5c994b413a85 100644 --- a/packages/material-ui/src/Modal/ModalManager.test.js +++ b/packages/material-ui/src/Modal/ModalManager.test.js @@ -135,6 +135,29 @@ describe('ModalManager', () => { assert.strictEqual(fixedNode.style.paddingRight, '14px'); }); + it('should disable the scroll even when not overflowing', () => { + // simulate non-overflowing container + const container2 = document.createElement('div'); + Object.defineProperty(container2, 'scrollHeight', { + value: 100, + writable: false, + }); + Object.defineProperty(container2, 'clientHeight', { + value: 100, + writable: false, + }); + document.body.appendChild(container2); + + const modal = {}; + modalManager.add(modal, container2); + modalManager.mount(modal, {}); + assert.strictEqual(container2.style.overflow, 'hidden'); + modalManager.remove(modal); + assert.strictEqual(container2.style.overflow, ''); + + document.body.removeChild(container2); + }); + it('should restore styles correctly if none existed before', () => { const modal = {}; modalManager.add(modal, container1); From f54613a26cc1c422f231a8ec44c1aba564366e0b Mon Sep 17 00:00:00 2001 From: Christoforos Varakliotis Date: Thu, 12 Dec 2019 19:07:33 +0200 Subject: [PATCH 2/3] [Modal] Fix jump issue on scroll disable --- packages/material-ui/src/Modal/ModalManager.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/material-ui/src/Modal/ModalManager.js b/packages/material-ui/src/Modal/ModalManager.js index 9d0e9982b493de..74641720fc45b5 100644 --- a/packages/material-ui/src/Modal/ModalManager.js +++ b/packages/material-ui/src/Modal/ModalManager.js @@ -75,11 +75,8 @@ function handleContainer(containerInfo, props) { el: scrollContainer, }); - // Block the scroll even if no scrollbar is visible to account for mobile keyboard - // screensize shrink. - scrollContainer.style.overflow = 'hidden'; - if (overflowing) { + // Compute the size before applying overflow hidden to avoid any scroll jumps. const scrollbarSize = getScrollbarSize(); restoreStyle.push({ @@ -87,6 +84,8 @@ function handleContainer(containerInfo, props) { key: 'padding-right', el: container, }); + // Disable scroll here to be processed on the same frame with padding change. + 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`; @@ -96,6 +95,10 @@ function handleContainer(containerInfo, props) { restorePaddings.push(node.style.paddingRight); node.style.paddingRight = `${getPaddingRight(node) + scrollbarSize}px`; }); + } else { + // Block the scroll even if no scrollbar is visible to account for mobile keyboard + // screensize shrink. + scrollContainer.style.overflow = 'hidden'; } } From a30251e00d9809b916bf755fbef699d92b86820e Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Thu, 12 Dec 2019 18:32:13 +0100 Subject: [PATCH 3/3] avoid duplicate overflow hidden --- .../material-ui/src/Modal/ModalManager.js | 41 ++++++++----------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/packages/material-ui/src/Modal/ModalManager.js b/packages/material-ui/src/Modal/ModalManager.js index 74641720fc45b5..2fdad7524449af 100644 --- a/packages/material-ui/src/Modal/ModalManager.js +++ b/packages/material-ui/src/Modal/ModalManager.js @@ -59,23 +59,7 @@ 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, - }); - - if (overflowing) { + if (isOverflowing(container)) { // Compute the size before applying overflow hidden to avoid any scroll jumps. const scrollbarSize = getScrollbarSize(); @@ -84,8 +68,6 @@ function handleContainer(containerInfo, props) { key: 'padding-right', el: container, }); - // Disable scroll here to be processed on the same frame with padding change. - 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`; @@ -95,11 +77,24 @@ function handleContainer(containerInfo, props) { restorePaddings.push(node.style.paddingRight); node.style.paddingRight = `${getPaddingRight(node) + scrollbarSize}px`; }); - } else { - // Block the scroll even if no scrollbar is visible to account for mobile keyboard - // screensize shrink. - scrollContainer.style.overflow = 'hidden'; } + + // 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 = () => {