perf(modal): optimize model.resetScrollbar, resolves #1831 (#1837)

rinick · tmorehouse · commit a622358444cd · 2018-10-23T11:26:21.000-03:00
* modal resetScrollbar should revert the change only when the change was done in the first place, resolves #1831 * add test to show modal * add test to hide modal * fix modal hide test * add test for fixed and sticky content
diff --git a/src/components/modal/fixtures/modal.html b/src/components/modal/fixtures/modal.html
@@ -27,4 +27,12 @@
 
     </b-modal>
 
+    <b-btn ref="modalButton2" v-b-modal.modal2>Launch demo modal</b-btn>
+
+    <!-- Modal Component -->
+    <b-modal ref="modal2"
+             id="modal2">
+            <b-navbar sticky>
+            <b-navbar fixed="top">
+    </b-modal>
 </div>
diff --git a/src/components/modal/modal.js b/src/components/modal/modal.js
@@ -762,13 +762,16 @@ export default {
         const computedStyle = window.getComputedStyle
         const body = document.body
         const scrollbarWidth = this.scrollbarWidth
+        this._marginChangedForScroll = []
+        this._paddingChangedForScroll = []
         // Adjust fixed content padding
         selectAll(Selector.FIXED_CONTENT).forEach(el => {
           const actualPadding = el.style.paddingRight
           const calculatedPadding = computedStyle(el).paddingRight || 0
           setAttr(el, 'data-padding-right', actualPadding)
           el.style.paddingRight = `${parseFloat(calculatedPadding) +
             scrollbarWidth}px`
+          this._paddingChangedForScroll.push(el)
         })
         // Adjust sticky content margin
         selectAll(Selector.STICKY_CONTENT).forEach(el => {
@@ -777,6 +780,7 @@ export default {
           setAttr(el, 'data-margin-right', actualMargin)
           el.style.marginRight = `${parseFloat(calculatedMargin) -
             scrollbarWidth}px`
+          this._marginChangedForScroll.push(el)
         })
         // Adjust navbar-toggler margin
         selectAll(Selector.NAVBAR_TOGGLER).forEach(el => {
@@ -785,6 +789,7 @@ export default {
           setAttr(el, 'data-margin-right', actualMargin)
           el.style.marginRight = `${parseFloat(calculatedMargin) +
             scrollbarWidth}px`
+          this._marginChangedForScroll.push(el)
         })
         // Adjust body padding
         const actualPadding = body.style.paddingRight
@@ -795,27 +800,29 @@ export default {
       }
     },
     resetScrollbar () {
-      // Restore fixed content padding
-      selectAll(Selector.FIXED_CONTENT).forEach(el => {
-        if (hasAttr(el, 'data-padding-right')) {
-          el.style.paddingRight = getAttr(el, 'data-padding-right') || ''
-          removeAttr(el, 'data-padding-right')
-        }
-      })
-      // Restore sticky content and navbar-toggler margin
-      selectAll(
-        `${Selector.STICKY_CONTENT}, ${Selector.NAVBAR_TOGGLER}`
-      ).forEach(el => {
-        if (hasAttr(el, 'data-margin-right')) {
-          el.style.marginRight = getAttr(el, 'data-margin-right') || ''
-          removeAttr(el, 'data-margin-right')
+      if (this._marginChangedForScroll && this._paddingChangedForScroll) {
+        // Restore fixed content padding
+        this._paddingChangedForScroll.forEach(el => {
+          if (hasAttr(el, 'data-padding-right')) {
+            el.style.paddingRight = getAttr(el, 'data-padding-right') || ''
+            removeAttr(el, 'data-padding-right')
+          }
+        })
+        // Restore sticky content and navbar-toggler margin
+        this._marginChangedForScroll.forEach(el => {
+          if (hasAttr(el, 'data-margin-right')) {
+            el.style.marginRight = getAttr(el, 'data-margin-right') || ''
+            removeAttr(el, 'data-margin-right')
+          }
+        })
+        this._paddingChangedForScroll = null
+        this._marginChangedForScroll = null
+        // Restore body padding
+        const body = document.body
+        if (hasAttr(body, 'data-padding-right')) {
+          body.style.paddingRight = getAttr(body, 'data-padding-right') || ''
+          removeAttr(body, 'data-padding-right')
         }
-      })
-      // Restore body padding
-      const body = document.body
-      if (hasAttr(body, 'data-padding-right')) {
-        body.style.paddingRight = getAttr(body, 'data-padding-right') || ''
-        removeAttr(body, 'data-padding-right')
       }
     }
   },
diff --git a/src/components/modal/modal.spec.js b/src/components/modal/modal.spec.js
@@ -17,4 +17,21 @@ describe('modal', async () => {
     await nextTick()
     expect(app.$refs.button).not.toHaveProperty('__BV_boundEventListeners__.click')
   })
+
+  it('Should show hide modal', async () => {
+    const { app: { $refs } } = window
+    const { modalButton2, modal2 } = $refs
+
+    // show the modal
+    modalButton2.click()
+    await nextTick()
+    expect(Array.isArray(modal2._marginChangedForScroll)).toBe(true)
+
+    // hide the modal
+    modal2.hide()
+    await nextTick()
+    // manually run resetScrollbar because JSDOM doesn't support it
+    modal2.resetScrollbar()
+    expect(modal2._marginChangedForScroll).toBe(null)
+  })
 })
