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

Use Chromium's new system-ui font alias #25570

Merged
merged 1 commit into from May 15, 2017
Merged

Conversation

jhasse
Copy link
Contributor

@jhasse jhasse commented Apr 27, 2017

https://bugs.chromium.org/p/chromium/issues/detail?id=654679

Fixes #10144.

As @Tyriar pointed out, sans-serif didn't work. But Chromium 56 added system-ui (similar to -apple-system but for all operation systems) in Electron :)

@mention-bot
Copy link

@jhasse, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bpasero and @jrieken to be potential reviewers.

@bpasero
Copy link
Member

bpasero commented Apr 28, 2017

This is changing all of our fonts on all OS for anything outside the editor, @Tyriar is that really what you had in mind?

@jhasse
Copy link
Contributor Author

jhasse commented Apr 28, 2017

The actually used font should just change on non-Ubuntu Linux systems.

For example here's a screenshot from Windows 10:

unbenannt

And from the Chromium commit messages: "This patch enables the "system-ui" generic font family on all platforms." and for macOS "This patch replaces internal uses of "BlinkMacSystemFont", Blink's non-standard extension on Mac, with the standard "system-ui" generic font family."

@Tyriar
Copy link
Member

Tyriar commented May 1, 2017

@bpasero not sure what's best for Windows and macOS, but for Linux I think this is what we want - use the standard system font. on Ubuntu this should map to "Ubuntu Regular" which is what we used anyway.

@bpasero
Copy link
Member

bpasero commented May 2, 2017

@jhasse @Tyriar it seems we are trying to fix an issue on Linux but this PR impacts Windows and macOS too. If so, I suggest to make this PR fix it only on Linux. Does that make sense?

/cc @joaomoreno

@jhasse
Copy link
Contributor Author

jhasse commented May 2, 2017

This won't impact Windows and macOS, as system-ui is the new alias for BlinkMacSystemFont on macOS and translates into Segoe on Windows. Try this HTML snippet in Chrome on Windows and macOS:

<span style="font-family: -apple-system, BlinkMacSystemFont, 'Segoe WPC', 'Segoe UI', 'HelveticaNeue-Light', 'Ubuntu', 'Droid Sans', sans-serif">The quick brown fox jumps over the lazy dog</span><br/>
<span style="font-family: system-ui">The quick brown fox jumps over the lazy dog</span>

From Chromium's commit message (emphasis mine):

With this change, all platforms use the correct font. On Windows, the
browser already pass menu_font_family_name to the renderer. On Mac and
Android, the render can find the system font without needing the
browser to set.

@bpasero
Copy link
Member

bpasero commented May 2, 2017

@jhasse wouldn't it make sense to just change this

font-family: -apple-system, BlinkMacSystemFont, "Segoe WPC", "Segoe UI", "HelveticaNeue-Light", "Ubuntu", "Droid Sans", sans-serif;

to this:

font-family: system-ui, "Segoe WPC", "Segoe UI", "HelveticaNeue-Light", "Ubuntu", "Droid Sans", sans-serif;

Just in case for some reason system-ui does not work? Or is there a guarantee it will work.

Since this change impacts standalone editor, markdown and terminal, I am adding more people to give feedback on this cross-cutting change. Especially in the standalone editor I think we cannot just use the system-ui font because we also need to run on other browsers. As such I think system-ui should just replace -apple-system, BlinkMacSystemFont everywhere.

@bpasero bpasero requested review from Tyriar, alexdima and mjbvz May 2, 2017 11:53
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Imho we should just search+replace -apple-system, BlinkMacSystemFont with system-ui.

Also, why is the font not changed for the language specific overrides (e.g. .monaco-shell:lang(zh-Hans) ) here?

@bpasero bpasero added this to the May 2017 milestone May 2, 2017
@jhasse
Copy link
Contributor Author

jhasse commented May 2, 2017

Just in case for some reason system-ui does not work? Or is there a guarantee it will work.

In Chromium >= 56 it is guaranteed to work, but as you said:

Especially in the standalone editor I think we cannot just use the system-ui font because we also need to run on other browsers.

I didn't know that. It won't work in e.g. Firefox.

Imho we should just search+replace -apple-system, BlinkMacSystemFont with system-ui.

Yes, good idea. Those never worked in other browsers anyway AFAIK.

Also, why is the font not changed for the language specific overrides (e.g. .monaco-shell:lang(zh-Hans) ) here?

Those didn't include the Ubuntu font and therefore I was sceptical.

@Tyriar
Copy link
Member

Tyriar commented May 2, 2017

Imho we should just search+replace -apple-system, BlinkMacSystemFont with system-ui.

If I understand it correctly, I think system-ui is going to work on all platforms so that is the same as font-family: system-ui. If Windows is an issue then maybe we should define the Windows ones first and then fallback to system-ui?

font-family: 
  "Segoe WPC",
  "Segoe UI",
  "HelveticaNeue-Light", /*is this mac only too?*/ 
  system-ui,
  sans-serif; /*just in case*/

@jhasse
Copy link
Contributor Author

jhasse commented May 3, 2017

If Windows is an issue then maybe we should define the Windows ones first and then fallback to system-ui?

Windows isn't an issue (see #25570 (comment)), but browsers other than Chrome/Chromium/Electron are. Therefore this approach (Segoe after system-ui) works, as system-ui won't be definied in e.g. Firefox.

@Tyriar
Copy link
Member

Tyriar commented May 3, 2017

So it should probably only apply to src/vs/editor/browser/standalone/media/standalone-tokens.css for monaco and everything else should be simply system-ui? @bpasero?

@Tyriar
Copy link
Member

Tyriar commented May 9, 2017

@jhasse I think we only need the non system-ui fonts in Monaco which is src/vs/editor/browser/standalone/media/standalone-tokens.css, everything else assumes we're running under Electron.

@jhasse
Copy link
Contributor Author

jhasse commented May 10, 2017

Fixed :)

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

LGTM

@alexdima alexdima removed their assignment May 10, 2017
@bpasero bpasero removed their assignment May 14, 2017
@Tyriar
Copy link
Member

Tyriar commented May 15, 2017

Here goes 😄

Thanks @jhasse, this should be in tomorrow's Insiders build.

@Tyriar Tyriar merged commit f8bd850 into microsoft:master May 15, 2017
@manu-st
Copy link

manu-st commented May 17, 2017

Unfortunately this change breaks the VS code UI on Windows. The reason is most likely because I do not use Segoe UI as a system font, but Tahoma (Segoe UI is mapped to Tahoma in the Windows registry key via FontSubstitutes). It seems that whatever is used in the back end to render fonts does not use FontSubstitues and thus render using a default font (looks like Times New Roman or similar which makes the UI not so great).

My case is special but wouldn't it be possible to have this configurable (even after a restart of VS code)? For the time being, I'm hacking directly the file workbench.main.css to fix this rendering.

Thanks

@jhasse
Copy link
Contributor Author

jhasse commented May 17, 2017

What font do the menus (File, Edit, ...) use in your setup? Tahoma or also the Times New Roman like font?

@manu-st
Copy link

manu-st commented May 17, 2017

The menu uses Tahoma as expected. Only what is in the client area of the window is using the wrong font.

@manu-st
Copy link

manu-st commented May 17, 2017

By default here is what I get: image

if I override to use Tahoma:
image

@jhasse
Copy link
Contributor Author

jhasse commented May 17, 2017

Strange. Does adding , Segoe UI after system-ui in workbench.main.css fix it for you?

Do you see any similar bugs in Chromium? I'm not sure where they use system-ui atm, but I guess they expect it to work and this might be a good time to report the broken FontSubstitutes behaviour upstream.

Btw: What is the preferred way to change the UI font on Windows?

@manu-st
Copy link

manu-st commented May 17, 2017

@jhasse The issue is Chromium as specifying system-ui or Segoe UI does the same :-( it uses the wrong font. Maybe I should fill a bug over there instead. Still having the choice of choosing your UI font seems reasonable.

On Windows, there is no more supported way of changing the default font (in Pre windows 8, it was possible), but basically I did exactly what this article recommends: http://www.windowscentral.com/how-change-default-system-font-windows-10, ie wiping out the entries for Segoe UI and adding a FontSubstitutes for Segoe UI to map to Tahoma.

@jhasse
Copy link
Contributor Author

jhasse commented May 17, 2017

The issue is Chromium as specifying system-ui or Segoe UI does the same :-(

Hm ... why (how) did it work before this PR then?

@manu-st
Copy link

manu-st commented May 17, 2017

Not completely sure but maybe it failed loading up Segoe UI so it continued through the list whereas now, it defaults to Segoe UI no matter what. Unfortunately I cannot test the non-insider version as I'm blocked by issue #26666 which is the reason why I found this font issue with the latest insider.

On the positive side, it forced me to reconsider how I do font override and found how to ensure that VS Code/Chromium is using what I want by following the steps here: https://www.windows10forums.com/threads/change-font-type.1308/ which is more or less how you would do it in the old days of Windows.

So for now I'm happy. Thanks for the interest in my issue!

@bpasero
Copy link
Member

bpasero commented May 18, 2017

Given what @manu-silicon describes I would vote for reverting this changes as it potentially has a very bad impact for windows users.

@Tyriar @jhasse would you agree?

@jhasse
Copy link
Contributor Author

jhasse commented May 18, 2017

Wait a second. If I'm not mistaken this

it failed loading up Segoe UI so it continued through the list whereas now, it defaults to Segoe UI no matter what.

means that we can add , Tahoma after system-ui to get the old behaviour back without breaking the fonts for some Linux users again. @manu-silicon can you confirm that?

@manu-st
Copy link

manu-st commented May 18, 2017

I'll check this in about 10h.

@manu-st
Copy link

manu-st commented May 19, 2017

I have side by side 1.12 and 1.13 unchanged:
image

And again side by side with 1.13 version of workbench.main.css changed to add Tahoma right after system-ui:
image

For 1.12, I cannot tell which font it is actually using.

@jhasse
Copy link
Contributor Author

jhasse commented May 19, 2017

IMHO Tahoma looks even better then than 1.12 (it matches the menu for example). 1.12 might be 'Segoe WPC'.

I've reported the bug at Chromium's bug tracker and created a pull request for adding Tahoma: #26912

@manu-st
Copy link

manu-st commented May 19, 2017

Note that it looks good on my screenshots because my whole system is using Tahoma. It should look equally well if you use Segoe UI or some other fonts as your default.

@jhasse
Copy link
Contributor Author

jhasse commented Jun 21, 2017

@manu-silicon I've followed this guide https://www.windowscentral.com/how-change-default-system-font-windows-10 but couldn't reproduce the Times New Roman issue. system-ui still was Segoe UI, which is also not quite correct, but matches the current behavior.
Did you do any additional steps?

@manu-st
Copy link

manu-st commented Jun 21, 2017

Actually there is something else in my setting on top of not liking Segoe UI, I do not like anti-aliased fonts as they appear too blurry on low dpi display. So in the Windows advanced settings I disable font smoothing and in the registry key I have the following settings in HKCU\Control Panel\Desktop:

  • FontSmoothingType set to 2.
  • FontSmoothing set to 1.

You might need to log off to see the effects.

@jhasse
Copy link
Contributor Author

jhasse commented Jul 28, 2017

Thanks, I've changed my settings and now also have anti-aliasing disabled. Still I couldn't reproduce the serif font being used for system-ui - it still was Segoe UI. As it should be Tahoma though, let's see what the Chrome developers think.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use system-ui as font on Linux
8 participants