Scrollbar.js Sanitizations

* Remove `.modal-open` class as it is already handled by scrollbar.js * _modal.scss make modal `overflowY: auto` by default (was on ``.modal-open`, which is the same) * scrollbar.js: keep initial body `overflowY` value as attribute and re-set it properly & test * scrollbar.js: avoid to create data-bs attributes for null values & test * Revisit modal, offcanvas, scrollbar tests * set the base to support different rootElement (than body)
twbs · Apr 17, 2021 · 0d877f5 · 0d877f5
1 parent f61a021
commit 0d877f5
Show file tree

Hide file tree

Showing 8 changed files with 176 additions and 122 deletions.
diff --git a/js/src/modal.js b/js/src/modal.js
@@ -59,7 +59,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_OPEN = 'modal-open'
 const CLASS_NAME_FADE = 'fade'
 const CLASS_NAME_SHOW = 'show'
 const CLASS_NAME_STATIC = 'modal-static'
@@ -124,8 +123,6 @@ class Modal extends BaseComponent {
 
     scrollBarHide()
 
-    document.body.classList.add(CLASS_NAME_OPEN)
-
     this._adjustDialog()
 
     this._setEscapeEvent()
@@ -322,7 +319,6 @@ class Modal extends BaseComponent {
     this._element.removeAttribute('role')
     this._isTransitioning = false
     this._backdrop.hide(() => {
-      document.body.classList.remove(CLASS_NAME_OPEN)
       this._resetAdjustments()
       scrollBarReset()
       EventHandler.trigger(this._element, EVENT_HIDDEN)

diff --git a/js/src/util/scrollbar.js b/js/src/util/scrollbar.js
@@ -7,56 +7,82 @@
 
 import SelectorEngine from '../dom/selector-engine'
 import Manipulator from '../dom/manipulator'
+import { isElement } from './index'
 
 const SELECTOR_FIXED_CONTENT = '.fixed-top, .fixed-bottom, .is-fixed, .sticky-top'
 const SELECTOR_STICKY_CONTENT = '.sticky-top'
 
+const _doc = document.documentElement
+
 const getWidth = () => {
   // https://developer.mozilla.org/en-US/docs/Web/API/Window/innerWidth#usage_notes
-  const documentWidth = document.documentElement.clientWidth
+  const documentWidth = _doc.clientWidth
   return Math.abs(window.innerWidth - documentWidth)
 }
 
 const hide = (width = getWidth()) => {
-  document.body.style.overflow = 'hidden'
+  _disableOverFlow()
+  _setElementAttributes(_doc, 'paddingRight', calculatedValue => calculatedValue + width)
   // 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)
+}
+
+const _disableOverFlow = () => {
+  const actualValue = _doc.style.overflowY
+  if (actualValue.length) {
+    Manipulator.setDataAttribute(_doc, 'overflowY', actualValue)
+  }
+
+  _doc.style.overflowY = 'hidden'
 }
 
 const _setElementAttributes = (selector, styleProp, callback) => {
   const scrollbarWidth = getWidth()
-  SelectorEngine.find(selector)
-    .forEach(element => {
-      if (element !== document.body && window.innerWidth > element.clientWidth + scrollbarWidth) {
-        return
-      }
-
-      const actualValue = element.style[styleProp]
-      const calculatedValue = window.getComputedStyle(element)[styleProp]
+  const manipulationCallBack = element => {
+    if (element !== _doc && window.innerWidth > element.clientWidth + scrollbarWidth) {
+      return
+    }
+
+    const actualValue = element.style[styleProp]
+    if (actualValue.length) {
       Manipulator.setDataAttribute(element, styleProp, actualValue)
-      element.style[styleProp] = `${callback(Number.parseFloat(calculatedValue))}px`
-    })
+    }
+
+    const calculatedValue = window.getComputedStyle(element)[styleProp]
+    element.style[styleProp] = `${callback(Number.parseFloat(calculatedValue))}px`
+  }
+
+  _applyManipulationCallback(selector, manipulationCallBack)
 }
 
 const reset = () => {
-  document.body.style.overflow = 'auto'
+  _resetElementAttributes(_doc, 'overflowY')
+  _resetElementAttributes(_doc, 'paddingRight')
   _resetElementAttributes(SELECTOR_FIXED_CONTENT, 'paddingRight')
   _resetElementAttributes(SELECTOR_STICKY_CONTENT, 'marginRight')
-  _resetElementAttributes('body', 'paddingRight')
 }
 
 const _resetElementAttributes = (selector, styleProp) => {
-  SelectorEngine.find(selector).forEach(element => {
+  const manipulationCallBack = element => {
     const value = Manipulator.getDataAttribute(element, styleProp)
     if (typeof value === 'undefined') {
       element.style.removeProperty(styleProp)
     } else {
       Manipulator.removeDataAttribute(element, styleProp)
       element.style[styleProp] = value
     }
-  })
+  }
+
+  _applyManipulationCallback(selector, manipulationCallBack)
+}
+
+const _applyManipulationCallback = (selector, callBack) => {
+  if (isElement(selector)) {
+    callBack(selector)
+  } else {
+    SelectorEngine.find(selector).forEach(callBack)
+  }
 }
 
 const isBodyOverflowing = () => {

diff --git a/js/tests/helpers/fixture.js b/js/tests/helpers/fixture.js
@@ -39,3 +39,10 @@ export const jQueryMock = {
     })
   }
 }
+
+export const clearBodyAndDocument = () => {
+  document.documentElement.removeAttribute('style')
+  document.body.removeAttribute('style')
+  document.documentElement.removeAttribute('data-bs-padding-right')
+  document.body.removeAttribute('data-bs-padding-right')
+}
diff --git a/js/tests/unit/modal.spec.js b/js/tests/unit/modal.spec.js
@@ -3,7 +3,7 @@ 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'
+import { clearBodyAndDocument, clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture'
 
 describe('Modal', () => {
   let fixtureEl
@@ -14,11 +14,7 @@ describe('Modal', () => {
 
   afterEach(() => {
     clearFixture()
-
-    document.body.classList.remove('modal-open')
-    document.documentElement.removeAttribute('style')
-    document.body.removeAttribute('style')
-    document.body.removeAttribute('data-bs-padding-right')
+    clearBodyAndDocument()
 
     document.querySelectorAll('.modal-backdrop')
       .forEach(backdrop => {
@@ -27,9 +23,7 @@ describe('Modal', () => {
   })
 
   beforeEach(() => {
-    document.documentElement.removeAttribute('style')
-    document.body.removeAttribute('style')
-    document.body.removeAttribute('data-bs-padding-right')
+    clearBodyAndDocument()
   })
 
   describe('VERSION', () => {
@@ -67,22 +61,23 @@ 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 doc = document.documentElement
+      doc.style.overflowY = 'scroll'
       const modalEl = fixtureEl.querySelector('.modal')
       const modal = new Modal(modalEl)
       const originalPadding = '0px'
 
-      document.body.style.paddingRight = originalPadding
+      doc.style.paddingRight = originalPadding
 
       modalEl.addEventListener('shown.bs.modal', () => {
-        expect(document.body.getAttribute('data-bs-padding-right')).toEqual(originalPadding, 'original body padding should be stored in data-bs-padding-right')
+        expect(doc.getAttribute('data-bs-padding-right')).toEqual(originalPadding, 'original body padding should be stored in data-bs-padding-right')
         modal.toggle()
       })
 
       modalEl.addEventListener('hidden.bs.modal', () => {
-        expect(document.body.getAttribute('data-bs-padding-right')).toBeNull()
+        expect(doc.getAttribute('data-bs-padding-right')).toBeNull()
         expect().nothing()
-        document.documentElement.style.overflowY = 'auto'
+        doc.style.overflowY = 'auto'
         done()
       })
 
@@ -95,7 +90,8 @@ describe('Modal', () => {
         '<div class="modal"><div class="modal-dialog"></div></div>'
       ].join('')
 
-      document.documentElement.style.overflowY = 'scroll'
+      const doc = document.documentElement
+      doc.style.overflowY = 'scroll'
       const fixedEl = fixtureEl.querySelector('.fixed-top')
       const originalPadding = Number.parseInt(window.getComputedStyle(fixedEl).paddingRight, 10)
       const modalEl = fixtureEl.querySelector('.modal')
@@ -115,7 +111,6 @@ describe('Modal', () => {
 
         expect(fixedEl.hasAttribute('data-bs-padding-right')).toEqual(false, '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()
       })
 
@@ -124,33 +119,35 @@ describe('Modal', () => {
 
     it('should adjust the inline margin of sticky elements when opening and restore when closing', done => {
       fixtureEl.innerHTML = [
-        '<div class="sticky-top" style="margin-right: 0px;"></div>',
+        '<div class="sticky-top" style="margin-right: 10px;"></div>',
         '<div class="modal"><div class="modal-dialog"></div></div>'
       ].join('')
 
-      document.documentElement.style.overflowY = 'scroll'
+      const doc = document.documentElement
+      doc.style.overflowY = 'scroll'
 
       const stickyTopEl = fixtureEl.querySelector('.sticky-top')
-      const originalMargin = Number.parseInt(window.getComputedStyle(stickyTopEl).marginRight, 10)
+      const getStickyMargin = () => Number.parseInt(window.getComputedStyle(stickyTopEl).marginRight, 10)
+      const originalMargin = getStickyMargin()
       const modalEl = fixtureEl.querySelector('.modal')
       const modal = new Modal(modalEl)
+      const scrollBarWidth = getScrollBarWidth()
 
       modalEl.addEventListener('shown.bs.modal', () => {
-        const expectedMargin = originalMargin - getScrollBarWidth()
-        const currentMargin = Number.parseInt(window.getComputedStyle(stickyTopEl).marginRight, 10)
+        const expectedMargin = originalMargin - scrollBarWidth
+        const currentMargin = getStickyMargin()
 
         expect(stickyTopEl.getAttribute('data-bs-margin-right')).toEqual(`${originalMargin}px`, 'original sticky element margin should be stored in data-bs-margin-right')
         expect(currentMargin).toEqual(expectedMargin, 'sticky element margin should be adjusted while opening')
         modal.toggle()
       })
 
       modalEl.addEventListener('hidden.bs.modal', () => {
-        const currentMargin = Number.parseInt(window.getComputedStyle(stickyTopEl).marginRight, 10)
+        const currentMargin = getStickyMargin()
 
         expect(stickyTopEl.hasAttribute('data-bs-margin-right')).toEqual(false, '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()
       })
 
@@ -670,16 +667,14 @@ describe('Modal', () => {
       const originalPadding = window.getComputedStyle(document.body).paddingRight
 
       // Hide scrollbars to prevent the body overflowing
-      document.body.style.overflow = 'hidden'
-      document.documentElement.style.paddingRight = '0px'
+      const doc = document.documentElement
+      doc.style.overflowY = 'hidden'
+      doc.style.paddingRight = '0px'
 
       modalEl.addEventListener('shown.bs.modal', () => {
         const currentPadding = window.getComputedStyle(document.body).paddingRight
 
         expect(currentPadding).toEqual(originalPadding, 'body padding should not be adjusted')
-
-        // Restore scrollbars
-        document.body.style.overflow = 'auto'
         done()
       })
 
@@ -692,25 +687,21 @@ describe('Modal', () => {
       const modalEl = fixtureEl.querySelector('.modal')
       const modal = new Modal(modalEl)
       const originalPadding = window.getComputedStyle(document.body).paddingRight
-
+      const doc = document.documentElement
       // Remove body margins as would be done by Bootstrap css
       document.body.style.margin = '0'
 
       // Hide scrollbars to prevent the body overflowing
-      document.body.style.overflow = 'hidden'
+      doc.style.overflowY = 'hidden'
 
       // Simulate a discrepancy between exact, i.e. floating point body width, and rounded body width
       // as it can occur when zooming or scaling the display to something else than 100%
-      document.documentElement.style.paddingRight = '.48px'
+      doc.style.paddingRight = '.48px'
 
       modalEl.addEventListener('shown.bs.modal', () => {
         const currentPadding = window.getComputedStyle(document.body).paddingRight
 
         expect(currentPadding).toEqual(originalPadding, 'body padding should not be adjusted')
-
-        // Restore overridden css
-        document.body.style.removeProperty('margin')
-        document.body.style.removeProperty('overflow')
         done()
       })
 

diff --git a/js/tests/unit/offcanvas.spec.js b/js/tests/unit/offcanvas.spec.js
@@ -2,7 +2,7 @@ import Offcanvas from '../../src/offcanvas'
 import EventHandler from '../../src/dom/event-handler'
 
 /** Test helpers */
-import { clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture'
+import { clearBodyAndDocument, clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture'
 
 describe('Offcanvas', () => {
   let fixtureEl
@@ -14,15 +14,11 @@ describe('Offcanvas', () => {
   afterEach(() => {
     clearFixture()
     document.body.classList.remove('offcanvas-open')
-    document.documentElement.removeAttribute('style')
-    document.body.removeAttribute('style')
-    document.body.removeAttribute('data-bs-padding-right')
+    clearBodyAndDocument()
   })
 
   beforeEach(() => {
-    document.documentElement.removeAttribute('style')
-    document.body.removeAttribute('style')
-    document.body.removeAttribute('data-bs-padding-right')
+    clearBodyAndDocument()
   })
 
   describe('VERSION', () => {
@@ -160,15 +156,16 @@ describe('Offcanvas', () => {
 
       const offCanvasEl = fixtureEl.querySelector('.offcanvas')
       const offCanvas = new Offcanvas(offCanvasEl, { scroll: true })
-      const initialOverFlow = document.body.style.overflow
+      const doc = document.documentElement
+      const initialOverFlow = doc.style.overflowY
 
       offCanvasEl.addEventListener('shown.bs.offcanvas', () => {
-        expect(document.body.style.overflow).toEqual(initialOverFlow)
+        expect(doc.style.overflowY).toEqual(initialOverFlow)
 
         offCanvas.hide()
       })
       offCanvasEl.addEventListener('hidden.bs.offcanvas', () => {
-        expect(document.body.style.overflow).toEqual(initialOverFlow)
+        expect(doc.style.overflowY).toEqual(initialOverFlow)
         done()
       })
       offCanvas.show()
@@ -177,16 +174,18 @@ describe('Offcanvas', () => {
     it('if scroll is disabled, should not allow body to scroll while offcanvas is open', done => {
       fixtureEl.innerHTML = '<div class="offcanvas"></div>'
 
+      const doc = document.documentElement
+      doc.style.overflowY = 'scroll'
       const offCanvasEl = fixtureEl.querySelector('.offcanvas')
       const offCanvas = new Offcanvas(offCanvasEl, { scroll: false })
 
       offCanvasEl.addEventListener('shown.bs.offcanvas', () => {
-        expect(document.body.style.overflow).toEqual('hidden')
+        expect(doc.style.overflowY).toEqual('hidden')
 
         offCanvas.hide()
       })
       offCanvasEl.addEventListener('hidden.bs.offcanvas', () => {
-        expect(document.body.style.overflow).not.toEqual('hidden')
+        expect(doc.style.overflowY).toEqual('scroll')
         done()
       })
       offCanvas.show()