From 8982964d832e47ff846d8e0cb06d3dc97a2332bf Mon Sep 17 00:00:00 2001 From: Robin Townsend Date: Mon, 5 Dec 2022 13:21:10 -0500 Subject: [PATCH 1/4] Don't prefer STIXGeneral over the default font STIXGeneral contains some glyphs for non-LGC scripts, but often doesn't implement these scripts fully. We should always try the browser's default fonts, which are likely to look nicer and have broader script support, before falling back to STIXGeneral. --- res/themes/legacy-light/css/_legacy-light.pcss | 4 ++-- res/themes/light/css/_light.pcss | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/res/themes/legacy-light/css/_legacy-light.pcss b/res/themes/legacy-light/css/_legacy-light.pcss index 6972d5a20bb..55865c9af6d 100644 --- a/res/themes/legacy-light/css/_legacy-light.pcss +++ b/res/themes/legacy-light/css/_legacy-light.pcss @@ -10,9 +10,9 @@ /* Noto Color Emoji contains digits, in fixed-width, therefore causing digits in flowed text to stand out. TODO: Consider putting all emoji fonts to the end rather than the front. */ -$font-family: 'Nunito', 'Twemoji', 'Apple Color Emoji', 'Segoe UI Emoji', 'STIXGeneral', 'Arial', 'Helvetica', sans-serif, 'Noto Color Emoji'; +$font-family: 'Nunito', 'Twemoji', 'Apple Color Emoji', 'Segoe UI Emoji', 'Arial', 'Helvetica', sans-serif, 'STIXGeneral', 'Noto Color Emoji'; -$monospace-font-family: 'Inconsolata', 'Twemoji', 'Apple Color Emoji', 'Segoe UI Emoji', 'STIXGeneral', 'Courier', monospace, 'Noto Color Emoji'; +$monospace-font-family: 'Inconsolata', 'Twemoji', 'Apple Color Emoji', 'Segoe UI Emoji', 'Courier', monospace, 'STIXGeneral', 'Noto Color Emoji'; /* unified palette */ /* try to use these colors when possible */ diff --git a/res/themes/light/css/_light.pcss b/res/themes/light/css/_light.pcss index 708b943c7c2..b032420b0e3 100644 --- a/res/themes/light/css/_light.pcss +++ b/res/themes/light/css/_light.pcss @@ -14,19 +14,19 @@ $font-family: 'Inter', 'Twemoji', 'Apple Color Emoji', 'Segoe UI Emoji', - 'STIXGeneral', 'Arial', 'Helvetica', sans-serif, + 'STIXGeneral', 'Noto Color Emoji'; $monospace-font-family: 'Inconsolata', 'Twemoji', 'Apple Color Emoji', 'Segoe UI Emoji', - 'STIXGeneral', 'Courier', monospace, + 'STIXGeneral', 'Noto Color Emoji'; /* Colors from Figma Compound https://www.figma.com/file/X4XTH9iS2KGJ2wFKDqkyed/Compound?node-id=559%3A120 */ From 9cfede675f285d96fc2e2045d590f7d6578c61a7 Mon Sep 17 00:00:00 2001 From: Robin Townsend Date: Tue, 3 Jan 2023 22:03:20 -0500 Subject: [PATCH 2/4] Explain why STIXGeneral shouldn't have precedence --- res/themes/legacy-light/css/_legacy-light.pcss | 5 ++++- res/themes/light/css/_light.pcss | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/res/themes/legacy-light/css/_legacy-light.pcss b/res/themes/legacy-light/css/_legacy-light.pcss index 74e7e3bebc6..c0d8ba9d11d 100644 --- a/res/themes/legacy-light/css/_legacy-light.pcss +++ b/res/themes/legacy-light/css/_legacy-light.pcss @@ -4,7 +4,10 @@ Arial empirically gets it right, hence prioritising Arial here. We also include STIXGeneral explicitly to support a wider range of combining diacritics (Chrome fails without it, as per - https://bugs.chromium.org/p/chromium/issues/detail?id=1328898) */ + https://bugs.chromium.org/p/chromium/issues/detail?id=1328898). + We should never actively *prefer* STIXGeneral over the default font though, + since it looks pretty rough and implements some non-LGC scripts only + partially, making, for example, Japanese text look patchy and sad. */ /* We fall through to Twemoji for emoji rather than falling through to native Emoji fonts (if any) to ensure cross-browser consistency */ /* Noto Color Emoji contains digits, in fixed-width, therefore causing diff --git a/res/themes/light/css/_light.pcss b/res/themes/light/css/_light.pcss index 598139847d7..6f9e90fb8c5 100644 --- a/res/themes/light/css/_light.pcss +++ b/res/themes/light/css/_light.pcss @@ -4,7 +4,10 @@ Arial empirically gets it right, hence prioritising Arial here. We also include STIXGeneral explicitly to support a wider range of combining diacritics (Chrome fails without it, as per - https://bugs.chromium.org/p/chromium/issues/detail?id=1328898) */ + https://bugs.chromium.org/p/chromium/issues/detail?id=1328898). + We should never actively *prefer* STIXGeneral over the default font though, + since it looks pretty rough and implements some non-LGC scripts only + partially, making, for example, Japanese text look patchy and sad. */ /* We fall through to Twemoji for emoji rather than falling through to native Emoji fonts (if any) to ensure cross-browser consistency */ /* Noto Color Emoji contains digits, in fixed-width, therefore causing From 3b81d2e5bbcaf3b53a572e0513bdfeb804bc9c67 Mon Sep 17 00:00:00 2001 From: Robin Townsend Date: Tue, 3 Jan 2023 22:22:06 -0500 Subject: [PATCH 3/4] Add a regression test --- cypress/e2e/polls/polls.spec.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cypress/e2e/polls/polls.spec.ts b/cypress/e2e/polls/polls.spec.ts index ce0962410c7..c092d4f6478 100644 --- a/cypress/e2e/polls/polls.spec.ts +++ b/cypress/e2e/polls/polls.spec.ts @@ -115,7 +115,10 @@ describe("Polls", () => { const pollParams = { title: "Does the polls feature work?", - options: ["Yes", "No", "Maybe"], + // Since we're going to take a screenshot anyways, we include some + // non-ASCII characters here to stress test the app's font config + // while we're at it. + options: ["Yes", "Noo⃐o⃑o⃩o⃪o⃫o⃬o⃭o⃮o⃯", "のらねこ Maybe?"], }; createPoll(pollParams); From 57b531351b834567af8ca1e5165fb3f43003d1b1 Mon Sep 17 00:00:00 2001 From: Robin Townsend Date: Thu, 5 Jan 2023 14:00:41 -0500 Subject: [PATCH 4/4] Install STIXGeneral in Cypress runners --- .github/workflows/cypress.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/cypress.yaml b/.github/workflows/cypress.yaml index 59e904ee0e9..5f71523fa19 100644 --- a/.github/workflows/cypress.yaml +++ b/.github/workflows/cypress.yaml @@ -92,6 +92,12 @@ jobs: # # Run 4 instances in Parallel # runner: [1, 2, 3, 4] steps: + - uses: tecolicom/actions-use-apt-tools@v1 + with: + # Our test suite includes some screenshot tests with unusual diacritics, which are + # supposed to be covered by STIXGeneral. + tools: fonts-stix + - uses: actions/checkout@v3 with: # XXX: We're checking out untrusted code in a secure context