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

Fix unwanted body padding when a modal is opened #23718

Merged
merged 7 commits into from Sep 2, 2017

Conversation

valin4tor
Copy link

@valin4tor valin4tor commented Aug 28, 2017

Fixes #23681

This prevents padding being added to the body when it's not overflowing but has a margin (e.g. the default 8px) by measuring its outer width (i.e. including the margin).

@Johann-S:

if it's isn't possible to test that with Phantomjs a visual test would be a solution

The problem is that this is breaking other existing PhantomJS tests... but as stated before, it works fine when running those same tests in Chrome.

When the body does not overflow (achieved by hiding the QUnit container), it should not be given a margin.
Prevents the test from failing
@valin4tor
Copy link
Author

valin4tor commented Aug 28, 2017

Hmm, only one is failing on Travis. Guess I'll try pushing the jQuery outerWidth version and see what happens

edit: this version doesn't work in Phantom or Chrome because it still doesn't account for margins, the two below are the ones referenced in issue #23681.

@valin4tor
Copy link
Author

So this is doing what I described - 8 tests are failing in PhantomJS but the same tests are all working in Chrome. And now I'll check using the first proposed solution (using getBoundingClientRect).

Use a virtual scrollbar as this is simpler than having a real one (overflow: scroll doesn't seem to work in Phantom), and disable it for the new test.

One test has also been altered to prevent erroneous fails when other inline styles are added to the body (e.g. overflow).
@valin4tor
Copy link
Author

valin4tor commented Aug 29, 2017

I figured out the problem! - in Phantom there are no vertical scrollbars, and this was breaking the other tests because they need vertical scrollbars to be present. However, these tests were previously working because of the buggy _checkScrollbar which thought that the 8px margin was a scrollbar - so in other words, those tests have been technically broken for a while but working thanks to this bug.

To fix the tests, I've given the html element padding-right to simulate a scrollbar, and disabled this for the new test as it tests for the body element when it isn't overflowing.

Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix thanks @TechDavid 👍

@Johann-S Johann-S merged commit 10ff1c7 into twbs:v4-dev Sep 2, 2017
@mdo mdo mentioned this pull request Sep 2, 2017
@mdo mdo added this to Shipped in v4 Beta 2 Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v4 Beta 2
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

3 participants