From 664fac104f9b1e5a9c88010018cdb65b8227e2d3 Mon Sep 17 00:00:00 2001 From: fkwp Date: Wed, 16 Sep 2026 14:02:01 +0200 Subject: [PATCH] Use radio controls to mark the selected device in the quick menu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Mark the selection with a radio control instead of a trailing check icon beside a device glyph, matching the design and the settings modal, so both device pickers read the same way. - Take the visual from Compound's RadioInput rather than restyling a span of our own. RadioControl is wrapped in a Radix form control and needs a Form ancestor, which a dropdown menu has no business providing; RadioInput does not. - Keep rows as menuitemradio with aria-checked, rendered through MenuItem as="div" so an input is never nested inside a button. - Render the radio aria-hidden and not focusable: Radix owns focus inside the menu, and aria-checked carries the state. - Leave the activating spinner unchanged. - Keep every scope.behavior call inside the function that owns the scope, which no-observablescope-leak requires. - Cover the speaker section, an output that cannot be chosen, and a lone device shown disabled, with unit tests and stories for each. Spec: FEATURES_SPEC/2026-09_Quick_Audio_Menu.md — AC1, AC7 --- src/components/CallFooterViewModel.tsx | 64 +++------ .../MediaMuteAndSwitchButton.stories.tsx | 84 +++++++++++ .../MediaMuteAndSwitchButton.test.tsx | 136 ++++++++++++++++-- src/components/MediaMuteAndSwitchButton.tsx | 75 ++++------ 4 files changed, 252 insertions(+), 107 deletions(-) diff --git a/src/components/CallFooterViewModel.tsx b/src/components/CallFooterViewModel.tsx index a8612dfa7..08f5a6cd2 100644 --- a/src/components/CallFooterViewModel.tsx +++ b/src/components/CallFooterViewModel.tsx @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { combineLatest, map, switchMap } from "rxjs"; +import { combineLatest, map, type Observable, switchMap } from "rxjs"; import { supportsBackgroundProcessors } from "@livekit/track-processors"; import { type CallViewModel } from "../state/CallViewModel/CallViewModel"; @@ -76,63 +76,35 @@ function buildDeviceBehaviors( | "toggleBlur$" | "videoBlurEnabled$" > { - return { - audioOptions$: scope.behavior( - disableSwitcher$.pipe( - switchMap((disable) => - disable - ? constant([] as MenuOptions[]) - : mediaDevices.audioInput.available$.pipe( - map((available) => - [...available.entries()].map(([id, label]) => ({ - id, - label, - })), - ), + const options$ = ( + available$: Behavior>, + ): Observable => + disableSwitcher$.pipe( + switchMap((disable) => + disable + ? constant([] as MenuOptions[]) + : available$.pipe( + map((available) => + [...available.entries()].map(([id, label]) => ({ id, label })), ), - ), + ), ), - ), + ); + + return { + audioOptions$: scope.behavior(options$(mediaDevices.audioInput.available$)), selectedAudio$: scope.behavior( mediaDevices.audioInput.selected$.pipe(map((s) => s?.id)), ), selectAudioButtonOption$: constant(mediaDevices.audioInput.select), audioOutputOptions$: scope.behavior( - disableSwitcher$.pipe( - switchMap((disable) => - disable - ? constant([] as MenuOptions[]) - : mediaDevices.audioOutput.available$.pipe( - map((available) => - [...available.entries()].map(([id, label]) => ({ - id, - label, - })), - ), - ), - ), - ), + options$(mediaDevices.audioOutput.available$), ), selectedAudioOutput$: scope.behavior( mediaDevices.audioOutput.selected$.pipe(map((s) => s?.id)), ), selectAudioOutputOption$: constant(mediaDevices.audioOutput.select), - videoOptions$: scope.behavior( - disableSwitcher$.pipe( - switchMap((disable) => - disable - ? constant([] as MenuOptions[]) - : mediaDevices.videoInput.available$.pipe( - map((available) => - [...available.entries()].map(([id, label]) => ({ - id, - label, - })), - ), - ), - ), - ), - ), + videoOptions$: scope.behavior(options$(mediaDevices.videoInput.available$)), selectedVideo$: scope.behavior( mediaDevices.videoInput.selected$.pipe(map((s) => s?.id)), ), diff --git a/src/components/MediaMuteAndSwitchButton.stories.tsx b/src/components/MediaMuteAndSwitchButton.stories.tsx index 21def4007..30fc129c1 100644 --- a/src/components/MediaMuteAndSwitchButton.stories.tsx +++ b/src/components/MediaMuteAndSwitchButton.stories.tsx @@ -113,3 +113,87 @@ export const VideoUnmute: Story = { selectedOption: "2", }, }; + +export const SpeakerAndMicrophoneSections: Story = { + args: { + ...Default.args, + title: "Microphone", + iconsAndLabels: "audio", + enabled: true, + options: [ + { label: { type: "name", name: "Microphone 1" }, id: "mic1" }, + { label: { type: "name", name: "Microphone 2" }, id: "mic2" }, + ], + selectedOption: "mic1", + outputOptions: [ + { label: { type: "default", name: "Built-in Output" }, id: "default" }, + { label: { type: "name", name: "Headset" }, id: "spk2" }, + ], + selectedOutputOption: "default", + onSelectOutput: fn(), + }, + play: async ({ args, canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole("button", { name: "Microphone" })); + + const headset = await within(document.body).findByRole("menuitemradio", { + name: "Headset", + }); + await userEvent.click(headset); + await expect(args.onSelectOutput).toHaveBeenCalledWith("spk2"); + }, +}; + +export const OutputCannotBeChosen: Story = { + args: { + ...Default.args, + title: "Microphone", + iconsAndLabels: "audio", + enabled: true, + options: [ + { label: { type: "name", name: "Microphone 1" }, id: "mic1" }, + { label: { type: "name", name: "Microphone 2" }, id: "mic2" }, + ], + selectedOption: "mic1", + outputOptions: [ + { label: { type: "name", name: "Speakers" }, id: "spk1" }, + { label: { type: "name", name: "Headset" }, id: "spk2" }, + ], + selectedOutputOption: "spk1", + // No callback: the speakers are listed, but none can be picked. + onSelectOutput: undefined, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole("button", { name: "Microphone" })); + + const speakers = await within(document.body).findByRole("menuitemradio", { + name: "Speakers", + }); + await expect(speakers).toHaveAttribute("aria-disabled", "true"); + }, +}; + +export const OnlyOneDevice: Story = { + args: { + ...Default.args, + title: "Microphone", + iconsAndLabels: "audio", + enabled: true, + options: [{ label: { type: "name", name: "Microphone 1" }, id: "mic1" }], + selectedOption: "mic1", + outputOptions: [{ label: { type: "name", name: "Speakers" }, id: "spk1" }], + selectedOutputOption: "spk1", + onSelectOutput: fn(), + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole("button", { name: "Microphone" })); + + // Shown rather than hidden, so the menu keeps its shape everywhere. + const only = await within(document.body).findByRole("menuitemradio", { + name: "Microphone 1", + }); + await expect(only).toHaveAttribute("aria-disabled", "true"); + }, +}; diff --git a/src/components/MediaMuteAndSwitchButton.test.tsx b/src/components/MediaMuteAndSwitchButton.test.tsx index fcc9f0b4e..d7edacff1 100644 --- a/src/components/MediaMuteAndSwitchButton.test.tsx +++ b/src/components/MediaMuteAndSwitchButton.test.tsx @@ -327,7 +327,7 @@ describe("MediaMuteAndSwitchButton", () => { expect(onVideoBlurToggle).toHaveBeenCalled(); }); - test("renders check icon to mark the selected menu item", async () => { + test("marks the selected menu item as checked", async () => { const user = userEvent.setup(); const { getByRole } = renderComponent( { />, ); - // open menu await user.click(getByRole("button", { name: "Microphone" })); - // The selected item (mic2) renders both an IconOptions SVG and a CheckIcon SVG - const mic1Item = screen.getByRole("menuitemradio", { - name: "Microphone 2", - }); - expect(mic1Item.querySelectorAll("svg").length).toBe(2); + screen.getByRole("menuitemradio", { name: "Microphone 2", checked: true }); + screen.getByRole("menuitemradio", { name: "Microphone 1", checked: false }); + }); - // The unselected item (mic1) only renders its IconOptions SVG - const mic2Item = screen.getByRole("menuitemradio", { - name: "Microphone 1", + test("lists speaker and microphone sections", async () => { + const user = userEvent.setup(); + const { getByRole } = renderComponent( + , + ); + + await user.click(getByRole("button", { name: "Microphone" })); + + screen.getByRole("menuitemradio", { + name: "Default (Built-in Output)", + checked: true, }); - expect(mic2Item.querySelectorAll("svg").length).toBe(1); + screen.getByRole("menuitemradio", { name: "Headset", checked: false }); + screen.getByRole("menuitemradio", { name: "Microphone 1", checked: true }); + screen.getByRole("menuitemradio", { name: "Microphone 2", checked: false }); + }); + + test("calls the output select callback on speaker click", async () => { + const user = userEvent.setup(); + const onSelectOutput = vi.fn(); + const { getByRole } = renderComponent( + , + ); + + await user.click(getByRole("button", { name: "Microphone" })); + await user.click(screen.getByRole("menuitemradio", { name: "Headset" })); + + expect(onSelectOutput).toHaveBeenCalledWith("spk2"); + }); + + test("shows a single device entry disabled", async () => { + const user = userEvent.setup(); + const onSelect = vi.fn(); + const { getByRole } = renderComponent( + , + ); + + await user.click(getByRole("button", { name: "Microphone" })); + + // Shown rather than hidden, so the menu keeps its shape, but not choosable. + const only = screen.getByRole("menuitemradio", { name: "Microphone 1" }); + expect(only).toHaveAttribute("aria-disabled", "true"); + }); + + test("shows the speaker section disabled when no output can be chosen", async () => { + const user = userEvent.setup(); + const { getByRole } = renderComponent( + , + ); + + await user.click(getByRole("button", { name: "Microphone" })); + + expect( + screen.getByRole("menuitemradio", { name: "Speakers" }), + ).toHaveAttribute("aria-disabled", "true"); + expect( + screen.getByRole("menuitemradio", { name: "Headset" }), + ).toHaveAttribute("aria-disabled", "true"); + // The microphone section is unaffected. + expect( + screen.getByRole("menuitemradio", { name: "Microphone 2" }), + ).not.toHaveAttribute("aria-disabled", "true"); }); }); diff --git a/src/components/MediaMuteAndSwitchButton.tsx b/src/components/MediaMuteAndSwitchButton.tsx index f4374dbb5..8b4eb7acf 100644 --- a/src/components/MediaMuteAndSwitchButton.tsx +++ b/src/components/MediaMuteAndSwitchButton.tsx @@ -5,29 +5,20 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { - type ComponentType, - useState, - type FC, - useEffect, - type ReactElement, -} from "react"; +import { useState, type FC, useEffect, type ReactElement } from "react"; import { Button, Menu, MenuItem, MenuTitle, + RadioInput, Separator, ToggleMenuItem, } from "@vector-im/compound-web"; import { - CheckIcon, ChevronUpIcon, ChevronDownIcon, - MicOnIcon, SpinnerIcon, - VideoCallIcon, - VolumeOnIcon, } from "@vector-im/compound-design-tokens/assets/web/icons"; import classNames from "classnames"; import { useTranslation } from "react-i18next"; @@ -48,8 +39,7 @@ export interface MenuOptions { export interface MediaMuteAndSwitchButtonProps { /** * The accessible name of the menu. Defaults to a translated name for the - * media kind; the menu's own title is not shown, since each section carries - * its own heading. + * media kind. Never shown: each section carries its own heading. */ title?: string; /** If the Mute button is enabled */ @@ -71,8 +61,8 @@ export interface MediaMuteAndSwitchButtonProps { /** The output option currently rendered as selected */ selectedOutputOption?: string; /** - * Called when an output device is picked. Undefined means the platform does - * not permit choosing an output, and the section renders disabled. + * Called when an output device is picked. Undefined means no output can be + * chosen here, and the section renders disabled. */ onSelectOutput?: (id: string) => void; videoBlurToggleClick?: () => void; @@ -155,20 +145,17 @@ export const MediaMuteAndSwitchButton: FC = ({ break; } - let IconOptions: ComponentType> | undefined; let optionsButtonLabel: string; let defaultMenuTitle: string; let numberedLabel: (number: number) => string; switch (iconsAndLabels) { case "video": - IconOptions = VideoCallIcon; optionsButtonLabel = t("settings.devices.camera"); defaultMenuTitle = t("settings.devices.camera_source"); numberedLabel = (n): string => t("settings.devices.camera_numbered", { n }); break; case "audio": - IconOptions = MicOnIcon; optionsButtonLabel = t("settings.devices.microphone"); defaultMenuTitle = t("settings.devices.mic_source"); numberedLabel = (n): string => @@ -176,7 +163,8 @@ export const MediaMuteAndSwitchButton: FC = ({ break; } - const labelToText = ( + /** The text shown for a device, whichever kind of label it carries. */ + const labelText = ( label: MenuOptions["label"], numbered: (n: number) => string, ): string => { @@ -201,27 +189,30 @@ export const MediaMuteAndSwitchButton: FC = ({ selected: string | undefined, select: ((id: string) => void) | undefined, numbered: (n: number) => string, - Icon: ComponentType> | undefined, ): ReactElement[] => { const list = items ?? []; - // Shown but not choosable when the platform will not switch this kind of - // device, or when there is only one of them. The entry stays visible so the - // menu keeps the same shape everywhere. + // Shown but not choosable when nothing can be picked here, or when there is + // only one device. The entry stays visible so the menu keeps the same shape + // on every platform. const disabled = select === undefined || list.length <= 1; return list.map(({ label, id }) => ( - ) + } onSelect={(e) => { e.preventDefault(); @@ -233,13 +224,6 @@ export const MediaMuteAndSwitchButton: FC = ({ role="menuitemradio" aria-checked={selected === id} > - {selected === id && ( - - )} {selected !== id && plannedSelection === id && ( = ({ )); }; - const showOutputSection = iconsAndLabels === "audio" && outputOptions; - return (
= ({ /> } > - {showOutputSection && ( + {iconsAndLabels === "audio" && outputOptions && ( <> {deviceItems( @@ -293,19 +275,12 @@ export const MediaMuteAndSwitchButton: FC = ({ selectedOutputOption, onSelectOutput, (n) => t("settings.devices.speaker_numbered", { n }), - VolumeOnIcon, )} )} - {deviceItems( - options, - selectedOption, - onSelect, - numberedLabel, - IconOptions, - )} + {deviceItems(options, selectedOption, onSelect, numberedLabel)} {(toggles?.length ?? 0) > 0 &&
} {toggles?.map((toggle) => (