From d9ed39c7aaac3c9f12bca166bda8c29ad4e3e5c8 Mon Sep 17 00:00:00 2001 From: fkwp Date: Wed, 23 Sep 2026 20:23:50 +0200 Subject: [PATCH] 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) --- playwright/component/audio-menu.spec.ts | 10 +-- .../MediaMuteAndSwitchButton.module.css | 2 +- .../MediaMuteAndSwitchButton.stories.tsx | 17 +---- .../MediaMuteAndSwitchButton.test.tsx | 46 ++---------- src/components/MediaMuteAndSwitchButton.tsx | 74 +++++-------------- 5 files changed, 31 insertions(+), 118 deletions(-) diff --git a/playwright/component/audio-menu.spec.ts b/playwright/component/audio-menu.spec.ts index 03c88e30c..f13c99fb7 100644 --- a/playwright/component/audio-menu.spec.ts +++ b/playwright/component/audio-menu.spec.ts @@ -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"); }); /** diff --git a/src/components/MediaMuteAndSwitchButton.module.css b/src/components/MediaMuteAndSwitchButton.module.css index 4faa52ee1..053dd1a93 100644 --- a/src/components/MediaMuteAndSwitchButton.module.css +++ b/src/components/MediaMuteAndSwitchButton.module.css @@ -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)); } diff --git a/src/components/MediaMuteAndSwitchButton.stories.tsx b/src/components/MediaMuteAndSwitchButton.stories.tsx index 474240cb0..fb9aa376b 100644 --- a/src/components/MediaMuteAndSwitchButton.stories.tsx +++ b/src/components/MediaMuteAndSwitchButton.stories.tsx @@ -122,7 +122,6 @@ type Story = StoryObj; 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) => ({ diff --git a/src/components/MediaMuteAndSwitchButton.test.tsx b/src/components/MediaMuteAndSwitchButton.test.tsx index 05f77ca3c..902a3bf29 100644 --- a/src/components/MediaMuteAndSwitchButton.test.tsx +++ b/src/components/MediaMuteAndSwitchButton.test.tsx @@ -68,7 +68,7 @@ describe("MediaMuteAndSwitchButton", () => { test("renders", () => { const { container } = renderComponent( - + , ); expect(container).toMatchSnapshot(); @@ -80,11 +80,7 @@ describe("MediaMuteAndSwitchButton", () => { enabled: boolean, ): RenderResult => { return renderComponent( - , + , ); }; const renderAudioEndabled = renderLabels("audio", true); @@ -111,7 +107,6 @@ describe("MediaMuteAndSwitchButton", () => { const onMute = vi.fn(); const { getByRole } = renderComponent( { const onMute = vi.fn(); const { getByRole } = renderComponent( { const onMute = vi.fn(); const { getByRole } = renderComponent( { const user = userEvent.setup(); const requestDeviceNames = vi.fn(); renderComponent( - , + , { requestDeviceNames }, ); @@ -187,7 +176,6 @@ describe("MediaMuteAndSwitchButton", () => { renderComponent( <> { selectedOption="mic1" /> { const onSelect = vi.fn(); const { getByRole } = renderComponent( { const onSelect = vi.fn(); const { getByRole } = renderComponent( { const [selectedOption, setSelectedOption] = useState("mic1"); return ( { const onVideoBlurToggle = vi.fn(); const { getByRole } = renderComponent( { const user = userEvent.setup(); const { getByRole } = renderComponent( { const [selectedOption, setSelectedOption] = useState("mic1"); return ( { // was asked for never becomes the selection. const { getByRole } = renderComponent( { ]; const menu = (options: MenuOptions[]): JSX.Element => ( { const { getByRole } = renderComponent( <> { const user = userEvent.setup(); const { getByRole } = renderComponent( { 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]"); + .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( { const user = userEvent.setup(); const { getByRole } = renderComponent( { const user = userEvent.setup(); const { getByRole } = renderComponent( { const user = userEvent.setup(); const { getByRole } = renderComponent( { const onSelectOutput = vi.fn(); const { getByRole } = renderComponent( { const onSelect = vi.fn(); const { getByRole } = renderComponent( { const user = userEvent.setup(); const { getByRole } = renderComponent( { const user = userEvent.setup(); const { getByRole } = renderComponent( = ({ - title, enabled, busy, onMuteClick, @@ -144,31 +119,17 @@ export const MediaMuteAndSwitchButton: FC = ({ 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('[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 = ({ : []; 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 = ({ {button} = ({ } >