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

Jump to bottom

Prevent scrollbar replacement on non-integer width #30772

Merged
merged 2 commits into from May 12, 2020
Merged

Prevent scrollbar replacement on non-integer width #30772

merged 2 commits into from May 12, 2020

Conversation

kremerd
Copy link

@kremerd kremerd commented May 10, 2020 •
edited by XhmikosR

Fixes #29681, Equals #30017, but also includes a test case.

@kremerd kremerd requested a review from a team as a code owner May 10, 2020 21:16
kremerd
kremerd commented May 10, 2020
View reviewed changes
js/tests/unit/modal.spec.js Outdated
// Enable the scrollbar measurer and reset body margin
const css = `
.modal-scrollbar-measure { position: absolute; top: -9999px; width: 50px; height: 50px; overflow: scroll; }
body { margin: 0; }
Copy link
Author

Choose a reason for hiding this comment

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

While not strictly necessary for the test to work, I believe it makes sense to include this style definition here: The values of rect.left and rect.right in the _checkScrollbar method depend on this margin, and when not set explicitly, a browser default will be used (e.g. 8px in Chrome) instead of the value actually set by Bootstrap.

@XhmikosR XhmikosR added this to Inbox in v5 via automation May 11, 2020
@XhmikosR XhmikosR added this to Inbox in v4.5.0 via automation May 11, 2020
@XhmikosR
Copy link
Member

Thanks @kremerd! I rebased this to include the original patch and authorship, so please don't overwrite it.

BTW for the v4-dev backport the test will have to be adapted since we use QUnit there. But that's after this is merged.

@XhmikosR
Copy link
Member

Tested this on BrowserStack too, seems to be fine.

@Johann-S please review ASAP so that we backport it to #30768 and include it in 4.4.2 🙂

@XhmikosR XhmikosR mentioned this pull request May 11, 2020
Closed
@kremerd kremerd closed this May 11, 2020
@kremerd kremerd deleted the master-modal-scrollbar branch May 11, 2020 20:07
@kremerd kremerd restored the master-modal-scrollbar branch May 11, 2020 20:28
@kremerd kremerd reopened this May 11, 2020
v5 automation moved this from Inbox to Approved May 12, 2020
@XhmikosR
Copy link
Member

@kremerd thanks! Now the backport to v4-dev is left. I'll try to do it so that we include the patch in the upcoming v4.4.2, but if you have time feel free to backport it to v4-dev and ping me.

@XhmikosR XhmikosR merged commit d59de33 into twbs:master May 12, 2020
v5 automation moved this from Approved to Shipped May 12, 2020
@XhmikosR XhmikosR moved this from Inbox to Needs manual backport in v4.5.0 May 12, 2020
@XhmikosR XhmikosR moved this from Needs manual backport to Cherry-picked in v4.5.0 May 12, 2020
@XhmikosR
Copy link
Member

All good, I cherry picked it in #30768, so it'll be in v4.4.2. Thanks again @kremerd!

XhmikosR pushed a commit that referenced this pull request May 12, 2020
@mendozagioo @XhmikosR
Avoid bad scrollbar replacement into width values
c4be502 
Refs #30772
XhmikosR added a commit that referenced this pull request May 12, 2020
@XhmikosR
Backport d59de33 from #30772
3871b07 
Add a test about the scrollbar issue on non-integer width
XhmikosR pushed a commit that referenced this pull request May 12, 2020
@mendozagioo @XhmikosR
Avoid bad scrollbar replacement into width values
4211255 
Refs #30772
XhmikosR added a commit that referenced this pull request May 12, 2020
@XhmikosR
Backport d59de33 from #30772
24cbce6 
Add a test about the scrollbar issue on non-integer width
XhmikosR pushed a commit that referenced this pull request May 12, 2020
@mendozagioo @XhmikosR
Avoid bad scrollbar replacement into width values
f985012 
Refs #30772
XhmikosR added a commit that referenced this pull request May 12, 2020
@XhmikosR
Backport d59de33 from #30772
cf5e6a5 
Add a test about the scrollbar issue on non-integer width
XhmikosR pushed a commit that referenced this pull request May 12, 2020
@mendozagioo @XhmikosR
Avoid bad scrollbar replacement into width values
f1827ce 
Refs #30772
XhmikosR added a commit that referenced this pull request May 12, 2020
@XhmikosR
Backport d59de33 from #30772
30e7df6 
Add a test about the scrollbar issue on non-integer width
@XhmikosR XhmikosR moved this from Cherry-picked to Shipped in v4.5.0 May 12, 2020
@nelsonni nelsonni mentioned this pull request Jan 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XhmikosR XhmikosR XhmikosR approved these changes

Assignees
No one assigned
Labels
backport-to-v4 js v4 v5
Projects
No open projects
v4.5.0
  
Shipped
v5
  
Shipped
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bad scrollbar replacement on non-integer width windows
3 participants
@kremerd @XhmikosR @mendozagioo