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

Fix CJK fonts again and misc. font issues #14575

Merged
merged 20 commits into from
Mar 18, 2021
Merged

Conversation

CL-Jeremy
Copy link
Contributor

Could be considered WIP. Testing by fellow CJK users are highly appreciated. Summary of changes:

  • Removes BlinkMacSystemFont (Update font stack to bootstrap's latest #13834 did not address this, while the original Bootstrap commit includes this change)
  • Dynamically replaces system-ui for Windows (known to have caused problem as detailed in Make our font stack consistent microsoft/vscode#99428 - I didn't encounter this when working on Avoid unnecessary system-ui expansion #12522 before, as my local system wasn't set to an East Asian locale - now it's obvious to me that the 'system-ui-font-family' flag of postcss-preset-env is aimed at solving this exact issue for Windows) - please do comment below if anyone finds a more elegant solution than this
  • Fixes overlooked renaming of Language contextual variable in header template to Lang (has already been done for footer)
  • Fixes regression of Yu Gothic Bold in @font-face which should remain being of weight 700 to avoid faux-boldening in cases it does get set to bold (the real local boldface should still gets used when the whole stack is set to 500 after this change)

Now the critical change:

  • Overrides the whole stack for CJK locales. Initially intended to introduce --fonts-fallback à la #12522/#issuecomment-681057176. However cross-browser testing has shown problems with nested partial overriding above 2 levels (that is, overriding a component variable in a higher cascade that is nested >=2 levels deeper in the lower cascade). So far only Safari 14 manages to excel in this test, showing all fonts correctly even in the language chooser popup (but may also be the browser ignoring all overrides, since macOS would natively choose the correct fonts with -apple-system). Ignoring the fact that nested locale tag (i. e. <html lang="en"><a lang="zh-CN"></a></html>) may become uneffective (which is puzzling, since DevTools have always shown the correct cascading consistently between browsers), this could be a great leap forward towards solving the various regressions caused by the initial refactoring into CSS variables.
  • Note that sans-serif gets pushed to last place again. The problem mentioned in Css specifies emoji fonts for body text #13784 should have been taken care by the introduction of Liberation fonts (unless one insists on including the ancient DejaVu Sans, which has already been proved to cause problems with emojis and more). GNU FreeSans could be added if someone finds that really necessary.

@CL-Jeremy
Copy link
Contributor Author

CL-Jeremy commented Feb 5, 2021

Recapping the problem of system-ui in Windows: this would basically emulate real system UI fonts for pixel-grade replication of UI elements in browsers. The use case is quite narrow and does not conform to the industry consensus in (more fluid) web pages (as mentioned in the discussion in the VS Code issue cited above). Some significant problems are as follows:

  • Some localized system fonts have Western glyphs of very different dimensions (e. g. for Japanese) potentially causing significant hardship while arranging UI elements
  • Almost all localized fonts lack weights beyond regular, bold (and sometimes light)
  • In case of Japanese, UI font is condensed to cram enough words into native UI elements to fit lower resolutions, the exact opposite of web design of today

Some might find it less of a necessity to use localized fonts in the first place. Unfortunately, the current encoding of CJK characters means localized glyph forms could never be displayed by one monolithic font - doing so means at least users from one region would be unhappy with the rendering of text (or find words illegible at worst). Thus there is significant interest in maintaining correct and good rendering of localized texts on as many platforms as possible.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 5, 2021
@CL-Jeremy
Copy link
Contributor Author

FYI: https://piramisancchi.live-on.net:9175/ has my previous deployment before the conversion to CSS variables. Except for the system-ui problem on East Asian Windows systems (which most of you would never observe anyway), this should be the goal in terms of CJK rendering.

@CL-Jeremy
Copy link
Contributor Author

Oh, I just noticed Segoe UI Symbols got added to match Bootstrap. I find that unnecessary and would now suggest removing it again, if you don't mind.

@6543 6543 added the topic/ui Change the appearance of the Gitea UI label Feb 5, 2021
@silverwind
Copy link
Member

Can this Windows issue be solved without JS? I'd prefer it to be CSS only and I think there must be some way to get the right fonts everywhere just be ordering alone, no?

@CL-Jeremy
Copy link
Contributor Author

As stated above, the purpose of system-ui is to emulate native UI font choice on every platform. The problem with that under Windows is twofold: first, on older platforms, Windows's native font choice looks awful, including even bitmap fonts, thus unusable for our purpose (whereas for real UI mimicking, this behavior may be advantageous); second, browsers do not respect lang under Windows once system-ui is used. That's frustrating, but understandable.

Many have thought of banning system-ui entirely for our type of use cases. I am not a fan of that idea since it's quite useful under Linux, where the DE, as well as the end user, have the ability to control the look of system UI much more thoroughly than all other platforms. Bootstrap, in particular, removed it from the stack and later added it back. As with sites with more focus on commercial platforms, removing it isn't really a issue (as support of system-ui came quite late), while FOSS-related sites would rather care less about the quirks on Windows should that mean to sacrifice experience in a FOSS environment.

The design choice of adding that JS snippet came with several considerations in mind. First, without JS, the page would still render with system-ui to cater the needs on most platforms. Its location in document ensures that the transition time while loading is kept minimal. The code itself is rather concise and should not bother too much. It's unlikely that that code would ever break on whatever browser under whatever version of Windows.

To sum up, without JS, there are 3 options (all of which leave Segoe UI as first replacement after -apple-system):

  1. ditch system-ui, expand that into Cantarell, Ubuntu, Oxygen-Sans; those with multiple DE's installed would suffer from this effect (e. g. lab PCs from my university)
  2. ditch system-ui and prioritize Noto Sans for FOSS platforms (Android phones using Roboto typically don't have Noto Sans installed anyway), meaning those with Noto Sans installed would see that all the time
  3. retain system-ui and leave that at the beginning of FOSS part of the font stack (i.e. splitting the emoji stack and put the FOSS part after system-ui), so only platforms without any commercial fonts specified in the stack would render with their system font, and remove Helvetica/Arial. This means that while all Mac/Windows users, mobile users and most desktop Linux users would be fine, those who are on Linux and specifically have those commercial fonts installed (which are very, very few to my knowledge) would start to see Western glyphs in CJK font (the exact opposite of current situation on Windows). I personally have had similar experiences with website font havoc with some other basic font on Linux, and I suppose the affected user group would have more expertise in solving these problems than those trying to deal with the current problem on Windows.

@CL-Jeremy
Copy link
Contributor Author

Tried so hard but still cannot get the styling with variables done right. So frustrating.

Ignoring the problem with system-ui for now, the main problem remains that currently, when variables are used, inheritance is broken. When <html> has a different <lang> than a <span> the variable gets evaluated before the value of the variable gets overridden further down the hierarchy. Changing the definition scope of --fonts-regular to :root * was the best I could do while investigating this, but I simply could not achieve what I've been able to do previously with mixins.

I sincerely call for help with understanding how variables should be used for it to behave like before, that lang="locale1" attribute of a child node make the effect defined by :root :lang(locale1) selector to override that of a parent element with lang="locale2".

@CL-Jeremy
Copy link
Contributor Author

CL-Jeremy commented Feb 9, 2021

Please kindly refer to the deployed example here for further changes to the font stacks (not all are intended to be merged, of course). Changes are covered in CL-Jeremy#4

@CL-Jeremy
Copy link
Contributor Author

I've pushed some more tweaks to my repo/private PR regarding design language, specifically the use of Medium weight.

As noticed already, Medium (500) is a fairly niche weight that is absent in many fonts. It's specifically wanted for body-like headings where simply using regular would be functionally inadequate. GitHub currently uses this only for highlighted buttons. I'm extending this to cover distinctions of navbar radio buttons, normal buttons and basic buttons, for selection/highlighting purposes. Previously, frame-only basic buttons would likely be thought as unclickable coming from a more traditional UI, like that of Windows XP/Aero. This would make the distinction between button status more perceptible.

In browsers, weight 500 would first resolve to a thinner weight, making its usage less of an impact on systems lacking this weight, or when fallen back to such a font (note that I added some specific handling for Japanese under Mac).

Everything from CL-Jeremy#4 serves as a sort of mock-ups to improve this PR based on master. I'm just working on the older branch since variables for fallback fonts don't work (and to better test it on my deployment). I tend to want to go back to Less mixins just for this part (emojis and proportional fonts, as well as monospaced fonts could remain as variables). As with system-ui, considering my inability to create a better solution, I insist on including that one-liner for Windows. I believe messing around with variables too much would likely to impact browsers more than this simple fix.

@codecov-io
Copy link

codecov-io commented Feb 14, 2021

Codecov Report

Merging #14575 (49664ba) into master (487f2ee) will decrease coverage by 0.09%.
The diff coverage is 46.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14575      +/-   ##
==========================================
- Coverage   42.21%   42.11%   -0.10%     
==========================================
  Files         767      771       +4     
  Lines       81624    82079     +455     
==========================================
+ Hits        34458    34571     +113     
- Misses      41531    41885     +354     
+ Partials     5635     5623      -12     
Impacted Files Coverage Δ
models/migrations/migrations.go 2.59% <ø> (ø)
models/migrations/v172.go 0.00% <0.00%> (ø)
models/session.go 0.00% <0.00%> (ø)
modules/session/virtual.go 60.20% <0.00%> (-1.26%) ⬇️
modules/structs/issue.go 0.00% <ø> (ø)
routers/user/auth.go 12.24% <0.00%> (ø)
modules/session/db.go 2.63% <2.63%> (ø)
modules/queue/unique_queue_disk_channel.go 53.73% <50.00%> (-0.12%) ⬇️
models/models.go 57.42% <100.00%> (+0.21%) ⬆️
modules/forms/user_form.go 40.24% <100.00%> (+3.06%) ⬆️
... and 62 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebddee8...49664ba. Read the comment docs.

@CL-Jeremy
Copy link
Contributor Author

Made my mind to pull the trigger here. Might not need to make all the @font-face's now, but still willing to do it that way since it's more robust.

Anyhow, this magically fixed my Linux Firefox issue here: #12966 (comment). It's now using system-ui just fine (probably I defined some bind rule in Fontconfig years ago, or probably different implementation with -apple-system? It might take some digging in the source to find out the root cause).

@CL-Jeremy
Copy link
Contributor Author

Bildschirmfoto 2021-02-15 um 01 30 45
For the first time since probably a year or so, Firefox is displaying CJK exactly like Chromium on this Stock Fedora VM. 😅

@CL-Jeremy
Copy link
Contributor Author

CL-Jeremy commented Feb 15, 2021

Okay, the screenshot shown above was actually of bug with Firefox, which for some reason displays symbols from U+300x (specifically, ideographic comma and period) for newline characters using the emoji font when locale is set to zh-CN, zh-TW or ja. Fixed that and fiddled with Unicode ranges a bit more.

The current solution introduces an artifact with the octicon of the language chooser (possibly somewhere else as well, but the globe icon is the most apparent one). This change tries to adress that by setting the vertical alignment to middle. The fix looks best with Safari. Mac Firefox also does well. Chromium looks better without the fix. Maybe consider placing the icon differently instead of floating?

Edit: Found the culprit and provided a solution for the artifact. Now looking perfectly aligned everywhere. Still investigating the Firefox bug, though. Seems to affect Linux as well.

@CL-Jeremy
Copy link
Contributor Author

The last commit deals with the size difference between Safari and other platforms, as visualized in the comment section of #12623.

Screenshot gallery at: https://try.gitea.io/CL-Jeremy/Test/issues/2

As always, please advise if there is a better approach than this.

@CL-Jeremy CL-Jeremy changed the title Fix CJK fonts again as per #12522 Fix CJK fonts again and misc. font issues Feb 16, 2021
@CL-Jeremy CL-Jeremy mentioned this pull request Feb 20, 2021
@zeripath
Copy link
Contributor

zeripath commented Mar 5, 2021

Heya @CL-Jeremy when you think this is ready it probably would be useful to put up some screenshots showing how and what it fixes. Thanks though for spending so much time on this.

@CL-Jeremy
Copy link
Contributor Author

Heya @CL-Jeremy when you think this is ready it probably would be useful to put up some screenshots showing how and what it fixes. Thanks though for spending so much time on this.

Thanks for understanding, I'm afraid this would get even larger and less reliable still. That's why I always would have liked someone to try out/review soundness of the code before reviewing for merging.

Currently this code is working best with Chromium/Blink across all platforms. No alignment issues whatsoever. But targeting Chromium alone would have removed a ton of the workarounds here.

Safari is working fine, or rather, macOS and iOS, being my most used platforms, have been specifically fixed with pretty high confidence (I even use 10.10), but that's also because they are least variable (shipped fonts are always preferred).

Windows is fine but has some variability. It's also the reason to come up with this cumbersome @font-face stack and unicode-range. It's working, but not as good as with Chromium on Linux or anything on Apple platforms, mostly regarding alignment. Take a close look with different browsers on these elements: octicons on menu items (specifically repo buttons), the down arrow on drop-down menus, and places involving spacing with whitespace characters.

To recap, the PR tries to fix a regression, aiming to display localized variants of CJK glyphs when the OS is unable to do so, a feature that worked since the introduction of local font stacks, and hopefully generalizing enough to become customizable thanks to CSS variables. The goals of this PR in logical order is:

  1. fix the mistake of lang attribute not applied to <html>, a fairly recent regression, which fundamentally cripples the entire multilingual font display strategy with or without a font stack, since the browser would always use system locale, cf. try.gitea.io
  2. try to tackle the problem with system-ui on Windows (localized UI font idiosyncrasies not respecting lang, making system-ui unusable at any position in the font stack) after latest font stack reworks following the introduction of CSS variables, cf. try.gitea.io when lang attribute of <html> manually set in DevTools
  3. fix the problem by replacing system-ui with Segoe UI just for Windows using JavaScript, a less appealing solution, during which the native font stacks have been rethought, including where emoji fonts should go
  4. fix the problem by means of partial overriding via @font-face and unicode-range, leading to discovery of alignment calculation issue with all browsers other than Blink, as well as certain Firefox-specific bugs, during which the emoji substack was remade with @font-face, only to be reverted and finally placed to the initial locations back in the old days with Lato
  5. (now) various attempts to deal with the alignment calculation issue mentioned in 4: macOS and iOS were fixed by introducing whitespace (space and nbsp) into the CJK stacks in Helvetica Neue, which is the UI font of 10.10 and remains metric-compatible with San Francisco; Windows is not really affected since English is also somehow affected in the same way on my test platforms, and the same fix with Segoe UI is not effective at all; Linux is affected but not fixed at all, so Firefox and WebKit (WebKitGTK/Epiphany) have problems with fonts like Noto Sans CJK if system-ui isn't Noto Sans - a replacement like Helvetica Neue is impossible, and there is unfortunately no way to refer to system-ui using family matching in @font-face

Therefore it is highly probable that this fix will not get merged as it is right now, and remains as a reference for more elegant solutions. The best I could think of right now is still using JavaScript to eliminate system-ui for Windows, or else somebody could try and submit bug reports to browsers (which, given the problem with install base, would fruit only after multiple years).

As with screenshots, you are all heartily welcome to try my own deployment. I believe it's sufficient to go through existing pages without registration, and see how things differ on different platforms with different languages (NB: Windows depends on system UI language settings).

@zeripath
Copy link
Contributor

zeripath commented Mar 6, 2021

The problem is hat most of us have no knowledge to judge what is broken and what it is you've fixed, so without labelled screenshots we're at a loss to know if there are actually even changes!

@CL-Jeremy
Copy link
Contributor Author

Okay, so the current situation is:
Snipaste_2021-03-06_18-37-44
which is -apple-system showing San Francisco falling back to a Chinese font. Adding back lang yields:
Snipaste_2021-03-06_18-37-11
which is using purely Japanese, making all Western glyphs much wider than expected. This this has been up in master for some time, but is obviously not acceptable. The issue affects all platforms, or could simply be taken as wrong by design (leading to my approach with unicode-range).

This is stock behavior with Japanese system locale.
Snipaste_2021-03-06_18-40-51
Maybe nice to have, but not matching the behavior on other browsers, being thinner and shorter in height (most probably by matching lowercase height internally):
Bildschirmfoto 2021-03-06 um 19 02 26

Now emulate font stack by explicitly calling for .SF NS (note: only working on 10.15+, here just for demonstration purpose):
Snipaste_2021-03-06_18-43-22
The same approach done with @font-face (suggested in this PR):
Snipaste_2021-03-06_18-45-32

Bonus: Emulating stock with hidden font face again, note the difference in weight:
Snipaste_2021-03-06_18-47-51
whereas emulating with Chinese system font makes no difference compared to stock -apple-system from the very beginning (ignore San Francisco, for irrelevent):
Snipaste_2021-03-06_18-49-45
both would not make any difference on 3rd party browsers.

Apart from this: the Hong Kong variant of Apple's stock font has been available since iOS 9/El Capitan, but not enabled until iOS 12/Mojave (cf. https://zhuanlan.zhihu.com/p/29856016). The custom font stack still caters this need, but needs the [lang] selector fix as implemented in this PR. This was not an issue when the stack was first implemented in 1.9.x.

@CL-Jeremy
Copy link
Contributor Author

Regarding system-ui on Windows: not only does this break when displaying another language (here: Simp. Chinese on Japanese system locale, note the glyphs fallen back to sans-serif):
2021-03-06 20_03_51-Greenshot
スクリーンショット (1411)

But also when displaying language of the same locale:
スクリーンショット (1410)

All screenshots taken from unmodified, stock versions of try.gitea.io, with system-ui at 1st place in the font stack.

The issue mentioned in CL-Jeremy@7c1c2c3 is also a hardly noticeable example of this problem. The commit itself only solves the part with Western glyphs and is thus not really useful unless some approach like unicode-range from this PR is implemented.

@CL-Jeremy
Copy link
Contributor Author

Examples of how things should look like on stock Windows with properly implemented font stacks, which may be improved (or worsened due to lack of hinting/ClearType support under circumstances) with the installation of some other fonts, such as the open-source Noto Sans CJK.

ja-JP:
スクリーンショット (1412)

zh-CN:
スクリーンショット (1413)

zh-TW:
スクリーンショット (1414)

@zeripath
Copy link
Contributor

zeripath commented Mar 6, 2021

OK, so do you feel that you now have a working and decent set of results? If so, although I dislike the safari javascript hack I think the structure of your changes look OK and I would be happy to approve

@CL-Jeremy
Copy link
Contributor Author

OK, so do you feel that you now have a working and decent set of results? If so, although I dislike the safari javascript hack I think the structure of your changes look OK and I would be happy to approve

It's now looking OK.

Before:
Bildschirmfreigabe Bild 7  März 2021 um 05 41 07 MEZ
Bildschirmfreigabe Bild 7  März 2021 um 05 43 04 MEZ

After:
Bildschirmfreigabe Bild 7  März 2021 um 05 40 57 MEZ
Bildschirmfreigabe Bild 7  März 2021 um 05 42 53 MEZ

@CL-Jeremy CL-Jeremy force-pushed the fontstack-var branch 2 times, most recently from ef5fc66 to d88b0fb Compare March 7, 2021 07:13
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 7, 2021
@lafriks lafriks added this to the 1.14.0 milestone Mar 7, 2021
@CL-Jeremy
Copy link
Contributor Author

@zeripath I'm done. The last commit is due to the practice of aliasing font families with replacements in Fontconfig. This "aliasing" has nothing to do with render artifacts or so. It's just I couldn't come up with something concise and unambiguous as commit message, so I'm explaining here: some Linux distributions define fallbacks to a Western font (here: Noto Sans) much similar to @font-face on web. Using Noto Sans would result in rendering with Droid Sans Japanese for CJK unconditionally on any browser other than Blink, which was also a reason to use unicode-range at first place.

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

doesn't break my test instance :)

and it looks plausible to work on Mac now ...

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 18, 2021
@6543 6543 merged commit 0e5c6c4 into go-gitea:master Mar 18, 2021
@6543 6543 added the type/bug label Mar 18, 2021
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants