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

Remove emoji fonts from font-family #1204

Closed
wants to merge 3 commits into from

Conversation

koddsson
Copy link
Contributor

This PR does a few things.

  • Removes Segoe UI Symbol from the marketing font-family. It was already removed from the default font-family in Remove "Segoe UI Symbol" from font stack #906
  • Remove the Apple and Windows Emoji fonts from both font families. The assumption is that most if not all modern systems can render emojis without these denoted in the font family.
  • Remove Segoe UI from all font families. This font is causing emoji rendering issues on Windows 10 on github.com and as far as I can tell can be safely removed.

/cc @primer/ds-core

@vercel
Copy link

vercel bot commented Dec 14, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-css/gmhfvsv2a
✅ Preview: https://primer-css-git-remove-emoji-fonts-from-body-font.primer.now.sh

@simurai
Copy link
Contributor

simurai commented Dec 14, 2020

Remove Segoe UI from all font families. This font is causing emoji rendering issues on Windows 10 on github.com and as far as I can tell can be safely removed.

Since the font stack would end up being -apple-system, BlinkMacSystemFont, Helvetica, Arial, sans-serif... wouldn't that mean that on Windows, the font would be Arial rather than the default system font which I think is "Segoe UI"? Was gonna say we could try with system-ui instead, but this article mentions there were issues with Simplified Chinese.

@simurai simurai changed the base branch from master to mkt/color-modes-whee December 14, 2020 12:24
@simurai simurai changed the base branch from mkt/color-modes-whee to master December 14, 2020 12:25
@koddsson
Copy link
Contributor Author

wouldn't that mean that on Windows, the font would be Arial rather than the default system font which I think is "Segoe UI"?

I didn't realize that Windows doesn't have Helvetica 🤔

Here's how the changes look (this includes the internal g-emoji change). I loaded up https://github.com/ikatyang/emoji-cheat-sheet running Windows 10/Edge 87 through Browserstack to get these screenshots.

Before this change After this change
Screenshot 2020-12-14 at 14 03 26 Screenshot 2020-12-14 at 14 03 19
image image

I'm not sure what we can do here if we want to keep the "Segoe UI" font since the keycap emojis are broken on it

@simurai
Copy link
Contributor

simurai commented Dec 14, 2020

I'm not sure what we can do here if we want to keep the "Segoe UI" font since the keycap emojis are broken on it

Hmm.. me neither. 🤔

If having Segoe UI in the font stack causes the problem with the keycap emoji, I assume lots of other apps/sites have the same problem. I can test it tomorrow on my old Windows laptop and see if I can reproduce it.

@koddsson
Copy link
Contributor Author

If having Segoe UI in the font stack causes the problem with the keycap emoji, I assume lots of other apps/sites have the same problem. I can test it tomorrow on my old Windows laptop and see if I can reproduce it.

Yes! I'd love a second pair of eyes on this in case I'm making assumptions that don't hold up to scrutiny. I didn't find any good documentation around this. I did quickly reproduce this on codepen so I'm wondering how other websites handle this.

image

My guess is that we use emojis much more than other websites and include them in our UI to a much greater extent than others. Or that they just don't use Segoe UI at all 🤷🏻

Excited to see if you find something out.

@simurai
Copy link
Contributor

simurai commented Dec 15, 2020

Ok, I can reproduce this in Chrome on Windows 10 too:

image

Removing Segoe UI does fix it, well, except that now all text is shown in Arial.

Interestingly it works fine in Firefox. In the screenshot below I reduced the font stack to just Segoe UI, Arial, sans-serif. It appears that Firefox rightly recognizes the emoji so it skips past Segoe UI and Arial and uses Segoe UI Emoji.

Screen Shot 2020-12-15 at 22 25 44

But I guess Chrome somehow thinks to render the #️⃣ emoji it's fine to use Segoe UI. You can force Chrome to use Segoe UI Emoji by adding it in front of Segoe UI. But that has some other side effects where it messes with bold text.

Screen Shot 2020-12-15 at 22 46 52

Hmm.. so one last idea.. in https://github.com/github/github/pull/165445/ maybe we can only remove Segoe UI from the font stack.

- font-family: "Apple Color Emoji", "Segoe UI", "Segoe UI Emoji", "Segoe UI Symbol";
+ font-family: "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol";

Then emoji should work at least when they get wrapped in a <g-emoji>.

@koddsson
Copy link
Contributor Author

Hmm.. so one last idea.. in github/github#165445 maybe we can only remove Segoe UI from the font stack.

This was it! I'll continue with github/github#165445 and close this out. Thanks for the help here ❤️

@koddsson koddsson closed this Dec 16, 2020
@koddsson koddsson deleted the remove-emoji-fonts-from-body-font branch December 16, 2020 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants