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.

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

Add option to prevent body shift on modal open #12883

Closed
wants to merge 5 commits into from
Closed

Add option to prevent body shift on modal open #12883

wants to merge 5 commits into from

Conversation

thcipriani
Copy link
Contributor

* Add `scrollpad` option to modal.js, disable by default
* Add description for `scrollpad` modal option in javascript docs
@cvrebert
Copy link
Collaborator

It was alleged by two of the commenters that this doesn't work in some browsers:

@cvrebert cvrebert added the js label Feb 28, 2014
@cvrebert
Copy link
Collaborator

Also, why even have an option for this, as opposed to just always enabling it?

@thcipriani
Copy link
Contributor Author

Chrome 33 Windows 7 works fine. Don't see why it wouldn't work in Chrome 32 unless there's some kind of bug.

The reason to disable this by default is two-fold:

  1. People may be depending on this functionality working the way it does
  2. If you have a page without scrollbars, you don't want to add padding that's correcting for nothing

@thcipriani
Copy link
Contributor Author

Also, the solution that mine is based on missed the key point of having overflow: scroll; in the css. If an element doesn't have scrollbars, then, obviously, the offsetWidth is going to match the clientWidth

@thcipriani
Copy link
Contributor Author

WRT to IE10 and this comment: #9855 (comment)

The problem seems to be a part of responsive-utilities.less, specifically the @-ms-viewport { width: device-width; } which is needed for making IE10 respond to device width vs viewport width. This rule also has the unintended(?) consequence of implicitly setting -ms-overflow-style to -ms-autohiding-scrollbar

What this means is that modals in IE10 do not need extra body padding since there is effectively never a scrollbar.

  1. You could get around this by enforcing a slightly uglier style, e.g. make explicit the -ms-overflow-style on the html (as suggested in the comment).
  2. I could add javascript to detect IE10/11 and check if there are scrollbars present on the page inside modal.js and only apply the padding if that evaluates to false
if (! (!! navigator.userAgent.match(/MSIE 1/) && 
document.documentElement.clientWidth === window.innerWidth) && options.scrollpad) {
  // IE 10/11 AND scrollbar shown or a different browser AND scrollpad wanted; apply body padding
}

-- OR --

  1. Allow users to toggle the method themselves via the scrollpad option:
$('#myModal').modal({
  scrollpad: !  (!! navigator.userAgent.match(/MSIE 1/) && 
document.documentElement.clientWidth === window.innerWidth)
})

I'm somewhat opposed to idea 2, since, while I feel this is the intended behavior for most cases, it would be impossible to alter the behavior downstream. It may be better to implement idea 2 with checking if the root element's -ms-overflow-style has been explicitly overridden:

// Is not MSIE 10/11 where there is no visible scrollbar and document root has no explicit setting for -ms-overflow-style AND options.scrollpad is true
if (! (!! navigator.userAgent.match(/MSIE 1/) && 
document.documentElement.clientWidth === window.innerWidth &&
document.documentElement.style.msOverflowStyle === '') && 
options.scrollpad) {
  // autohiding-scrollbar not is enabled, or there is a scrollbar and options.scrollpad is true; apply body padding
}

Idea 1 would likely create inexplicably awful looking pages by default in Windows Metro and on the Windows tablets, so I think that idea is a clunker.

I think that idea 3, while more complicated for the end user, would allow users for whom the background shift is an issue to have a solution with very little "magic".

I'm open to any thoughts on this.

@thcipriani
Copy link
Contributor Author

OK, made some changes.

scrollpad now attempts to detect if the page has scrollbars and, if so, add padding to the body.

I also changed this to be the default for modals, but left it as an option so it can be turned off.

This is tested as working on:
Debian 7/Iceweasel 17.0.10
Debian 7/Chrome 32.0.1700.107
Windows 7/IE 10
Window 7/Firefox latest
Windows 7/Chrome 33
Windows XP/IE8
OSX 10.9/Chrome 33
OSX 10.9/Firefox 27.0.1

@fat
Copy link
Member

fat commented Mar 17, 2014

fixed already, thanks man!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants