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: Work around IE scrollbars not taking up page width #15378

Merged
merged 1 commit into from Feb 24, 2015

Conversation

hnrch02
Copy link
Collaborator

@hnrch02 hnrch02 commented Dec 16, 2014

Fixes #15353.

/cc @thcipriani

@hnrch02 hnrch02 added the js label Dec 16, 2014
@hnrch02 hnrch02 added this to the v3.3.2 milestone Dec 16, 2014
@cvrebert
Copy link
Collaborator

Which IE versions are affected?

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Dec 16, 2014

10 and 11.

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Dec 16, 2014

IE8 also had the same problem though there the scrollbar takes up window width, hence the workaround.

@thcipriani
Copy link
Contributor

I can't seem to reproduce this in IE8.

Are you using getbootstrap.com? Or do you have a gist?

I actually think that you could drop document.body.scrollHeight > document.documentElement.clientHeight I can't think of a single situation where that check would correctly determine that there were scrollbars when the document.body.clientWidth < fullWindowWidth would disagree.

One caution I have, that I'm a bit confused about, in IE
document.documentElement.getBoundingClientRect().right - document.documentElement.getBoundingClientRect().left
DOES produce effectively the same number as window.innerWidth; however, in Chrome 39, it produces a number equvalent to document.documentElement.clientWdith—what's up with that?

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Dec 18, 2014

I can't seem to reproduce this in IE8.

Are you using getbootstrap.com? Or do you have a gist?

I was using the visual JS test for the modal though I'm now also unable to reproduce. I think I was talking about another modal related problem.

I actually think that you could drop document.body.scrollHeight > document.documentElement.clientHeight I can't think of a single situation where that check would correctly determine that there were scrollbars when the document.body.clientWidth < fullWindowWidth would disagree.

Good point.

One caution I have, that I'm a bit confused about, in IE
document.documentElement.getBoundingClientRect().right - document.documentElement.getBoundingClientRect().left
DOES produce effectively the same number as window.innerWidth; however, in Chrome 39, it produces a number equvalent to document.documentElement.clientWdith—what's up with that?

I don't know but that's why the workaround method is only used when window.innerWidth is unavailable which is only the case in IE8.

@hnrch02 hnrch02 force-pushed the fix-ie-modal-scrollbar-for-realz branch from 5196438 to 32f62bc Compare December 18, 2014 06:29
@thcipriani
Copy link
Contributor

My thought is if we can't confirm IE8 having the problem (or IE < 10) we can ditch the getBoundingClientRect() piece for now.

Then, once the regression is fixed via the merge, maybe try moving away from even looking to scrollHeight vs clientHeight.

Thoughts?

@mdo mdo modified the milestones: v3.3.2, v3.3.3 Jan 19, 2015
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Feb 24, 2015

@thcipriani

Then, once the regression is fixed via the merge, maybe try moving away from even looking to scrollHeight vs clientHeight.

Isn't that what this PR would do? We'd be looking at the width instead of the height, or am I misunderstanding something here?

@thcipriani
Copy link
Contributor

@hnrch02 yup, lgtm. We should merge a fix for this asap, been hanging out for quite a while :shipit:

Thanks!

hnrch02 added a commit that referenced this pull request Feb 24, 2015
Modal: Work around IE scrollbars not taking up page width
@hnrch02 hnrch02 merged commit 16479e9 into master Feb 24, 2015
@hnrch02 hnrch02 deleted the fix-ie-modal-scrollbar-for-realz branch February 24, 2015 02:33
@cvrebert cvrebert mentioned this pull request Feb 24, 2015
@cvrebert
Copy link
Collaborator

cvrebert commented Mar 2, 2015

Apparently this also fixed #15718.

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

4 participants