From 251918a6adb013d749aea0919c36a65c18f772b1 Mon Sep 17 00:00:00 2001 From: Robin Date: Thu, 5 Sep 2024 16:55:38 -0400 Subject: [PATCH 01/13] Enable test isolation I had experimented with turning this off in order to improve test performance, and apparently that ended up being merged. Now if we're to do component testing, we'll be changing things globally on the document, so isolation is very much necessary. --- src/useCallViewKeyboardShortcuts.test.tsx | 8 +++----- vitest.config.js | 1 - 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/useCallViewKeyboardShortcuts.test.tsx b/src/useCallViewKeyboardShortcuts.test.tsx index 3c51df03..8d7118db 100644 --- a/src/useCallViewKeyboardShortcuts.test.tsx +++ b/src/useCallViewKeyboardShortcuts.test.tsx @@ -25,8 +25,6 @@ import { useCallViewKeyboardShortcuts } from "../src/useCallViewKeyboardShortcut // Test Explanation: // - The main objective is to test `useCallViewKeyboardShortcuts`. // The TestComponent just wraps a button around that hook. -// - We need to set `userEvent` to the `{document = window.document}` since we are testing the -// `useCallViewKeyboardShortcuts` hook here. Which is listening to window. interface TestComponentProps { setMicrophoneMuted: (muted: boolean) => void; @@ -52,7 +50,7 @@ const TestComponent: FC = ({ }; test("spacebar unmutes", async () => { - const user = userEvent.setup({ document: window.document }); + const user = userEvent.setup(); let muted = true; render( { }); test("spacebar prioritizes pressing a button", async () => { - const user = userEvent.setup({ document: window.document }); + const user = userEvent.setup(); const setMuted = vi.fn(); const onClick = vi.fn(); @@ -86,7 +84,7 @@ test("spacebar prioritizes pressing a button", async () => { }); test("unmuting happens in place of the default action", async () => { - const user = userEvent.setup({ document: window.document }); + const user = userEvent.setup(); const defaultPrevented = vi.fn(); // In the real application, we mostly just want the spacebar shortcut to avoid // scrolling the page. But to test that here in JSDOM, we need some kind of diff --git a/vitest.config.js b/vitest.config.js index 43b407b7..d9790f63 100644 --- a/vitest.config.js +++ b/vitest.config.js @@ -12,7 +12,6 @@ export default defineConfig((configEnv) => classNameStrategy: "non-scoped", }, }, - isolate: false, setupFiles: ["src/vitest.setup.ts"], coverage: { reporter: ["html", "json"], From 40fc1aa46b84c0320a7951e5ce4a3fd1534d506c Mon Sep 17 00:00:00 2001 From: Robin Date: Fri, 6 Sep 2024 18:39:14 -0400 Subject: [PATCH 02/13] Upgrade Compound Web This patch release fixes a bug where tooltips would label non-interactive triggers in an inaccessible way. --- src/room/EncryptionLock.tsx | 1 - src/tile/MediaView.tsx | 1 - yarn.lock | 6 +++--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/room/EncryptionLock.tsx b/src/room/EncryptionLock.tsx index 53b8e430..3adc7e1e 100644 --- a/src/room/EncryptionLock.tsx +++ b/src/room/EncryptionLock.tsx @@ -40,7 +40,6 @@ export const EncryptionLock: FC = ({ encrypted }) => { height={16} className={styles.lock} data-encrypted={encrypted} - aria-hidden /> ); diff --git a/src/tile/MediaView.tsx b/src/tile/MediaView.tsx index 4d073092..423f1903 100644 --- a/src/tile/MediaView.tsx +++ b/src/tile/MediaView.tsx @@ -110,7 +110,6 @@ export const MediaView = forwardRef( width={20} height={20} className={styles.errorIcon} - aria-hidden /> )} diff --git a/yarn.lock b/yarn.lock index b2abbac6..4e0a2feb 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3088,9 +3088,9 @@ integrity sha512-PtQMG7kDzwtjw/fLKD63uWP5rJ8cgWc/aXarfEzxYUf9ceWxBajnYOBI2jDqtE3WIUe9uTVBzNEvmOBG/VIgTA== "@vector-im/compound-web@^6.0.0": - version "6.1.1" - resolved "https://registry.yarnpkg.com/@vector-im/compound-web/-/compound-web-6.1.1.tgz#872ec411fce600358812fe6c868b2521b57a9a9f" - integrity sha512-13cw8XFtt0oqGbUS9bPlvikZoJxrU+zZoziZGE+1Xv6h4JwNHIBB8CvqVOoUfEhta4U1BHOClgV1XD/YzbZXdA== + version "6.1.2" + resolved "https://registry.yarnpkg.com/@vector-im/compound-web/-/compound-web-6.1.2.tgz#3f8bbd69f2b06a2b8dd12c68df813356e5486074" + integrity sha512-kWOcprUsGGrGPRT9lPE3tnyGYWzNEIQSZ57BT/vMx9NX4QqT/Dpq0VhkBKm8W583NoYtxDOgSWWkvFQhyHVkWw== dependencies: "@floating-ui/react" "^0.26.9" "@radix-ui/react-context-menu" "^2.1.5" From d9333d68294628aa56b72524946beefec1889b26 Mon Sep 17 00:00:00 2001 From: Robin Date: Thu, 5 Sep 2024 16:23:44 -0400 Subject: [PATCH 03/13] Test RoomHeaderInfo --- package.json | 3 ++- src/Header.test.tsx | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/Toast.test.tsx | 14 +++----------- src/vitest.setup.ts | 15 ++++----------- yarn.lock | 24 ++++++++++++++++++++++++ 5 files changed, 78 insertions(+), 23 deletions(-) create mode 100644 src/Header.test.tsx diff --git a/package.json b/package.json index a02ac4f5..bce1ead7 100644 --- a/package.json +++ b/package.json @@ -108,6 +108,7 @@ "vite": "^5.0.0", "vite-plugin-html-template": "^1.1.0", "vite-plugin-svgr": "^4.0.0", - "vitest": "^2.0.0" + "vitest": "^2.0.0", + "vitest-axe": "^1.0.0-pre.3" } } diff --git a/src/Header.test.tsx b/src/Header.test.tsx new file mode 100644 index 00000000..aff15a45 --- /dev/null +++ b/src/Header.test.tsx @@ -0,0 +1,45 @@ +/* +Copyright 2024 New Vector Ltd + +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 { expect, test, vi } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { axe } from "vitest-axe"; +import { TooltipProvider } from "@vector-im/compound-web"; + +import { RoomHeaderInfo } from "./Header"; + +global.matchMedia = vi.fn().mockReturnValue({ + matches: true, + addEventListener: () => {}, + removeEventListener: () => {}, +}); + +test("RoomHeaderInfo is accessible", async () => { + const { container } = render( + + + , + ); + expect(await axe(container)).toHaveNoViolations(); + // Check that the room name acts as a heading + screen.getByRole("heading", { name: "Mission Control" }); +}); diff --git a/src/Toast.test.tsx b/src/Toast.test.tsx index e2e2f9f7..cf41ced4 100644 --- a/src/Toast.test.tsx +++ b/src/Toast.test.tsx @@ -15,19 +15,12 @@ limitations under the License. */ import { describe, expect, test, vi } from "vitest"; -import { render, configure } from "@testing-library/react"; +import { render } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { Toast } from "../src/Toast"; import { withFakeTimers } from "./utils/test"; -configure({ - defaultHidden: true, -}); - -// Test Explanation: -// This test the toast. We need to use { document: window.document } because the toast listens -// for user input on `window`. describe("Toast", () => { test("renders", () => { const { queryByRole } = render( @@ -45,7 +38,7 @@ describe("Toast", () => { }); test("dismisses when Esc is pressed", async () => { - const user = userEvent.setup({ document: window.document }); + const user = userEvent.setup(); const onDismiss = vi.fn(); render( @@ -59,7 +52,7 @@ describe("Toast", () => { test("dismisses when background is clicked", async () => { const user = userEvent.setup(); const onDismiss = vi.fn(); - const { getByRole, unmount } = render( + const { getByRole } = render( Hello world! , @@ -67,7 +60,6 @@ describe("Toast", () => { const background = getByRole("dialog").previousSibling! as Element; await user.click(background); expect(onDismiss).toHaveBeenCalled(); - unmount(); }); test("dismisses itself after the specified timeout", () => { diff --git a/src/vitest.setup.ts b/src/vitest.setup.ts index e4210b7c..80b704cc 100644 --- a/src/vitest.setup.ts +++ b/src/vitest.setup.ts @@ -13,13 +13,14 @@ 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 "global-jsdom/register"; -import globalJsdom from "global-jsdom"; import i18n from "i18next"; import posthog from "posthog-js"; import { initReactI18next } from "react-i18next"; -import { afterEach, beforeEach } from "vitest"; +import { afterEach } from "vitest"; import { cleanup } from "@testing-library/react"; +import "vitest-axe/extend-expect"; import { Config } from "./config/Config"; @@ -35,12 +36,4 @@ i18n.use(initReactI18next).init({ Config.initDefault(); posthog.opt_out_capturing(); -// We need to cleanup the global jsDom -// Otherwise we will run into issues with async input test overlapping and throwing. - -let cleanupJsDom: { (): void }; -beforeEach(() => (cleanupJsDom = globalJsdom())); -afterEach(() => { - cleanupJsDom(); - cleanup(); -}); +afterEach(cleanup); diff --git a/yarn.lock b/yarn.lock index 4e0a2feb..e6dc0380 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3441,6 +3441,11 @@ available-typed-arrays@^1.0.7: dependencies: possible-typed-array-names "^1.0.0" +axe-core@^4.7.2: + version "4.10.0" + resolved "https://registry.yarnpkg.com/axe-core/-/axe-core-4.10.0.tgz#d9e56ab0147278272739a000880196cdfe113b59" + integrity sha512-Mr2ZakwQ7XUAjp7pAwQWRhhK8mQQ6JAaNWSjmjxil0R8BPioMtQsTLOolGYkji1rcL++3dCqZA3zWqpT+9Ew6g== + axe-core@^4.9.1: version "4.9.1" resolved "https://registry.yarnpkg.com/axe-core/-/axe-core-4.9.1.tgz#fcd0f4496dad09e0c899b44f6c4bb7848da912ae" @@ -3677,6 +3682,11 @@ chalk@^4.0.0, chalk@^4.1.0: ansi-styles "^4.1.0" supports-color "^7.1.0" +chalk@^5.3.0: + version "5.3.0" + resolved "https://registry.yarnpkg.com/chalk/-/chalk-5.3.0.tgz#67c20a7ebef70e7f3970a01f90fa210cb6860385" + integrity sha512-dLitG79d+GV1Nb/VYcCDFivJeK1hiukt9QjRNVOsUtTy1rR1YJsmpGGTZ3qJos+uw7WmWF4wUwBd9jxjocFC2w== + check-error@^2.1.1: version "2.1.1" resolved "https://registry.yarnpkg.com/check-error/-/check-error-2.1.1.tgz#87eb876ae71ee388fa0471fe423f494be1d96ccc" @@ -5839,6 +5849,11 @@ locate-path@^6.0.0: dependencies: p-locate "^5.0.0" +lodash-es@^4.17.21: + version "4.17.21" + resolved "https://registry.yarnpkg.com/lodash-es/-/lodash-es-4.17.21.tgz#43e626c46e6591b7750beb2b50117390c609e3ee" + integrity sha512-mKnC+QJ9pWVzv+C4/U3rRsHapFfHvQFoFB92e52xeyGMcX6/OlIl78je1u8vePzYZSkkogMPJ2yjxxsb89cxyw== + lodash.debounce@^4.0.8: version "4.0.8" resolved "https://registry.yarnpkg.com/lodash.debounce/-/lodash.debounce-4.0.8.tgz#82d79bff30a67c4005ffd5e2515300ad9ca4d7af" @@ -8213,6 +8228,15 @@ vite@^5.0.0: optionalDependencies: fsevents "~2.3.3" +vitest-axe@^1.0.0-pre.3: + version "1.0.0-pre.3" + resolved "https://registry.yarnpkg.com/vitest-axe/-/vitest-axe-1.0.0-pre.3.tgz#0ea646c4ebe21c9b7ffb9ff3d6dff60b1c5a6124" + integrity sha512-vrsyixV225vMe0vGZV0aZjOYez2Pan5MxIx2RqnYnpbbRrUN2lJpQS9ong6dfF5a7BfQenR0LOD6hei3IQIPSw== + dependencies: + axe-core "^4.7.2" + chalk "^5.3.0" + lodash-es "^4.17.21" + vitest@^2.0.0: version "2.0.5" resolved "https://registry.yarnpkg.com/vitest/-/vitest-2.0.5.tgz#2f15a532704a7181528e399cc5b754c7f335fd62" From 0c0be8a8628c3b0d66f4c0a5df419d333ab4de4b Mon Sep 17 00:00:00 2001 From: Robin Date: Fri, 6 Sep 2024 13:15:34 -0400 Subject: [PATCH 04/13] Test InviteModal --- src/Header.test.tsx | 8 +------ src/Modal.tsx | 3 ++- src/room/InviteModal.test.tsx | 44 +++++++++++++++++++++++++++++++++++ src/vitest.setup.ts | 8 +++++++ 4 files changed, 55 insertions(+), 8 deletions(-) create mode 100644 src/room/InviteModal.test.tsx diff --git a/src/Header.test.tsx b/src/Header.test.tsx index aff15a45..57a9d240 100644 --- a/src/Header.test.tsx +++ b/src/Header.test.tsx @@ -14,19 +14,13 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { expect, test, vi } from "vitest"; +import { expect, test } from "vitest"; import { render, screen } from "@testing-library/react"; import { axe } from "vitest-axe"; import { TooltipProvider } from "@vector-im/compound-web"; import { RoomHeaderInfo } from "./Header"; -global.matchMedia = vi.fn().mockReturnValue({ - matches: true, - addEventListener: () => {}, - removeEventListener: () => {}, -}); - test("RoomHeaderInfo is accessible", async () => { const { container } = render( diff --git a/src/Modal.tsx b/src/Modal.tsx index e2d0ba14..6f5c7e6c 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -98,6 +98,7 @@ export const Modal: FC = ({ styles.drawer, { [styles.tabbed]: tabbed }, )} + aria-describedby={undefined} {...rest} >
@@ -120,7 +121,7 @@ export const Modal: FC = ({ - + null; + +test("InviteModal is accessible", async () => { + const user = userEvent.setup(); + const room = { + roomId: "!a:example.org", + name: "Mission Control", + } as unknown as Room; + const onDismiss = vi.fn(); + const { container } = render( + , + { wrapper: BrowserRouter }, + ); + + expect(await axe(container)).toHaveNoViolations(); + await user.click(screen.getByRole("button", { name: "action.copy_link" })); + expect(onDismiss).toBeCalled(); +}); diff --git a/src/vitest.setup.ts b/src/vitest.setup.ts index 80b704cc..aabe4e46 100644 --- a/src/vitest.setup.ts +++ b/src/vitest.setup.ts @@ -37,3 +37,11 @@ Config.initDefault(); posthog.opt_out_capturing(); afterEach(cleanup); + +// Used by a lot of components +window.matchMedia = global.matchMedia = (): MediaQueryList => + ({ + matches: false, + addEventListener: () => {}, + removeEventListener: () => {}, + }) as Partial as MediaQueryList; From ba36cfa2395b5a79026d0616297a0163360d9605 Mon Sep 17 00:00:00 2001 From: Robin Date: Fri, 6 Sep 2024 17:27:08 -0400 Subject: [PATCH 05/13] Test GridTile --- src/state/MediaViewModel.test.ts | 61 ++++++--------------------- src/tile/GridTile.test.tsx | 52 +++++++++++++++++++++++ src/utils/test.ts | 71 ++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 48 deletions(-) create mode 100644 src/tile/GridTile.test.tsx diff --git a/src/state/MediaViewModel.test.ts b/src/state/MediaViewModel.test.ts index f65b7775..c36f9124 100644 --- a/src/state/MediaViewModel.test.ts +++ b/src/state/MediaViewModel.test.ts @@ -14,52 +14,17 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { RoomMember } from "matrix-js-sdk/src/matrix"; import { expect, test, vi } from "vitest"; -import { LocalParticipant, RemoteParticipant } from "livekit-client"; import { - LocalUserMediaViewModel, - RemoteUserMediaViewModel, -} from "./MediaViewModel"; -import { withTestScheduler } from "../utils/test"; + withLocalMedia, + withRemoteMedia, + withTestScheduler, +} from "../utils/test"; -function withLocal(continuation: (vm: LocalUserMediaViewModel) => void): void { - const member = {} as unknown as RoomMember; - const vm = new LocalUserMediaViewModel( - "a", - member, - {} as unknown as LocalParticipant, - true, - ); - try { - continuation(vm); - } finally { - vm.destroy(); - } -} - -function withRemote( - participant: Partial, - continuation: (vm: RemoteUserMediaViewModel) => void, -): void { - const member = {} as unknown as RoomMember; - const vm = new RemoteUserMediaViewModel( - "a", - member, - { setVolume() {}, ...participant } as RemoteParticipant, - true, - ); - try { - continuation(vm); - } finally { - vm.destroy(); - } -} - -test("set a participant's volume", () => { +test("set a participant's volume", async () => { const setVolumeSpy = vi.fn(); - withRemote({ setVolume: setVolumeSpy }, (vm) => + await withRemoteMedia({}, { setVolume: setVolumeSpy }, async (vm) => withTestScheduler(({ expectObservable, schedule }) => { schedule("-a|", { a() { @@ -72,9 +37,9 @@ test("set a participant's volume", () => { ); }); -test("mute and unmute a participant", () => { +test("mute and unmute a participant", async () => { const setVolumeSpy = vi.fn(); - withRemote({ setVolume: setVolumeSpy }, (vm) => + await withRemoteMedia({}, { setVolume: setVolumeSpy }, async (vm) => withTestScheduler(({ expectObservable, schedule }) => { schedule("-abc|", { a() { @@ -99,8 +64,8 @@ test("mute and unmute a participant", () => { ); }); -test("toggle fit/contain for a participant's video", () => { - withRemote({}, (vm) => +test("toggle fit/contain for a participant's video", async () => { + await withRemoteMedia({}, {}, async (vm) => withTestScheduler(({ expectObservable, schedule }) => { schedule("-ab|", { a: () => vm.toggleFitContain(), @@ -115,15 +80,15 @@ test("toggle fit/contain for a participant's video", () => { ); }); -test("local media remembers whether it should always be shown", () => { - withLocal((vm) => +test("local media remembers whether it should always be shown", async () => { + await withLocalMedia(async (vm) => withTestScheduler(({ expectObservable, schedule }) => { schedule("-a|", { a: () => vm.setAlwaysShow(false) }); expectObservable(vm.alwaysShow).toBe("ab", { a: true, b: false }); }), ); // Next local media should start out *not* always shown - withLocal((vm) => + await withLocalMedia(async (vm) => withTestScheduler(({ expectObservable, schedule }) => { schedule("-a|", { a: () => vm.setAlwaysShow(true) }); expectObservable(vm.alwaysShow).toBe("ab", { a: false, b: true }); diff --git a/src/tile/GridTile.test.tsx b/src/tile/GridTile.test.tsx new file mode 100644 index 00000000..0e181dbf --- /dev/null +++ b/src/tile/GridTile.test.tsx @@ -0,0 +1,52 @@ +/* +Copyright 2024 New Vector Ltd + +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 { RemoteTrackPublication } from "livekit-client"; +import { test, expect } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { axe } from "vitest-axe"; + +import { GridTile } from "./GridTile"; +import { withRemoteMedia } from "../utils/test"; + +test("GridTile is accessible", async () => { + await withRemoteMedia( + { + rawDisplayName: "Alice", + getMxcAvatarUrl: () => "mxc://adfsg", + }, + { + setVolume() {}, + getTrackPublication: () => + ({}) as Partial as RemoteTrackPublication, + }, + async (vm) => { + const { container } = render( + {}} + targetWidth={300} + targetHeight={200} + showVideo + showSpeakingIndicators + />, + ); + expect(await axe(container)).toHaveNoViolations(); + // Name should be visible + screen.getByText("Alice"); + }, + ); +}); diff --git a/src/utils/test.ts b/src/utils/test.ts index 43329be0..7a640215 100644 --- a/src/utils/test.ts +++ b/src/utils/test.ts @@ -16,6 +16,13 @@ limitations under the License. import { map } from "rxjs"; import { RunHelpers, TestScheduler } from "rxjs/testing"; import { expect, vi } from "vitest"; +import { RoomMember } from "matrix-js-sdk/src/matrix"; +import { LocalParticipant, RemoteParticipant } from "livekit-client"; + +import { + LocalUserMediaViewModel, + RemoteUserMediaViewModel, +} from "../state/MediaViewModel"; export function withFakeTimers(continuation: () => void): void { vi.useFakeTimers(); @@ -58,3 +65,67 @@ export function withTestScheduler( }), ); } + +export async function withLocalMedia( + continuation: (vm: LocalUserMediaViewModel) => Promise, +): Promise { + const member = {} as unknown as RoomMember; + const vm = new LocalUserMediaViewModel( + "a", + member, + {} as Partial as LocalParticipant, + true, + ); + try { + await continuation(vm); + } finally { + vm.destroy(); + } +} + +export async function withRemoteMedia( + member: Partial, + participant: Partial, + continuation: (vm: RemoteUserMediaViewModel) => Promise, +): Promise { + const vm = new RemoteUserMediaViewModel( + "a", + { + on() { + return this; + }, + off() { + return this; + }, + addListener() { + return this; + }, + removeListener() { + return this; + }, + ...member, + } as RoomMember, + { + setVolume() {}, + on() { + return this; + }, + off() { + return this; + }, + addListener() { + return this; + }, + removeListener() { + return this; + }, + ...participant, + } as RemoteParticipant, + true, + ); + try { + await continuation(vm); + } finally { + vm.destroy(); + } +} From 982bd6d06b396a2fe894daac02539f37404907d6 Mon Sep 17 00:00:00 2001 From: Robin Date: Fri, 6 Sep 2024 18:31:07 -0400 Subject: [PATCH 06/13] Test SpotlightTile --- src/tile/SpotlightTile.test.tsx | 57 +++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 src/tile/SpotlightTile.test.tsx diff --git a/src/tile/SpotlightTile.test.tsx b/src/tile/SpotlightTile.test.tsx new file mode 100644 index 00000000..0303f401 --- /dev/null +++ b/src/tile/SpotlightTile.test.tsx @@ -0,0 +1,57 @@ +/* +Copyright 2024 New Vector Ltd + +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 { RemoteTrackPublication } from "livekit-client"; +import { test, expect } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { axe } from "vitest-axe"; + +import { SpotlightTile } from "./SpotlightTile"; +import { withRemoteMedia } from "../utils/test"; + +global.IntersectionObserver = class MockIntersectionObserver { + public observe(): void {} + public unobserve(): void {} +} as unknown as typeof IntersectionObserver; + +test("SpotlightTile is accessible", async () => { + await withRemoteMedia( + { + rawDisplayName: "Alice", + getMxcAvatarUrl: () => "mxc://adfsg", + }, + { + getTrackPublication: () => + ({}) as Partial as RemoteTrackPublication, + }, + async (vm) => { + const { container } = render( + {}} + showIndicators + />, + ); + expect(await axe(container)).toHaveNoViolations(); + // Name should be visible + screen.getByText("Alice"); + }, + ); +}); From fa36fcd3a2f278eee4411b21dd18fee388952112 Mon Sep 17 00:00:00 2001 From: Robin Date: Tue, 10 Sep 2024 16:23:00 -0400 Subject: [PATCH 07/13] Exclude test utilities from coverage report --- vitest.config.js | 1 + 1 file changed, 1 insertion(+) diff --git a/vitest.config.js b/vitest.config.js index d9790f63..098a0b0c 100644 --- a/vitest.config.js +++ b/vitest.config.js @@ -16,6 +16,7 @@ export default defineConfig((configEnv) => coverage: { reporter: ["html", "json"], include: ["src/"], + exclude: ["src/**/*.{d,test}.{ts,tsx}", "src/utils/test.ts"], }, }, }), From 8872b879d84f552b3b5f4b2ed84db18c4e403192 Mon Sep 17 00:00:00 2001 From: Robin Date: Tue, 10 Sep 2024 16:24:58 -0400 Subject: [PATCH 08/13] Explain why I've added aria-describedby={undefined} --- src/Modal.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Modal.tsx b/src/Modal.tsx index 6f5c7e6c..6e8d1115 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -98,6 +98,8 @@ export const Modal: FC = ({ styles.drawer, { [styles.tabbed]: tabbed }, )} + // Suppress the warning about there being no description; the modal + // has an accessible title aria-describedby={undefined} {...rest} > @@ -121,6 +123,8 @@ export const Modal: FC = ({ + {/* Suppress the warning about there being no description; the modal + has an accessible title */} Date: Tue, 10 Sep 2024 17:35:50 -0400 Subject: [PATCH 09/13] Test SpotlightTile more thoroughly Catching two accessibility issues along the way: we were putting the wrong accessible labels on the 'expand' button, and even the off-screen pages of the spotlight tile were being exposed to accessibility technologies rather than hidden. --- public/locales/en-GB/app.json | 4 +- src/state/MediaViewModel.test.ts | 4 +- src/tile/SpotlightTile.test.tsx | 69 ++++++++++++++++++++++---------- src/tile/SpotlightTile.tsx | 24 +++++++++-- src/utils/test.ts | 68 +++++++++++++++++++++---------- 5 files changed, 118 insertions(+), 51 deletions(-) diff --git a/public/locales/en-GB/app.json b/public/locales/en-GB/app.json index 6581274a..2eb2b5c3 100644 --- a/public/locales/en-GB/app.json +++ b/public/locales/en-GB/app.json @@ -161,8 +161,8 @@ "video_tile": { "always_show": "Always show", "change_fit_contain": "Fit to frame", - "exit_full_screen": "Exit full screen", - "full_screen": "Full screen", + "collapse": "Collapse", + "expand": "Expand", "mute_for_me": "Mute for me", "volume": "Volume" } diff --git a/src/state/MediaViewModel.test.ts b/src/state/MediaViewModel.test.ts index c36f9124..698443dd 100644 --- a/src/state/MediaViewModel.test.ts +++ b/src/state/MediaViewModel.test.ts @@ -81,14 +81,14 @@ test("toggle fit/contain for a participant's video", async () => { }); test("local media remembers whether it should always be shown", async () => { - await withLocalMedia(async (vm) => + await withLocalMedia({}, async (vm) => withTestScheduler(({ expectObservable, schedule }) => { schedule("-a|", { a: () => vm.setAlwaysShow(false) }); expectObservable(vm.alwaysShow).toBe("ab", { a: true, b: false }); }), ); // Next local media should start out *not* always shown - await withLocalMedia(async (vm) => + await withLocalMedia({}, async (vm) => withTestScheduler(({ expectObservable, schedule }) => { schedule("-a|", { a: () => vm.setAlwaysShow(true) }); expectObservable(vm.alwaysShow).toBe("ab", { a: false, b: true }); diff --git a/src/tile/SpotlightTile.test.tsx b/src/tile/SpotlightTile.test.tsx index 0303f401..866db105 100644 --- a/src/tile/SpotlightTile.test.tsx +++ b/src/tile/SpotlightTile.test.tsx @@ -14,13 +14,13 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { RemoteTrackPublication } from "livekit-client"; -import { test, expect } from "vitest"; -import { render, screen } from "@testing-library/react"; +import { test, expect, vi } from "vitest"; +import { isInaccessible, render, screen } from "@testing-library/react"; import { axe } from "vitest-axe"; +import userEvent from "@testing-library/user-event"; import { SpotlightTile } from "./SpotlightTile"; -import { withRemoteMedia } from "../utils/test"; +import { withLocalMedia, withRemoteMedia } from "../utils/test"; global.IntersectionObserver = class MockIntersectionObserver { public observe(): void {} @@ -33,25 +33,50 @@ test("SpotlightTile is accessible", async () => { rawDisplayName: "Alice", getMxcAvatarUrl: () => "mxc://adfsg", }, - { - getTrackPublication: () => - ({}) as Partial as RemoteTrackPublication, - }, - async (vm) => { - const { container } = render( - {}} - showIndicators - />, + {}, + async (vm1) => { + await withLocalMedia( + { + rawDisplayName: "Bob", + getMxcAvatarUrl: () => "mxc://dlskf", + }, + async (vm2) => { + const user = userEvent.setup(); + const toggleExpanded = vi.fn(); + const { container } = render( + , + ); + + expect(await axe(container)).toHaveNoViolations(); + // Alice should be in the spotlight, with her name and avatar on the + // first page + screen.getByText("Alice"); + const aliceAvatar = screen.getByRole("img"); + expect(screen.queryByRole("button", { name: "common.back" })).toBe( + null, + ); + // Bob should be out of the spotlight, and therefore invisible + expect(isInaccessible(screen.getByText("Bob"))).toBe(true); + // Now navigate to Bob + await user.click(screen.getByRole("button", { name: "common.next" })); + screen.getByText("Bob"); + expect(screen.getByRole("img")).not.toBe(aliceAvatar); + expect(isInaccessible(screen.getByText("Alice"))).toBe(true); + // Can toggle whether the tile is expanded + await user.click( + screen.getByRole("button", { name: "video_tile.expand" }), + ); + expect(toggleExpanded).toHaveBeenCalled(); + }, ); - expect(await axe(container)).toHaveNoViolations(); - // Name should be visible - screen.getByText("Alice"); }, ); }); diff --git a/src/tile/SpotlightTile.tsx b/src/tile/SpotlightTile.tsx index f920d01e..5d233cb0 100644 --- a/src/tile/SpotlightTile.tsx +++ b/src/tile/SpotlightTile.tsx @@ -61,6 +61,7 @@ interface SpotlightItemBaseProps { member: RoomMember | undefined; unencryptedWarning: boolean; displayName: string; + "aria-hidden"?: boolean; } interface SpotlightUserMediaItemBaseProps extends SpotlightItemBaseProps { @@ -118,10 +119,21 @@ interface SpotlightItemProps { * Whether this item should act as a scroll snapping point. */ snap: boolean; + "aria-hidden"?: boolean; } const SpotlightItem = forwardRef( - ({ vm, targetWidth, targetHeight, intersectionObserver, snap }, theirRef) => { + ( + { + vm, + targetWidth, + targetHeight, + intersectionObserver, + snap, + "aria-hidden": ariaHidden, + }, + theirRef, + ) => { const ourRef = useRef(null); const ref = useMergedRefs(ourRef, theirRef); const displayName = useDisplayName(vm); @@ -153,6 +165,7 @@ const SpotlightItem = forwardRef( member: vm.member, unencryptedWarning, displayName, + "aria-hidden": ariaHidden, }; return vm instanceof ScreenShareViewModel ? ( @@ -280,7 +293,12 @@ export const SpotlightTile = forwardRef( targetWidth={targetWidth} targetHeight={targetHeight} intersectionObserver={intersectionObserver} + // This is how we get the container to scroll to the right media + // when the previous/next buttons are clicked: we temporarily + // remove all scroll snap points except for just the one media + // that we want to bring into view snap={scrollToId === null || scrollToId === vm.id} + aria-hidden={(scrollToId ?? visibleId) !== vm.id} /> ))}
@@ -288,9 +306,7 @@ export const SpotlightTile = forwardRef(