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

Broken vertical font alignment on Fedora #1209

Closed
nikwen opened this issue Dec 27, 2020 · 25 comments
Closed

Broken vertical font alignment on Fedora #1209

nikwen opened this issue Dec 27, 2020 · 25 comments

Comments

@nikwen
Copy link

nikwen commented Dec 27, 2020

Describe the bug
Text is not properly vertically centered when using Firefox on Fedora 33.

To Reproduce
Steps to reproduce the behavior:

  1. Get a working installation of Fedora 33 (VM, live USB stick, etc.)
  2. In Firefox, open https://github.com/mdo
  3. See how the font is not aligned properly as marked in the screenshot below

Expected behavior
The text marked in the screenshot below should be properly vertically centered.

Screenshots

Screenshot

Desktop (please complete the following information):

  • OS: Fedora 33
  • Browser: Firefox
  • Version: 84.0

Additional context
Fedora does not ship the Helvetica and Arial fonts used by Primer.

Firefox on Fedora uses Nimbus Sans as a replacement for Helvetica:

Screenshot

If we remove Helvetica from the font declaration, the system uses Liberation Sans, which looks okay-ish:

Screenshot

@nikwen nikwen changed the title Broken vertical text alignment on Fedora Broken vertical font alignment on Fedora Dec 27, 2020
@nikwen
Copy link
Author

nikwen commented Jan 5, 2021

@simurai
Copy link
Contributor

simurai commented Jan 7, 2021

Primer CSS currently uses this font-stack:

$body-font: -apple-system, BlinkMacSystemFont, "Segoe UI", Helvetica, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji" !default;

Based on this comment it looks like Firefox on Fedora 33 does the following:

  • -apple-system skip
  • BlinkMacSystemFont skip
  • Segoe UI skip
  • Helvetica 👉 Nimbus Sans
  • Arial 👉 Liberation Sans
  • sans-serif 👉 Bitstream Vera Sans
  • Apple Color Emoji 👉 Noto Color Emoji
  • Segoe UI Emoji 👉 Noto Color Emoji

If we remove Helvetica from the font declaration, the system uses Liberation Sans, which looks okay-ish:

Hmm.. we might could also swap the order of Helvetica and Arial so that it would pick Arial/Liberation Sans before Helvetica/Nimbus Sans. Although it's hard to say if that has other negative side effects. There might be other Linux distros that look worse when Arial comes first? 🤔

Another option could be to not try to pick the right font for Linux and just let the OS/browser decide by removing both Helvetica and Arial. It then should fall back all the way to sans-serif and I assume Firefox on Fedora 33 would use Bitstream Vera Sans?

$body-font: -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif, "Apple Color Emoji", "Segoe UI Emoji" !default;

@nikwen How would sans-serif/Bitstream Vera Sans look? Is it better than Arial/Liberation Sans?

@vdepizzol
Copy link
Contributor

@simurai I'd replace Helvetica with "Helvetica Neue", and try to be a little more specific with the available OS options before fallbacking to Arial/Liberation Sans. Reading a little bit through some of the options Linux users utilize got me to this list:

$body-font: -apple-system, BlinkMacSystemFont, "Segoe UI", "Helvetica Neue", "Inter", "Noto Sans", Cantarell, Roboto, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji" !default; 

@simurai
Copy link
Contributor

simurai commented Feb 1, 2021

I'd replace Helvetica with "Helvetica Neue"

Yeah, we could give that a try. Although I think the problem of this issue is that if Helvetica is added to the font-stack but not installed, on Fedora it uses an alternative font instead of just skip it and try to use the next font in the stack. In this case the alternative to Helvetica used is Nimbus Sans that has the alignment issues. Hard to say without testing if any of the "Helvetica Neue", "Inter", "Noto Sans", Cantarell, Roboto, Arial, fonts would have alternatives that are also buggy? Also no clue if this is only a problem on Fedora or also other Linux distros? 🤔

@arzg suggested in #1219 to already add ui-monospace. It is part of the CSS Fonts Module Level 4 Draft and already supported in Safari. If so, we could also consider adding the ui-sans-serif? And maybe also give system-ui another try while keeping an eye on this.

So the most "hands-off" font-stack that only tries to use "system" fonts might be:

$body-font:
  system-ui, // use default OS font
  ui-sans-serif,  // use default sans-serif OS font
  sans-serif; // use fallback sans-serif font

But for now I guess we'd still have to add a fallback of -apple-system for Firefox, noted in that article and also Segoe UI. So it would be (without emoji fonts):

$body-font: -apple-system, Segoe UI, system-ui, ui-sans-serif, sans-serif;

Anyways, might be worth doing:

  1. Some Browserstack testing
  2. Add the changes behind a feature flag, so it can be tested on a sub-set of users and quickly turned off again.

@lunacookies
Copy link
Contributor

lunacookies commented Feb 1, 2021

Why prioritise system-ui over ui-sans-serif? If the system is using, say, a serif font as the system font, would Primer really want to use that? If yes, then why use ui-sans-serif as a fallback? It has (and will likely always have) less support than system-ui, so it will never happen that system-ui isn’t defined but ui-sans-serif is. If it isn’t ok for Primer to use a serif font, then shouldn’t the font stack be something like ui-sans-serif, -apple-system, etc?

@simurai
Copy link
Contributor

simurai commented Feb 1, 2021

Why prioritise system-ui over ui-sans-serif? If the system is using, say, a serif font as the system font, would Primer really want to use that?

AFAIK, Primer tries to stay as close to the default system font as possible. So I would say yes.

It has (and will likely always have) less support than system-ui, so it will never happen that system-ui isn’t defined but ui-sans-serif is.

Yeah, if system-ui will have the same or better browser support, we could drop ui-sans-serif again. 👍

@nikwen
Copy link
Author

nikwen commented Feb 2, 2021

@simurai, @vdepizzol and @arzg, thank you so much for looking into this issue. ❤️

Sorry for being late. I'm trying to answer all questions now.


From @simurai:

$body-font: -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif, "Apple Color Emoji", "Segoe UI Emoji" !default;

@nikwen How would sans-serif/Bitstream Vera Sans look? Is it better than Arial/Liberation Sans?

To test this, I set the following CSS on the <body> tag using the dev tools:

font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif, "Apple Color Emoji", "Segoe UI Emoji";

Note that I removed the !default which Firefox didn't know what to do with.

In general, it looks a lot better already, but text is still not 100% centered. Have a look at buttons or the "Staff" badge, for example. The text is still 1 or 2 pixels away from being centered.

Here's a screenshot:

Screenshot


From @vdepizzol:

I'd replace Helvetica with "Helvetica Neue", and try to be a little more specific with the available OS options before fallbacking to Arial/Liberation Sans. Reading a little bit through some of the options Linux users utilize got me to this list:

$body-font: -apple-system, BlinkMacSystemFont, "Segoe UI", "Helvetica Neue", "Inter", "Noto Sans", Cantarell, Roboto, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji" !default; 

To test this, I set the following CSS on the <body> tag using the dev tools:

font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", "Helvetica Neue", "Inter", "Noto Sans", Cantarell, Roboto, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji";

Note that I removed the !default which Firefox didn't know what to do with.

Here's what that looks like:

Screenshot

It's using the Droid Sans font now.

The Highlights section is broken and buttons and badges are sadly still a bit off.


From @simurai:

$body-font: -apple-system, Segoe UI, system-ui, ui-sans-serif, sans-serif;

Another suggestion, another screenshot. :)

To test this, I set the following CSS on the <body> tag using the dev tools:

font-family: -apple-system, Segoe UI, system-ui, ui-sans-serif, sans-serif;

Here's the screenshot:

Screenshot

It's using Cantarell Regular now.

@simurai
Copy link
Contributor

simurai commented Feb 11, 2021

@nikwen Thanks for all the testing. 🙇

Hmm.. based this test, I would say

font-family: -apple-system, Segoe UI, system-ui, ui-sans-serif, sans-serif;

that uses Cantarell Regular seems the best option? It also leaves it open for the OS/browser to pick a better "system" font in a future version.

/cc @vdepizzol any thoughts?

@nikwen
Copy link
Author

nikwen commented Feb 12, 2021

@simurai Thanks for taking the time to look at this again!

Cantarell Regular was the best-looking font out of the options I tested. Still not pixel-perfect (look at the STAFF badge or the Twitter icon for example) but a lot better than what GitHub currently uses.

@github-actions
Copy link
Contributor

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

@github-actions github-actions bot added the Stale Automatically marked as stale. label Aug 11, 2021
@nikwen
Copy link
Author

nikwen commented Aug 11, 2021

The issue is not fixed. Parts of the GitHub website look quite bad in Firefox on Fedora:

image
image
image

(Mixed up issues, deleted the old response.)

@github-actions github-actions bot removed the Stale Automatically marked as stale. label Aug 11, 2021
simurai added a commit that referenced this issue Aug 12, 2021
* Improve $body-font stack

This is an attempt at fixing #1209. Further reading: 

- https://infinnie.github.io/blog/2017/systemui.html
- https://blog.jim-nielsen.com/2020/system-fonts-on-the-web/

* pause stylelint complaints in documented stack

* Create silent-geckos-do.md

Co-authored-by: simurai <simurai@github.com>
@simurai
Copy link
Contributor

simurai commented Sep 14, 2021

Update: This issue still needs fixing. We tried with #1529, but that needed another change #1573 that caused other problems...

so both PRs got reverted with #1592.

Next steps

An approach we might can try next is to put these font changes behind a feature flag and then enable it only for certain users that would like to beta test it (Linux, Windows, Japanese, Chinese etc.). Once we find a good font stack that works for most, we can ship it to production to all users.

@apollo13
Copy link

This also seems to affect the display of alerts on https://primer.style/css/components/alerts#with-icon :
Screenshot from 2021-10-22 14-59-44

@apollo13
Copy link

Actually it is also misaligned on Windows with latest Firefox. Not sure if those alignment problems are solely related to Fedora

@apollo13
Copy link

Same goes for the caret icons in the topbar of https://primer.style/design/ (top is linux, bottom is windows):
Screenshot from 2021-10-22 15-23-53

I am not sure if those are the "same" issue as this one, so please let me know if I should report those somewhere else.

@ghost
Copy link

ghost commented Apr 16, 2022

This mundane bug report has consumed me... been digging into this all afternoon. There's a fixed version of the Nimbus font available here https://github.com/scriptableorg/NimbusSans

This is GPL'd URW++ Nimbus Sans and Nimbus Sans Narrow fonts. It's a close relative of Helvetica/Arial/etc. This particular set has slightly modified vertical metrics to make it align properly under GNU/Linux.

Specifically, "Win Ascent" and "Win Descent" were decreased/increased respectively by 44 points. Additionally, "HHead Ascent" and "HHead Descent" has been assigned corresponding values from "Win..." counterparts, and "HHead Line Gap" has been set to "0".

Nimbus (presumably the broken version) is in a few of the Fedora packages, as well as the gsfonts package which is installed as a grandchild dependent of gimp (gimp<-libwmf<-gsfonts) and a few gnome packages. Those fonts end up in the standard system font directories which are then scanned recursively by fontconfig for proprietary font substitutions defined here

Firefox uses fontconfig to determine font substitutions, and ultimately, it's using a broken version of the Nimbus font that is contained within these packages.

The core problem is that fontconfig is relying on unreliable fonts where consistency can't be guaranteed (due to blindly scanning directories instead of having its own font dependencies for making substitutions), and Firefox relies on fontconfig which, by extension, can't guarantee that any standards are met. As it stands.. Firefox font rendering can break from something as simple as a user installing Gimp on their machine.

I think this issue ultimately rests with fontconfig. If that package is important enough to decide on system-wide font substitutions that other applications rely on (and is installed on nearly every Linux distribution), then it should be providing consistent substitutions to those applications.

@apollo13
Copy link

apollo13 commented Apr 18, 2022

I can confirm that github looks massively better with the fixes suggested in https://nolanlawson.com/2020/05/02/customizing-fonts-in-firefox-on-linux/ 👍

@noahtallen
Copy link

I can confirm it's not Fedora specific as well -- this is what it looks like on Arch:

image

@NightFurySL2001
Copy link

@simurai Given that FireFox has fixed system-ui in mdn/content#7751 (noted here: #1209 (comment)):

But for now I guess we'd still have to add a fallback of -apple-system for Firefox, noted in that article and also Segoe UI. So it would be (without emoji fonts):

Is there any plans to reintroduce system-ui back into the GitHub font stack? There should be no major roadblocks anymore regarding the usage of system-ui anymore since most issues becomes non-issue by now.

@simurai
Copy link
Contributor

simurai commented Sep 27, 2022

Is there any plans to reintroduce system-ui back into the GitHub font stack?

It has been a year since we tried last time, but so far it's still in the backlog. Personally, I like this suggestion. So we would use system-ui as the default, but at the same time let users override it in the settings. Something like:

Screen Shot 2022-09-27 at 15 41 47

The only caveat with that is that users that have no GitHub account and are just browsing without logging in don't have an option to change the font. So we still have to somewhat be sure that system-ui is ok in most cases.

@NightFurySL2001
Copy link

It is ok in most (modern) cases. I'm looking forward to GitHub reintroducing system-ui and overridable font settings.

@simurai
Copy link
Contributor

simurai commented Nov 1, 2022

New short term idea from @vdepizzol:

Add Noto Sans to the font stack just after Segoe UI. Apparently Noto Sans should be installed by default on most Linux distros and should improve the misalignments.

So the font stack would be:

font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', 'Noto Sans', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji';

Let us know in case anyone already wants to test above font stack. 🙇 PR -> #2300

@simurai
Copy link
Contributor

simurai commented Nov 18, 2022

Update: Noto Sans is now part of the font-stack that is used on github.com.

@nikwen Let us know if that improved the alignment issues you were seeing. 🙇

@nikwen
Copy link
Author

nikwen commented Nov 18, 2022

Thank you, @simurai! I switched from Fedora to macOS a while ago, but I'm sure someone else can test your changes.

@simurai
Copy link
Contributor

simurai commented Dec 22, 2022

Let's close this for now. Further improvements that could be done:

  • At some point switch to system-ui. Depends a bit on OS/browser adoption.
  • Let users choose their font, similar how you can set your preferred font in code editors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

7 participants