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`
2025-09-26 11:45:06 +02:00
parent 88d4f369eb
commit c31444bfda
7 changed files with 256 additions and 34 deletions
--- a/src/shared-components/hooks/useListKeyDown.test.ts
+++ 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<KeyboardEvent<HTMLUListElement>>;
+
+    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<HTMLUListElement | null>;
+            onKeyDown: React.KeyboardEventHandler<HTMLUListElement>;
+        };
+    } {
+        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<HTMLUListElement>);
+
+        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<HTMLUListElement>);
+
+        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<HTMLUListElement>);
+
+        // 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<HTMLUListElement>);
+
+        expect(mockEvent.preventDefault).not.toHaveBeenCalled();
+    });
+});
--- a/src/shared-components/hooks/useListKeyDown.ts
+++ 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<HTMLUListElement | null>;
+    onKeyDown: KeyboardEventHandler<HTMLUListElement>;
+} {
+    const listRef = useRef<HTMLUListElement>(null);
+
+    const onKeyDown = useCallback((evt: KeyboardEvent<HTMLUListElement>) => {
+        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 };
+}
--- 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;
 }
--- 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<HTMLButtonElement> {
+export interface RichItemProps extends HTMLAttributes<HTMLLIElement> {
    /**
     * Avatar to display at the start of the item
     */
@@ -64,10 +64,10 @@ export const RichItem = memo(function RichItem({
    ...props
 }: RichItemProps): JSX.Element {
    return (
-        <button
+        <li
            className={styles.richItem}
-            type="button"
            role="option"
+            tabIndex={0}
            aria-selected={selected}
            aria-label={title}
            {...props}
@@ -80,7 +80,7 @@ export const RichItem = memo(function RichItem({
                    {humanizeTime(timestamp)}
                </span>
            )}
-        </button>
+        </li>
    );
 });

--- 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;"
  >
-    <button
+    <li
      aria-label="Rich Item Title"
      class="richItem"
      role="option"
-      type="button"
+      tabindex="0"
    >
      <div
        class="flex avatar"
@@ -36,7 +36,7 @@ exports[`RichItem renders the item in default state 1`] = `
      >
        145 days ago
      </span>
-    </button>
+    </li>
  </ul>
 </div>
 `;
@@ -47,12 +47,12 @@ exports[`RichItem renders the item in selected state 1`] = `
    role="listbox"
    style="all: unset; list-style: none;"
  >
-    <button
+    <li
      aria-label="Rich Item Title"
      aria-selected="true"
      class="richItem"
      role="option"
-      type="button"
+      tabindex="0"
    >
      <div
        aria-hidden="true"
@@ -88,7 +88,7 @@ exports[`RichItem renders the item in selected state 1`] = `
      >
        145 days ago
      </span>
-    </button>
+    </li>
  </ul>
 </div>
 `;
@@ -99,11 +99,11 @@ exports[`RichItem renders the item without timestamp 1`] = `
    role="listbox"
    style="all: unset; list-style: none;"
  >
-    <button
+    <li
      aria-label="Rich Item Title"
      class="richItem"
      role="option"
-      type="button"
+      tabindex="0"
    >
      <div
        class="flex avatar"
@@ -123,7 +123,7 @@ exports[`RichItem renders the item without timestamp 1`] = `
      >
        This is a description of the rich item.
      </span>
-    </button>
+    </li>
  </ul>
 </div>
 `;
--- 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<HTMLDivElement> {
    /**
@@ -51,15 +52,25 @@ export function RichList({
    isEmpty = false,
    ...props
 }: PropsWithChildren<RichListProps>): JSX.Element {
+    const id = useId();
+    const { listRef, onKeyDown } = useListKeyDown();
+
    return (
        <Flex className={classNames(styles.richList, className)} direction="column" {...props}>
-            <span className={styles.title} {...titleAttributes}>
+            <span id={id} className={styles.title} {...titleAttributes}>
                {title}
            </span>
            {isEmpty ? (
                <span className={styles.empty}>{children}</span>
            ) : (
-                <ul role="listbox" className={styles.content} aria-label={title}>
+                <ul
+                    ref={listRef}
+                    role="listbox"
+                    className={styles.content}
+                    aria-labelledby={id}
+                    tabIndex={0}
+                    onKeyDown={onKeyDown}
+                >
                    {children}
                </ul>
            )}
--- 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`] = `
    >
      <span
        class="title"
+        id="«r0»"
      >
        Rich List Title
      </span>
      <ul
-        aria-label="Rich List Title"
+        aria-labelledby="«r0»"
        class="content"
        role="listbox"
+        tabindex="0"
      >
-        <button
+        <li
          aria-label="First Item"
          class="richItem"
          role="option"
-          type="button"
+          tabindex="0"
        >
          <div
            class="flex avatar"
@@ -43,13 +45,13 @@ exports[`RichItem renders the list 1`] = `
          >
            description
          </span>
-        </button>
-        <button
+        </li>
+        <li
          aria-label="Second Item"
          aria-selected="true"
          class="richItem"
          role="option"
-          type="button"
+          tabindex="0"
        >
          <div
            aria-hidden="true"
@@ -79,12 +81,12 @@ exports[`RichItem renders the list 1`] = `
          >
            description
          </span>
-        </button>
-        <button
+        </li>
+        <li
          aria-label="Third Item"
          class="richItem"
          role="option"
-          type="button"
+          tabindex="0"
        >
          <div
            class="flex avatar"
@@ -104,12 +106,12 @@ exports[`RichItem renders the list 1`] = `
          >
            description
          </span>
-        </button>
-        <button
+        </li>
+        <li
          aria-label="Fourth Item"
          class="richItem"
          role="option"
-          type="button"
+          tabindex="0"
        >
          <div
            class="flex avatar"
@@ -129,12 +131,12 @@ exports[`RichItem renders the list 1`] = `
          >
            description
          </span>
-        </button>
-        <button
+        </li>
+        <li
          aria-label="Fifth Item"
          class="richItem"
          role="option"
-          type="button"
+          tabindex="0"
        >
          <div
            class="flex avatar"
@@ -154,7 +156,7 @@ exports[`RichItem renders the list 1`] = `
          >
            description
          </span>
-        </button>
+        </li>
      </ul>
    </div>
  </div>
@@ -172,6 +174,7 @@ exports[`RichItem renders the list with isEmpty=true 1`] = `
    >
      <span
        class="title"
+        id="«r1»"
      >
        Rich List Title
      </span>