Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple Modal's scrollbar functionality #33245

Merged
merged 2 commits into from
Apr 11, 2021
Merged

Decouple Modal's scrollbar functionality #33245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfd72bd
Decouple scrollbar functionality on Modal.js using Scrollbar.js
GeoSot Mar 2, 2021
28e0101
bring back sticky-top
GeoSot Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 11 additions & 71 deletions js/src/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'

/**
Expand Down Expand Up @@ -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'
Expand All @@ -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'
GeoSot marked this conversation as resolved.
Show resolved Hide resolved
const SELECTOR_STICKY_CONTENT = '.sticky-top'

/**
* ------------------------------------------------------------------------
Expand All @@ -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
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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)
})
}
Expand Down Expand Up @@ -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`
}
}

Expand All @@ -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) {
Expand Down
5 changes: 3 additions & 2 deletions js/src/util/scrollbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

@GeoSot GeoSot Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish would be me :P

In this PR I just remove the copied the code, out of modal.
It was written like this. I am trying to finish this and a new issue waits #33580 (afraid of how many tests need to tweak)

This comment was marked as resolved.

Sign in to view

const SELECTOR_STICKY_CONTENT = '.sticky-top'

const getWidth = () => {
Expand All @@ -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)
Expand Down Expand Up @@ -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') {
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just thinking if the value was not stored on the data attribute then why the style prop it is being removed from the element? 🤔

Since if the value here:

const value = Manipulator.getDataAttribute(element, styleProp)

is undefined, it means that the style prop was not added by the bootstrap and that's why should not be removed (that style prop might be present on the dom element already added by the end-user)

Copy link
Member Author

@GeoSot GeoSot Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right on this.
I will keep it as default, and we can discuss it on a next PR

element.style.removeProperty(styleProp)
} else {
Manipulator.removeDataAttribute(element, styleProp)
Expand Down
52 changes: 13 additions & 39 deletions js/tests/unit/modal.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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(() => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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'
Expand All @@ -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()
})

Expand All @@ -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')
Expand All @@ -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()
})

Expand All @@ -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')
Expand All @@ -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()
})

Expand Down Expand Up @@ -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', () => {
Expand Down
9 changes: 0 additions & 9 deletions scss/_modal.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down