From c31444bfdaca5602dcb0a4ee4d905165eb30bb19 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Fri, 26 Sep 2025 11:45:06 +0200 Subject: [PATCH] Fix a11y issue on list in invite dialog (#30878) * fix: focus decoration when tabbing on rich item * feat: add `useListKeyDown` hook * fix: improve keyboard navigation on `RichList` and `RichItem` --- .../hooks/useListKeyDown.test.ts | 140 ++++++++++++++++++ src/shared-components/hooks/useListKeyDown.ts | 64 ++++++++ .../rich-list/RichItem/RichItem.module.css | 8 +- .../rich-list/RichItem/RichItem.tsx | 8 +- .../__snapshots__/RichItem.test.tsx.snap | 18 +-- .../rich-list/RichList/RichList.tsx | 17 ++- .../__snapshots__/RichList.test.tsx.snap | 35 +++-- 7 files changed, 256 insertions(+), 34 deletions(-) create mode 100644 src/shared-components/hooks/useListKeyDown.test.ts create mode 100644 src/shared-components/hooks/useListKeyDown.ts diff --git a/src/shared-components/hooks/useListKeyDown.test.ts b/src/shared-components/hooks/useListKeyDown.test.ts new file mode 100644 index 0000000000..31d93aef3b --- /dev/null +++ b/src/shared-components/hooks/useListKeyDown.test.ts @@ -0,0 +1,140 @@ +/* + * 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 { type KeyboardEvent } from "react"; +import { renderHook } from "jest-matrix-react"; + +import { useListKeyDown } from "./useListKeyDown"; + +describe("useListKeyDown", () => { + let mockList: HTMLUListElement; + let mockItems: HTMLElement[]; + let mockEvent: Partial>; + + beforeEach(() => { + // Create mock DOM elements + mockList = document.createElement("ul"); + mockItems = [document.createElement("li"), document.createElement("li"), document.createElement("li")]; + + // Set up the DOM structure + mockItems.forEach((item, index) => { + item.setAttribute("tabindex", "0"); + item.setAttribute("data-testid", `item-${index}`); + mockList.appendChild(item); + }); + + document.body.appendChild(mockList); + + // Mock event object + mockEvent = { + preventDefault: jest.fn(), + key: "", + }; + + // Mock focus methods + mockItems.forEach((item) => { + item.focus = jest.fn(); + item.click = jest.fn(); + }); + }); + + afterEach(() => { + document.body.removeChild(mockList); + jest.clearAllMocks(); + }); + + function render(): { + current: { + listRef: React.RefObject; + onKeyDown: React.KeyboardEventHandler; + }; + } { + const { result } = renderHook(() => useListKeyDown()); + result.current.listRef.current = mockList; + return result; + } + + it.each([ + ["Enter", "Enter"], + ["Space", " "], + ])("should handle %s key to click active element", (name, key) => { + const result = render(); + + // Mock document.activeElement + Object.defineProperty(document, "activeElement", { + value: mockItems[1], + configurable: true, + }); + + // Simulate key press + result.current.onKeyDown({ + ...mockEvent, + key, + } as KeyboardEvent); + + expect(mockItems[1].click).toHaveBeenCalledTimes(1); + expect(mockEvent.preventDefault).toHaveBeenCalledTimes(1); + }); + + it.each( + // key, finalPosition, startPosition + [ + ["ArrowDown", 1, 0], + ["ArrowUp", 1, 2], + ["Home", 0, 1], + ["End", 2, 1], + ], + )("should handle %s to focus the %inth element", (key, finalPosition, startPosition) => { + const result = render(); + mockList.contains = jest.fn().mockReturnValue(true); + + Object.defineProperty(document, "activeElement", { + value: mockItems[startPosition], + configurable: true, + }); + + result.current.onKeyDown({ + ...mockEvent, + key, + } as KeyboardEvent); + + expect(mockItems[finalPosition].focus).toHaveBeenCalledTimes(1); + expect(mockEvent.preventDefault).toHaveBeenCalledTimes(1); + }); + + it.each([["ArrowDown"], ["ArrowUp"]])("should not handle %s when active element is not in list", (key) => { + const result = render(); + mockList.contains = jest.fn().mockReturnValue(false); + + const outsideElement = document.createElement("button"); + + Object.defineProperty(document, "activeElement", { + value: outsideElement, + configurable: true, + }); + + result.current.onKeyDown({ + ...mockEvent, + key, + } as KeyboardEvent); + + // No item should be focused + mockItems.forEach((item) => expect(item.focus).not.toHaveBeenCalled()); + expect(mockEvent.preventDefault).toHaveBeenCalledTimes(1); + }); + + it("should not prevent default for unhandled keys", () => { + const result = render(); + + result.current.onKeyDown({ + ...mockEvent, + key: "Tab", + } as KeyboardEvent); + + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + }); +}); diff --git a/src/shared-components/hooks/useListKeyDown.ts b/src/shared-components/hooks/useListKeyDown.ts new file mode 100644 index 0000000000..9948511b9f --- /dev/null +++ b/src/shared-components/hooks/useListKeyDown.ts @@ -0,0 +1,64 @@ +/* + * 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 { useCallback, useRef, type RefObject, type KeyboardEvent, type KeyboardEventHandler } from "react"; + +/** + * A hook that provides keyboard navigation for a list of options. + */ +export function useListKeyDown(): { + listRef: RefObject; + onKeyDown: KeyboardEventHandler; +} { + const listRef = useRef(null); + + const onKeyDown = useCallback((evt: KeyboardEvent) => { + const { key } = evt; + + let handled = false; + + switch (key) { + case "Enter": + case " ": { + handled = true; + (document.activeElement as HTMLElement).click(); + break; + } + case "ArrowDown": { + handled = true; + const currentFocus = document.activeElement; + if (listRef.current?.contains(currentFocus) && currentFocus) { + (currentFocus.nextElementSibling as HTMLElement)?.focus(); + } + break; + } + case "ArrowUp": { + handled = true; + const currentFocus = document.activeElement; + if (listRef.current?.contains(currentFocus) && currentFocus) { + (currentFocus.previousElementSibling as HTMLElement)?.focus(); + } + break; + } + case "Home": { + handled = true; + (listRef.current?.firstElementChild as HTMLElement)?.focus(); + break; + } + case "End": { + handled = true; + (listRef.current?.lastElementChild as HTMLElement)?.focus(); + break; + } + } + + if (handled) { + evt.preventDefault(); + } + }, []); + return { listRef, onKeyDown }; +} diff --git a/src/shared-components/rich-list/RichItem/RichItem.module.css b/src/shared-components/rich-list/RichItem/RichItem.module.css index a00e0cf9cb..d1028f33e4 100644 --- a/src/shared-components/rich-list/RichItem/RichItem.module.css +++ b/src/shared-components/rich-list/RichItem/RichItem.module.css @@ -6,11 +6,14 @@ */ .richItem { - all: unset; + /* Remove browser button style */ + background: transparent; + border: none; padding: var(--cpd-space-2x) var(--cpd-space-4x) var(--cpd-space-2x) var(--cpd-space-4x); width: 100%; box-sizing: border-box; cursor: pointer; + text-align: start; display: grid; column-gap: var(--cpd-space-3x); @@ -20,7 +23,8 @@ "avatar description time"; } -.richItem:hover { +.richItem:hover, +.richItem:focus { background-color: var(--cpd-color-bg-subtle-secondary); border-radius: 12px; } diff --git a/src/shared-components/rich-list/RichItem/RichItem.tsx b/src/shared-components/rich-list/RichItem/RichItem.tsx index 0ff574e91e..2d7a0ae57f 100644 --- a/src/shared-components/rich-list/RichItem/RichItem.tsx +++ b/src/shared-components/rich-list/RichItem/RichItem.tsx @@ -12,7 +12,7 @@ import styles from "./RichItem.module.css"; import { humanizeTime } from "../../utils/humanize"; import { Flex } from "../../utils/Flex"; -export interface RichItemProps extends HTMLAttributes { +export interface RichItemProps extends HTMLAttributes { /** * Avatar to display at the start of the item */ @@ -64,10 +64,10 @@ export const RichItem = memo(function RichItem({ ...props }: RichItemProps): JSX.Element { return ( - + ); }); diff --git a/src/shared-components/rich-list/RichItem/__snapshots__/RichItem.test.tsx.snap b/src/shared-components/rich-list/RichItem/__snapshots__/RichItem.test.tsx.snap index 7a64249990..fced60db2a 100644 --- a/src/shared-components/rich-list/RichItem/__snapshots__/RichItem.test.tsx.snap +++ b/src/shared-components/rich-list/RichItem/__snapshots__/RichItem.test.tsx.snap @@ -6,11 +6,11 @@ exports[`RichItem renders the item in default state 1`] = ` role="listbox" style="all: unset; list-style: none;" > - + `; @@ -47,12 +47,12 @@ exports[`RichItem renders the item in selected state 1`] = ` role="listbox" style="all: unset; list-style: none;" > - + `; @@ -99,11 +99,11 @@ exports[`RichItem renders the item without timestamp 1`] = ` role="listbox" style="all: unset; list-style: none;" > - + `; diff --git a/src/shared-components/rich-list/RichList/RichList.tsx b/src/shared-components/rich-list/RichList/RichList.tsx index a2859f19df..6e34ab2287 100644 --- a/src/shared-components/rich-list/RichList/RichList.tsx +++ b/src/shared-components/rich-list/RichList/RichList.tsx @@ -5,11 +5,12 @@ * Please see LICENSE files in the repository root for full details. */ -import React, { type HTMLProps, type JSX, type PropsWithChildren } from "react"; +import React, { type HTMLProps, type JSX, type PropsWithChildren, useId } from "react"; import classNames from "classnames"; import styles from "./RichList.module.css"; import { Flex } from "../../utils/Flex"; +import { useListKeyDown } from "../../hooks/useListKeyDown"; export interface RichListProps extends HTMLProps { /** @@ -51,15 +52,25 @@ export function RichList({ isEmpty = false, ...props }: PropsWithChildren): JSX.Element { + const id = useId(); + const { listRef, onKeyDown } = useListKeyDown(); + return ( - + {title} {isEmpty ? ( {children} ) : ( -
    +
      {children}
    )} diff --git a/src/shared-components/rich-list/RichList/__snapshots__/RichList.test.tsx.snap b/src/shared-components/rich-list/RichList/__snapshots__/RichList.test.tsx.snap index 529652c080..34cd1af3fb 100644 --- a/src/shared-components/rich-list/RichList/__snapshots__/RichList.test.tsx.snap +++ b/src/shared-components/rich-list/RichList/__snapshots__/RichList.test.tsx.snap @@ -11,19 +11,21 @@ exports[`RichItem renders the list 1`] = ` > Rich List Title
      - - - - - +
    @@ -172,6 +174,7 @@ exports[`RichItem renders the list with isEmpty=true 1`] = ` > Rich List Title