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

Bugfix for "Open modal is shifting body content to the left" #9855 #12627

Closed
wants to merge 1 commit into from

Conversation

tlindig
Copy link
Contributor

@tlindig tlindig commented Feb 7, 2014

This solution do cover up the scrollbar of body with the scrollbar of .modal. So we have no shifting body content.

This solution removes the one property of class .modal_open and so this class seams obsolete.
May be the setter for this class should be removed in modal.js too.

A demo of this solution is here: http://jsbin.com/oHiPIJi/33

@mdo
Copy link
Member

mdo commented Feb 7, 2014

What other testing have you done for this? Does it change anything with Windows Phone, iOS, Android Chrome? Does it work in IE8-11?

@mdo mdo added the css label Feb 7, 2014
@cvrebert cvrebert added this to the v3.1.1 milestone Feb 7, 2014
@tlindig
Copy link
Contributor Author

tlindig commented Feb 7, 2014

see #9855 (comment)

@tlindig
Copy link
Contributor Author

tlindig commented Feb 7, 2014

I test it on Windows 7 with:

FireFox 27.0
Chrome 32.0.1700.107 m
IE 11 in Browser Mode 8, 9, 10, Edge (Desktop)
IE 11 in Browser Mode 8, 9, 10, Edge (windows phone)

IE has background (=body) scrolling with mouse wheel, if nothing more to scroll in the foreground.

Other new issues I could not find.

@tlindig
Copy link
Contributor Author

tlindig commented Feb 7, 2014

We could also remove the special IE class -ms-overflow-style: scrollbar;.
This is not required for the bugfix. Than every browser could handle scrollbars in it default way.

http://jsbin.com/oHiPIJi/37/

@tlindig
Copy link
Contributor Author

tlindig commented Feb 8, 2014

This patch requires also an update in documentation for fixed elements like navbar fixed.
The padding have to be set to html instead of body.

Here is an example with fixed navbar: http://jsbin.com/oHiPIJi/38/

@mdo
Copy link
Member

mdo commented Feb 8, 2014

That last comment probably negates this as a potential fix in v3.x. Can't go changing that on folks in a minor or patch release.

@tlindig
Copy link
Contributor Author

tlindig commented Feb 8, 2014

Yes I agree with you. It is nothing for a bugfix version like 3.1.x.
But if it is for a minor or only for a major version, I can not say. It dose break in one point with the current behavior. Without change own css for navbar fixed the browser scrollbar would be hidden partial from the navbar. That make the page not unusable, but it would look like a bug.

@mdo
Copy link
Member

mdo commented Feb 8, 2014

Yeah, until we get a change that doesn't affect folks' sites, we can't use this. That kind of change would affect so many sites. Can't do it.

Nice work on this though, we'll definitely check it out again for v4.

@mdo mdo closed this Feb 8, 2014
@mdo mdo removed this from the v3.1.1 milestone Feb 8, 2014
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.

3 participants