Decouple Modal's scrollbar functionality #33245

GeoSot · 2021-03-02T22:49:26Z

Scorllbar. js is the exact same code of modal.js which handles all the scrollbar functionality.
It is extracted, self-tested and used on Offcanvas with success, and it can be used on modal.js through methods

preview

GeoSot · 2021-03-02T22:50:59Z

Please can somebody explain me the usage and the checks of this code block?

    if ((!isBodyOverflowing && isModalOverflowing && !isRTL()) || (isBodyOverflowing && !isModalOverflowing && isRTL())) {
      this._element.style.paddingLeft = `${scrollbarWidth}px`
    }

    if ((isBodyOverflowing && !isModalOverflowing && !isRTL()) || (!isBodyOverflowing && isModalOverflowing && isRTL())) {
      this._element.style.paddingRight = `${scrollbarWidth}px`
    }

js/src/modal.js

alpadev · 2021-04-07T06:19:56Z

Please can somebody explain me the usage and the checks of this code block?

    if ((!isBodyOverflowing && isModalOverflowing && !isRTL()) || (isBodyOverflowing && !isModalOverflowing && isRTL())) {
      this._element.style.paddingLeft = `${scrollbarWidth}px`
    }

    if ((isBodyOverflowing && !isModalOverflowing && !isRTL()) || (!isBodyOverflowing && isModalOverflowing && isRTL())) {
      this._element.style.paddingRight = `${scrollbarWidth}px`
    }

~~I can be wrong about this but I think this got something to do with issues about modal shifting/jumping or not being centered with/without scrollbars on the body.~~

Found it: #14933

But that's not everything.. I decided to go down the rabbit hole.

So let's begin:

isBodyOverflowing is using getBoundingClientRect() to check if the body's width is less than the window's innerWidth ("the viewport width including the size of a rendered scroll bar (if any)"). My guess, this was done to see if there is a scrollbar on the document root or not. (I was a bit confused as we compare widths but yeah - the overflow means on the y-axis.)

isModalOverflowing is checking if the element's scrollHeight ("the height of an element's content, including content not visible on the screen due to overflow.") is bigger than the document root's clientHeight ("the viewport height excluding the size of a rendered scroll bar (if any)."). My guess, this was done to see if the modal is higher than the document root and would cause a scrollbar.

The paddingLeft is set to shift the modal dialog to the right and create a spacing (the size of a scrollbar) and vice versa. My guess, this was done to compensate for a scrollbar on the opposite side.

Thoughts:

So I checked the examples in our documentation and this doesn't seem to work properly. (This is true for older Bootstrap versions too. Maybe it did at some point tho.) The result, the dialog isn't centered properly. (At least in FF and Chrome on Windows. Safari 14 and Chrome on Android is fine tho.)
This is partly caused by the fact that when we meassure scrollHeight the dialog isn't yet visible so it's 0 and isModalOverflowing is always false.

About isRTL(). If I add dir="rtl" to the html tag the body's scrollbar is still on the right side. The modal's scrollbar on the left. 🤯 Not sure how that is handled when the OS is set rtl.

In general without those extra paddings it seems to work fine for me.

GeoSot · 2021-04-07T06:39:41Z

@alpadev what a lovely approach 🛩️

129 line this._adjustDialog() is pointless for sure, cause it hidden as you told

I am not sure for the other two usages (resize 304 / handleUpdate 211)
Did you test it on resize too?

And one more cherry, that is out of this scope: lines 405|407 & 414|419 seems redundant as element overflow-y is auto

alpadev · 2021-04-07T07:15:53Z

I am not sure for the other two usages (resize 304 / handleUpdate 211)
Did you test it on resize too?

So I made a codepen for the case when the body doesn't overflow. Since isModalOverflowing is false there is no padding added so it looks good. If I resize isModalOverflowing becomes true and it's adding padding-left that makes it not centered. (Case !isBodyOverflowing && isModalOverflowing && !isRTL())

I tried it on the documentation page. There is padding-right added because the body does overflow (Case isBodyOverflowing && !isModalOverflowing && !isRTL()).

There is no case for when both are overflowing or both are not overflowing.

Also the _adjustDialog is only adding paddings but never take them away. So at some point it's adding paddings making it uncentered and they will never go away until closed.

alpadev · 2021-04-07T07:53:50Z

And one more cherry, that is out of this scope: lines 405|407 & 414|419 seems redundant as element overflow-y is auto

My guess this was added so in case when you click on a static backdrop and it would do this little animation, it's not causing any scrollbars on the backdrop if it reaches outside but not sure.

alpadev · 2021-04-07T08:21:24Z

Likely related: #31845

GeoSot · 2021-04-07T10:11:49Z

@alpadev REALLY HELPFUL
I will not touch it on this pr but nice to have it in mind too
Issue created #33580

Any other comments over the changes?

alpadev

Everything else LGTM 👍

GeoSot · 2021-04-08T12:47:09Z

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'


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)

rohit2sharma95 · 2021-04-09T10:45:21Z

My guess this was added so in case when you click on a static backdrop and it would do this little animation

That animation is transform scale, and transforming any element does not affect the layout of the real DOM element 🤔

rohit2sharma95 · 2021-04-09T11:11:31Z

js/src/util/scrollbar.js

@@ -49,7 +49,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') {


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)

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

alpadev · 2021-04-09T18:01:05Z

My guess this was added so in case when you click on a static backdrop and it would do this little animation

That animation is transform scale, and transforming any element does not affect the layout of the real DOM element 🤔

@rohit2sharma95 Check for yourself 🙂 https://jsfiddle.net/z1pb7c9w/

XhmikosR · 2021-04-11T06:42:38Z

@GeoSot BrowserStack tests broke. Please temporarily push any PRs that are not made from an upstream branch so that you check if everything is fine before we merge :)

EDIT: NVM, it seems it was just a fluke. That being said, let's make sure BrowserStack tests pass for PRs coming from your fork.

GeoSot requested review from a team as code owners March 2, 2021 22:49

GeoSot force-pushed the use-scollbarjs-on-modal branch from eef7dd0 to eac1d74 Compare March 2, 2021 22:57

rohit2sharma95 reviewed Mar 5, 2021

View reviewed changes

js/src/modal.js Show resolved Hide resolved

rohit2sharma95 added js v5 labels Mar 7, 2021

GeoSot force-pushed the use-scollbarjs-on-modal branch 2 times, most recently from d1d9489 to 02404f7 Compare March 18, 2021 08:23

GeoSot force-pushed the use-scollbarjs-on-modal branch from 6d51216 to 21dd1e4 Compare March 23, 2021 23:38

GeoSot force-pushed the use-scollbarjs-on-modal branch from 21dd1e4 to 582d484 Compare March 31, 2021 09:55

GeoSot added the refactoring label Mar 31, 2021

GeoSot force-pushed the use-scollbarjs-on-modal branch from 582d484 to 1104ebc Compare April 4, 2021 20:52

XhmikosR added this to Inbox in v5.0.0 via automation Apr 6, 2021

GeoSot force-pushed the use-scollbarjs-on-modal branch from 54f11b1 to fc47b68 Compare April 6, 2021 16:04

mdo requested a review from rohit2sharma95 April 7, 2021 05:03

XhmikosR moved this from Inbox to Review in v5.0.0 Apr 7, 2021

GeoSot force-pushed the use-scollbarjs-on-modal branch 2 times, most recently from ee866fe to 8538f88 Compare April 7, 2021 09:35

GeoSot mentioned this pull request Apr 8, 2021

Modal.js: adjustDialog misaligns the dialog box #33584

Closed

alpadev reviewed Apr 8, 2021

View reviewed changes

alpadev approved these changes Apr 8, 2021

View reviewed changes

v5.0.0 automation moved this from Review to Approved Apr 9, 2021

rohit2sharma95 approved these changes Apr 9, 2021

View reviewed changes

GeoSot force-pushed the use-scollbarjs-on-modal branch from 0302957 to c197559 Compare April 9, 2021 11:40

GeoSot mentioned this pull request Apr 9, 2021

ScrollBar.js: handle properly, undefined properties during reset #33596

Closed

GeoSot added 2 commits April 11, 2021 02:38

Decouple scrollbar functionality on Modal.js using Scrollbar.js

dfd72bd

bring back sticky-top

28e0101

GeoSot force-pushed the use-scollbarjs-on-modal branch from c197559 to 28e0101 Compare April 10, 2021 23:38

XhmikosR changed the title ~~Decouple scrollbar functionality on Modal.js using Scrollbar.js~~ Decouple Modal's scrollbar functionality Apr 11, 2021

XhmikosR merged commit 7b7f4a5 into twbs:main Apr 11, 2021

v5.0.0 automation moved this from Approved to Done Apr 11, 2021

GeoSot deleted the use-scollbarjs-on-modal branch April 11, 2021 19:08

This was referenced Apr 17, 2021

Scrollbar.js Sanitizations #33665

Closed

Scrollbar: respect the initial body overflow value #33706

Merged

GeoSot mentioned this pull request May 12, 2021

Refactor scrollbar.js to be used as a Class #33947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple Modal's scrollbar functionality #33245

Decouple Modal's scrollbar functionality #33245

GeoSot commented Mar 2, 2021 •

edited

GeoSot commented Mar 2, 2021 •

edited

alpadev commented Apr 7, 2021 •

edited

GeoSot commented Apr 7, 2021

alpadev commented Apr 7, 2021

alpadev commented Apr 7, 2021

alpadev commented Apr 7, 2021

GeoSot commented Apr 7, 2021 •

edited

alpadev left a comment •

edited

This comment was marked as resolved.

GeoSot Apr 8, 2021 •

edited

This comment was marked as resolved.

rohit2sharma95 commented Apr 9, 2021

rohit2sharma95 Apr 9, 2021 •

edited

GeoSot Apr 9, 2021 •

edited

alpadev commented Apr 9, 2021 •

edited

XhmikosR commented Apr 11, 2021 •

edited

Decouple Modal's scrollbar functionality #33245

Decouple Modal's scrollbar functionality #33245

Conversation

GeoSot commented Mar 2, 2021 • edited

GeoSot commented Mar 2, 2021 • edited

alpadev commented Apr 7, 2021 • edited

GeoSot commented Apr 7, 2021

alpadev commented Apr 7, 2021

alpadev commented Apr 7, 2021

alpadev commented Apr 7, 2021

GeoSot commented Apr 7, 2021 • edited

alpadev left a comment • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

GeoSot Apr 8, 2021 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

rohit2sharma95 commented Apr 9, 2021

rohit2sharma95 Apr 9, 2021 • edited

Choose a reason for hiding this comment

GeoSot Apr 9, 2021 • edited

Choose a reason for hiding this comment

alpadev commented Apr 9, 2021 • edited

XhmikosR commented Apr 11, 2021 • edited

GeoSot commented Mar 2, 2021 •

edited

GeoSot commented Mar 2, 2021 •

edited

alpadev commented Apr 7, 2021 •

edited

GeoSot commented Apr 7, 2021 •

edited

alpadev left a comment •

edited

GeoSot Apr 8, 2021 •

edited

rohit2sharma95 Apr 9, 2021 •

edited

GeoSot Apr 9, 2021 •

edited

alpadev commented Apr 9, 2021 •

edited

XhmikosR commented Apr 11, 2021 •

edited