Use browser's font size instead of hardcoded 16px as root font size (…

…#12246) * WIP Use browser font size instead of hardcoded 16px * Add font migration to v3 * Remove custom font size input * Use a dropdown instead of a slider * Add margin to the font size dropdown * Fix `UpdateFontSizeDelta` action typo * Fix `fontScale`in `Call.ts` * Rename `baseFontSizeV3` to `fontSizeDelta` * Update playwright test * Add `default` next to the browser font size * Remove remaining `TODO` * Remove falsy `private` * Improve doc * Update snapshots after develop merge * Remove commented import
element-hq · Feb 21, 2024 · 6d55ce0 · 6d55ce0
1 parent 36a8d50
commit 6d55ce0
Show file tree

Hide file tree

Showing 17 changed files with 456 additions and 369 deletions.
diff --git a/playwright/e2e/settings/appearance-user-settings-tab.spec.ts b/playwright/e2e/settings/appearance-user-settings-tab.spec.ts
@@ -73,48 +73,18 @@ test.describe("Appearance user settings tab", () => {
         await expect(page.locator(".mx_RoomView_body[data-layout='bubble']")).toBeVisible();
     });
 
-    test("should support changing font size by clicking the font slider", async ({ page, app, user }) => {
+    test("should support changing font size by using the font size dropdown", async ({ page, app, user }) => {
         await app.settings.openUserSettings("Appearance");
 
         const tab = page.getByTestId("mx_AppearanceUserSettingsTab");
-        const fontSliderSection = tab.locator(".mx_FontScalingPanel_fontSlider");
+        const fontDropdown = tab.locator(".mx_FontScalingPanel_Dropdown");
+        await expect(fontDropdown.getByLabel("Font size")).toBeVisible();
 
-        await expect(fontSliderSection.getByLabel("Font size")).toBeVisible();
+        // Default browser font size is 16px and the select value is 0
+        // -4 value is 12px
+        await fontDropdown.getByLabel("Font size").selectOption({ value: "-4" });
 
-        const slider = fontSliderSection.getByRole("slider");
-        // Click the left position of the slider
-        await slider.click({ position: { x: 0, y: 10 } });
-
-        const MIN_FONT_SIZE = 11;
-        // Assert that the smallest font size is selected
-        await expect(fontSliderSection.locator(`input[value='${MIN_FONT_SIZE}']`)).toBeVisible();
-        await expect(
-            fontSliderSection.locator("output .mx_Slider_selection_label", { hasText: String(MIN_FONT_SIZE) }),
-        ).toBeVisible();
-
-        await expect(fontSliderSection).toMatchScreenshot(`font-slider-${MIN_FONT_SIZE}.png`);
-
-        // Click the right position of the slider
-        await slider.click({ position: { x: 572, y: 10 } });
-
-        const MAX_FONT_SIZE = 21;
-        // Assert that the largest font size is selected
-        await expect(fontSliderSection.locator(`input[value='${MAX_FONT_SIZE}']`)).toBeVisible();
-        await expect(
-            fontSliderSection.locator("output .mx_Slider_selection_label", { hasText: String(MAX_FONT_SIZE) }),
-        ).toBeVisible();
-
-        await expect(fontSliderSection).toMatchScreenshot(`font-slider-${MAX_FONT_SIZE}.png`);
-    });
-
-    test("should disable font size slider when custom font size is used", async ({ page, app, user }) => {
-        await app.settings.openUserSettings("Appearance");
-
-        const panel = page.getByTestId("mx_FontScalingPanel");
-        await panel.locator("label", { hasText: "Use custom size" }).click();
-
-        // Assert that the font slider is disabled
-        await expect(panel.locator(".mx_FontScalingPanel_fontSlider input[disabled]")).toBeVisible();
+        await expect(page).toMatchScreenshot("window-12px.png");
     });
 
     test("should support enabling compact group (modern) layout", async ({ page, app, user }) => {

diff --git a/...napshots/settings/appearance-user-settings-tab.spec.ts/appearance-tab-linux.png b/...napshots/settings/appearance-user-settings-tab.spec.ts/appearance-tab-linux.png
diff --git a/...t/snapshots/settings/appearance-user-settings-tab.spec.ts/window-12px-linux.png b/...t/snapshots/settings/appearance-user-settings-tab.spec.ts/window-12px-linux.png
diff --git a/res/css/views/settings/_FontScalingPanel.pcss b/res/css/views/settings/_FontScalingPanel.pcss
@@ -36,26 +36,8 @@ limitations under the License.
     }
 }
 
-.mx_FontScalingPanel_fontSlider {
-    display: flex;
-    align-items: center;
-    padding: 15px $spacing-20 35px;
-    background: $panels;
-    border-radius: 10px;
-    font-size: $font-10px;
-
-    .mx_FontScalingPanel_fontSlider_smallText,
-    .mx_FontScalingPanel_fontSlider_largeText {
-        font-weight: 500;
-    }
-
-    .mx_FontScalingPanel_fontSlider_smallText {
-        font-size: $font-15px;
-        padding-inline-end: $spacing-20;
-    }
-
-    .mx_FontScalingPanel_fontSlider_largeText {
-        font-size: $font-18px;
-        padding-inline-start: $spacing-20;
-    }
+.mx_FontScalingPanel_Dropdown {
+    width: 120px;
+    /* Override default mx_Field margin */
+    margin-bottom: var(--cpd-space-2x) !important;
 }
diff --git a/src/components/views/settings/FontScalingPanel.tsx b/src/components/views/settings/FontScalingPanel.tsx
@@ -14,29 +14,26 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React, { ChangeEvent } from "react";
+import React from "react";
 
 import EventTilePreview from "../elements/EventTilePreview";
-import Field from "../elements/Field";
-import SettingsFlag from "../elements/SettingsFlag";
 import SettingsStore from "../../../settings/SettingsStore";
-import Slider from "../elements/Slider";
-import { FontWatcher } from "../../../settings/watchers/FontWatcher";
-import { IValidationResult, IFieldState } from "../elements/Validation";
 import { Layout } from "../../../settings/enums/Layout";
 import { MatrixClientPeg } from "../../../MatrixClientPeg";
 import { SettingLevel } from "../../../settings/SettingLevel";
 import { _t } from "../../../languageHandler";
-import { clamp } from "../../../utils/numbers";
 import SettingsSubsection from "./shared/SettingsSubsection";
+import Field from "../elements/Field";
+import { FontWatcher } from "../../../settings/watchers/FontWatcher";
 
 interface IProps {}
 
 interface IState {
+    browserFontSize: number;
     // String displaying the current selected fontSize.
-    // Needs to be string for things like '17.' without
+    // Needs to be string for things like '1.' without
     // trailing 0s.
-    fontSize: string;
+    fontSizeDelta: number;
     useCustomFontSize: boolean;
     layout: Layout;
     // User profile data for the message preview
@@ -47,14 +44,19 @@ interface IState {
 
 export default class FontScalingPanel extends React.Component<IProps, IState> {
     private readonly MESSAGE_PREVIEW_TEXT = _t("common|preview_message");
+    /**
+     * Font sizes available (in px)
+     */
+    private readonly sizes = [9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 20, 22, 24, 26, 28, 30, 32, 34, 36];
     private layoutWatcherRef?: string;
     private unmounted = false;
 
     public constructor(props: IProps) {
         super(props);
 
         this.state = {
-            fontSize: SettingsStore.getValue("baseFontSizeV2", null).toString(),
+            fontSizeDelta: SettingsStore.getValue<number>("fontSizeDelta", null),
+            browserFontSize: FontWatcher.getBrowserDefaultFontSize(),
             useCustomFontSize: SettingsStore.getValue("useCustomFontSize"),
             layout: SettingsStore.getValue("layout"),
         };
@@ -90,30 +92,22 @@ export default class FontScalingPanel extends React.Component<IProps, IState> {
         }
     }
 
-    private onFontSizeChanged = (size: number): void => {
-        this.setState({ fontSize: size.toString() });
-        SettingsStore.setValue("baseFontSizeV2", null, SettingLevel.DEVICE, size);
+    /**
+     * Save the new font size
+     * @param delta
+     */
+    private onFontSizeChanged = async (delta: string): Promise<void> => {
+        const parsedDelta = parseInt(delta, 10) || 0;
+        this.setState({ fontSizeDelta: parsedDelta });
+        await SettingsStore.setValue("fontSizeDelta", null, SettingLevel.DEVICE, parsedDelta);
     };
 
-    private onValidateFontSize = async ({ value }: Pick<IFieldState, "value">): Promise<IValidationResult> => {
-        const parsedSize = parseFloat(value!);
-        const min = FontWatcher.MIN_SIZE;
-        const max = FontWatcher.MAX_SIZE;
-
-        if (isNaN(parsedSize)) {
-            return { valid: false, feedback: _t("settings|appearance|font_size_nan") };
-        }
-
-        if (!(min <= parsedSize && parsedSize <= max)) {
-            return {
-                valid: false,
-                feedback: _t("settings|appearance|font_size_limit", { min, max }),
-            };
-        }
-
-        SettingsStore.setValue("baseFontSizeV2", null, SettingLevel.DEVICE, parseInt(value!, 10));
-
-        return { valid: true, feedback: _t("settings|appearance|font_size_valid", { min, max }) };
+    /**
+     * Compute the difference between the selected font size and the browser font size
+     * @param fontSize
+     */
+    private computeDeltaFontSize = (fontSize: number): number => {
+        return fontSize - this.state.browserFontSize;
     };
 
     public render(): React.ReactNode {
@@ -123,6 +117,21 @@ export default class FontScalingPanel extends React.Component<IProps, IState> {
                 stretchContent
                 data-testid="mx_FontScalingPanel"
             >
+                <Field
+                    element="select"
+                    className="mx_FontScalingPanel_Dropdown"
+                    label={_t("settings|appearance|font_size")}
+                    value={this.state.fontSizeDelta.toString()}
+                    onChange={(e) => this.onFontSizeChanged(e.target.value)}
+                >
+                    {this.sizes.map((size) => (
+                        <option key={size} value={this.computeDeltaFontSize(size)}>
+                            {size === this.state.browserFontSize
+                                ? _t("settings|appearance|font_size_default", { fontSize: size })
+                                : size}
+                        </option>
+                    ))}
+                </Field>
                 <EventTilePreview
                     className="mx_FontScalingPanel_preview"
                     message={this.MESSAGE_PREVIEW_TEXT}
@@ -131,49 +140,6 @@ export default class FontScalingPanel extends React.Component<IProps, IState> {
                     displayName={this.state.displayName}
                     avatarUrl={this.state.avatarUrl}
                 />
-                <div className="mx_FontScalingPanel_fontSlider">
-                    <div className="mx_FontScalingPanel_fontSlider_smallText">Aa</div>
-                    <Slider
-                        min={FontWatcher.MIN_SIZE}
-                        max={FontWatcher.MAX_SIZE}
-                        step={1}
-                        value={parseInt(this.state.fontSize, 10)}
-                        onChange={this.onFontSizeChanged}
-                        displayFunc={(_) => ""}
-                        disabled={this.state.useCustomFontSize}
-                        label={_t("settings|appearance|font_size")}
-                    />
-                    <div className="mx_FontScalingPanel_fontSlider_largeText">Aa</div>
-                </div>
-
-                <SettingsFlag
-                    name="useCustomFontSize"
-                    level={SettingLevel.ACCOUNT}
-                    onChange={(checked) => {
-                        this.setState({ useCustomFontSize: checked });
-                        if (!checked) {
-                            const size = parseInt(this.state.fontSize, 10);
-                            const clamped = clamp(size, FontWatcher.MIN_SIZE, FontWatcher.MAX_SIZE);
-                            if (clamped !== size) {
-                                this.onFontSizeChanged(clamped);
-                            }
-                        }
-                    }}
-                    useCheckbox={true}
-                />
-
-                <Field
-                    type="number"
-                    label={_t("settings|appearance|font_size")}
-                    autoComplete="off"
-                    placeholder={this.state.fontSize.toString()}
-                    value={this.state.fontSize.toString()}
-                    id="font_size_field"
-                    onValidate={this.onValidateFontSize}
-                    onChange={(value: ChangeEvent<HTMLInputElement>) => this.setState({ fontSize: value.target.value })}
-                    disabled={!this.state.useCustomFontSize}
-                    className="mx_AppearanceUserSettingsTab_checkboxControlledField"
-                />
             </SettingsSubsection>
         );
     }

diff --git a/src/dispatcher/actions.ts b/src/dispatcher/actions.ts
@@ -107,9 +107,11 @@ export enum Action {
     MigrateBaseFontSize = "migrate_base_font_size",
 
     /**
-     * Sets the apps root font size. Should be used with UpdateFontSizePayload
+     * Sets the apps root font size delta. Should be used with UpdateFontSizeDeltaPayload
+     * It will add the delta to the current font size.
+     * The delta should be between {@link FontWatcher.MIN_DELTA} and {@link FontWatcher.MAX_DELTA}.
      */
-    UpdateFontSize = "update_font_size",
+    UpdateFontSizeDelta = "update_font_size_delta",
 
     /**
      * Sets a system font. Should be used with UpdateSystemFontPayload

diff --git a/...patcher/payloads/UpdateFontSizePayload.ts → ...er/payloads/UpdateFontSizeDeltaPayload.ts b/...patcher/payloads/UpdateFontSizePayload.ts → ...er/payloads/UpdateFontSizeDeltaPayload.ts
@@ -17,11 +17,12 @@ limitations under the License.
 import { ActionPayload } from "../payloads";
 import { Action } from "../actions";
 
-export interface UpdateFontSizePayload extends ActionPayload {
-    action: Action.UpdateFontSize;
+export interface UpdateFontSizeDeltaPayload extends ActionPayload {
+    action: Action.UpdateFontSizeDelta;
 
     /**
-     * The font size to set the root to
+     * The delta is added to the current font size.
+     * The delta should be between {@link FontWatcher.MIN_DELTA} and {@link FontWatcher.MAX_DELTA}.
      */
-    size: number;
+    delta: number;
 }
diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json
@@ -2422,9 +2422,7 @@
             "custom_theme_success": "Theme added!",
             "custom_theme_url": "Custom theme URL",
             "font_size": "Font size",
-            "font_size_limit": "Custom font size can only be between %(min)s pt and %(max)s pt",
-            "font_size_nan": "Size must be a number",
-            "font_size_valid": "Use between %(min)s pt and %(max)s pt",
+            "font_size_default": "%(fontSize)s (default)",
             "heading": "Customise your appearance",
             "image_size_default": "Default",
             "image_size_large": "Large",

diff --git a/src/models/Call.ts b/src/models/Call.ts
@@ -52,11 +52,11 @@ import WidgetStore from "../stores/WidgetStore";
 import { WidgetMessagingStore, WidgetMessagingStoreEvent } from "../stores/widgets/WidgetMessagingStore";
 import ActiveWidgetStore, { ActiveWidgetStoreEvent } from "../stores/ActiveWidgetStore";
 import { getCurrentLanguage } from "../languageHandler";
-import { FontWatcher } from "../settings/watchers/FontWatcher";
 import { PosthogAnalytics } from "../PosthogAnalytics";
 import { UPDATE_EVENT } from "../stores/AsyncStore";
 import { getJoinedNonFunctionalMembers } from "../utils/room/getJoinedNonFunctionalMembers";
 import { isVideoRoom } from "../utils/video-rooms";
+import { FontWatcher } from "../settings/watchers/FontWatcher";
 
 const TIMEOUT_MS = 16000;
 
@@ -687,7 +687,8 @@ export class ElementCall extends Call {
             roomId: roomId,
             baseUrl: client.baseUrl,
             lang: getCurrentLanguage().replace("_", "-"),
-            fontScale: `${(SettingsStore.getValue("baseFontSizeV2") ?? 16) / FontWatcher.DEFAULT_SIZE}`,
+            fontScale: (FontWatcher.getRootFontSize() / FontWatcher.getBrowserDefaultFontSize()).toString(),
+
             analyticsID,
         });
 

diff --git a/src/settings/Settings.tsx b/src/settings/Settings.tsx
@@ -511,6 +511,9 @@ export const SETTINGS: { [setting: string]: ISetting } = {
         supportedLevels: [SettingLevel.CONFIG],
         default: 0,
     },
+    /**
+     * @deprecated in favor of {@link fontSizeDelta}
+     */
     "baseFontSize": {
         displayName: _td("settings|appearance|font_size"),
         supportedLevels: LEVELS_ACCOUNT_SETTINGS,
@@ -530,12 +533,22 @@ export const SETTINGS: { [setting: string]: ISetting } = {
      * With the transition to Compound we are moving to a base font size
      * of 16px. We're taking the opportunity to move away from the `baseFontSize`
      * setting that had a 5px offset.
-     *
+     * @deprecated in favor {@link fontSizeDelta}
      */
     "baseFontSizeV2": {
         displayName: _td("settings|appearance|font_size"),
         supportedLevels: [SettingLevel.DEVICE],
-        default: FontWatcher.DEFAULT_SIZE,
+        default: "",
+        controller: new FontSizeController(),
+    },
+    /**
+     * This delta is added to the browser default font size
+     * Moving from `baseFontSizeV2` to `fontSizeDelta` to replace the default 16px to --cpd-font-size-root (browser default font size) + fontSizeDelta
+     */
+    "fontSizeDelta": {
+        displayName: _td("settings|appearance|font_size"),
+        supportedLevels: [SettingLevel.DEVICE],
+        default: FontWatcher.DEFAULT_DELTA,
         controller: new FontSizeController(),
     },
     "useCustomFontSize": {

diff --git a/src/settings/controllers/FontSizeController.ts b/src/settings/controllers/FontSizeController.ts
@@ -16,7 +16,7 @@ limitations under the License.
 
 import SettingController from "./SettingController";
 import dis from "../../dispatcher/dispatcher";
-import { UpdateFontSizePayload } from "../../dispatcher/payloads/UpdateFontSizePayload";
+import { UpdateFontSizeDeltaPayload } from "../../dispatcher/payloads/UpdateFontSizeDeltaPayload";
 import { Action } from "../../dispatcher/actions";
 import { SettingLevel } from "../SettingLevel";
 
@@ -34,9 +34,9 @@ export default class FontSizeController extends SettingController {
             dis.fire(Action.MigrateBaseFontSize);
         } else if (newValue !== "") {
             // Dispatch font size change so that everything open responds to the change.
-            dis.dispatch<UpdateFontSizePayload>({
-                action: Action.UpdateFontSize,
-                size: newValue,
+            dis.dispatch<UpdateFontSizeDeltaPayload>({
+                action: Action.UpdateFontSizeDelta,
+                delta: newValue,
             });
         }
     }