Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Support browser font scaling #11972

Closed
wants to merge 5 commits into from
Closed

Support browser font scaling #11972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d4a17a
set html font-size to 100%
Nov 29, 2023
c761c66
remove customFontSize setting
Nov 29, 2023
09dcdfe
use labels relative to default on font slider
Nov 29, 2023
1dea9b3
Merge branch 'develop' into kerry/26669/browser-font-scaling
Nov 29, 2023
cbdca71
update snapshot
Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 4 additions & 13 deletions cypress/e2e/settings/appearance-user-settings-tab.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ describe("Appearance user settings tab", () => {
const MIN_FONT_SIZE = 11;
// Assert that the smallest font size is selected
cy.get(`input[value='${MIN_FONT_SIZE}']`).should("exist");
cy.get("output .mx_Slider_selection_label").findByText(MIN_FONT_SIZE);
// 16 - 5 = 11
cy.get("output .mx_Slider_selection_label").findByText("-5");
});

cy.get(".mx_FontScalingPanel_fontSlider").percySnapshotElement("Font size slider - smallest (13)", {
Expand All @@ -139,7 +140,8 @@ describe("Appearance user settings tab", () => {
const MAX_FONT_SIZE = 21;
// Assert that the largest font size is selected
cy.get(`input[value='${MAX_FONT_SIZE}']`).should("exist");
cy.get("output .mx_Slider_selection_label").findByText(MAX_FONT_SIZE);
// 16 + 5 = 21
cy.get("output .mx_Slider_selection_label").findByText("+5");
});

cy.get(".mx_FontScalingPanel_fontSlider").percySnapshotElement("Font size slider - largest (21)", {
Expand All @@ -148,17 +150,6 @@ describe("Appearance user settings tab", () => {
});
});

it("should disable font size slider when custom font size is used", () => {
cy.openUserSettings("Appearance");

cy.findByTestId("mx_FontScalingPanel").within(() => {
cy.findByLabelText("Use custom size").click({ force: true }); // force click as checkbox size is zero

// Assert that the font slider is disabled
cy.get(".mx_FontScalingPanel_fontSlider input[disabled]").should("exist");
});
});

it("should support enabling compact group (modern) layout", () => {
// Create and view a room first
cy.createRoom({ name: "Test Room" }).viewRoomByName("Test Room");
Expand Down
11 changes: 7 additions & 4 deletions src/components/views/elements/Slider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ interface IProps {
step: number | "any";

// A function for formatting the values
displayFunc: (value: number) => string;
displayFunc?: (value: number) => string;

// Whether the slider is disabled
disabled: boolean;
disabled?: boolean;

label: string;
}
Expand All @@ -53,8 +53,11 @@ export default class Slider extends React.Component<IProps> {

public render(): React.ReactNode {
let selection: JSX.Element | undefined;
const { value, displayFunc, disabled } = this.props;

if (!this.props.disabled) {
const formattedValue = displayFunc ? displayFunc(value) : value;

if (!disabled) {
const position = this.position;
selection = (
<output
Expand All @@ -63,7 +66,7 @@ export default class Slider extends React.Component<IProps> {
left: `calc(2px + ${position}% + ${THUMB_SIZE / 2}em - ${(position / 100) * THUMB_SIZE}em)`,
}}
>
<span className="mx_Slider_selection_label">{this.props.value}</span>
<span className="mx_Slider_selection_label">{formattedValue}</span>
</output>
);
}
Expand Down
69 changes: 10 additions & 59 deletions src/components/views/settings/FontScalingPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,16 @@ 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";

interface IProps {}
Expand All @@ -37,14 +33,21 @@ interface IState {
// Needs to be string for things like '17.' without
// trailing 0s.
Comment on lines 33 to 34
Copy link
Member

Choose a reason for hiding this comment

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

is this still true, particularly given we use a custom formatter? Maybe we can switch to a numeric representation and do a bit less toString and parseInt below?

(It might also be nice to switch it to holding a relative value, to make it closer to the displayed value and what it actually does? But happy for us to declare that out of scope for this PR)

fontSize: string;
useCustomFontSize: boolean;
layout: Layout;
// User profile data for the message preview
userId?: string;
displayName?: string;
avatarUrl?: string;
}

const formatRelativeFontSize = (fontSize: number): string => {
const delta = fontSize - FontWatcher.DEFAULT_SIZE;
if (delta === 0) {
return _t("settings|appearance|font_size_default");
}
return new Intl.NumberFormat(undefined, { signDisplay: "always" }).format(delta);
};

export default class FontScalingPanel extends React.Component<IProps, IState> {
private readonly MESSAGE_PREVIEW_TEXT = _t("common|preview_message");
private layoutWatcherRef?: string;
Expand All @@ -55,7 +58,6 @@ export default class FontScalingPanel extends React.Component<IProps, IState> {

this.state = {
fontSize: SettingsStore.getValue("baseFontSizeV2", null).toString(),
useCustomFontSize: SettingsStore.getValue("useCustomFontSize"),
layout: SettingsStore.getValue("layout"),
};
}
Expand Down Expand Up @@ -95,27 +97,6 @@ export default class FontScalingPanel extends React.Component<IProps, IState> {
SettingsStore.setValue("baseFontSizeV2", null, SettingLevel.DEVICE, size);
};

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 }) };
};

public render(): React.ReactNode {
return (
<SettingsSubsection
Expand All @@ -139,41 +120,11 @@ export default class FontScalingPanel extends React.Component<IProps, IState> {
step={1}
value={parseInt(this.state.fontSize, 10)}
onChange={this.onFontSizeChanged}
displayFunc={(_) => ""}
disabled={this.state.useCustomFontSize}
displayFunc={formatRelativeFontSize}
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>
);
}
Expand Down
5 changes: 1 addition & 4 deletions src/i18n/strings/en_EN.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2404,16 +2404,13 @@
"custom_font": "Use a system font",
"custom_font_description": "Set the name of a font installed on your system & %(brand)s will attempt to use it.",
"custom_font_name": "System font name",
"custom_font_size": "Use custom size",
"custom_theme_add_button": "Add theme",
"custom_theme_error_downloading": "Error downloading theme information.",
"custom_theme_invalid": "Invalid theme schema.",
"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": "Default",
"heading": "Customise your appearance",
"image_size_default": "Default",
"image_size_large": "Large",
Expand Down
5 changes: 0 additions & 5 deletions src/settings/Settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,11 +494,6 @@ export const SETTINGS: { [setting: string]: ISetting } = {
default: FontWatcher.DEFAULT_SIZE,
Copy link
Member

Choose a reason for hiding this comment

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

I note there is a reference on line 487 to "16px". It seems like that needs updating, or at least clarifying?

Generally it might be nice to add a little documentation about what baseFontSizeV2 actually does and how its value relates to fontSize on the root element.

controller: new FontSizeController(),
},
"useCustomFontSize": {
displayName: _td("settings|appearance|custom_font_size"),
supportedLevels: LEVELS_ACCOUNT_SETTINGS,
default: false,
},
"MessageComposerInput.suggestEmoji": {
supportedLevels: LEVELS_ACCOUNT_SETTINGS,
displayName: _td("settings|emoji_autocomplete"),
Expand Down
10 changes: 9 additions & 1 deletion src/settings/watchers/FontWatcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,15 @@ export class FontWatcher implements IWatcher {
if (fontSize !== size) {
await SettingsStore.setValue("baseFontSizeV2", null, SettingLevel.DEVICE, fontSize);
}
document.querySelector<HTMLElement>(":root")!.style.fontSize = toPx(fontSize);

// To respect browser font scaling we need to set the base font-size to 100%
// When user has set a custom font size, apply it as a delta to 100% using calc
const cssFontSize =
fontSize === FontWatcher.DEFAULT_SIZE
? "100%"
: `calc(100% + ${toPx(fontSize - FontWatcher.DEFAULT_SIZE)})`;
Comment on lines +125 to +128
Copy link
Member

Choose a reason for hiding this comment

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

For a bit DRYer:

Suggested change
const cssFontSize =
fontSize === FontWatcher.DEFAULT_SIZE
? "100%"
: `calc(100% + ${toPx(fontSize - FontWatcher.DEFAULT_SIZE)})`;
const relativeSize = fontSize - FontWatcher.DEFAULT_SIZE;
const cssFontSize =
relativeSize === 0
? "100%"
: `calc(100% + ${toPx(relativeSize})`;

(I'd also be inclined to s/${toPx(relativeSize})/${relativeSize}px, but YMMV. I'd also be tempted to get rid of the conditional and just let it be calc(100% + 0px) when it's the default size, but again YMMV)


document.querySelector<HTMLElement>(":root")!.style.fontSize = cssFontSize;
};

public static readonly FONT_FAMILY_CUSTOM_PROPERTY = "--cpd-font-family-sans";
Expand Down
28 changes: 15 additions & 13 deletions test/components/views/settings/FontScalingPanel-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,33 @@
*/

import React from "react";
import { fireEvent, render, waitFor } from "@testing-library/react";
import { fireEvent, render, screen } from "@testing-library/react";

import * as TestUtils from "../../../test-utils";
import FontScalingPanel from "../../../../src/components/views/settings/FontScalingPanel";
import SettingsStore from "../../../../src/settings/SettingsStore";

describe("FontScalingPanel", () => {
afterEach(() => {
jest.resetAllMocks();
});
it("renders the font scaling UI", () => {
TestUtils.stubClient();
const { asFragment } = render(<FontScalingPanel />);
expect(asFragment()).toMatchSnapshot();

Check failure on line 31 in test/components/views/settings/FontScalingPanel-test.tsx

View workflow job for this annotation

GitHub Actions / Jest

FontScalingPanel › renders the font scaling UI 

expect(received).toMatchSnapshot()

Snapshot name: `FontScalingPanel renders the font scaling UI 1`

- Snapshot  - 1
+ Received  + 1

@@ -55,11 +55,11 @@
              style="left: calc(2px + 50% + 1.2em - 1.2em);"
            >
              <span
                class="mx_Slider_selection_label"
              >
-               16
+               Default
              </span>
            </output>
          </div>
          <div
            class="mx_FontScalingPanel_fontSlider_largeText"

    at Object.toMatchSnapshot (test/components/views/settings/FontScalingPanel-test.tsx:31:30)
});

it("should clamp custom font size when disabling it", async () => {
jest.spyOn(SettingsStore, "setValue").mockResolvedValue(undefined);
it("updates the font size", () => {
jest.spyOn(SettingsStore, "setValue");
TestUtils.stubClient();
const { container, getByText } = render(<FontScalingPanel />);
fireEvent.click(getByText("Use custom size"));
await waitFor(() => {
expect(container.querySelector("input[checked]")).toBeDefined();
});
fireEvent.change(container.querySelector("#font_size_field")!, { target: { value: "25" } });
fireEvent.click(getByText("Use custom size"));
await waitFor(() => {
expect(container.querySelector("#font_size_field")).toHaveValue(21);
});
render(<FontScalingPanel />);

const slider = screen.getByLabelText("Font size");

fireEvent.change(slider, { target: { value: 19 } });

expect(SettingsStore.setValue).toHaveBeenCalledWith("baseFontSizeV2", null, "device", 19);

expect(screen.getByLabelText("Font size")).toHaveValue("19");
});
});
40 changes: 0 additions & 40 deletions test/components/views/settings/__snapshots__/FontScalingPanel-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,46 +70,6 @@ exports[`FontScalingPanel renders the font scaling UI 1`] = `
Aa
</div>
</div>
<span
class="mx_Checkbox mx_Checkbox_hasKind mx_Checkbox_kind_solid"
>
<input
id="checkbox_vY7Q4uEh9K"
type="checkbox"
/>
<label
for="checkbox_vY7Q4uEh9K"
>
<div
class="mx_Checkbox_background"
>
<div
class="mx_Checkbox_checkmark"
/>
</div>
<div>
Use custom size
</div>
</label>
</span>
<div
class="mx_Field mx_Field_input mx_AppearanceUserSettingsTab_checkboxControlledField"
>
<input
autocomplete="off"
disabled=""
id="font_size_field"
label="Font size"
placeholder="16"
type="number"
value="16"
/>
<label
for="font_size_field"
>
Font size
</label>
</div>
</div>
</div>
</DocumentFragment>
Expand Down
42 changes: 1 addition & 41 deletions ...components/views/settings/tabs/user/__snapshots__/AppearanceUserSettingsTab-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ exports[`AppearanceUserSettingsTab should render 1`] = `
<span
class="mx_Slider_selection_label"
>
16
Default
</span>
</output>
</div>
Expand All @@ -299,46 +299,6 @@ exports[`AppearanceUserSettingsTab should render 1`] = `
Aa
</div>
</div>
<span
class="mx_Checkbox mx_Checkbox_hasKind mx_Checkbox_kind_solid"
>
<input
id="checkbox_vY7Q4uEh9K"
type="checkbox"
/>
<label
for="checkbox_vY7Q4uEh9K"
>
<div
class="mx_Checkbox_background"
>
<div
class="mx_Checkbox_checkmark"
/>
</div>
<div>
Use custom size
</div>
</label>
</span>
<div
class="mx_Field mx_Field_input mx_AppearanceUserSettingsTab_checkboxControlledField"
>
<input
autocomplete="off"
disabled=""
id="font_size_field"
label="Font size"
placeholder="16"
type="number"
value="16"
/>
<label
for="font_size_field"
>
Font size
</label>
</div>
</div>
</div>
<div
Expand Down
Loading
Loading