From c33a159334237a92a00a46d5b9b9eb687fa58e21 Mon Sep 17 00:00:00 2001 From: David Langley Date: Tue, 30 Jun 2026 12:36:57 +0100 Subject: [PATCH] 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 . * 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%. --- .../room-list-panel/room-list.spec.ts | 34 ++++++++++++++++++ .../RoomListItemView.module.css | 5 ++- .../RoomListItemView.test.tsx | 17 ++++++++- .../RoomListItemView/RoomListItemView.tsx | 35 +++++++++++++++++-- .../RoomListSectionHeaderView.module.css | 5 ++- .../RoomListSectionHeaderView.test.tsx | 13 ++++++- .../RoomListSectionHeaderView.tsx | 29 +++++++++++++-- 7 files changed, 130 insertions(+), 8 deletions(-) diff --git a/apps/web/playwright/e2e/left-panel/room-list-panel/room-list.spec.ts b/apps/web/playwright/e2e/left-panel/room-list-panel/room-list.spec.ts index 82e6692ea8..70d11b5f24 100644 --- a/apps/web/playwright/e2e/left-panel/room-list-panel/room-list.spec.ts +++ b/apps/web/playwright/e2e/left-panel/room-list-panel/room-list.spec.ts @@ -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 + // . 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 . + // 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); diff --git a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.module.css b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.module.css index 626d60a953..4359634380 100644 --- a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.module.css +++ b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.module.css @@ -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 */ diff --git a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.test.tsx b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.test.tsx index 646e114de3..db239a9c4c 100644 --- a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.test.tsx +++ b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.test.tsx @@ -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("", () => { const { container } = render(); 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(); + 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(); + }); }); diff --git a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.tsx b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.tsx index b28662070e..fbfa2f9e28 100644 --- a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.tsx +++ b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListItemWrapper/RoomListItemView/RoomListItemView.tsx @@ -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 . 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): 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): 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 . 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) => onFocus(item.id, e)} + onFocus={onItemFocus} + onBlur={onItemBlur} tabIndex={isFocused ? 0 : -1} aria-selected={props.role === "option" ? isSelected : undefined} {...props} diff --git a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.module.css b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.module.css index 348e986ae5..147d64f5c0 100644 --- a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.module.css +++ b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.module.css @@ -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); diff --git a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.test.tsx b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.test.tsx index 7438a0648d..7552c72e0b 100644 --- a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.test.tsx +++ b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.test.tsx @@ -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(" 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(); + 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(); diff --git a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.tsx b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.tsx index 9010aa889c..cb4765da3a 100644 --- a/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.tsx +++ b/packages/shared-components/src/room-list/VirtualizedRoomListView/RoomListSectionHeaderView/RoomListSectionHeaderView.tsx @@ -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): 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): 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 . Clear once focus leaves. + if ( + !e.currentTarget.contains(e.relatedTarget as Node | null) && + !e.currentTarget.querySelector('[data-state="open"]') + ) { + setKeyboardActive(false); + } + }; + return (
onFocus(id, e)} + onFocus={onHeaderFocus} + onBlur={onHeaderBlur} tabIndex={isFocused ? 0 : -1} aria-label={ isUnread