-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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 system-ui alias for UI fonts (fixes #10144) #96948
Conversation
Commit 45d93e9 applied this change in some areas, but it was reverted to fix #28619. The underlying cause of the regression was Chromium bug 733219 [1], which has now been fixed, so this change should be safe to apply now. The old font stacks have been kept with lower priorities to work around Chromium bug 724393 [2]. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=733219 [2] https://bugs.chromium.org/p/chromium/issues/detail?id=724393
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine on my Windows box.
Thanks @joaomoreno. I can test on macOS, maybe Daniel could on Linux. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding @alexdima given there is a change to |
@kdrag0n I am worried when I see reports (even though a bit dated) about not using
What can you say about the claim it will regress for users with non-us locale? |
There was also this user on Windows (#25570 (comment)) that reported bad results when we added |
The problem with #25570 was the removal of all the other fonts, caused by Chromium bug 724393 which is marked as wontfix. That's why I kept the old font stack after I'm fine with limiting this change to Linux since that's where it affects me, but it's nice to be consistent across platforms in case someone wants to change the system font on another OS. Even without a custom system font, using I suspect there are probably similar cases of this fixing incorrect fonts with other languages and possibly even on macOS, though I haven't tested that. |
Let's roll with it in insiders and gather more feedback. I am merging this in given prior approval by people in #25570 which is essentially the same change. |
Unfortunately, that issue mentioned in #25570 (or similar) still exists on non-English systems. Setup: Win7 x64 Simplified Chinese system. All the words are in proper English fonts. All the words are in MS Yahei (default Chinese font for Simp. Chinese Windows), which is not optimal for displaying English. (I use English UI to demonstrate the issue better. The same issue exists on Chinese UI too as soon as there are English letters.) In general, I don't think using system-ui on Windows is a good idea, for the reasons stated in https://infinnie.github.io/blog/2017/systemui.html. |
@kdrag0n ? |
The new font is different, but according to what I saw when testing on a Chinese Windows 10 system, the new font is more correct. The old font was different from the English UI font used in the rest of the system, which changed when I set the language to Chinese. Are you sure that this isn't the case on Windows 7? |
I believe what you described is exactly the case for both Win 10 or Win 7; if you use "system-ui", it will apply a Chinese font (MS YaHei) to all the glyphs, which is not optimized for Latin letters. Yes, it is the default font for Windows in Chinese locale, but no one likes it for displaying Latin letters and it shouldn't be used on anything but Chinese characters. Just to make it clear, this is not just my personal opinion, more or less an industry consensus. Please check the details in https://infinnie.github.io/blog/2017/systemui.html and why GitHub and Bootstrap along with batch of other major websites removed "system-ui" from their font stack. This is a Windows only issue. The root cause is it doesn't use fallback to display UI like Mac OS (twbs/bootstrap#22328 (comment)) to ensure both Latin and Chinese characters look good. So by all means use "system-ui" on Linux, just like At the very least, the Chinese font should never be used if the user is deliberately using English UI, like I showed above. This is not just about look and feel, but functionality: these Latin letters in those Chinese fonts are often wider, which cause the text to overflow all over the places. Probably less an issue for VS Code, but it has been a long-standing problem for any program that doesn't test with non-Western locales since Windows 9X. Feel free to ask some i18n experts from MS team for their insight, if above is not convincing. |
If that is the consensus, maybe another PR could be made for Windows only. Happy to review. |
I checked Bootstrap's current font stack and it looks like they brought system-ui back in twbs/bootstrap#30561. I'm still not entirely convinced about the Windows Chinese font issue because I think that apps should respect the system conventions whenever possible, even if it might not be the best option in the app developer's opinion. Inconsistent fonts (and mismatching line heights for CJK fonts, which can be distracting when reading mnemonics in menus) can be quite annoying. For example, in the blog post linked above, the system-ui font is used in the Chrome UI: It's clearly not the best font for that content, but it is consistent with the rest of the system. As far as I can tell, there's no instances of text overflow in VS Code either when using Microsoft YaHei for the UI. However, if this change is to be reverted for Windows, adding Segoe UI before system-ui would cause problems on Linux and macOS systems that happen to have Segoe UI installed — this already introduces some inconsistency for me with the old font stack, where most of the UI is using Ubuntu but some parts are using Segoe UI. This is especially an issue when it comes to the components that have fonts set by TypeScript code rather than CSS, e.g. VCS commit message editor and Markdown preview, as it's harder to use different font stacks based on the OS in those cases. I'm not sure what the best way to proceed is in this case, considering consistency with the rest of the system vs. how the font looks when rendering Latin text. @bpasero thoughts? |
I tested 4.5.x before posting, didn't notice they changed it later. Sorry! But their wording there (twbs/bootstrap#30561 (comment)) doesn't sound to me they actually tested that i18n issue brought by a dev.
Chrome must have changed that too. This is what Simp. Chinese Chrome in Simp. Chinese Windows looks like: You can see the font for Latin letters on tab is clearly not MS Yahei. I checked some UI elements and this is what they use:
I agree. If we can do so per OS there is no need for such workaround. I get your point of consistency and understand that's also important. But I think at the very least, the UI shouldn't be in a Chinese font if a Western display language is chosen in VS Code, just because the user is using a Chinese OS. |
Can we not simply change the rule from:
to
My understanding is that:
Wouldn't this solve the original intent of fixing the font stack on Linux? It would not change a lot for Windows I guess, but given there seem to be issues, maybe we shouldn't try to solve it. |
As I said in my previous comment, this can cause issues on other OSes:
What do you think about the fix in https://github.com/kdrag0n/vscode/commit/91ac1372c4c0e8bce6ff3d2ab50d635a44b5524c? I haven't tested it myself, but it's similar to what @fireattack suggested above and it appears to be a reasonable compromise while avoiding issues on other setups. Let me know if you want me to create a new PR for it. |
@kdrag0n sorry I am confused now, my suggestion was to add Yes, the fix you linked to seems going into the right direction, however this PR changed the font in many more places, some of them have no notion of OS. Hence my suggestion to fix it for all OS given the suggestion I made. |
The problem with putting system-ui after Segoe UI is that Linux or macOS systems can have Segoe UI installed. While that font is typically only found on Windows systems, it is still possible to install on other OSes and this can be useful in many cases, e.g. creating mockups. I believe that changes would need to be made to the TS code and the CSS selectors in other places to fix the issue for all possible setups. |
@kdrag0n I understand, but isn't the presence of the Ubuntu font on a non-Ubuntu distro a good indication that maybe that font should be used? |
Not really. Many Linux users (myself included) aren't using Ubuntu and have the Ubuntu font installed for one reason or another, but that doesn't mean it should be used. On Linux, the UI font is customizable and there are many people who do change it, so VS Code should respect the user's choice. It's one of the only apps on my system that ignores my preferences and chooses to use a different font. That's the reason I opened this PR in the first place: to fix the font disparity and use the correct font. It's hard to generalize OSes and use a single font stack for all of them because of the Windows CJK issue, so I think the only option is to make the other places OS-dependent as well. I'm not entirely sure whether this is the case, but one plausible out-of-the-box example is Kubuntu where the Ubuntu font would be installed because it's a Ubuntu-based distro, but the UI font would be set to Noto Sans as per KDE's defaults. |
@kdrag0n given we do not have platform specific rules in some places, why can we not do this rule instead, putting
My understanding:
So this would only cause issues if someone installed |
Yes, that would work fine for most setups, except Linux systems that have Segoe installed. That's one of the problems that I originally intended to solve with this PR as I'm seeing both inconsistent (Segoe) and non-native (Ubuntu) UI fonts with the old font stacks. If your proposed umbrella stack is used in other places, the Segoe inconsistency issue would come back and some Linux users would see inconsistent fonts again — the very issue that this was supposed to fix. Again, I don't see any way to fix this issue everywhere unless the other places are updated to add platform-specific stacks (or unify all/most of the scattered stacks into one somehow to reduce maintenance costs) because system UI fonts are an inherently platform-specific detail. |
@kdrag0n upon closer looking, it seems to me we are already inconsistent given that our platform rules for the workbench define a different font stack compared to those places where we have the combined font stack: vscode/src/vs/workbench/browser/media/style.css Lines 8 to 24 in af3d378
Maybe I can get the platform differences into the other places too. Btw on Windows we already have different fonts if your locale is |
I think you got it backwards. Having Actually, as you can see here, since corresponding system font is already in the rule ("Microsoft YaHei" for zh-Hans for example) after Same goes to Western locales: the To summarize: |
@kdrag0n @fireattack I tried my best in #99429 please take a look. |
Looks good to me, will test once it launched on Insider. |
@fireattack we have since released a new insiders with these changes |
Just tested, it looks good in both Simplified Chinese and English on my Simplified Chinese Windows. Thanks! (Just a side note: if you look carefully, you will find that some formatted strings are not displayed correctly when using zh-cn display language.
Both the anchor and the monospace parts are not displayed correctly. But this is unrelated to this patch, it has been like this for a long time.) |
Thanks for taking the time to test this 👍 |
This PR fixes #10144.
Commit 45d93e9 applied this change in some areas, but it was reverted to fix #28619. The underlying cause of the regression was Chromium bug 733219, which has now been fixed, so this change should be safe to apply now.
The old font stacks have been kept with lower priorities to work around Chromium bug 724393.