Cut the menu's comments down to what the code does not say

- Drop the title prop: no caller passes it, and the menu's name is for screen
  readers only.
- One line each for the output options, the Default placeholder id and the
  list's height bounds.
- Keep only the two non-obvious reasons behind the focus tracking.
- Call what moved the focus its source rather than its modality.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
fkwp
2026-09-23 20:23:50 +02:00
co-authored by Claude Opus 5.5
parent cf486cf42a
commit d9ed39c7aa
5 changed files with 31 additions and 118 deletions
+5 -5
View File
@@ -111,7 +111,7 @@ test("keeps every device reachable in a small container", async ({ page }) => {
await expect(last).toHaveAttribute("aria-checked", "true");
});
test("tracks the focus modality of its own call, not the page", async ({
test("tracks the focus source of its own call, not the page", async ({
page,
}) => {
await installFakeDevices(page);
@@ -124,7 +124,7 @@ test("tracks the focus modality of its own call, not the page", async ({
timeout: 60_000,
});
await openDeviceList(page, pane);
// The menu owns the modality, because every item it can focus has to answer
// The menu owns the focus source, because every item it can focus has to answer
// to it — the device rows and the camera menu's blur toggle alike.
const menu = page.getByRole("menu");
@@ -134,7 +134,7 @@ test("tracks the focus modality of its own call, not the page", async ({
// suppresses the browser's own reaches this menu. The paint is asserted
// standalone instead — in the story and in audio-menu.spec.ts. What is on
// trial here is which call the tracking answers for.
await expect(menu).toHaveAttribute("data-focus-modality", "pointer");
await expect(menu).toHaveAttribute("data-focus-source", "pointer");
// A key pressed in the other call on this page — or anywhere in the host's
// own page — says nothing about how this menu is being used.
@@ -143,11 +143,11 @@ test("tracks the focus modality of its own call, not the page", async ({
new KeyboardEvent("keydown", { key: "ArrowDown", bubbles: true }),
),
);
await expect(menu).toHaveAttribute("data-focus-modality", "pointer");
await expect(menu).toHaveAttribute("data-focus-source", "pointer");
// A key pressed in this menu does.
await page.keyboard.press("ArrowDown");
await expect(menu).toHaveAttribute("data-focus-modality", "keyboard");
await expect(menu).toHaveAttribute("data-focus-source", "keyboard");
});
/**
@@ -131,7 +131,7 @@ Please see LICENSE in the repository root for full details.
}
/* Shown only when the keyboard is what moved the focus. */
.menu[data-focus-modality="keyboard"] [role^="menuitem"]:focus {
.menu[data-focus-source="keyboard"] [role^="menuitem"]:focus {
outline: var(--cpd-border-width-2) solid var(--cpd-color-border-focused);
outline-offset: calc(-1 * var(--cpd-border-width-2));
}
@@ -122,7 +122,6 @@ type Story = StoryObj<typeof meta>;
export const Default: Story = {
args: {
title: "SomeMenu",
iconsAndLabels: "audio",
enabled: true,
options: [
@@ -148,7 +147,6 @@ export const Default: Story = {
export const AudioMute: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: false,
options: [
@@ -172,7 +170,6 @@ export const AudioMute: Story = {
export const AudioUnmute: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: [
@@ -185,7 +182,6 @@ export const AudioUnmute: Story = {
export const VideoMute: Story = {
args: {
title: "Camera",
iconsAndLabels: "video",
enabled: false,
options: [
@@ -199,7 +195,6 @@ export const VideoMute: Story = {
export const VideoUnmute: Story = {
args: {
title: "Camera",
iconsAndLabels: "video",
enabled: true,
options: [
@@ -215,7 +210,6 @@ export const VideoUnmute: Story = {
export const SpeakerAndMicrophoneSections: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: [
@@ -296,7 +290,6 @@ export const SpeakerAndMicrophoneSections: Story = {
export const OutputCannotBeChosen: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: [
@@ -326,7 +319,6 @@ export const OutputCannotBeChosen: Story = {
export const OnlyOneDevice: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: [{ label: { type: "name", name: "Microphone 1" }, id: "mic1" }],
@@ -354,7 +346,6 @@ export const OnlyOneDevice: Story = {
export const SelectionSettling: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: [
@@ -390,14 +381,13 @@ export const SelectionSettling: Story = {
* The focus ring belongs to the keyboard. Radix focuses whatever the pointer is
* over, so a ring that followed focus alone would trail the mouse.
*
* Asserted on the painted outline rather than on `data-focus-modality`: the
* Asserted on the painted outline rather than on `data-focus-source`: the
* attribute is what the stylesheet keys off, so asserting it would pass even
* with the rule deleted.
*/
export const KeyboardFocusRing: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: [
@@ -435,7 +425,6 @@ export const KeyboardFocusRing: Story = {
export const ManyDevices: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: Array.from({ length: 20 }, (_, i) => ({
@@ -520,7 +509,6 @@ function outlineWidth(element: HTMLElement): number {
export const OutputNotEnumerated: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: [
@@ -555,7 +543,6 @@ export const OutputNotEnumerated: Story = {
export const MeterAlignsWithTheDeviceRows: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: [
@@ -598,7 +585,6 @@ function centre(element: Element): number {
export const KeyboardReachesEveryDevice: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
// Enough of them that the list scrolls well past its own height, so that
@@ -663,7 +649,6 @@ export const KeyboardReachesEveryDevice: Story = {
export const HeadingsStayWhileScrolling: Story = {
args: {
...Default.args,
title: "Microphone",
iconsAndLabels: "audio",
enabled: true,
options: Array.from({ length: 20 }, (_, i) => ({
@@ -68,7 +68,7 @@ describe("MediaMuteAndSwitchButton", () => {
test("renders", () => {
const { container } = renderComponent(
<TooltipProvider>
<MediaMuteAndSwitchButton title={"Switcher"} iconsAndLabels={"audio"} />
<MediaMuteAndSwitchButton iconsAndLabels={"audio"} />
</TooltipProvider>,
);
expect(container).toMatchSnapshot();
@@ -80,11 +80,7 @@ describe("MediaMuteAndSwitchButton", () => {
enabled: boolean,
): RenderResult => {
return renderComponent(
<MediaMuteAndSwitchButton
title={"Switcher"}
iconsAndLabels={type}
enabled={enabled}
/>,
<MediaMuteAndSwitchButton iconsAndLabels={type} enabled={enabled} />,
);
};
const renderAudioEndabled = renderLabels("audio", true);
@@ -111,7 +107,6 @@ describe("MediaMuteAndSwitchButton", () => {
const onMute = vi.fn();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title={"Switcher"}
onMuteClick={onMute}
iconsAndLabels="audio"
enabled={true}
@@ -128,7 +123,6 @@ describe("MediaMuteAndSwitchButton", () => {
const onMute = vi.fn();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title={"Switcher"}
onMuteClick={onMute}
iconsAndLabels="audio"
enabled={true}
@@ -149,7 +143,6 @@ describe("MediaMuteAndSwitchButton", () => {
const onMute = vi.fn();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title={"Switcher"}
onMuteClick={onMute}
iconsAndLabels="video"
enabled={true}
@@ -169,11 +162,7 @@ describe("MediaMuteAndSwitchButton", () => {
const user = userEvent.setup();
const requestDeviceNames = vi.fn();
renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled
/>,
<MediaMuteAndSwitchButton iconsAndLabels="audio" enabled />,
{ requestDeviceNames },
);
@@ -187,7 +176,6 @@ describe("MediaMuteAndSwitchButton", () => {
renderComponent(
<>
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled
options={[
@@ -197,7 +185,6 @@ describe("MediaMuteAndSwitchButton", () => {
selectedOption="mic1"
/>
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="video"
enabled
options={[
@@ -223,7 +210,6 @@ describe("MediaMuteAndSwitchButton", () => {
const onSelect = vi.fn();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -247,7 +233,6 @@ describe("MediaMuteAndSwitchButton", () => {
const onSelect = vi.fn();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -276,7 +261,6 @@ describe("MediaMuteAndSwitchButton", () => {
const [selectedOption, setSelectedOption] = useState("mic1");
return (
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -336,7 +320,6 @@ describe("MediaMuteAndSwitchButton", () => {
const onVideoBlurToggle = vi.fn();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="video"
enabled={true}
videoBlurToggleClick={onVideoBlurToggle}
@@ -361,7 +344,6 @@ describe("MediaMuteAndSwitchButton", () => {
const user = userEvent.setup();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -385,7 +367,6 @@ describe("MediaMuteAndSwitchButton", () => {
const [selectedOption, setSelectedOption] = useState("mic1");
return (
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -442,7 +423,6 @@ describe("MediaMuteAndSwitchButton", () => {
// was asked for never becomes the selection.
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -485,7 +465,6 @@ describe("MediaMuteAndSwitchButton", () => {
];
const menu = (options: MenuOptions[]): JSX.Element => (
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={options}
@@ -527,7 +506,6 @@ describe("MediaMuteAndSwitchButton", () => {
const { getByRole } = renderComponent(
<>
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -580,7 +558,6 @@ describe("MediaMuteAndSwitchButton", () => {
const user = userEvent.setup();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="video"
enabled={true}
options={[
@@ -608,7 +585,6 @@ describe("MediaMuteAndSwitchButton", () => {
const user = userEvent.setup();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -623,27 +599,26 @@ describe("MediaMuteAndSwitchButton", () => {
await user.click(getByRole("button", { name: "Microphone" }));
const list = screen
.getByRole("menuitemradio", { name: "Microphone 1" })
.closest("[data-focus-modality]");
.closest("[data-focus-source]");
// 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");
expect(list).toHaveAttribute("data-focus-source", "pointer");
await user.keyboard("{ArrowDown}");
expect(list).toHaveAttribute("data-focus-modality", "keyboard");
expect(list).toHaveAttribute("data-focus-source", "keyboard");
await user.pointer({
target: screen.getByRole("menuitemradio", { name: "Microphone 2" }),
coords: { clientX: 10, clientY: 10 },
});
expect(list).toHaveAttribute("data-focus-modality", "pointer");
expect(list).toHaveAttribute("data-focus-source", "pointer");
});
test("marks the selected device with the accent fill", async () => {
const user = userEvent.setup();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -670,7 +645,6 @@ describe("MediaMuteAndSwitchButton", () => {
const user = userEvent.setup();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -700,7 +674,6 @@ describe("MediaMuteAndSwitchButton", () => {
const user = userEvent.setup();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -734,7 +707,6 @@ describe("MediaMuteAndSwitchButton", () => {
const user = userEvent.setup();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -771,7 +743,6 @@ describe("MediaMuteAndSwitchButton", () => {
const onSelectOutput = vi.fn();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -799,7 +770,6 @@ describe("MediaMuteAndSwitchButton", () => {
const onSelect = vi.fn();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -821,7 +791,6 @@ describe("MediaMuteAndSwitchButton", () => {
const user = userEvent.setup();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
@@ -861,7 +830,6 @@ describe("MediaMuteAndSwitchButton", () => {
const user = userEvent.setup();
const { getByRole } = renderComponent(
<MediaMuteAndSwitchButton
title="Switcher"
iconsAndLabels="audio"
enabled={true}
options={[
+17 -57
View File
@@ -47,11 +47,6 @@ export interface MenuOptions {
}
export interface MediaMuteAndSwitchButtonProps {
/**
* The accessible name of the menu. Defaults to a translated name for the
* media kind. Never shown: each section carries its own heading.
*/
title?: string;
/** If the Mute button is enabled */
enabled?: boolean;
/** Callback if the mute button is clicked */
@@ -63,10 +58,7 @@ export interface MediaMuteAndSwitchButtonProps {
options?: MenuOptions[];
/** The option that will currently be rendered as the selected option */
selectedOption?: string;
/**
* Output (speaker) devices, shown as their own section above the input
* section. Audio menu only; omitted entirely for video.
*/
/** Output (speaker) devices. Audio menu only. */
outputOptions?: MenuOptions[];
/** The output option currently rendered as selected */
selectedOutputOption?: string;
@@ -86,33 +78,16 @@ export interface MediaMuteAndSwitchButtonProps {
const BLUR_ID = "blur";
/**
* Stands for wherever the platform is sending audio, where it will not say.
*
* Not a device id the browser would recognise: nothing can be selected on a
* platform that lists no outputs, so this is only ever shown, never sent.
*/
/** Id of the placeholder "Default" row, shown when the platform lists no outputs. */
const DEFAULT_OUTPUT_ID = "default";
/**
* The share of the call area the device list may fill.
*
* The menu carries its headings and the level meter as well, and a list that
* took the whole call would hide the call it belongs to.
*/
/** Largest share of the call area's height the device list may take. */
const LIST_SHARE_OF_CALL = 0.6;
/**
* The shortest the device list may be, whatever the call measures.
*
* A share alone collapses in a small call to a list that shows one device and
* gives no sign that there are others. Scrolling a short list is the better
* failure.
*/
/** Smallest device list height in px, so a short call still shows more than one device. */
const MIN_LIST_HEIGHT = 160;
export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
title,
enabled,
busy,
onMuteClick,
@@ -144,31 +119,17 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
const devices = useMediaDevices();
/**
* Tracks which modality moved the focus, for as long as the list is mounted.
*
* - Ours to track, because Radix focuses whatever the pointer is over, so
* `:focus-visible` answers for the pointer: Chromium says yes to anything
* after a key press, Firefox says no to programmatic focus.
* - A ref, not an effect on `menuOpen`: that state is ours and the open menu
* is Radix's, and an effect keyed on ours can run before Radix has mounted
* the content. The list existing is the honest signal.
* - On the menu, not the document: Element Call can be mounted twice in a
* host's page and this menu is portalled out of the call root, so a
* document listener would answer for the other instance too.
* - On the menu, not the list, because the first arrow key arrives while the
* menu itself holds focus — and because the blur toggle is the menu's
* child, not the list's, and has to answer to it as well.
* - In a dataset rather than state: which modality someone is using changes
* nothing that has to be rendered again.
* Records on the menu whether the keyboard or the pointer moved focus, for
* the focus ring. `:focus-visible` can't tell: Radix focuses whatever the
* pointer is over. Listened for on the menu, not the document, so a second
* Element Call on the page doesn't answer for this one.
*/
const trackFocusModality = useCallback(
const trackFocusSource = useCallback(
(list: HTMLDivElement | null): (() => void) | undefined => {
const menu = list?.closest<HTMLElement>('[role="menu"]');
if (menu === null || menu === undefined) return;
// Each opening starts over: the modality belongs to whoever is using this
// menu now, not to whoever last used it.
const record = (modality: "keyboard" | "pointer"): void => {
menu.dataset.focusModality = modality;
const record = (source: "keyboard" | "pointer"): void => {
menu.dataset.focusSource = source;
};
record("pointer");
const usedKeyboard = (): void => record("keyboard");
@@ -253,18 +214,18 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
: [];
let optionsButtonLabel: string;
let defaultMenuTitle: string;
let menuTitle: string;
let numberedLabel: (number: number) => string;
switch (iconsAndLabels) {
case "video":
optionsButtonLabel = t("settings.devices.camera");
defaultMenuTitle = t("settings.devices.camera_source");
menuTitle = t("settings.devices.camera_source");
numberedLabel = (n): string =>
t("settings.devices.camera_numbered", { n });
break;
case "audio":
optionsButtonLabel = t("settings.devices.microphone");
defaultMenuTitle = t("settings.devices.mic_source");
menuTitle = t("settings.devices.mic_source");
numberedLabel = (n): string =>
t("settings.devices.microphone_numbered", { n });
break;
@@ -394,9 +355,8 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
{button}
<Menu
className={styles.menu}
title={title ?? defaultMenuTitle}
// Each section carries its own heading, so the menu's own title would
// sit on top of the first one. Kept for the accessible name only.
// Named for screen readers only: each section has its own heading.
title={menuTitle}
showTitle={false}
open={menuOpen}
onOpenChange={onOpenChange}
@@ -416,7 +376,7 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
}
>
<div
ref={trackFocusModality}
ref={trackFocusSource}
// Transparent to assistive technology, so the menu still sees its
// items as its own children.
role="none"