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

Use EditInPlace control for Identity Server picker to improve a11y #29280

Merged
merged 18 commits into from
Feb 21, 2025
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
53 changes: 47 additions & 6 deletions playwright/e2e/settings/security-user-settings-tab.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ test.describe("Security user settings tab", () => {
});

test.describe("AnalyticsLearnMoreDialog", () => {
test("should be rendered properly", { tag: "@screenshot" }, async ({ app, page }) => {
test("should be rendered properly", { tag: "@screenshot" }, async ({ app, page, user }) => {
const tab = await app.settings.openUserSettings("Security");
await tab.getByRole("button", { name: "Learn more" }).click();
await expect(page.locator(".mx_AnalyticsLearnMoreDialog_wrapper .mx_Dialog")).toMatchScreenshot(
Expand All @@ -41,16 +41,57 @@ test.describe("Security user settings tab", () => {
});
});

test("should contain section to set ID server", async ({ app }) => {
test("should be able to set an ID server", async ({ app, context, user, page }) => {
const tab = await app.settings.openUserSettings("Security");

const setIdServer = tab.locator(".mx_SetIdServer");
await context.route("https://identity.example.org/_matrix/identity/v2", async (route) => {
await route.fulfill({
status: 200,
json: {},
});
});
await context.route("https://identity.example.org/_matrix/identity/v2/account/register", async (route) => {
await route.fulfill({
status: 200,
json: {
token: "AToken",
},
});
});
await context.route("https://identity.example.org/_matrix/identity/v2/account", async (route) => {
await route.fulfill({
status: 200,
json: {
user_id: user.userId,
},
});
});
await context.route("https://identity.example.org/_matrix/identity/v2/terms", async (route) => {
await route.fulfill({
status: 200,
json: {
policies: {},
},
});
});
const setIdServer = tab.locator(".mx_IdentityServerPicker");
await setIdServer.scrollIntoViewIfNeeded();
// Assert that an input area for identity server exists
await expect(setIdServer.getByRole("textbox", { name: "Enter a new identity server" })).toBeVisible();

const textElement = setIdServer.getByRole("textbox", { name: "Enter a new identity server" });
await textElement.click();
await textElement.fill("https://identity.example.org");
await setIdServer.getByRole("button", { name: "Change" }).click();

await expect(setIdServer.getByText("Checking server")).toBeVisible();
// Accept terms
await page.getByTestId("dialog-primary-button").click();
// Check identity has changed.
await expect(setIdServer.getByText("Your identity server has been changed")).toBeVisible();
// Ensure section title is updated.
await expect(tab.getByText(`Identity server (identity.example.org)`, { exact: true })).toBeVisible();
});

test("should enable show integrations as enabled", async ({ app, page }) => {
test("should enable show integrations as enabled", async ({ app, page, user }) => {
const tab = await app.settings.openUserSettings("Security");

const setIntegrationManager = tab.locator(".mx_SetIntegrationManager");
Expand Down
19 changes: 11 additions & 8 deletions res/css/_common.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -589,18 +589,21 @@ legend {
* in the app look the same by being AccessibleButtons, or possibly by having explict button classes.
* We should go through and have one consistent set of styles for buttons throughout the app.
* For now, I am duplicating the selectors here for mx_Dialog and mx_DialogButtons.
*
* Elements that should not be styled like a dialog button are mentioned in a :not() pseudo-class.
* For the widest browser support, we use multiple :not pseudo-classes instead of :not(.a, .b).
*/
.mx_Dialog
button:not(.mx_Dialog_nonDialogButton):not([class|="maplibregl"]):not(.mx_AccessibleButton):not(
.mx_UserProfileSettings button
):not(.mx_ThemeChoicePanel_CustomTheme button):not(.mx_UnpinAllDialog button):not(.mx_ShareDialog button):not(
.mx_EncryptionUserSettingsTab button
button:not(
Copy link
Member Author

Choose a reason for hiding this comment

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

Browser support for this is now okay https://developer.mozilla.org/en-US/docs/Web/CSS/:not#browser_compatibility.

I caused a bug here, and it was borderline unreadable before, having a list of selectors like this makes this a bit easier to parse.

.mx_EncryptionUserSettingsTab button,
.mx_UserProfileSettings button,
.mx_ShareDialog button,
.mx_UnpinAllDialog button,
.mx_ThemeChoicePanel_CustomTheme button,
.mx_Dialog_nonDialogButton,
.mx_AccessibleButton,
.mx_IdentityServerPicker button,
[class|="maplibregl"]
),
.mx_Dialog_buttons button:not(.mx_Dialog_nonDialogButton, .mx_AccessibleButton),
.mx_Dialog input[type="submit"],
.mx_Dialog_buttons button:not(.mx_Dialog_nonDialogButton):not(.mx_AccessibleButton),
.mx_Dialog_buttons input[type="submit"] {
@mixin mx_DialogButton;
margin-left: 0px;
Expand Down
1 change: 0 additions & 1 deletion res/css/_components.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,6 @@
@import "./views/settings/_PowerLevelSelector.pcss";
@import "./views/settings/_RoomProfileSettings.pcss";
@import "./views/settings/_SecureBackupPanel.pcss";
@import "./views/settings/_SetIdServer.pcss";
@import "./views/settings/_SetIntegrationManager.pcss";
@import "./views/settings/_SettingsFieldset.pcss";
@import "./views/settings/_SettingsHeader.pcss";
Expand Down
23 changes: 0 additions & 23 deletions res/css/views/settings/_SetIdServer.pcss

This file was deleted.

76 changes: 28 additions & 48 deletions src/components/views/settings/SetIdServer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Please see LICENSE files in the repository root for full details.
import React, { type ReactNode } from "react";
import { logger } from "matrix-js-sdk/src/logger";
import { type IThreepid } from "matrix-js-sdk/src/matrix";
import { EditInPlace, ErrorMessage } from "@vector-im/compound-web";

import { _t } from "../../../languageHandler";
import { MatrixClientPeg } from "../../../MatrixClientPeg";
Expand All @@ -22,7 +23,6 @@ import { timeout } from "../../../utils/promise";
import { type ActionPayload } from "../../../dispatcher/payloads";
import InlineSpinner from "../elements/InlineSpinner";
import AccessibleButton from "../elements/AccessibleButton";
import Field from "../elements/Field";
import QuestionDialog from "../dialogs/QuestionDialog";
import SettingsFieldset from "./SettingsFieldset";
import { SettingsSubsectionText } from "./shared/SettingsSubsection";
Expand Down Expand Up @@ -86,10 +86,12 @@ export default class SetIdServer extends React.Component<IProps, IState> {
defaultIdServer = abbreviateUrl(getDefaultIdentityServerUrl());
}

const currentClientIdServer = MatrixClientPeg.safeGet().getIdentityServerUrl();

this.state = {
defaultIdServer,
currentClientIdServer: MatrixClientPeg.safeGet().getIdentityServerUrl(),
idServer: "",
currentClientIdServer,
idServer: currentClientIdServer ?? "",
busy: false,
disconnectBusy: false,
checking: false,
Expand Down Expand Up @@ -117,26 +119,7 @@ export default class SetIdServer extends React.Component<IProps, IState> {
private onIdentityServerChanged = (ev: React.ChangeEvent<HTMLInputElement>): void => {
const u = ev.target.value;

this.setState({ idServer: u });
};

private getTooltip = (): JSX.Element | undefined => {
if (this.state.checking) {
return (
<div>
<InlineSpinner />
{_t("identity_server|checking")}
</div>
);
} else if (this.state.error) {
return <strong className="warning">{this.state.error}</strong>;
} else {
return undefined;
}
};

private idServerChangeEnabled = (): boolean => {
return !!this.state.idServer && !this.state.busy;
this.setState({ idServer: u, error: undefined });
};

private saveIdServer = (fullUrl: string): void => {
Expand All @@ -148,7 +131,7 @@ export default class SetIdServer extends React.Component<IProps, IState> {
busy: false,
error: undefined,
currentClientIdServer: fullUrl,
idServer: "",
idServer: fullUrl,
});
};

Expand All @@ -175,7 +158,7 @@ export default class SetIdServer extends React.Component<IProps, IState> {
// Double check that the identity server even has terms of service.
const hasTerms = await doesIdentityServerHaveTerms(MatrixClientPeg.safeGet(), fullUrl);
if (!hasTerms) {
const [confirmed] = await this.showNoTermsWarning(fullUrl);
const [confirmed] = await this.showNoTermsWarning();
save = !!confirmed;
}

Expand Down Expand Up @@ -213,7 +196,7 @@ export default class SetIdServer extends React.Component<IProps, IState> {
});
};

private showNoTermsWarning(fullUrl: string): Promise<[ok?: boolean]> {
private showNoTermsWarning(): Promise<[ok?: boolean]> {
const { finished } = Modal.createDialog(QuestionDialog, {
title: _t("terms|identity_server_no_terms_title"),
description: (
Expand Down Expand Up @@ -393,28 +376,25 @@ export default class SetIdServer extends React.Component<IProps, IState> {

return (
<SettingsFieldset legend={sectionTitle} description={bodyText}>
<form className="mx_SetIdServer" onSubmit={this.checkIdServer}>
<Field
label={_t("identity_server|url_field_label")}
type="text"
autoComplete="off"
placeholder={this.state.defaultIdServer}
value={this.state.idServer}
onChange={this.onIdentityServerChanged}
tooltipContent={this.getTooltip()}
tooltipClassName="mx_SetIdServer_tooltip"
disabled={this.state.busy}
forceValidity={this.state.error ? false : undefined}
/>
<AccessibleButton
kind="primary_sm"
onClick={this.checkIdServer}
disabled={!this.idServerChangeEnabled()}
>
{_t("action|change")}
</AccessibleButton>
{discoSection}
</form>
<EditInPlace
className="mx_IdentityServerPicker"
cancelButtonLabel={_t("action|reset")}
disabled={!!this.state.busy}
label={_t("identity_server|url_field_label")}
onCancel={() => this.setState((s) => ({ idServer: s.currentClientIdServer ?? "" }))}
onChange={this.onIdentityServerChanged}
onClearServerErrors={() => this.setState({ error: undefined })}
onSave={this.checkIdServer}
placeholder={this.state.defaultIdServer}
saveButtonLabel={_t("action|change")}
savedLabel={this.state.error ? undefined : _t("identity_server|changed")}
savingLabel={_t("identity_server|checking")}
serverInvalid={!!this.state.error}
value={this.state.idServer}
>
{this.state.error && <ErrorMessage>{this.state.error}</ErrorMessage>}
</EditInPlace>
{discoSection}
</SettingsFieldset>
);
}
Expand Down
1 change: 1 addition & 0 deletions src/i18n/strings/en_EN.json
Original file line number Diff line number Diff line change
Expand Up @@ -1243,6 +1243,7 @@
"change": "Change identity server",
"change_prompt": "Disconnect from the identity server <current /> and connect to <new /> instead?",
"change_server_prompt": "If you don't want to use <server /> to discover and be discoverable by existing contacts you know, enter another identity server below.",
"changed": "Your identity server has been changed",
"checking": "Checking server",
"description_connected": "You are currently using <server></server> to discover and be discoverable by existing contacts you know. You can change your identity server below.",
"description_disconnected": "You are not currently using an identity server. To discover and be discoverable by existing contacts you know, add one below.",
Expand Down
102 changes: 102 additions & 0 deletions test/unit-tests/components/views/settings/SetIdServer-test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
Copyright 2024 New Vector Ltd.
Copyright 2023 The Matrix.org Foundation C.I.C.

SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
Please see LICENSE files in the repository root for full details.
*/

import React from "react";
import { render, waitFor } from "jest-matrix-react";
import userEvent from "@testing-library/user-event";
import fetchMock from "fetch-mock-jest";

import SetIdServer from "../../../../../src/components/views/settings/SetIdServer";
import MatrixClientContext from "../../../../../src/contexts/MatrixClientContext";
import { getMockClientWithEventEmitter, mockClientMethodsUser, mockClientMethodsServer } from "../../../../test-utils";

describe("<SetIdServer />", () => {
const userId = "@alice:server.org";

const mockClient = getMockClientWithEventEmitter({
...mockClientMethodsUser(userId),
...mockClientMethodsServer(),
getOpenIdToken: jest.fn().mockResolvedValue("a_token"),
getTerms: jest.fn(),
setAccountData: jest.fn(),
});

const getComponent = () => (
<MatrixClientContext.Provider value={mockClient}>
<SetIdServer missingTerms={false} />
</MatrixClientContext.Provider>
);

afterAll(() => {
jest.resetAllMocks();
});

it("renders expected fields", () => {
const { asFragment } = render(getComponent());
expect(asFragment()).toMatchSnapshot();
});

it("should allow setting an identity server", async () => {
const { getByLabelText, getByRole } = render(getComponent());

fetchMock.get("https://identity.example.org/_matrix/identity/v2", {
body: {},
});
fetchMock.get("https://identity.example.org/_matrix/identity/v2/account", {
body: { user_id: userId },
});
fetchMock.post("https://identity.example.org/_matrix/identity/v2/account/register", {
body: { token: "foobar" },
});

const identServerField = getByLabelText("Enter a new identity server");
await userEvent.type(identServerField, "https://identity.example.org");
await userEvent.click(getByRole("button", { name: "Change" }));
await userEvent.click(getByRole("button", { name: "Continue" }));
});

it("should clear input on cancel", async () => {
const { getByLabelText, getByRole } = render(getComponent());
const identServerField = getByLabelText("Enter a new identity server");
await userEvent.type(identServerField, "https://identity.example.org");
await userEvent.click(getByRole("button", { name: "Reset" }));
expect((identServerField as HTMLInputElement).value).toEqual("");
});

it("should show error when an error occurs", async () => {
const { getByLabelText, getByRole, getByText } = render(getComponent());

fetchMock.get("https://invalid.example.org/_matrix/identity/v2", {
body: {},
status: 404,
});
fetchMock.get("https://invalid.example.org/_matrix/identity/v2/account", {
body: {},
status: 404,
});
fetchMock.post("https://invalid.example.org/_matrix/identity/v2/account/register", {
body: {},
status: 404,
});

const identServerField = getByLabelText("Enter a new identity server");
await userEvent.type(identServerField, "https://invalid.example.org");
await userEvent.click(getByRole("button", { name: "Change" }));

await waitFor(
() => {
expect(getByText("Not a valid identity server (status code 404)")).toBeVisible();
},
{ timeout: 3000 },
);

// Check the error vanishes when the input is edited.
await userEvent.type(identServerField, "https://identity2.example.org");
expect(() => getByText("Not a valid identity server (status code 404)")).toThrow();
});
});
Loading
Loading