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(frontend): use system-ui for system font #11177

Merged
merged 4 commits into from
Jul 9, 2023

Conversation

enitimeago
Copy link
Contributor

@enitimeago enitimeago commented Jul 8, 2023

What

Fix font-family value applied by system font setting.

Why

This was originally set to Hiragino Maru Gothic Pro, which is the same as the current default font.

Therefore, no visible effect would be seen.

This PR changes the font-family value for system font setting to system-ui, which has support from all modern browser versions: https://caniuse.com/font-family-system-ui

(The initial PR of this version used this font stack, however was simplified to just system-ui as we don't need to care about older browser versions per code review #11177 (comment))

Additional info (optional)

packages/misskey-js test$ pnpm jest && pnpm tsd
[8 lines collapsed]
│  api.ts       |     100 |      100 |     100 |     100 |                                                                                     
│  streaming.ts |   92.22 |    85.41 |   82.85 |   92.22 | 102-103,112-113,126-128,166-167,185-187,190-191,194-195,242-244,255,270-273,356-359 
│ --------------|---------|----------|---------|---------|-------------------------------------------------------------------------------------
│ Test Suites: 2 passed, 2 total
│ Tests:       12 passed, 12 total
│ Snapshots:   0 total
│ Time:        1.085 s
│ Ran all test suites.
...
└─ Done in 10.4s
packages/frontend test$ vitest --run
...
│  ✓ test/url-preview.test.ts  (6 tests) 176ms
│  ✓ test/home.test.ts  (2 tests) 150ms
└─ Done in 23.1s passed (4)
│       Tests  24 passed (24)
...
│ PASS test/unit/misc/id.ts (861 MB heap size)
│ PASS test/unit/MetaService.ts (14.599 s, 882 MB heap size)
│ PASS test/unit/extract-mentions.ts (888 MB heap size)
│ A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detect…
│ Test Suites: 33 passed, 33 total
│ Tests:       2 skipped, 16 todo, 919 passed, 937 total
│ Snapshots:   0 total
│ Time:        1085.225 s
│ Ran all test suites.
│ Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished?
└─ Done in 18m 10.5s

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Jul 8, 2023
@codecov
Copy link

codecov bot commented Jul 8, 2023

Codecov Report

Merging #11177 (b4ead3f) into develop (5059d4d) will increase coverage by 0.14%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop   #11177      +/-   ##
===========================================
+ Coverage    77.70%   77.85%   +0.14%     
===========================================
  Files          908      910       +2     
  Lines        91750    91909     +159     
  Branches      7670     7697      +27     
===========================================
+ Hits         71295    71555     +260     
+ Misses       20455    20354     -101     

see 20 files with indirect coverage changes

@enitimeago enitimeago force-pushed the patch-1 branch 2 times, most recently from 4ce4048 to 88709fa Compare July 8, 2023 10:46
This was originally set to Hiragino Maru Gothic Pro, which is the same as the current default font.
@enitimeago enitimeago marked this pull request as ready for review July 8, 2023 10:47
@enitimeago enitimeago changed the title fix(frontend): correct system font stack fix(frontend): use system-ui for system font Jul 9, 2023
@enitimeago enitimeago requested a review from saschanaz July 9, 2023 04:51
@saschanaz saschanaz merged commit 53b1684 into misskey-dev:develop Jul 9, 2023
18 checks passed
@saschanaz
Copy link
Member

簡単ですのでマージしました👍

@enitimeago enitimeago deleted the patch-1 branch July 9, 2023 12:26
@acid-chicken
Copy link
Member

@fireattack May I have your opinion about this changes?

@fireattack
Copy link

fireattack commented Jul 9, 2023

It looks to me this change is under the switch "Use the system's default font", so I think it's totally fine since user can choose to not use it.

Below are just comments about system-ui, don't think it's very relevant here but might be good to know.

In general using system-ui is a bad idea in certain OS, mainly Windows. This is a route GitHub, Twitter, StackOverflow, and lots of other big websites have gone through (started using system-ui -> immediately reverted the change).

Because on Chinese and Japanese Windows, it would end up using Microsoft Yahei and Yu Gothic UI fonts, respectively. The former isn't optimized to show non-Chinese content (its Japanese glyphs are especially ugly), while the latter isn't even suitable to display Japanese content: as the name suggests, it's intended to be a "UI" font - it's very narrow to accommodate long Japanese words in UI elements such as context menu, button etc. You're supposed to use "Yu Gothic" to show normal text in Japanese. This is very different from MacOS or some Linux desktop OS where the default system font is also supposed to be used to display normal text.

And because these two fonts are unicode, it won't fallback to other fonts either. English Windows does not suffer from this issue the other way precisely because its default UI font, "Segoe UI", not only is great to show English characters, it also does not contain any CJK glyphs so these would fallback to other "proper" fonts.

slofp pushed a commit to Secineralyr/misskey.dream that referenced this pull request Jul 21, 2023
* fix(frontend): correct system font stack

This was originally set to Hiragino Maru Gothic Pro, which is the same as the current default font.

* just use system-ui

per code review misskey-dev#11177 (comment)

---------

Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants