Decouple Modal's scrollbar functionality (#33245)

diff --git a/js/src/modal.js b/js/src/modal.js
index b2a2e80ebc049bbfd0565dc7b89710a57cb3704b..dea90ec0a75843b4d23898bcdd06b58ee32ba5f8 100644 (file)
--- a/js/src/modal.js
+++ b/js/src/modal.js
@@ -18,6 +18,7 @@ import {
 import EventHandler from './dom/event-handler'
 import Manipulator from './dom/manipulator'
 import SelectorEngine from './dom/selector-engine'
+import { getWidth as getScrollBarWidth, hide as scrollBarHide, reset as scrollBarReset } from './util/scrollbar'
 import BaseComponent from './base-component'
 
 /**
@@ -57,7 +58,6 @@ const EVENT_MOUSEUP_DISMISS = `mouseup.dismiss${EVENT_KEY}`
 const EVENT_MOUSEDOWN_DISMISS = `mousedown.dismiss${EVENT_KEY}`
 const EVENT_CLICK_DATA_API = `click${EVENT_KEY}${DATA_API_KEY}`
 
-const CLASS_NAME_SCROLLBAR_MEASURER = 'modal-scrollbar-measure'
 const CLASS_NAME_BACKDROP = 'modal-backdrop'
 const CLASS_NAME_OPEN = 'modal-open'
 const CLASS_NAME_FADE = 'fade'
@@ -68,8 +68,6 @@ const SELECTOR_DIALOG = '.modal-dialog'
 const SELECTOR_MODAL_BODY = '.modal-body'
 const SELECTOR_DATA_TOGGLE = '[data-bs-toggle="modal"]'
 const SELECTOR_DATA_DISMISS = '[data-bs-dismiss="modal"]'
-const SELECTOR_FIXED_CONTENT = '.fixed-top, .fixed-bottom, .is-fixed, .sticky-top'
-const SELECTOR_STICKY_CONTENT = '.sticky-top'
 
 /**
  * ------------------------------------------------------------------------
@@ -85,10 +83,8 @@ class Modal extends BaseComponent {
     this._dialog = SelectorEngine.findOne(SELECTOR_DIALOG, this._element)
     this._backdrop = null
     this._isShown = false
-    this._isBodyOverflowing = false
     this._ignoreBackdropClick = false
     this._isTransitioning = false
-    this._scrollbarWidth = 0
   }
 
   // Getters
@@ -126,8 +122,9 @@ class Modal extends BaseComponent {
 
     this._isShown = true
 
-    this._checkScrollbar()
-    this._setScrollbar()
+    scrollBarHide()
+
+    document.body.classList.add(CLASS_NAME_OPEN)
 
     this._adjustDialog()
 
@@ -206,10 +203,8 @@ class Modal extends BaseComponent {
     this._dialog = null
     this._backdrop = null
     this._isShown = null
-    this._isBodyOverflowing = null
     this._ignoreBackdropClick = null
     this._isTransitioning = null
-    this._scrollbarWidth = null
   }
 
   handleUpdate() {
@@ -321,7 +316,7 @@ class Modal extends BaseComponent {
     this._showBackdrop(() => {
       document.body.classList.remove(CLASS_NAME_OPEN)
       this._resetAdjustments()
-      this._resetScrollbar()
+      scrollBarReset()
       EventHandler.trigger(this._element, EVENT_HIDDEN)
     })
   }
@@ -433,13 +428,15 @@ class Modal extends BaseComponent {
 
   _adjustDialog() {
     const isModalOverflowing = this._element.scrollHeight > document.documentElement.clientHeight
+    const scrollbarWidth = getScrollBarWidth()
+    const isBodyOverflowing = scrollbarWidth > 0
 
-    if ((!this._isBodyOverflowing && isModalOverflowing && !isRTL()) || (this._isBodyOverflowing && !isModalOverflowing && isRTL())) {
-      this._element.style.paddingLeft = `${this._scrollbarWidth}px`
+    if ((!isBodyOverflowing && isModalOverflowing && !isRTL()) || (isBodyOverflowing && !isModalOverflowing && isRTL())) {
+      this._element.style.paddingLeft = `${scrollbarWidth}px`
     }
 
-    if ((this._isBodyOverflowing && !isModalOverflowing && !isRTL()) || (!this._isBodyOverflowing && isModalOverflowing && isRTL())) {
-      this._element.style.paddingRight = `${this._scrollbarWidth}px`
+    if ((isBodyOverflowing && !isModalOverflowing && !isRTL()) || (!isBodyOverflowing && isModalOverflowing && isRTL())) {
+      this._element.style.paddingRight = `${scrollbarWidth}px`
     }
   }
 
@@ -448,63 +445,6 @@ class Modal extends BaseComponent {
     this._element.style.paddingRight = ''
   }
 
-  _checkScrollbar() {
-    const rect = document.body.getBoundingClientRect()
-    this._isBodyOverflowing = Math.round(rect.left + rect.right) < window.innerWidth
-    this._scrollbarWidth = this._getScrollbarWidth()
-  }
-
-  _setScrollbar() {
-    if (this._isBodyOverflowing) {
-      this._setElementAttributes(SELECTOR_FIXED_CONTENT, 'paddingRight', calculatedValue => calculatedValue + this._scrollbarWidth)
-      this._setElementAttributes(SELECTOR_STICKY_CONTENT, 'marginRight', calculatedValue => calculatedValue - this._scrollbarWidth)
-      this._setElementAttributes('body', 'paddingRight', calculatedValue => calculatedValue + this._scrollbarWidth)
-    }
-
-    document.body.classList.add(CLASS_NAME_OPEN)
-  }
-
-  _setElementAttributes(selector, styleProp, callback) {
-    SelectorEngine.find(selector)
-      .forEach(element => {
-        if (element !== document.body && window.innerWidth > element.clientWidth + this._scrollbarWidth) {
-          return
-        }
-
-        const actualValue = element.style[styleProp]
-        const calculatedValue = window.getComputedStyle(element)[styleProp]
-        Manipulator.setDataAttribute(element, styleProp, actualValue)
-        element.style[styleProp] = `${callback(Number.parseFloat(calculatedValue))}px`
-      })
-  }
-
-  _resetScrollbar() {
-    this._resetElementAttributes(SELECTOR_FIXED_CONTENT, 'paddingRight')
-    this._resetElementAttributes(SELECTOR_STICKY_CONTENT, 'marginRight')
-    this._resetElementAttributes('body', 'paddingRight')
-  }
-
-  _resetElementAttributes(selector, styleProp) {
-    SelectorEngine.find(selector).forEach(element => {
-      const value = Manipulator.getDataAttribute(element, styleProp)
-      if (typeof value === 'undefined' && element === document.body) {
-        element.style[styleProp] = ''
-      } else {
-        Manipulator.removeDataAttribute(element, styleProp)
-        element.style[styleProp] = value
-      }
-    })
-  }
-
-  _getScrollbarWidth() { // thx d.walsh
-    const scrollDiv = document.createElement('div')
-    scrollDiv.className = CLASS_NAME_SCROLLBAR_MEASURER
-    document.body.appendChild(scrollDiv)
-    const scrollbarWidth = scrollDiv.getBoundingClientRect().width - scrollDiv.clientWidth
-    document.body.removeChild(scrollDiv)
-    return scrollbarWidth
-  }
-
   // Static
 
   static jQueryInterface(config, relatedTarget) {

diff --git a/js/src/util/scrollbar.js b/js/src/util/scrollbar.js
index e63a66bf218b21318c5dfcadfee53561fd7453e7..31b614375603e7722fbcdbc61223ea8222319b2a 100644 (file)
--- a/js/src/util/scrollbar.js
+++ b/js/src/util/scrollbar.js
@@ -8,7 +8,7 @@
 import SelectorEngine from '../dom/selector-engine'
 import Manipulator from '../dom/manipulator'
 
-const SELECTOR_FIXED_CONTENT = '.fixed-top, .fixed-bottom, .is-fixed'
+const SELECTOR_FIXED_CONTENT = '.fixed-top, .fixed-bottom, .is-fixed, .sticky-top'
 const SELECTOR_STICKY_CONTENT = '.sticky-top'
 
 const getWidth = () => {
@@ -19,6 +19,7 @@ const getWidth = () => {
 
 const hide = (width = getWidth()) => {
   document.body.style.overflow = 'hidden'
+  // trick: We adjust positive paddingRight and negative marginRight to sticky-top elements, to keep shown fullwidth
   _setElementAttributes(SELECTOR_FIXED_CONTENT, 'paddingRight', calculatedValue => calculatedValue + width)
   _setElementAttributes(SELECTOR_STICKY_CONTENT, 'marginRight', calculatedValue => calculatedValue - width)
   _setElementAttributes('body', 'paddingRight', calculatedValue => calculatedValue + width)
@@ -49,7 +50,7 @@ const reset = () => {
 const _resetElementAttributes = (selector, styleProp) => {
   SelectorEngine.find(selector).forEach(element => {
     const value = Manipulator.getDataAttribute(element, styleProp)
-    if (typeof value === 'undefined' && element === document.body) {
+    if (typeof value === 'undefined') {
       element.style.removeProperty(styleProp)
     } else {
       Manipulator.removeDataAttribute(element, styleProp)

diff --git a/js/tests/unit/modal.spec.js b/js/tests/unit/modal.spec.js
index f10a932d2a51e0f2137464e4b5205e572a53bc54..99ebbe4b37b5673be975f968cdb3749342195d06 100644 (file)
--- a/js/tests/unit/modal.spec.js
+++ b/js/tests/unit/modal.spec.js
@@ -1,27 +1,15 @@
 import Modal from '../../src/modal'
 import EventHandler from '../../src/dom/event-handler'
+import { getWidth as getScrollBarWidth } from '../../src/util/scrollbar'
 
 /** Test helpers */
 import { clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture'
 
 describe('Modal', () => {
   let fixtureEl
-  let style
 
   beforeAll(() => {
     fixtureEl = getFixture()
-
-    // Enable the scrollbar measurer
-    const css = '.modal-scrollbar-measure { position: absolute; top: -9999px; width: 50px; height: 50px; overflow: scroll; }'
-
-    style = document.createElement('style')
-    style.type = 'text/css'
-    style.appendChild(document.createTextNode(css))
-
-    document.head.appendChild(style)
-
-    // Simulate scrollbars
-    document.documentElement.style.paddingRight = '16px'
   })
 
   afterEach(() => {
@@ -36,12 +24,11 @@ describe('Modal', () => {
         document.body.removeChild(backdrop)
       })
 
-    document.body.style.paddingRight = '0px'
+    document.body.style.removeProperty('paddingRight')
   })
 
   afterAll(() => {
-    document.head.removeChild(style)
-    document.documentElement.style.paddingRight = '0px'
+    document.documentElement.style.removeProperty('paddingRight')
   })
 
   describe('VERSION', () => {
@@ -79,6 +66,7 @@ describe('Modal', () => {
     it('should toggle a modal', done => {
       fixtureEl.innerHTML = '<div class="modal"><div class="modal-dialog"></div></div>'
 
+      document.documentElement.style.overflowY = 'scroll'
       const modalEl = fixtureEl.querySelector('.modal')
       const modal = new Modal(modalEl)
       const originalPadding = '0px'
@@ -93,6 +81,7 @@ describe('Modal', () => {
       modalEl.addEventListener('hidden.bs.modal', () => {
         expect(document.body.getAttribute('data-bs-padding-right')).toBeNull()
         expect().nothing()
+        document.documentElement.style.overflowY = 'auto'
         done()
       })
 
@@ -105,13 +94,14 @@ describe('Modal', () => {
         '<div class="modal"><div class="modal-dialog"></div></div>'
       ].join('')
 
+      document.documentElement.style.overflowY = 'scroll'
       const fixedEl = fixtureEl.querySelector('.fixed-top')
       const originalPadding = Number.parseInt(window.getComputedStyle(fixedEl).paddingRight, 10)
       const modalEl = fixtureEl.querySelector('.modal')
       const modal = new Modal(modalEl)
 
       modalEl.addEventListener('shown.bs.modal', () => {
-        const expectedPadding = originalPadding + modal._getScrollbarWidth()
+        const expectedPadding = originalPadding + getScrollBarWidth()
         const currentPadding = Number.parseInt(window.getComputedStyle(modalEl).paddingRight, 10)
 
         expect(fixedEl.getAttribute('data-bs-padding-right')).toEqual('0px', 'original fixed element padding should be stored in data-bs-padding-right')
@@ -124,6 +114,7 @@ describe('Modal', () => {
 
         expect(fixedEl.getAttribute('data-bs-padding-right')).toEqual(null, 'data-bs-padding-right should be cleared after closing')
         expect(currentPadding).toEqual(originalPadding, 'fixed element padding should be reset after closing')
+        document.documentElement.style.overflowY = 'auto'
         done()
       })
 
@@ -136,13 +127,15 @@ describe('Modal', () => {
         '<div class="modal"><div class="modal-dialog"></div></div>'
       ].join('')
 
+      document.documentElement.style.overflowY = 'scroll'
+
       const stickyTopEl = fixtureEl.querySelector('.sticky-top')
       const originalMargin = Number.parseInt(window.getComputedStyle(stickyTopEl).marginRight, 10)
       const modalEl = fixtureEl.querySelector('.modal')
       const modal = new Modal(modalEl)
 
       modalEl.addEventListener('shown.bs.modal', () => {
-        const expectedMargin = originalMargin - modal._getScrollbarWidth()
+        const expectedMargin = originalMargin - getScrollBarWidth()
         const currentMargin = Number.parseInt(window.getComputedStyle(stickyTopEl).marginRight, 10)
 
         expect(stickyTopEl.getAttribute('data-bs-margin-right')).toEqual('0px', 'original sticky element margin should be stored in data-bs-margin-right')
@@ -155,6 +148,8 @@ describe('Modal', () => {
 
         expect(stickyTopEl.getAttribute('data-bs-margin-right')).toEqual(null, 'data-bs-margin-right should be cleared after closing')
         expect(currentMargin).toEqual(originalMargin, 'sticky element margin should be reset after closing')
+
+        document.documentElement.style.overflowY = 'auto'
         done()
       })
 
@@ -238,27 +233,6 @@ describe('Modal', () => {
 
       modal.toggle()
     })
-
-    it('should properly restore non-pixel inline body padding after closing', done => {
-      fixtureEl.innerHTML = '<div class="modal"><div class="modal-dialog"></div></div>'
-
-      document.body.style.paddingRight = '5%'
-
-      const modalEl = fixtureEl.querySelector('.modal')
-      const modal = new Modal(modalEl)
-
-      modalEl.addEventListener('shown.bs.modal', () => {
-        modal.toggle()
-      })
-
-      modalEl.addEventListener('hidden.bs.modal', () => {
-        expect(document.body.style.paddingRight).toEqual('5%')
-        document.body.removeAttribute('style')
-        done()
-      })
-
-      modal.toggle()
-    })
   })
 
   describe('show', () => {

diff --git a/scss/_modal.scss b/scss/_modal.scss
index 6dd4dd32941fdefcfb0137e1989b2ff53a95a888..513898644d269facf02002502d5e34819c67939e 100644 (file)
--- a/scss/_modal.scss
+++ b/scss/_modal.scss
@@ -159,15 +159,6 @@
   }
 }
 
-// Measure scrollbar width for padding body during modal show/hide
-.modal-scrollbar-measure {
-  position: absolute;
-  top: -9999px;
-  width: 50px;
-  height: 50px;
-  overflow: scroll;
-}
-
 // Scale up the modal
 @include media-breakpoint-up(sm) {
   // Automatically set modal's width for larger viewports