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

Line height rendering issue on Chromium 94+ and higher, and Firefox. #35553

Closed
3 tasks done
pricop opened this issue Dec 15, 2021 · 6 comments
Closed
3 tasks done

Line height rendering issue on Chromium 94+ and higher, and Firefox. #35553

pricop opened this issue Dec 15, 2021 · 6 comments
Labels

Comments

@pricop
Copy link

pricop commented Dec 15, 2021

Prerequisites

Describe the issue

Hey guys,

Not sure if you were aware, but starting with Chromium 94.0.4584 and higher, the Chromium got a fix related to the way they were calculating line-heights: https://chromium-review.googlesource.com/c/chromium/src/+/2824938

This change affects all Blink based browsers (Chrome, Edge, Opera, etc), but also Firefox.

Safari is NOT affected by this.

So what happens in recent versions, they're including the subpixels of line-height (when a line height has subpixels) when calculating the container's total height.

This comes with an unexpected behavior, which breaks any pixel perfect designs when using classes that have a line-height set to a ratio based unit, unless the line-height value calculated happens to be a number with no decimals.

I've created 2 simple tests that which can be seen at:

16px base font on the Browser's settings: https://codepen.io/pricop/pen/ExwWZJZ
17px base font on the Browser's settings: test: https://codepen.io/pricop/pen/bGoqgZJ

I've took screenshots of each of them, and measured what the browser has rendered, and the results are as follows:

16px results: https://i.imgur.com/swLGjh4.png
17px results: https://i.imgur.com/h3eOZZi.png

The culprit in this particular case, is the "small" helper class that Boostrap includes. This class has the font-size set to 80%. When a browser has the default font size set to 16px (which is what the vast majority of browsers have), the line-height in newer versions will not be floored, as in previous versions, so rather than the line-height being calculated at 19px, it is now calculated at 19.2px, which causes the issues presented in the results.

The fix would be have the line-height for classes that are using ratio based values (such as small, display-*, etc. classes) to a value with no decimals when the browser's calculates it.

This issue can be followed at: https://bugs.chromium.org/p/chromium/issues/detail?id=1278566 - but by the looks of it, this is something that's here to stay, and I've been instructed to adjust my CSS instead.

Reduced test cases

https://codepen.io/pricop/pen/ExwWZJZ
https://codepen.io/pricop/pen/bGoqgZJ

What operating system(s) are you seeing the problem on?

Windows

What browser(s) are you seeing the problem on?

Chrome, Firefox, Microsoft Edge, Opera

What version of Bootstrap are you using?

4.0.0 but it affects all the others.

@XhmikosR XhmikosR added the css label Dec 16, 2021
@folknor
Copy link

folknor commented Dec 22, 2021

Can you please drag and drop the screenshots you linked into a github comment? First of all I have no reason to trust imgur. Secondly, their servers are apparently too busy anyway so I can't see the images.

@pricop
Copy link
Author

pricop commented Dec 22, 2021

Chrome BS4 16px
chrome BS4 17px

@mdo
Copy link
Member

mdo commented Apr 12, 2022

I see a computed height of 69.2 when hovering over the DOM nodes in Chrome's web inspector (Chrome v100.x). Safari renders it as 69 even.

Do you still see this with the latest Chrome? Is it the computed height shown in inspector tools that's the issue, or a rendered height that's different than that shown value?

@pricop
Copy link
Author

pricop commented Apr 13, 2022

I hope you're doing well. Yes, the issue persists with the latest version of Chrome.

Safari still does line-height clamping like Chrome used to do (which was to round down the computed height), but they'll probably eventually switch to the same render method Chromium and Gecko do.

The 69.2 height value that you're seeing, is the correct computed height. However, since the browser can't render .2s of a pixel, it rounds the number when rendering some of the rows, and the remaining sum of decimals are being added (I presume arbitrary) to other rows so that the final container has the correct height.

In the example provided, the small class from Bootstrap 4 has the height set to 80%. 80% of 16 is 12.8, added with the rest of the heights, paddings, and borders, it equals to 69.2px.

By setting the $small-font-size to .875rem (for 14px) or .8125rem (13px), the height becomes a round number, which fixes this rendering issue.

Boostrap 5 already uses REM units and is not affected by this. I see no reason why this couldn't be backported to Bootstrap 4.

@mdo
Copy link
Member

mdo commented Apr 14, 2022

Want to open a new PR to change the small font-size?

pricop added a commit to pricop/bootstrap that referenced this issue Apr 14, 2022
This is a backport from BS5 to BS4.

As per twbs#35553

The ```small-font -size``` will actually increase from ```13px``` to ```14px``` with this proposed change. Personally I'd prefer keeping it the BS5 way (```14px```), as the text is more readable now.

If it's needed, I can do another PR with the ```$small-font-size:             .8125rem !default;``` to resemble BS4 more closely but I feel like this would be a move forward even for BS4.
@pricop
Copy link
Author

pricop commented Apr 14, 2022

Done.

mdo pushed a commit that referenced this issue Apr 14, 2022
This is a backport from BS5 to BS4.

As per #35553

The ```small-font -size``` will actually increase from ```13px``` to ```14px``` with this proposed change. Personally I'd prefer keeping it the BS5 way (```14px```), as the text is more readable now.

If it's needed, I can do another PR with the ```$small-font-size:             .8125rem !default;``` to resemble BS4 more closely but I feel like this would be a move forward even for BS4.
@mdo mdo closed this as completed Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants
@mdo @folknor @XhmikosR @pricop and others