From 08026493750580efc60ba0f2bdf82bfb5419eea3 Mon Sep 17 00:00:00 2001 From: fkwp Date: Wed, 16 Sep 2026 17:44:55 +0200 Subject: [PATCH] Show the focus ring for the keyboard alone, and fix the radio styling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Show the focus ring only when the keyboard moved the focus. Radix focuses whatever the pointer is over, so the browser cannot answer that question here: Chromium treats every focus after a key press as keyboard-driven, Firefox treats no programmatic focus as keyboard-driven. The menu records which modality arrived, watched at the document while it is open, since the first arrow key lands on the menu itself rather than on anything we render. - Suppress the browser's own ring on menu items, so there is one answer to that question rather than two. - Drop readOnly from the device radios. Compound paints a read-only control muted, and that rule comes after the checked rule, so it overrode the accent fill marking the selection and the menu stopped matching settings. - Make the decorative radio inert rather than aria-hidden. A negative tabindex inside an interactive control stays reachable to assistive technology even when hidden, which axe rejects. - Give each section a labelled group and mark its heading decorative. A menu may contain only items, separators and groups, and the headings were direct children of it. The list wrappers are role="none" for the same reason. - Run axe over the open menu, which is what found both of the above. Spec: FEATURES_SPEC/2026-09_Quick_Audio_Menu.md — AC23, AC24, AC25 --- .../MediaMuteAndSwitchButton.module.css | 14 +++ .../MediaMuteAndSwitchButton.test.tsx | 93 ++++++++++++++ src/components/MediaMuteAndSwitchButton.tsx | 113 +++++++++++++----- src/room/LobbyView.test.tsx | 78 +++++++++++- 4 files changed, 262 insertions(+), 36 deletions(-) diff --git a/src/components/MediaMuteAndSwitchButton.module.css b/src/components/MediaMuteAndSwitchButton.module.css index 95b35318a..5fd10d5cd 100644 --- a/src/components/MediaMuteAndSwitchButton.module.css +++ b/src/components/MediaMuteAndSwitchButton.module.css @@ -83,3 +83,17 @@ Please see LICENSE in the repository root for full details. margin-inline: var(--cpd-border-width-1); margin-block-end: var(--cpd-border-width-1); } + +/* Radix focuses whatever the pointer is over, so the browser's own ring marks + the item under the mouse. It cannot distinguish the two modalities here, so + it is suppressed and replaced by one that can. */ +.deviceList [role="menuitemradio"]:focus, +.deviceList [role="menuitemradio"]:focus-visible { + outline: none; +} + +/* Shown only when the keyboard is what moved the focus. */ +.deviceList[data-focus-modality="keyboard"] [role="menuitemradio"]:focus { + outline: var(--cpd-border-width-2) solid var(--cpd-color-border-focused); + outline-offset: calc(-1 * var(--cpd-border-width-2)); +} diff --git a/src/components/MediaMuteAndSwitchButton.test.tsx b/src/components/MediaMuteAndSwitchButton.test.tsx index 77219b85a..f2a55d48e 100644 --- a/src/components/MediaMuteAndSwitchButton.test.tsx +++ b/src/components/MediaMuteAndSwitchButton.test.tsx @@ -6,6 +6,7 @@ Please see LICENSE in the repository root for full details. */ import { describe, expect, test, vi } from "vitest"; +import { axe } from "vitest-axe"; import { act, render, screen, type RenderResult } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { type JSX, useState, type ReactNode } from "react"; @@ -433,6 +434,98 @@ describe("MediaMuteAndSwitchButton", () => { ).toBeInTheDocument(); }); + test("marks focus as keyboard-driven only when the keyboard moved it", async () => { + const user = userEvent.setup(); + const { getByRole } = renderComponent( + , + ); + + await user.click(getByRole("button", { name: "Microphone" })); + const list = screen + .getByRole("menuitemradio", { name: "Microphone 1" }) + .closest("[data-focus-modality]"); + + // The menu focuses whatever the pointer is over, so focus alone says + // nothing about how someone is navigating. + expect(list).toHaveAttribute("data-focus-modality", "pointer"); + + await user.keyboard("{ArrowDown}"); + expect(list).toHaveAttribute("data-focus-modality", "keyboard"); + + await user.pointer({ + target: screen.getByRole("menuitemradio", { name: "Microphone 2" }), + coords: { clientX: 10, clientY: 10 }, + }); + expect(list).toHaveAttribute("data-focus-modality", "pointer"); + }); + + test("marks the selected device with the accent fill", async () => { + const user = userEvent.setup(); + const { getByRole } = renderComponent( + , + ); + + await user.click(getByRole("button", { name: "Microphone" })); + + const selected = screen + .getByRole("menuitemradio", { name: "Microphone 2" }) + .querySelector("input[type=radio]"); + expect(selected).toBeChecked(); + // A read-only control is painted muted, which loses the accent fill that + // marks the selection and makes the menu differ from settings. + expect(selected).not.toHaveAttribute("readonly"); + }); + + test("the open menu has no accessibility violations", async () => { + const user = userEvent.setup(); + const { getByRole } = renderComponent( + , + ); + + await user.click(getByRole("button", { name: "Microphone" })); + + // Includes the menu's own structure: wrappers between the menu and its + // items break the relationship the roles describe. + const menu = document.querySelector('[role="menu"]'); + expect(await axe(menu as HTMLElement)).toHaveNoViolations(); + }); + test("lists speaker and microphone sections", async () => { const user = userEvent.setup(); const { getByRole } = renderComponent( diff --git a/src/components/MediaMuteAndSwitchButton.tsx b/src/components/MediaMuteAndSwitchButton.tsx index 6c44bb931..857f57271 100644 --- a/src/components/MediaMuteAndSwitchButton.tsx +++ b/src/components/MediaMuteAndSwitchButton.tsx @@ -117,8 +117,33 @@ export const MediaMuteAndSwitchButton: FC = ({ // can size it against the call. Measure the call area rather than the window, // or the menu is wrong wherever Element Call is not the whole page. Measured // when the menu opens: it is short-lived enough not to need watching. + // Radix focuses whatever the pointer is over, so the browser's own + // :focus-visible cannot tell us whether a person is navigating by keyboard: + // Chromium answers yes to everything after any key press, Firefox answers no + // to programmatic focus. Track it ourselves and let the styling follow. + const [focusModality, setFocusModality] = useState<"keyboard" | "pointer">( + "pointer", + ); + useEffect(() => { + if (!menuOpen) return; + // Watched at the document, and only while the menu is open. Which modality + // someone is using is not a property of any one element: the first arrow + // key arrives while the menu itself holds focus, above anything we render, + // and Radix moves focus around as the pointer travels. + const usedKeyboard = (): void => setFocusModality("keyboard"); + const usedPointer = (): void => setFocusModality("pointer"); + document.addEventListener("keydown", usedKeyboard, true); + document.addEventListener("pointermove", usedPointer, true); + return (): void => { + document.removeEventListener("keydown", usedKeyboard, true); + document.removeEventListener("pointermove", usedPointer, true); + }; + }, [menuOpen]); const rootElement = useRootElement(); const [listMaxHeight, setListMaxHeight] = useState(); + useEffect(() => { + if (menuOpen) setFocusModality("pointer"); + }, [menuOpen]); useEffect(() => { if (menuOpen) setListMaxHeight( @@ -247,15 +272,20 @@ export const MediaMuteAndSwitchButton: FC = ({ disabled={disabled} label={labelText(label, numbered)} Icon={ - + // Inert, not aria-hidden: a form control inside a menu item must be + // out of the focus order and out of the accessibility tree, and + // aria-hidden alone leaves it focusable. The item's aria-checked is + // what conveys the selection. + + {}} + /> + } onSelect={(e) => { e.preventDefault(); @@ -314,7 +344,11 @@ export const MediaMuteAndSwitchButton: FC = ({ } >
= ({ > {iconsAndLabels === "audio" && outputOptions && ( <> - - {deviceItems( - "output", - outputOptions, - selectedOutputOption, - onSelectOutput, - (n) => t("settings.devices.speaker_numbered", { n }), - )} + {/* A menu may only contain items, separators and groups, so each + heading belongs to a group rather than sitting beside the + items it names. */} +
+ {/* The heading is decoration: the group carries the name, and + a menu may only contain items, separators and groups. */} +
+ +
+ {deviceItems( + "output", + outputOptions, + selectedOutputOption, + onSelectOutput, + (n) => t("settings.devices.speaker_numbered", { n }), + )} +
)} - - {/* The heading sits outside, so the meter can never ride up over it: +
+
+ +
+ {/* The heading sits outside, so the meter can never ride up over it: sticky only holds while this block is in view. */} -
- {deviceItems( - "input", - options, - selectedOption, - onSelect, - numberedLabel, - )} - {iconsAndLabels === "audio" && ( - - )} +
+ {deviceItems( + "input", + options, + selectedOption, + onSelect, + numberedLabel, + )} + {iconsAndLabels === "audio" && ( + + )} +
{(toggles?.length ?? 0) > 0 &&
} diff --git a/src/room/LobbyView.test.tsx b/src/room/LobbyView.test.tsx index 7f03f2d29..a75dc1c79 100644 --- a/src/room/LobbyView.test.tsx +++ b/src/room/LobbyView.test.tsx @@ -5,8 +5,10 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { describe, expect, it, vi } from "vitest"; -import { render } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { of } from "rxjs"; import { LeaveToHomeProvider } from "../LeaveToHomeContext"; import { TooltipProvider } from "@vector-im/compound-web"; import { type MatrixClient } from "matrix-js-sdk"; @@ -19,6 +21,7 @@ import { import { LobbyView } from "./LobbyView"; import { E2eeType } from "../e2ee/e2eeType"; import { mockMediaDevices, mockMuteStates } from "../utils/test"; +import { type MediaDevices } from "../state/MediaDevices"; import { MediaDevicesContext } from "../MediaDevicesContext"; import { type ProcessorState } from "../livekit/TrackProcessorContext"; import { type EncryptionSystem } from "../e2ee/sharedKeyManagement"; @@ -77,9 +80,10 @@ function renderLobbyView( props: Partial[0]> = {}, withAppBar = false, platform = "android", + devices: Partial = {}, ): ReturnType { platformMock.mockReturnValue(platform); - const mediaDevices = mockMediaDevices({}); + const mediaDevices = mockMediaDevices(devices); const muteStates = mockMuteStates(); const hideHeader = withAppBar ? true : false; const lobbyView = ( @@ -178,3 +182,71 @@ describe("LobbyView", () => { expect(await axe(container)).toHaveNoViolations(); }); }); + +describe("LobbyView microphone level", () => { + const realMediaDevices = Object.getOwnPropertyDescriptor( + navigator, + "mediaDevices", + ); + + afterEach(() => { + vi.unstubAllGlobals(); + // Put navigator back, or every later test in the run inherits the stub. + if (realMediaDevices === undefined) { + Reflect.deleteProperty(navigator, "mediaDevices"); + } else { + Object.defineProperty(navigator, "mediaDevices", realMediaDevices); + } + }); + + /** Just enough of the Web Audio and capture APIs for the meter to run. */ + function stubAudioCapture(): void { + vi.stubGlobal( + "AudioContext", + class { + public readonly state = "running"; + public createAnalyser(): object { + return { + fftSize: 1024, + getByteTimeDomainData: (): void => {}, + }; + } + public createMediaStreamSource(): object { + return { connect: (): void => {} }; + } + public close(): void {} + }, + ); + // Only this property: replacing navigator wholesale drops the getters on + // its prototype, such as userAgent. + Object.defineProperty(navigator, "mediaDevices", { + configurable: true, + value: { + getUserMedia: vi.fn().mockResolvedValue({ getTracks: () => [] }), + }, + }); + } + + it("shows the microphone level meter", async () => { + stubAudioCapture(); + const user = userEvent.setup(); + const { getByRole } = renderLobbyView({}, false, "desktop", { + requestDeviceNames: (): void => {}, + audioInput: { + available$: of( + new Map([["mic1", { type: "name", name: "Microphone 1" }]]), + ), + selected$: of({ id: "mic1" }), + select: (): void => {}, + }, + } as unknown as Partial); + + // The meter lives with the microphone picker, which the pre-join screen + // reaches through the same chevron as a call in progress. + await user.click(getByRole("button", { name: "Microphone" })); + + expect( + await screen.findByRole("meter", { name: "Microphone level" }), + ).toBeInTheDocument(); + }); +});