Rewrite Modals.setScrollbar() and Modals.resetScrollbar() to properly handle padding-right of body and fixed elements

Created by: deilv

Fixes #18373 (closed), Fixes #20824 (closed), Fixes #21700 (closed), Fixes #21840 (closed) (duplicates)

The problem:

_setScrollbar() and _resetScrollbar() in file js/src/modal.js failed to properly adjust padding-right when opening and closing modals, while using fixed elements (ie fixed-top navbar). Both functions had to be rewritten.

Changes:

_setScrollbar() -- called when opening a modal

• Store the original body padding-right in data-padding-right
• Adjust the body padding-right according to the calculated _scrollbarWidth
• Store the fixed element's original padding-right in data-padding-right
• Adjust the element's padding-right according to the calculated _scrollbarWidth
• Store the navbar-toggler's original margin-right in data-margin-right
• Adjust the navbar-toggler's margin-right according to the calculated _scrollbarWidth

_resetScrollbar() -- called when closing a modal

• Restore original body padding-right and remove the data attribute
• Restore original fixed element's padding-right and remove the data attribute
• Restore original navbar-toggler's margin-right and remove the data attribute

Unit Tests

• Fixed missing modal-scrollbar-measure in modal unit tests
• Fixed test for body padding adjustment
• Added tests for fixed element padding adjustment
• Added tests for navbar-toggler margin adjustment
• Removed some unused tests from previous, incomplete patches

You can see the changes live in codepen.

js/src/modal.js

@@ -67,7 +67,8 @@ const Modal = (($) => {
     DIALOG             : '.modal-dialog',
     DATA_TOGGLE        : '[data-toggle="modal"]',
     DATA_DISMISS       : '[data-dismiss="modal"]',
-    FIXED_CONTENT      : '.fixed-top, .fixed-bottom, .is-fixed, .sticky-top'
+    FIXED_CONTENT      : '.fixed-top, .fixed-bottom, .is-fixed, .sticky-top',
+    NAVBAR_TOGGLER     : '.navbar-toggler'
   }
 
 
@@ -212,7 +213,6 @@ const Modal = (($) => {
       this._isShown             = null
       this._isBodyOverflowing   = null
       this._ignoreBackdropClick = null
-      this._originalBodyPadding = null
       this._scrollbarWidth      = null
     }
 
@@ -429,21 +429,53 @@ const Modal = (($) => {
     }
 
     _setScrollbar() {
-      const bodyPadding = parseInt(
-        $(Selector.FIXED_CONTENT).css('padding-right') || 0,
-        10
-      )
+      if (this._isBodyOverflowing) {
+        // Note: DOMNode.style.paddingRight returns the actual value or '' if not set
+        //   while $(DOMNode).css('padding-right') returns the calculated value or 0 if not set
+
+        // Adjust fixed content padding
+        $(Selector.FIXED_CONTENT).each((index, element) => {
+          const actualPadding = $(element)[0].style.paddingRight
+          const calculatedPadding = $(element).css('padding-right')
+          $(element).data('padding-right', actualPadding).css('padding-right', `${parseFloat(calculatedPadding) + this._scrollbarWidth}px`)
+        })
 
-      this._originalBodyPadding = document.body.style.paddingRight || ''
+        // Adjust navbar-toggler margin
+        $(Selector.NAVBAR_TOGGLER).each((index, element) => {
+          const actualMargin = $(element)[0].style.marginRight
+          const calculatedMargin = $(element).css('margin-right')
+          $(element).data('margin-right', actualMargin).css('margin-right', `${parseFloat(calculatedMargin) + this._scrollbarWidth}px`)
+        })
 
-      if (this._isBodyOverflowing) {
-        document.body.style.paddingRight =
-          `${bodyPadding + this._scrollbarWidth}px`
+        // Adjust body padding
+        const actualPadding = document.body.style.paddingRight
+        const calculatedPadding = $('body').css('padding-right')
+        $('body').data('padding-right', actualPadding).css('padding-right', `${parseFloat(calculatedPadding) + this._scrollbarWidth}px`)
       }
     }
 
     _resetScrollbar() {
-      document.body.style.paddingRight = this._originalBodyPadding
+      // Restore fixed content padding
+      $(Selector.FIXED_CONTENT).each((index, element) => {
+        const padding = $(element).data('padding-right')
+        if (typeof padding !== 'undefined') {
+          $(element).css('padding-right', padding).removeData('padding-right')
+        }
+      })
+
+      // Restore navbar-toggler margin
+      $(Selector.NAVBAR_TOGGLER).each((index, element) => {
+        const margin = $(element).data('margin-right')
+        if (typeof margin !== 'undefined') {
+          $(element).css('margin-right', margin).removeData('margin-right')
+        }
+      })
+
+      // Restore body padding
+      const padding = $('body').data('padding-right')
+      if (typeof padding !== 'undefined') {
+        $('body').css('padding-right', padding).removeData('padding-right')
+      }
     }
 
     _getScrollbarWidth() { // thx d.walsh

js/tests/unit/modal.js

@@ -9,6 +9,10 @@ $(function () {
   })
 
   QUnit.module('modal', {
+    before: function () {
+      // Enable the scrollbar measurer
+      $('<style type="text/css"> .modal-scrollbar-measure { position: absolute; top: -9999px; width: 50px; height: 50px; overflow: scroll; } </style>').appendTo('head')
+    },
     beforeEach: function () {
       // Run all tests in noConflict mode -- it's the only way to ensure that the plugin works in noConflict mode
       $.fn.bootstrapModal = $.fn.modal.noConflict()
@@ -336,81 +340,144 @@ $(function () {
     $toggleBtn.trigger('click')
   })
 
-  QUnit.test('should restore inline body padding after closing', function (assert) {
+  QUnit.test('should adjust the inline body padding when opening and restore when closing', function (assert) {
     assert.expect(2)
     var done = assert.async()
-    var originalBodyPad = 0
     var $body = $(document.body)
-
-    $body.css('padding-right', originalBodyPad)
+    var originalPadding = $body.css('padding-right')
 
     $('<div id="modal-test"/>')
       .on('hidden.bs.modal', function () {
-        var currentBodyPad = parseInt($body.css('padding-right'), 10)
-        assert.notStrictEqual($body.attr('style'), '', 'body has non-empty style attribute')
-        assert.strictEqual(currentBodyPad, originalBodyPad, 'original body padding was not changed')
+        var currentPadding = $body.css('padding-right')
+        assert.strictEqual(currentPadding, originalPadding, 'body padding should be reset after closing')
         $body.removeAttr('style')
         done()
       })
       .on('shown.bs.modal', function () {
+        var currentPadding = $body.css('padding-right')
+        assert.notStrictEqual(currentPadding, originalPadding, 'body padding should be adjusted while opening')
         $(this).bootstrapModal('hide')
       })
       .bootstrapModal('show')
   })
 
-  QUnit.test('should ignore values set via CSS when trying to restore body padding after closing', function (assert) {
-    assert.expect(1)
+  QUnit.test('should store the original body padding in data-padding-right before showing', function (assert) {
+    assert.expect(2)
     var done = assert.async()
     var $body = $(document.body)
-    var $style = $('<style>body { padding-right: 42px; }</style>').appendTo('head')
+    var originalPadding = '0px'
+    $body.css('padding-right', originalPadding)
 
     $('<div id="modal-test"/>')
       .on('hidden.bs.modal', function () {
-        assert.ok(!$body.attr('style'), 'body does not have inline padding set')
-        $style.remove()
+        assert.strictEqual($body.data('padding-right'), undefined, 'data-padding-right should be cleared after closing')
+        $body.removeAttr('style')
         done()
       })
       .on('shown.bs.modal', function () {
+        assert.strictEqual($body.data('padding-right'), originalPadding, 'original body padding should be stored in data-padding-right')
         $(this).bootstrapModal('hide')
       })
       .bootstrapModal('show')
   })
 
-  QUnit.test('should have a paddingRight when the modal is taller than the viewport', function (assert) {
+  QUnit.test('should adjust the inline padding of fixed elements when opening and restore when closing', function (assert) {
     assert.expect(2)
     var done = assert.async()
-    $('<div class="fixed-top fixed-bottom sticky-top is-fixed">@Johann-S</div>').appendTo('#qunit-fixture')
-    $('.fixed-top, .fixed-bottom, .is-fixed, .sticky-top').css('padding-right', '10px')
+    var $element = $('<div class="fixed-top"></div>').appendTo('#qunit-fixture')
+    var originalPadding = $element.css('padding-right')
 
     $('<div id="modal-test"/>')
+      .on('hidden.bs.modal', function () {
+        var currentPadding = $element.css('padding-right')
+        assert.strictEqual(currentPadding, originalPadding, 'fixed element padding should be reset after closing')
+        $element.remove()
+        done()
+      })
       .on('shown.bs.modal', function () {
-        var paddingRight = parseInt($(document.body).css('padding-right'), 10)
-        assert.strictEqual(isNaN(paddingRight), false)
-        assert.strictEqual(paddingRight !== 0, true)
-        $(document.body).css('padding-right', '') // Because test case "should ignore other inline styles when trying to restore body padding after closing" fail if not
+        var currentPadding = $element.css('padding-right')
+        assert.notStrictEqual(currentPadding, originalPadding, 'fixed element padding should be adjusted while opening')
+        $(this).bootstrapModal('hide')
+      })
+      .bootstrapModal('show')
+  })
+
+  QUnit.test('should store the original padding of fixed elements in data-padding-right before showing', function (assert) {
+    assert.expect(2)
+    var done = assert.async()
+    var $element = $('<div class="fixed-top"></div>').appendTo('#qunit-fixture')
+    var originalPadding = '0px'
+    $element.css('padding-right', originalPadding)
+
+    $('<div id="modal-test"/>')
+      .on('hidden.bs.modal', function () {
+        assert.strictEqual($element.data('padding-right'), undefined, 'data-padding-right should be cleared after closing')
+        $element.remove()
         done()
       })
+      .on('shown.bs.modal', function () {
+        assert.strictEqual($element.data('padding-right'), originalPadding, 'original fixed element padding should be stored in data-padding-right')
+        $(this).bootstrapModal('hide')
+      })
       .bootstrapModal('show')
   })
 
-  QUnit.test('should remove padding-right on modal after closing', function (assert) {
-    assert.expect(3)
+  QUnit.test('should adjust the inline margin of the navbar-toggler when opening and restore when closing', function (assert) {
+    assert.expect(2)
+    var done = assert.async()
+    var $element = $('<div class="navbar-toggler"></div>').appendTo('#qunit-fixture')
+    var originalMargin = $element.css('margin-right')
+
+    $('<div id="modal-test"/>')
+      .on('hidden.bs.modal', function () {
+        var currentMargin = $element.css('margin-right')
+        assert.strictEqual(currentMargin, originalMargin, 'navbar-toggler margin should be reset after closing')
+        $element.remove()
+        done()
+      })
+      .on('shown.bs.modal', function () {
+        var currentMargin = $element.css('margin-right')
+        assert.notStrictEqual(currentMargin, originalMargin, 'navbar-toggler margin should be adjusted while opening')
+        $(this).bootstrapModal('hide')
+      })
+      .bootstrapModal('show')
+  })
+
+  QUnit.test('should store the original margin of the navbar-toggler in data-margin-right before showing', function (assert) {
+    assert.expect(2)
     var done = assert.async()
-    $('<div class="fixed-top fixed-bottom is-fixed sticky-top">@Johann-S</div>').appendTo('#qunit-fixture')
-    $('.fixed-top, .fixed-bottom, .is-fixed, .sticky-top').css('padding-right', '10px')
+    var $element = $('<div class="navbar-toggler"></div>').appendTo('#qunit-fixture')
+    var originalMargin = '0px'
+    $element.css('margin-right', originalMargin)
 
     $('<div id="modal-test"/>')
+      .on('hidden.bs.modal', function () {
+        assert.strictEqual($element.data('margin-right'), undefined, 'data-margin-right should be cleared after closing')
+        $element.remove()
+        done()
+      })
       .on('shown.bs.modal', function () {
-        var paddingRight = parseInt($(document.body).css('padding-right'), 10)
-        assert.strictEqual(isNaN(paddingRight), false)
-        assert.strictEqual(paddingRight !== 0, true)
+        assert.strictEqual($element.data('margin-right'), originalMargin, 'original navbar-toggler margin should be stored in data-margin-right')
         $(this).bootstrapModal('hide')
       })
+      .bootstrapModal('show')
+  })
+
+  QUnit.test('should ignore values set via CSS when trying to restore body padding after closing', function (assert) {
+    assert.expect(1)
+    var done = assert.async()
+    var $body = $(document.body)
+    var $style = $('<style>body { padding-right: 42px; }</style>').appendTo('head')
+
+    $('<div id="modal-test"/>')
       .on('hidden.bs.modal', function () {
-        var paddingRight = parseInt($(document.body).css('padding-right'), 10)
-        assert.strictEqual(paddingRight, 0)
+        assert.ok(!$body.attr('style'), 'body does not have inline padding set')
+        $style.remove()
         done()
       })
+      .on('shown.bs.modal', function () {
+        $(this).bootstrapModal('hide')
+      })
       .bootstrapModal('show')
   })