[Modal] Use computed key to restore style (#16540)

* restore styles using hyphenated property keys and use getter/setter to mutate * Fix cleaning value
mui · Jul 12, 2019 · 279981f · 279981f
1 parent c4bda2b
commit 279981f
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 22 deletions.
diff --git a/packages/material-ui/src/Modal/ModalManager.js b/packages/material-ui/src/Modal/ModalManager.js
@@ -62,7 +62,7 @@ function handleNewContainer(containerInfo) {
   // We are only interested in the actual `style` here because we will override it.
   const restoreStyle = {
     overflow: containerInfo.container.style.overflow,
-    paddingRight: containerInfo.container.style.paddingRight,
+    'padding-right': containerInfo.container.style.paddingRight,
   };
 
   const style = {
@@ -76,15 +76,13 @@ function handleNewContainer(containerInfo) {
     const scrollbarSize = getScrollbarSize();
 
     // Use computed style, here to get the real padding to add our scrollbar width.
-    style.paddingRight = `${getPaddingRight(containerInfo.container) + scrollbarSize}px`;
+    style['padding-right'] = `${getPaddingRight(containerInfo.container) + scrollbarSize}px`;
 
     // .mui-fixed is a global helper.
     fixedNodes = ownerDocument(containerInfo.container).querySelectorAll('.mui-fixed');
-
     [].forEach.call(fixedNodes, node => {
-      const paddingRight = getPaddingRight(node);
-      restorePaddings.push(paddingRight);
-      node.style.paddingRight = `${paddingRight + scrollbarSize}px`;
+      restorePaddings.push(node.style.paddingRight);
+      node.style.paddingRight = `${getPaddingRight(node) + scrollbarSize}px`;
     });
   }
 
@@ -95,12 +93,20 @@ function handleNewContainer(containerInfo) {
   const restore = () => {
     if (fixedNodes) {
       [].forEach.call(fixedNodes, (node, i) => {
-        node.style.paddingRight = `${restorePaddings[i]}px`;
+        if (restorePaddings[i]) {
+          node.style.paddingRight = restorePaddings[i];
+        } else {
+          node.style.removeProperty('padding-right');
+        }
       });
     }
 
     Object.keys(restoreStyle).forEach(key => {
-      containerInfo.container.style[key] = restoreStyle[key];
+      if (restoreStyle[key]) {
+        containerInfo.container.style.setProperty(key, restoreStyle[key]);
+      } else {
+        containerInfo.container.style.removeProperty(key);
+      }
     });
   };
 

diff --git a/packages/material-ui/src/Modal/ModalManager.test.js b/packages/material-ui/src/Modal/ModalManager.test.js
@@ -9,7 +9,7 @@ describe('ModalManager', () => {
   before(() => {
     modalManager = new ModalManager();
     container1 = document.createElement('div');
-    container1.style.padding = '20px';
+    container1.style.paddingRight = '20px';
     Object.defineProperty(container1, 'scrollHeight', {
       value: 100,
       writable: false,
@@ -107,9 +107,10 @@ describe('ModalManager', () => {
     let fixedNode;
 
     beforeEach(() => {
+      container1.style.paddingRight = '20px';
+
       fixedNode = document.createElement('div');
       fixedNode.classList.add('mui-fixed');
-      fixedNode.style.padding = '14px';
       document.body.appendChild(fixedNode);
       window.innerWidth += 1; // simulate a scrollbar
     });
@@ -120,24 +121,31 @@ describe('ModalManager', () => {
     });
 
     it('should handle the scroll', () => {
+      fixedNode.style.paddingRight = '14px';
+
       const modal = {};
-      const paddingRightBefore = container1.style.paddingRight;
-      const paddingFixedRightBefore = fixedNode.style.paddingRight;
       modalManager.add(modal, container1);
       modalManager.mount(modal);
       assert.strictEqual(container1.style.overflow, 'hidden');
-      assert.strictEqual(
-        container1.style.paddingRight,
-        `${parseInt(paddingRightBefore, 10) + getScrollbarSize()}px`,
-      );
-      assert.strictEqual(
-        fixedNode.style.paddingRight,
-        `${parseInt(paddingFixedRightBefore, 10) + getScrollbarSize()}px`,
-      );
+      assert.strictEqual(container1.style.paddingRight, `${20 + getScrollbarSize()}px`);
+      assert.strictEqual(fixedNode.style.paddingRight, `${14 + getScrollbarSize()}px`);
+      modalManager.remove(modal);
+      assert.strictEqual(container1.style.overflow, '');
+      assert.strictEqual(container1.style.paddingRight, '20px');
+      assert.strictEqual(fixedNode.style.paddingRight, '14px');
+    });
+
+    it('should restore styles correctly if none existed before', () => {
+      const modal = {};
+      modalManager.add(modal, container1);
+      modalManager.mount(modal);
+      assert.strictEqual(container1.style.overflow, 'hidden');
+      assert.strictEqual(container1.style.paddingRight, `${20 + getScrollbarSize()}px`);
+      assert.strictEqual(fixedNode.style.paddingRight, `${0 + getScrollbarSize()}px`);
       modalManager.remove(modal);
       assert.strictEqual(container1.style.overflow, '');
-      assert.strictEqual(container1.style.paddingRight, paddingRightBefore);
-      assert.strictEqual(fixedNode.style.paddingRight, paddingFixedRightBefore);
+      assert.strictEqual(container1.style.paddingRight, '20px');
+      assert.strictEqual(fixedNode.style.paddingRight, '');
     });
   });