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

Modal window, scrollbar shift regression #15353

Closed

Conversation

thcipriani
Copy link
Contributor

Current getbootstrap.com:

Modal shift in IE11

This was a problem that was originally solved by 0907244

A regression was caused by the removal of the line:

if (document.body.clientWidth >= window.innerWidth) return 0;

from the measureScrollbar function in commit da74fba

This seems like a mistake. Further, checking if document.body.scrollHeight > document.documentElement.clientHeight (the current scrollbar detection solution) is insufficient to check for the presence of scrollbars on IE10/IE11. IE10/IE11 don't have a scrollbar width since the scrollbar is hidden a majority of the time because of the CSS @-ms-viewport { width: device-width; }

I believe the situation is similar for Chrome on OSX, but I haven't tested that.

Either way, the solution in this commit shouldn't interfere with the goals of da74fba as far as I can tell AND fixes an important regression.

@twbs-savage
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 914e68f
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/43875883

(Please note that this is a fully automated comment.)

@cvrebert
Copy link
Collaborator

Hmm, still having Sauce connection issues. Will retry again later.
All the Sauce tests that did run were successful though, so that's encouraging.

@thcipriani
Copy link
Contributor Author

@hnrch02—do you have any time to give this a quick review?

Just to make sure that I'm not missing anything critical about da74fba. There are definitely other ways to get at this issue if need be.

Thanks!

@hnrch02
Copy link
Collaborator

hnrch02 commented Dec 16, 2014

I'm unable to reproduce the problem in the first place. Which browser and OS did you use to take that GIF?

@thcipriani
Copy link
Contributor Author

@hnrch02—the gif was produced with windows7 IE11.

I was able to confirm that this doesn't happen in chrome on OSX, as I had erroneously stated earlier—this is an issue with IE10/IE11 only, I believe.

@martin-g
Copy link

I'm experiencing the same problem in my application with Chrome 39 on Ubuntu 14.04.
With Bootstrap 3.3.0 there is no such problem. The problem appeared after updating to 3.3.1.
Looks like a duplicate to: #15229 .

@Khrysller
Copy link

Happens on W7 with IE9

hnrch02 added a commit that referenced this pull request Dec 18, 2014
@liu-xinhui
Copy link

I'm experiencing the same problem in ie10/ie11 with v3.3.2, v3.3.0 is well

@thcipriani
Copy link
Contributor Author

Closing in favor of #15378

@thcipriani thcipriani closed this Feb 24, 2015
kkirsche pushed a commit to kkirsche/bootstrap that referenced this pull request Feb 24, 2015
Typo in URL

Fix copy/pasta'd id on accessibility skiplink callout

Fix a typo in contributing doc.

bump grunt-saucelabs to ~8.6.0

update shrinkwrap

[skip validator]

Use OS X 10.10 for Sauce iPhone simulator

[skip validator]

Modal: Work around IE scrollbars not taking up page width

Fixes twbs#15353.

automatic grunt dist

upgrade to QUnit v1.17.1

[skip validator]

Update footer.html

Update Creative Commons link to HTTPS.

JS unit tests: use modern QUnit assert object everywhere

JS unit tests: use QUnit.module() & QUnit.test() everywhere

[skip validator]

JS unit tests: equal() => strictEqual()

[skip validator]

Include test
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

8 participants