From ee485ffcfda09dc60047b57ae0e348cd7b09f7ef Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 23 Nov 2023 13:04:05 +0000 Subject: [PATCH] Default to system emoji font (#11925) * Disable Twemoji emoji font by default Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Force Twemoji font in SAS Verification Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Add tests Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Improve tests Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../verification/_VerificationShowSas.pcss | 2 + .../legacy-light/css/_legacy-light.pcss | 15 ++++--- res/themes/light/css/_light.pcss | 13 +++--- .../tabs/user/AppearanceUserSettingsTab.tsx | 8 ++++ .../payloads/UpdateSystemFontPayload.ts | 5 +++ src/i18n/strings/en_EN.json | 1 + src/settings/Settings.tsx | 9 +++- .../controllers/SystemFontController.ts | 6 +-- .../controllers/UseSystemFontController.ts | 37 --------------- src/settings/watchers/FontWatcher.ts | 45 +++++++++++++------ .../controllers/SystemFontController-test.ts | 14 +++--- .../UseSystemFontController-test.ts | 40 ----------------- test/settings/watchers/FontWatcher-test.tsx | 38 +++++++++++++++- 13 files changed, 119 insertions(+), 114 deletions(-) delete mode 100644 src/settings/controllers/UseSystemFontController.ts delete mode 100644 test/settings/controllers/UseSystemFontController-test.ts diff --git a/res/css/views/verification/_VerificationShowSas.pcss b/res/css/views/verification/_VerificationShowSas.pcss index f601933cc5..40588179b0 100644 --- a/res/css/views/verification/_VerificationShowSas.pcss +++ b/res/css/views/verification/_VerificationShowSas.pcss @@ -49,6 +49,8 @@ limitations under the License. .mx_VerificationShowSas_emojiSas_emoji { font-size: $font-32px; + /* Use the Twemoji font for consistency with other clients */ + font-family: Twemoji, var(--cpd-font-family-sans); } .mx_VerificationShowSas_emojiSas_label { diff --git a/res/themes/legacy-light/css/_legacy-light.pcss b/res/themes/legacy-light/css/_legacy-light.pcss index f10e674351..b1bb34c18e 100644 --- a/res/themes/legacy-light/css/_legacy-light.pcss +++ b/res/themes/legacy-light/css/_legacy-light.pcss @@ -1,17 +1,20 @@ +:root { + /* This is set to Twemoji when the user opts into the bundled emoji font */ + --emoji-font-family: ""; +} + /* Nunito lacks combining diacritics, so these will fall through to the next font. Helevetica's diacritics sometimes do not combine nicely (on OSX, at least) and result in a huge horizontal mess. Arial empirically gets it right, hence prioritising Arial here. */ -/* 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 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", "Arial", "Helvetica", sans-serif, - "Noto Color Emoji"; +$font-family: "Nunito", var(--emoji-font-family), "Apple Color Emoji", "Segoe UI Emoji", "Arial", "Helvetica", + sans-serif, "Noto Color Emoji"; -$monospace-font-family: "Inconsolata", "Twemoji", "Apple Color Emoji", "Segoe UI Emoji", "Courier", monospace, - "Noto Color Emoji"; +$monospace-font-family: "Inconsolata", var(--emoji-font-family), "Apple Color Emoji", "Segoe UI Emoji", "Courier", + monospace, "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 d3448a82c7..c47f1910ab 100644 --- a/res/themes/light/css/_light.pcss +++ b/res/themes/light/css/_light.pcss @@ -1,17 +1,20 @@ +:root { + /* This is set to Twemoji when the user opts into the bundled emoji font */ + --emoji-font-family: ""; +} + /* Nunito and Inter lacks combining diacritics, so these will fall through to the next font. Helevetica's diacritics sometimes do not combine nicely (on OSX, at least) and result in a huge horizontal mess. Arial empirically gets it right, hence prioritising Arial here. */ -/* 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 digits in flowed text to stand out. TODO: Consider putting all emoji fonts to the end rather than the front. */ -$font-family: "Inter", "Twemoji", "Apple Color Emoji", "Segoe UI Emoji", "Arial", "Helvetica", sans-serif, +$font-family: "Inter", var(--emoji-font-family), "Apple Color Emoji", "Segoe UI Emoji", "Arial", "Helvetica", sans-serif, "Noto Color Emoji"; -$monospace-font-family: "Inconsolata", "Twemoji", "Apple Color Emoji", "Segoe UI Emoji", "Courier", monospace, - "Noto Color Emoji"; +$monospace-font-family: "Inconsolata", var(--emoji-font-family), "Apple Color Emoji", "Segoe UI Emoji", "Courier", + monospace, "Noto Color Emoji"; /* Colors from Figma Compound https://www.figma.com/file/X4XTH9iS2KGJ2wFKDqkyed/Compound?node-id=559%3A120 */ /* ******************** */ diff --git a/src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx b/src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx index 343c0d26ec..9408274e9b 100644 --- a/src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx +++ b/src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx @@ -38,6 +38,7 @@ import MatrixClientContext from "../../../../../contexts/MatrixClientContext"; interface IProps {} interface IState { + useBundledEmojiFont: boolean; useSystemFont: boolean; systemFont: string; showAdvanced: boolean; @@ -60,6 +61,7 @@ export default class AppearanceUserSettingsTab extends React.Component + this.setState({ useBundledEmojiFont: checked })} + /> ({ action: Action.UpdateSystemFont, + useBundledEmojiFont: SettingsStore.getValue("useBundledEmojiFont"), useSystemFont: SettingsStore.getValue("useSystemFont"), - font: newValue, + font: SettingsStore.getValue("systemFont"), }); } } diff --git a/src/settings/controllers/UseSystemFontController.ts b/src/settings/controllers/UseSystemFontController.ts deleted file mode 100644 index c3fe6e339e..0000000000 --- a/src/settings/controllers/UseSystemFontController.ts +++ /dev/null @@ -1,37 +0,0 @@ -/* -Copyright 2020 The Matrix.org Foundation C.I.C. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -import SettingController from "./SettingController"; -import SettingsStore from "../SettingsStore"; -import dis from "../../dispatcher/dispatcher"; -import { UpdateSystemFontPayload } from "../../dispatcher/payloads/UpdateSystemFontPayload"; -import { Action } from "../../dispatcher/actions"; -import { SettingLevel } from "../SettingLevel"; - -export default class UseSystemFontController extends SettingController { - public constructor() { - super(); - } - - public onChange(level: SettingLevel, roomId: string, newValue: any): void { - // Dispatch font size change so that everything open responds to the change. - dis.dispatch({ - action: Action.UpdateSystemFont, - useSystemFont: newValue, - font: SettingsStore.getValue("systemFont"), - }); - } -} diff --git a/src/settings/watchers/FontWatcher.ts b/src/settings/watchers/FontWatcher.ts index 4af7a476c9..bb538d1809 100644 --- a/src/settings/watchers/FontWatcher.ts +++ b/src/settings/watchers/FontWatcher.ts @@ -86,6 +86,7 @@ export class FontWatcher implements IWatcher { private updateFont(): void { this.setRootFontSize(SettingsStore.getValue("baseFontSizeV2")); this.setSystemFont({ + useBundledEmojiFont: SettingsStore.getValue("useBundledEmojiFont"), useSystemFont: SettingsStore.getValue("useSystemFont"), font: SettingsStore.getValue("systemFont"), }); @@ -102,6 +103,7 @@ export class FontWatcher implements IWatcher { // Clear font overrides when logging out this.setRootFontSize(FontWatcher.DEFAULT_SIZE); this.setSystemFont({ + useBundledEmojiFont: false, useSystemFont: false, font: "", }); @@ -121,31 +123,46 @@ export class FontWatcher implements IWatcher { }; public static readonly FONT_FAMILY_CUSTOM_PROPERTY = "--cpd-font-family-sans"; + public static readonly EMOJI_FONT_FAMILY_CUSTOM_PROPERTY = "--emoji-font-family"; + public static readonly BUNDLED_EMOJI_FONT = "Twemoji"; private setSystemFont = ({ + useBundledEmojiFont, useSystemFont, font, - }: Pick): void => { + }: Pick): void => { if (useSystemFont) { + let fontString = font + .split(",") + .map((font) => { + font = font.trim(); + if (!font.startsWith('"') && !font.endsWith('"')) { + font = `"${font}"`; + } + return font; + }) + .join(","); + + if (useBundledEmojiFont) { + fontString += ", " + FontWatcher.BUNDLED_EMOJI_FONT; + } + /** * Overrides the default font family from Compound * Make sure that fonts with spaces in their names get interpreted properly */ - document.body.style.setProperty( - FontWatcher.FONT_FAMILY_CUSTOM_PROPERTY, - font - .split(",") - .map((font) => { - font = font.trim(); - if (!font.startsWith('"') && !font.endsWith('"')) { - font = `"${font}"`; - } - return font; - }) - .join(","), - ); + document.body.style.setProperty(FontWatcher.FONT_FAMILY_CUSTOM_PROPERTY, fontString); } else { document.body.style.removeProperty(FontWatcher.FONT_FAMILY_CUSTOM_PROPERTY); + + if (useBundledEmojiFont) { + document.body.style.setProperty( + FontWatcher.EMOJI_FONT_FAMILY_CUSTOM_PROPERTY, + FontWatcher.BUNDLED_EMOJI_FONT, + ); + } else { + document.body.style.removeProperty(FontWatcher.EMOJI_FONT_FAMILY_CUSTOM_PROPERTY); + } } }; } diff --git a/test/settings/controllers/SystemFontController-test.ts b/test/settings/controllers/SystemFontController-test.ts index c827557ca4..2450816ac4 100644 --- a/test/settings/controllers/SystemFontController-test.ts +++ b/test/settings/controllers/SystemFontController-test.ts @@ -17,22 +17,26 @@ limitations under the License. import { Action } from "../../../src/dispatcher/actions"; import dis from "../../../src/dispatcher/dispatcher"; import SystemFontController from "../../../src/settings/controllers/SystemFontController"; -import { SettingLevel } from "../../../src/settings/SettingLevel"; import SettingsStore from "../../../src/settings/SettingsStore"; const dispatchSpy = jest.spyOn(dis, "dispatch"); describe("SystemFontController", () => { - it("dispatches a font size action on change", () => { - const getValueSpy = jest.spyOn(SettingsStore, "getValue").mockReturnValue(true); + it("dispatches a system font update action on change", () => { const controller = new SystemFontController(); - controller.onChange(SettingLevel.ACCOUNT, "$room:server", 12); + const getValueSpy = jest.spyOn(SettingsStore, "getValue").mockImplementation((settingName) => { + if (settingName === "useBundledEmojiFont") return false; + if (settingName === "useSystemFont") return true; + if (settingName === "systemFont") return "Comic Sans MS"; + }); + controller.onChange(); expect(dispatchSpy).toHaveBeenCalledWith({ action: Action.UpdateSystemFont, + useBundledEmojiFont: false, useSystemFont: true, - font: 12, + font: "Comic Sans MS", }); expect(getValueSpy).toHaveBeenCalledWith("useSystemFont"); diff --git a/test/settings/controllers/UseSystemFontController-test.ts b/test/settings/controllers/UseSystemFontController-test.ts deleted file mode 100644 index 5b5003dce6..0000000000 --- a/test/settings/controllers/UseSystemFontController-test.ts +++ /dev/null @@ -1,40 +0,0 @@ -/* -Copyright 2022 The Matrix.org Foundation C.I.C. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -import { Action } from "../../../src/dispatcher/actions"; -import dis from "../../../src/dispatcher/dispatcher"; -import UseSystemFontController from "../../../src/settings/controllers/UseSystemFontController"; -import { SettingLevel } from "../../../src/settings/SettingLevel"; -import SettingsStore from "../../../src/settings/SettingsStore"; - -const dispatchSpy = jest.spyOn(dis, "dispatch"); - -describe("UseSystemFontController", () => { - it("dispatches a font size action on change", () => { - const getValueSpy = jest.spyOn(SettingsStore, "getValue").mockReturnValue(12); - const controller = new UseSystemFontController(); - - controller.onChange(SettingLevel.ACCOUNT, "$room:server", true); - - expect(dispatchSpy).toHaveBeenCalledWith({ - action: Action.UpdateSystemFont, - useSystemFont: true, - font: 12, - }); - - expect(getValueSpy).toHaveBeenCalledWith("systemFont"); - }); -}); diff --git a/test/settings/watchers/FontWatcher-test.tsx b/test/settings/watchers/FontWatcher-test.tsx index dd781994e3..6fb400508f 100644 --- a/test/settings/watchers/FontWatcher-test.tsx +++ b/test/settings/watchers/FontWatcher-test.tsx @@ -24,9 +24,15 @@ import { Action } from "../../../src/dispatcher/actions"; import { untilDispatch } from "../../test-utils"; import defaultDispatcher from "../../../src/dispatcher/dispatcher"; -async function setSystemFont(font: string): Promise { +async function setSystemFont(font: string | false): Promise { + await SettingsStore.setValue("systemFont", null, SettingLevel.DEVICE, font || ""); await SettingsStore.setValue("useSystemFont", null, SettingLevel.DEVICE, !!font); - await SettingsStore.setValue("systemFont", null, SettingLevel.DEVICE, font); + await untilDispatch(Action.UpdateSystemFont); + await sleep(1); // await the FontWatcher doing its action +} + +async function setUseBundledEmojiFont(use: boolean): Promise { + await SettingsStore.setValue("useBundledEmojiFont", null, SettingLevel.DEVICE, use); await untilDispatch(Action.UpdateSystemFont); await sleep(1); // await the FontWatcher doing its action } @@ -34,6 +40,9 @@ async function setSystemFont(font: string): Promise { const getFontFamily = () => { return document.body.style.getPropertyValue(FontWatcher.FONT_FAMILY_CUSTOM_PROPERTY); }; +const getEmojiFontFamily = () => { + return document.body.style.getPropertyValue(FontWatcher.EMOJI_FONT_FAMILY_CUSTOM_PROPERTY); +}; describe("FontWatcher", function () { it("should load font on start()", async () => { @@ -85,6 +94,31 @@ describe("FontWatcher", function () { }); }); + describe("Sets bundled emoji font as expected", () => { + let fontWatcher: FontWatcher; + beforeEach(async () => { + await setSystemFont(false); + fontWatcher = new FontWatcher(); + await fontWatcher.start(); + }); + afterEach(() => { + fontWatcher.stop(); + }); + + it("by default does not add Twemoji font", async () => { + expect(getEmojiFontFamily()).toMatchInlineSnapshot(`""`); + }); + it("adds Twemoji font when enabled", async () => { + await setUseBundledEmojiFont(true); + expect(getEmojiFontFamily()).toMatchInlineSnapshot(`"Twemoji"`); + }); + it("works in conjunction with useSystemFont", async () => { + await setSystemFont(`"Commodore 64"`); + await setUseBundledEmojiFont(true); + expect(getFontFamily()).toMatchInlineSnapshot(`""Commodore 64", Twemoji"`); + }); + }); + describe("Migrates baseFontSize", () => { let watcher: FontWatcher | undefined;