From 22943ee06a9fcc5e47c8ef856270b1bea030caba Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Fri, 21 Feb 2025 13:18:14 +0000 Subject: [PATCH] Use EditInPlace control for Identity Server picker to improve a11y (#29280) * Use EditInPlace for identity server picker. * Exclude picker from default dialog button styles. * Remove unused import. * Update test * Remove unused css * Update test * drop only * Add a test for setting an ID server. * Add a unit test for SetIdServer * fix tests * Reformat mx_Dialog button :not list to use a more readable selector. * Reformat other :not sections * forgot a comma * We're in 2025 now. * Update copyright + use class methods. --- .../security-user-settings-tab.spec.ts | 53 +++++++-- res/css/_common.pcss | 84 ++++++++++----- res/css/_components.pcss | 1 - res/css/views/settings/_SetIdServer.pcss | 23 ---- src/components/views/settings/SetIdServer.tsx | 85 ++++++--------- src/i18n/strings/en_EN.json | 1 + .../views/settings/SetIdServer-test.tsx | 101 ++++++++++++++++++ .../__snapshots__/SetIdServer-test.tsx.snap | 54 ++++++++++ .../SecurityUserSettingsTab-test.tsx.snap | 36 +++---- 9 files changed, 309 insertions(+), 129 deletions(-) delete mode 100644 res/css/views/settings/_SetIdServer.pcss create mode 100644 test/unit-tests/components/views/settings/SetIdServer-test.tsx create mode 100644 test/unit-tests/components/views/settings/__snapshots__/SetIdServer-test.tsx.snap diff --git a/playwright/e2e/settings/security-user-settings-tab.spec.ts b/playwright/e2e/settings/security-user-settings-tab.spec.ts index 9b9439796d6..9a5616ef597 100644 --- a/playwright/e2e/settings/security-user-settings-tab.spec.ts +++ b/playwright/e2e/settings/security-user-settings-tab.spec.ts @@ -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( @@ -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"); diff --git a/res/css/_common.pcss b/res/css/_common.pcss index fe8eff22860..75180013f62 100644 --- a/res/css/_common.pcss +++ b/res/css/_common.pcss @@ -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( + .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; @@ -616,32 +619,46 @@ legend { } .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( + button:not( + .mx_Dialog_nonDialogButton, + [class|="maplibregl"], + .mx_AccessibleButton, + .mx_UserProfileSettings button, + .mx_ThemeChoicePanel_CustomTheme button, + .mx_UnpinAllDialog button, + .mx_ShareDialog button, .mx_EncryptionUserSettingsTab button ):last-child { margin-right: 0px; } .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( + button:not( + .mx_Dialog_nonDialogButton, + [class|="maplibregl"], + .mx_AccessibleButton, + .mx_UserProfileSettings button, + .mx_ThemeChoicePanel_CustomTheme button, + .mx_UnpinAllDialog button, + .mx_ShareDialog button, .mx_EncryptionUserSettingsTab button ):focus, .mx_Dialog input[type="submit"]:focus, -.mx_Dialog_buttons button:not(.mx_Dialog_nonDialogButton):not(.mx_AccessibleButton):focus, +.mx_Dialog_buttons button:not(.mx_Dialog_nonDialogButton, .mx_AccessibleButton):focus, .mx_Dialog_buttons input[type="submit"]:focus { filter: brightness($focus-brightness); } -.mx_Dialog button.mx_Dialog_primary:not(.mx_Dialog_nonDialogButton):not([class|="maplibregl"]), +.mx_Dialog button.mx_Dialog_primary:not(.mx_Dialog_nonDialogButton, [class|="maplibregl"]), .mx_Dialog input[type="submit"].mx_Dialog_primary, .mx_Dialog_buttons - button.mx_Dialog_primary:not(.mx_Dialog_nonDialogButton):not(.mx_AccessibleButton):not( - .mx_UserProfileSettings button - ):not(.mx_ThemeChoicePanel_CustomTheme button):not(.mx_UnpinAllDialog button):not(.mx_ShareDialog button):not( + button:not( + .mx_Dialog_nonDialogButton, + .mx_AccessibleButton, + .mx_UserProfileSettings button, + .mx_ThemeChoicePanel_CustomTheme button, + .mx_UnpinAllDialog button, + .mx_ShareDialog button, .mx_EncryptionUserSettingsTab button ), .mx_Dialog_buttons input[type="submit"].mx_Dialog_primary { @@ -651,32 +668,43 @@ legend { min-width: 156px; } -.mx_Dialog button.danger:not(.mx_Dialog_nonDialogButton):not([class|="maplibregl"]), +.mx_Dialog button.danger:not(.mx_Dialog_nonDialogButton, [class|="maplibregl"]), .mx_Dialog input[type="submit"].danger, .mx_Dialog_buttons - button.danger:not(.mx_Dialog_nonDialogButton):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.danger:not( + .mx_Dialog_nonDialogButton, + .mx_AccessibleButton, + .mx_UserProfileSettings button, + .mx_ThemeChoicePanel_CustomTheme button, + .mx_UnpinAllDialog button, + .mx_ShareDialog button, + .mx_EncryptionUserSettingsTab button + ), .mx_Dialog_buttons input[type="submit"].danger { background-color: var(--cpd-color-bg-critical-primary); border: solid 1px var(--cpd-color-bg-critical-primary); color: var(--cpd-color-text-on-solid-primary); } -.mx_Dialog button.warning:not(.mx_Dialog_nonDialogButton):not([class|="maplibregl"]), +.mx_Dialog button.warning:not(.mx_Dialog_nonDialogButton, [class|="maplibregl"]), .mx_Dialog input[type="submit"].warning { border: solid 1px var(--cpd-color-border-critical-subtle); color: var(--cpd-color-text-critical-primary); } .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( + button:not( + .mx_Dialog_nonDialogButton, + [class|="maplibregl"], + .mx_AccessibleButton, + .mx_UserProfileSettings button, + .mx_ThemeChoicePanel_CustomTheme button, + .mx_UnpinAllDialog button, + .mx_ShareDialog button, .mx_EncryptionUserSettingsTab button ):disabled, .mx_Dialog input[type="submit"]:disabled, -.mx_Dialog_buttons button:not(.mx_Dialog_nonDialogButton):not(.mx_AccessibleButton):disabled, +.mx_Dialog_buttons button:not(.mx_Dialog_nonDialogButton, .mx_AccessibleButton):disabled, .mx_Dialog_buttons input[type="submit"]:disabled { background-color: $light-fg-color; border: solid 1px $light-fg-color; diff --git a/res/css/_components.pcss b/res/css/_components.pcss index 8cab127632a..13347e887a3 100644 --- a/res/css/_components.pcss +++ b/res/css/_components.pcss @@ -349,7 +349,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"; diff --git a/res/css/views/settings/_SetIdServer.pcss b/res/css/views/settings/_SetIdServer.pcss deleted file mode 100644 index 377292451ff..00000000000 --- a/res/css/views/settings/_SetIdServer.pcss +++ /dev/null @@ -1,23 +0,0 @@ -/* -Copyright 2024 New Vector Ltd. -Copyright 2019-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. -*/ - -.mx_SetIdServer { - display: flex; - flex-direction: column; - align-items: flex-start; - gap: $spacing-8; - - .mx_Field { - width: 100%; - margin: 0; - } -} - -.mx_SetIdServer_tooltip { - max-width: var(--SettingsTab_tooltip-max-width); -} diff --git a/src/components/views/settings/SetIdServer.tsx b/src/components/views/settings/SetIdServer.tsx index 86aa0a3df86..520c642172a 100644 --- a/src/components/views/settings/SetIdServer.tsx +++ b/src/components/views/settings/SetIdServer.tsx @@ -1,5 +1,5 @@ /* -Copyright 2024 New Vector Ltd. +Copyright 2024-2025 New Vector Ltd. Copyright 2019-2021 The Matrix.org Foundation C.I.C. SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial @@ -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"; @@ -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"; @@ -86,10 +86,12 @@ export default class SetIdServer extends React.Component { 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, @@ -117,26 +119,7 @@ export default class SetIdServer extends React.Component { private onIdentityServerChanged = (ev: React.ChangeEvent): void => { const u = ev.target.value; - this.setState({ idServer: u }); - }; - - private getTooltip = (): JSX.Element | undefined => { - if (this.state.checking) { - return ( -
- - {_t("identity_server|checking")} -
- ); - } else if (this.state.error) { - return {this.state.error}; - } else { - return undefined; - } - }; - - private idServerChangeEnabled = (): boolean => { - return !!this.state.idServer && !this.state.busy; + this.setState({ idServer: u, error: undefined }); }; private saveIdServer = (fullUrl: string): void => { @@ -148,7 +131,7 @@ export default class SetIdServer extends React.Component { busy: false, error: undefined, currentClientIdServer: fullUrl, - idServer: "", + idServer: fullUrl, }); }; @@ -175,7 +158,7 @@ export default class SetIdServer extends React.Component { // 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; } @@ -213,7 +196,7 @@ export default class SetIdServer extends React.Component { }); }; - 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: ( @@ -347,6 +330,9 @@ export default class SetIdServer extends React.Component { }); }; + private onInputCancel = (): void => this.setState((s) => ({ idServer: s.currentClientIdServer ?? "" })); + private onClearServerErrors = (): void => this.setState({ error: undefined }); + public render(): React.ReactNode { const idServerUrl = this.state.currentClientIdServer; let sectionTitle; @@ -356,13 +342,13 @@ export default class SetIdServer extends React.Component { bodyText = _t( "identity_server|description_connected", {}, - { server: (sub) => {abbreviateUrl(idServerUrl)} }, + { server: () => {abbreviateUrl(idServerUrl)} }, ); if (this.props.missingTerms) { bodyText = _t( "identity_server|change_server_prompt", {}, - { server: (sub) => {abbreviateUrl(idServerUrl)} }, + { server: () => {abbreviateUrl(idServerUrl)} }, ); } } else { @@ -393,28 +379,25 @@ export default class SetIdServer extends React.Component { return ( -
- - - {_t("action|change")} - - {discoSection} - + + {this.state.error && {this.state.error}} + + {discoSection}
); } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 1b8a504d5ec..d4ab22771e6 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1245,6 +1245,7 @@ "change": "Change identity server", "change_prompt": "Disconnect from the identity server and connect to instead?", "change_server_prompt": "If you don't want to use 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 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.", diff --git a/test/unit-tests/components/views/settings/SetIdServer-test.tsx b/test/unit-tests/components/views/settings/SetIdServer-test.tsx new file mode 100644 index 00000000000..d92b7c27377 --- /dev/null +++ b/test/unit-tests/components/views/settings/SetIdServer-test.tsx @@ -0,0 +1,101 @@ +/* +Copyright 2025 New Vector Ltd. + +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("", () => { + 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 = () => ( + + + + ); + + 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(); + }); +}); diff --git a/test/unit-tests/components/views/settings/__snapshots__/SetIdServer-test.tsx.snap b/test/unit-tests/components/views/settings/__snapshots__/SetIdServer-test.tsx.snap new file mode 100644 index 00000000000..cc3af24e3c8 --- /dev/null +++ b/test/unit-tests/components/views/settings/__snapshots__/SetIdServer-test.tsx.snap @@ -0,0 +1,54 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[` renders expected fields 1`] = ` + +
+ + Identity server + +
+
+ You are not currently using an identity server. To discover and be discoverable by existing contacts you know, add one below. +
+
+
+
+
+ +
+ +
+
+
+
+
+ +`; diff --git a/test/unit-tests/components/views/settings/tabs/user/__snapshots__/SecurityUserSettingsTab-test.tsx.snap b/test/unit-tests/components/views/settings/tabs/user/__snapshots__/SecurityUserSettingsTab-test.tsx.snap index 3aac2e2c026..1bd9fc48fa7 100644 --- a/test/unit-tests/components/views/settings/tabs/user/__snapshots__/SecurityUserSettingsTab-test.tsx.snap +++ b/test/unit-tests/components/views/settings/tabs/user/__snapshots__/SecurityUserSettingsTab-test.tsx.snap @@ -438,33 +438,29 @@ exports[` renders security section 1`] = ` class="mx_SettingsFieldset_content" >
- -
-
- Change +
+ +