-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Avoid CJK legacy fonts in Windows #84048
Conversation
Some changes occurred in HTML/CSS/JS. |
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ollie27 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
This would also affect fallback for all non-CJK text and I assume |
@the8472 I tried to impl that like
but neither
|
Adding Windows CJK sans-serif fonts to font-family manually can also solve this problem. |
Ah too bad, you're right this can't work. The spec says
Depending on which unicode ranges those fonts cover they might suffer from a similar problem as switching the fallback. But with specific fonts the |
Does adding a Windows font to font-family cause a license problem? I'm not sure about this. |
That sounds implausible, as long as we are not distributing it. The software is licensed to installers like Microsoft for distribution to installees, and the browser is the one "using" the font, not us. Rather, we are merely giving an ignorable direction to use something if it is convenient: the browser could ignore this direction due to license concerns if it saw fit. But if it was a license violation, the existing use of Arial would be a license violation also, and I am not aware of anyone trying to sue what must be half of the entire English-speaking internet for mentioning Arial in their CSS "without a license". |
Could someone add T-rustdoc label? |
Can you add screenshots to show us the comparison (before/after) please? |
I guess it's fine; Just to be sure: cc @jsha |
I don't think this is the right solution. I think we should do the alternate thing @ghost proposes:
We discussed that a bit over in #84035 (comment). As @ghost pointed out, the overall Noto Sans CJK is only available in OpenType, and it's too big (20MB). So I think we should add a stanza particularly for Noto Sans KR, and ship the font. Separately, somebody should check if the default JP, SC, TC, fonts have a similar problem, and we can apply a similar fix if so. My reasoning for preferring this approach: The fallback font is displayed while the custom font is loading. For English text, this change would mean showing a sans-serif font, and then replacing it with "Source Serif 4", which would result in more of a visual shift. Shipping the nice font that we want to use for Korean is consistent with the approach we use for English text: we want to make sure the docs people have taken the time to write are displayed in a consistent, beautiful font. And also it's hard know what the default fallback fonts are on various different systems, which makes it hard to select a set of fallbacks that is good for all systems and all languages. |
Can we fallback to system sans serif on KR specifically? I believe that such would be the appropriate minimal PR. |
This latest seems like a good approach to me! One question: Why does Malgun Gothic need a unicode-range property while Yu Gothic does not? |
Even Chinese characters with the same code might have different shapes depending on the font for each language (CN, JP, ...). So I decided to apply it only to Korean.
I've tried to implementing that, but since unicode-range with |
Can you indicate the origin and license of the font file in the commit message? And if the license requires that a license notice be distributed alongside, commit that as well. |
Thanks! There's also a slightly unintuitive thing: The various static files are bundled into the rustdoc binary at compile time ( |
This looks good to me. I'll take some time for others to weigh in before giving an r+, and would like to do some testing later this weekend. Thanks for your work on it. |
There are at least two major ways to render Chinese characters for Chinese alone (often given as
Alas. That seems weird, but this is probably for the best then. |
This comment has been minimized.
This comment has been minimized.
Could someone approve the workflow? |
📌 Commit 8a2d663 has been approved by |
Rollup of 12 pull requests Successful merges: - rust-lang#84048 (Avoid CJK legacy fonts in Windows) - rust-lang#85529 (doc: clarify Mutex::try_lock, etc. errors) - rust-lang#85590 (Fix bootstrap using host exe suffix for cargo) - rust-lang#85610 (Fix pointer provenance in <[T]>::copy_within) - rust-lang#85623 (Remove stray .stderr files) - rust-lang#85645 (Demote `ControlFlow::{from|into}_try` to `pub(crate)`) - rust-lang#85647 (Revert "Move llvm submodule updates to rustbuild") - rust-lang#85666 (Document shared_from_cow functions) - rust-lang#85668 (Fix tasklist example in rustdoc book.) - rust-lang#85672 (Move stability attribute for items under the `ip` feature) - rust-lang#85699 (Update books) - rust-lang#85701 (Update cargo) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
As metioned in #84035, the default serif CJK font in Windows is meh-looking.
To avoid this, we should use sans-serif font or provide CJK glyph supported font in
rustdoc.css
.