From d6985e005327951fe6bf8075d60af7aecfe5749c Mon Sep 17 00:00:00 2001 From: Robin Date: Tue, 10 Sep 2024 17:35:50 -0400 Subject: [PATCH] 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(