Fix: Focusing a room in the room list(without hovering) doesn't allow tabbing to the more menu (#34043)

* Fix more menu focus issue when focused with no hover. * Use a js based solution for focus * Return focus to the menu trigger when closing the menu with Escape Keep the keyboard-focus marker set while a row/section-header menu is open (focus is then in the portaled popover, outside the element). This keeps the trigger revealed so the menu's own focus restoration lands on it when closed with Escape, instead of dropping to <body>. * test: cover keyboard-focus reveal of the room list hover menus Adds unit tests that focus a room row / section header via the keyboard (:focus-visible on mount) and assert the hover menu is revealed, then cleared when focus leaves. Brings diff coverage of the focus handlers to 100%.
2026-06-30 11:36:57 +00:00
parent 75982e87e1
commit c33a159334
7 changed files with 130 additions and 8 deletions
@@ -258,6 +258,40 @@ test.describe("Room list", () => {
                await expect(notificationButton).toBeFocused();
            });

+            test("should reveal the options menu when a room is focused with the keyboard", async ({
+                page,
+                app,
+                user,
+            }) => {
+                // Regression test: navigating the room list with the keyboard must reveal a room's hover
+                // menu so the "More options" button is reachable by Tab, rather than focus escaping to
+                // <body>. The reveal must depend on keyboard focus alone, so we move focus with the
+                // keyboard to an adjacent room the pointer is NOT over — otherwise :hover would reveal
+                // the menu and mask the behaviour (which is why the other keyboard tests don't catch it).
+                const roomListView = getRoomList(page);
+                const room29 = roomListView.getByRole("option", { name: "Open room room29" });
+                const room28 = roomListView.getByRole("option", { name: "Open room room28" });
+                const moreButton = room28.getByRole("button", { name: "More options" });
+
+                // Open the room, then put focus back on the room list item.
+                await room29.click();
+                await room29.click();
+                await expect(room29).toBeFocused();
+
+                // Keyboard-focus the adjacent room (the pointer is still over room29, not room28), so the
+                // menu's visibility depends purely on keyboard focus and not on :hover.
+                await page.keyboard.press("ArrowDown");
+                await expect(room28).toBeFocused();
+
+                // The "More options" button must be revealed and reachable by Tab.
+                await page.keyboard.press("Tab");
+                await expect(moreButton).toBeFocused();
+
+                // TODO: once menu-close focus restoration is fixed, extend this to open the menu
+                // (Enter) and assert that Escape returns focus to a room list item rather than <body>.
+                // Today that focus restoration is broken, so it isn't asserted here.
+            });
+
            test("should navigate to the top and then bottom of the room list", async ({ page, app, user }) => {
                const roomListView = getRoomList(page);

@@ -36,7 +36,10 @@

 /* Show hover menu and background on hover/focus/menu-open states */
 .roomListItem:hover,
-.roomListItem:focus-visible,
+/* keyboardActive is toggled in JS for keyboard focus only (see RoomListItemView), and kept while
+   focus is on the hover-menu buttons so they stay reachable by Tab — without revealing the menu on
+   mouse-focused or selected rows the way :focus-within would */
+.roomListItem.keyboardActive,
 /* When the context menu is opened */
 .roomListItem[data-state="open"],
 /* When the options and notifications menu are opened */
@@ -6,7 +6,7 @@
 */

 import React from "react";
-import { render, screen } from "@test-utils";
+import { render, screen, waitFor } from "@test-utils";
 import userEvent from "@testing-library/user-event";
 import { composeStories } from "@storybook/react-vite";
 import { describe, it, expect } from "vitest";
@@ -132,4 +132,19 @@ describe("<RoomListItemView />", () => {
        const { container } = render(<WithoutHoverMenu />);
        expect(container.querySelector('[aria-label="More Options"]')).toBeNull();
    });
+
+    it("reveals the hover menu on keyboard focus and clears it when focus leaves", async () => {
+        // isFocused focuses the row via the keyboard on mount, so the hover menu is revealed.
+        const { container } = render(<WithHoverMenu isFocused={true} />);
+        const option = screen.getByRole("option");
+        const moreButton = container.querySelector('[aria-label="More Options"]');
+
+        expect(option.className).toMatch(/keyboardActive/);
+        expect(moreButton).toBeVisible();
+
+        // Focus leaving the row hides the menu again.
+        option.blur();
+        await waitFor(() => expect(option.className).not.toMatch(/keyboardActive/));
+        expect(moreButton).not.toBeVisible();
+    });
 });
@@ -5,7 +5,7 @@
 * Please see LICENSE files in the repository root for full details.
 */

-import React, { type JSX, memo, useEffect, useRef, type ReactNode, type Ref } from "react";
+import React, { type JSX, memo, useEffect, useRef, useState, type ReactNode, type Ref } from "react";
 import classNames from "classnames";
 import { useMergeRefs } from "react-merge-refs";

@@ -175,12 +175,41 @@ export const RoomListItemView = memo(function RoomListItemView({
    const mergedRef = useMergeRefs([ref, internalRef]);
    const item = useViewModel(vm);

+    // Reveal the hover menu when the row is focused via the keyboard (not the mouse), and keep it
+    // revealed while focus moves onto the menu buttons so they stay reachable by Tab. A pure CSS
+    // :focus-visible rule can't do the latter: it drops the instant focus leaves the row for a child,
+    // hiding the menu mid-Tab and dropping focus to <body>. A :focus-within rule reveals it for mouse
+    // focus too, which clutters selected/clicked rows. So we mark keyboard focus in JS (using the
+    // browser's own :focus-visible determination) and keep it set until focus leaves the row entirely.
+    const [keyboardActive, setKeyboardActive] = useState(false);
+
    useEffect(() => {
        if (isFocused) {
            internalRef.current?.focus({ preventScroll: true } as FocusOptions);
        }
    }, [isFocused]);

+    const onItemFocus = (e: React.FocusEvent<HTMLButtonElement>): void => {
+        onFocus(item.id, e);
+        // Only when focus enters the row from outside via the keyboard.
+        if (!e.currentTarget.contains(e.relatedTarget as Node | null) && e.currentTarget.matches(":focus-visible")) {
+            setKeyboardActive(true);
+        }
+    };
+
+    const onItemBlur = (e: React.FocusEvent<HTMLButtonElement>): void => {
+        // Keep it revealed while focus is on a child menu button, and while one of the menus is open
+        // (focus is then in the portaled popover, outside the row). The latter means that when the
+        // menu closes with Escape, the trigger is still revealed, so the popover's own focus
+        // restoration lands on it instead of dropping to <body>. Clear once focus leaves for good.
+        if (
+            !e.currentTarget.contains(e.relatedTarget as Node | null) &&
+            !e.currentTarget.querySelector('[data-state="open"]')
+        ) {
+            setKeyboardActive(false);
+        }
+    };
+
    // Generate a11y label from notification state and room name
    const a11yLabel = getA11yLabel(item.name, item.notification);

@@ -190,6 +219,7 @@ export const RoomListItemView = memo(function RoomListItemView({
                as="button"
                ref={mergedRef}
                className={classNames(styles.roomListItem, "mx_RoomListItemView", {
+                    [styles.keyboardActive]: keyboardActive,
                    [styles.selected]: isSelected,
                    [styles.bold]: item.isBold,
                    [styles.firstItem]: isFirstItem,
@@ -202,7 +232,8 @@ export const RoomListItemView = memo(function RoomListItemView({
                type="button"
                aria-label={a11yLabel}
                onClick={vm.onOpenRoom}
-                onFocus={(e: React.FocusEvent<HTMLButtonElement>) => onFocus(item.id, e)}
+                onFocus={onItemFocus}
+                onBlur={onItemBlur}
                tabIndex={isFocused ? 0 : -1}
                aria-selected={props.role === "option" ? isSelected : undefined}
                {...props}
@@ -19,7 +19,10 @@
    padding: var(--cpd-space-1x) 0;

    &:hover,
-    &:focus-visible,
+    /* keyboardActive is toggled in JS for keyboard focus only (see RoomListSectionHeaderView), and
+       kept while focus is on the menu button so it stays reachable by Tab — without revealing the
+       menu on a mouse-focused header the way :focus-within would */
+    &.keyboardActive,
    &:has(button[data-state="open"]) {
        color: var(--cpd-color-text-primary);

@@ -6,7 +6,7 @@
 */

 import React from "react";
-import { render, screen } from "@test-utils";
+import { render, screen, waitFor } from "@test-utils";
 import { composeStories } from "@storybook/react-vite";
 import { describe, it, expect, type Mock, afterEach, vi } from "vitest";
 import userEvent from "@testing-library/user-event";
@@ -44,6 +44,17 @@ describe("<RoomListSectionHeaderView /> stories", () => {
        expect(document.activeElement).toBe(button);
    });

+    it("reveals the section menu on keyboard focus and clears it when focus leaves", async () => {
+        // isFocused focuses the header via the keyboard on mount, so the menu is revealed.
+        render(<Default isFocused={true} />);
+        const button = screen.getByRole("button", { name: HEADER_NAME });
+        expect(button.className).toMatch(/keyboardActive/);
+
+        // Focus leaving the header hides the menu again.
+        button.blur();
+        await waitFor(() => expect(button.className).not.toMatch(/keyboardActive/));
+    });
+
    it("expands a collapsed section on ArrowRight", async () => {
        const user = userEvent.setup();
        render(<Collapsed isFocused={true} />);
@@ -5,7 +5,7 @@
 * Please see LICENSE files in the repository root for full details.
 */

-import React, { memo, type JSX, type FocusEvent, useEffect, useRef } from "react";
+import React, { memo, type JSX, type FocusEvent, useEffect, useRef, useState } from "react";
 import classNames from "classnames";
 import { useDraggable, useDragOperation, useDroppable } from "@dnd-kit/react";
 import { useMergeRefs } from "react-merge-refs";
@@ -168,6 +168,29 @@ export const RoomListSectionHeaderView = memo(function RoomListSectionHeaderView
        }
    }, [isFocused]);

+    // Reveal the menu only on keyboard focus (not mouse), kept while focus is on the menu button so
+    // it stays reachable by Tab. See RoomListItemView for the full rationale.
+    const [keyboardActive, setKeyboardActive] = useState(false);
+
+    const onHeaderFocus = (e: React.FocusEvent<HTMLButtonElement>): void => {
+        onFocus(id, e);
+        if (!e.currentTarget.contains(e.relatedTarget as Node | null) && e.currentTarget.matches(":focus-visible")) {
+            setKeyboardActive(true);
+        }
+    };
+
+    const onHeaderBlur = (e: React.FocusEvent<HTMLButtonElement>): void => {
+        // Keep it revealed while focus is on the menu button, and while the menu is open (focus is
+        // then in the portaled popover, outside the header). That way closing with Escape restores
+        // focus to the still-revealed trigger instead of dropping to <body>. Clear once focus leaves.
+        if (
+            !e.currentTarget.contains(e.relatedTarget as Node | null) &&
+            !e.currentTarget.querySelector('[data-state="open"]')
+        ) {
+            setKeyboardActive(false);
+        }
+    };
+
    return (
        <div
            aria-expanded={ariaExpanded}
@@ -178,6 +201,7 @@ export const RoomListSectionHeaderView = memo(function RoomListSectionHeaderView
                    ref={buttonRef}
                    type="button"
                    className={classNames(styles.header, {
+                        [styles.keyboardActive]: keyboardActive,
                        [styles.firstHeader]: sectionIndex === 0,
                        // If the section is collapsed and it's the last one
                        [styles.lastHeader]: !isExpanded && isLastSection,
@@ -207,7 +231,8 @@ export const RoomListSectionHeaderView = memo(function RoomListSectionHeaderView
                        }
                    }}
                    aria-expanded={ariaExpanded}
-                    onFocus={(e) => onFocus(id, e)}
+                    onFocus={onHeaderFocus}
+                    onBlur={onHeaderBlur}
                    tabIndex={isFocused ? 0 : -1}
                    aria-label={
                        isUnread