diff --git a/playwright/audio-menu.spec.ts b/playwright/audio-menu.spec.ts index c72673255..99385c890 100644 --- a/playwright/audio-menu.spec.ts +++ b/playwright/audio-menu.spec.ts @@ -18,11 +18,9 @@ test.describe("the quick audio menu", () => { await joinACall(page, "Menu user", "Audio menu"); await openAudioMenu(page); - // The speaker list is what the settings modal used to be the only home of. await expect(page.getByRole("group", { name: "Speaker" })).toBeVisible(); await expect(page.getByRole("group", { name: "Microphone" })).toBeVisible(); - // Named rather than counted: the browser contributes its own fake output - // and a "Default" entry, so a total would be a fact about the browser. + // By name, not count: the browser adds its own fake output and a Default. for (const n of [1, 2, 3]) await expect( page @@ -30,8 +28,6 @@ test.describe("the quick audio menu", () => { .getByRole("menuitemradio", { name: `Fake Speaker ${n}` }), ).toBeVisible(); - // Only one entry of a kind is marked, and the meter reports a number - // rather than a colour. await expect( page.getByRole("menuitemradio", { checked: true }), ).toHaveCount(2); @@ -40,10 +36,7 @@ test.describe("the quick audio menu", () => { await expect(meter).toHaveAttribute("aria-valuenow", /\d+/); await expect(meter).toHaveAttribute("aria-valuetext", /\d+ of \d+/); - // Both browsers in the matrix can route audio to a chosen output, so the - // section offers a real choice. The case where a platform cannot — Safari, - // and anything without setSinkId — is covered by a unit check, since no - // browser here can reach it. + // Both browsers here can route audio; the platform that can't is a unit check. await expect( page .getByRole("group", { name: "Speaker" }) @@ -55,7 +48,7 @@ test.describe("the quick audio menu", () => { test("moves the microphone and the speaker without disturbing the call", async ({ browser, }) => { - // Two browsers, two joins and a real call between them. + // Two browsers and a real call. test.slow(); const hostContext = await browser.newContext({ reducedMotion: "reduce" }); const host = await hostContext.newPage(); @@ -73,9 +66,7 @@ test.describe("the quick audio menu", () => { await openAudioMenu(host); await selectDevice(host, "Speaker", "Fake Speaker 2"); - // The point of the criterion: the switch is not a rejoin. Neither side - // sees the call drop, and the guest still has both tiles — so the host - // never left and came back. + // Not a rejoin: neither side drops, and the guest still has both tiles. await expect( host.getByRole("dialog", { name: "Reconnecting…" }), ).not.toBeVisible(); @@ -92,7 +83,7 @@ test.describe("the quick audio menu", () => { test("keeps the meter moving while muted, and sends nothing", async ({ browser, }) => { - // Two browsers, two joins and a real call between them. + // Two browsers and a real call. test.slow(); const hostContext = await browser.newContext({ reducedMotion: "reduce" }); const host = await hostContext.newPage(); @@ -110,15 +101,12 @@ test.describe("the quick audio menu", () => { await expect(mute).toHaveAttribute("aria-checked", "false"); await openAudioMenu(host); - // The microphone is held open while muted, so the meter still reports the - // hardware. The mute control is what says nothing is being transmitted. const meter = host.getByRole("meter", { name: "Microphone level" }); await expect(meter).toBeVisible(); - // Queried by test id, not by role: the menu is modal, so Radix takes the - // rest of the call out of the accessibility tree while it is open. + // By test id: the modal menu hides the rest of the call from the a11y tree. await expect(mute).toHaveAttribute("aria-checked", "false"); await expect(mute).toBeVisible(); - // And the listener is told so, rather than being left to guess from silence. + // And the guest is shown the mute. await expect( guest.getByTestId("videoTile").filter({ hasText: "Muted host" }), ).toBeVisible(); @@ -137,9 +125,8 @@ test.describe("the quick audio menu", () => { const meter = page.getByRole("meter", { name: "Microphone level" }); await expect(meter).toBeVisible(); - // Scrolled so the microphones start at the top of the list and run past its - // bottom: the position that tells a pinned meter from one that merely - // happens to be last. + // Scrolled so the microphones start at the top: only there does a pinned + // meter differ from one that is simply last. const list = page.locator("[role='menu'] div[role='none']").first(); await list.evaluate((element) => { const group = element.querySelector("[role='group'][aria-label*='icro']"); @@ -150,7 +137,6 @@ test.describe("the quick audio menu", () => { await expect(meter).toBeInViewport(); await expectPinnedInside(meter, list); - // Every entry stays reachable, which is what the scroll is for. await expect( page.getByRole("menuitemradio", { name: "Fake Microphone 20" }), ).toBeVisible(); @@ -169,16 +155,14 @@ test.describe("the quick audio menu", () => { await openAudioMenu(page); const first = page.getByRole("menuitemradio").first(); - // Opened by pointer, so no ring, even though Radix has moved focus into the - // menu already. + // Opened by pointer: no ring, though Radix has moved focus into the menu. await expect.poll(async () => outlineWidth(first)).toBe(0); await page.keyboard.press("ArrowDown"); const focused = page.locator("[role='menuitemradio']:focus"); await expect.poll(async () => outlineWidth(focused)).toBeGreaterThan(0); - // The pointer takes it away again: the menu focuses whatever it is over, so - // a ring that followed focus alone would trail the mouse. + // And the pointer takes it away again. await first.hover(); await expect.poll(async () => outlineWidth(focused)).toBe(0); }); @@ -193,8 +177,7 @@ async function joinACall( await page.goto("/"); await SpaHelpers.createCall(page, userName, callName, true); await expect(page.getByTestId("name_tag")).toContainText(userName); - // The media controls stay disabled until the devices have enumerated, and - // every test here drives them. + // The media controls stay disabled until devices have enumerated. await expect(page.getByTestId("incall_mute")).toBeEnabled({ timeout: 10_000, }); @@ -214,19 +197,12 @@ async function selectDevice( .getByRole("group", { name: section }) .getByRole("menuitemradio", { name }); await item.click(); - // Selecting does not close the menu — the component prevents the default so - // the list survives a mis-click — so it is dismissed explicitly. + // Selecting doesn't close the menu, so dismiss it. await page.keyboard.press("Escape"); await expect(page.getByRole("menu")).not.toBeVisible(); } -/** - * Asserts the meter sits within the scrollport, and inside the menu's frame. - * - * The meter is the one opaque element in the menu, so it is the one thing that - * can paint over the border. Whether it actually does needs a screenshot; this - * pins the geometry that decides it. - */ +/** Asserts the meter sits within the scrollport and inside the menu's frame. */ async function expectPinnedInside( meter: Locator, list: Locator, @@ -243,7 +219,7 @@ async function expectPinnedInside( expect(meterBox.x + meterBox.width).toBeLessThan(frame.x + frame.width); } -/** The painted outline width in pixels, however the stylesheet spells it. */ +/** Painted outline width, in px. */ async function outlineWidth(item: Locator): Promise { if ((await item.count()) === 0) return 0; return item.first().evaluate((element) => { diff --git a/playwright/component/audio-menu.spec.ts b/playwright/component/audio-menu.spec.ts index f13c99fb7..f4d811598 100644 --- a/playwright/component/audio-menu.spec.ts +++ b/playwright/component/audio-menu.spec.ts @@ -11,22 +11,12 @@ import { createUserAndRoom, resizeContainer, startHarness } from "./harness.ts"; import { installFakeDevices } from "../utils/fake-devices.ts"; /** - * The device menu where Element Call is a component in a host's page rather - * than the whole of one. - * - * This is the case the stylesheets cannot describe: the menu is portalled to - * the document, so a container query and a viewport unit both measure the wrong - * thing — the first has no container to resolve against out there, the second - * measures a page Element Call does not own. The menu has to be sized against - * the space the call is actually drawn in. - * - * Driven from the lobby rather than a joined call. The footer builds the same - * menu from the same device behaviours in both, and the container is the same - * size either way, so joining would only add two connections' worth of flake. + * The device menu with Element Call as a component in a host page, where the + * portalled menu must be sized against the call, not the window. Driven from + * the lobby: it builds the same menu, without two connections' worth of flake. */ -// Signing in, setting up crypto and syncing happen twice before anything is on -// screen, as in component-call.spec.ts +// Sign-in, crypto setup and sync happen twice before anything shows. test.describe.configure({ timeout: 180_000 }); test("sizes the device list against the call, not the window", async ({ @@ -37,8 +27,7 @@ test("sizes the device list against the call, not the window", async ({ const panes = await startHarness(page, username, roomId); const pane = panes.first(); - // A short call in a much taller page: the difference between measuring the - // call and measuring the window. + // A short call in a much taller page. const container = pane.getByTestId("call-container"); await resizeContainer(container, { width: 900, height: 400 }); await expect(pane.getByTestId("lobby_joinCall")).toBeVisible({ @@ -50,8 +39,6 @@ test("sizes the device list against the call, not the window", async ({ const windowHeight = page.viewportSize()!.height; const listHeight = (await list.boundingBox())!.height; - // Sized against the call. Were it sized against the window the list would be - // half as tall again, and the assertion below would not be able to tell. expect(callHeight).toBeLessThan(windowHeight * 0.75); expect(listHeight).toBeLessThanOrEqual(callHeight); expect(listHeight).toBeLessThan(windowHeight * 0.6); @@ -72,9 +59,7 @@ test("follows the call area when the host resizes it", async ({ page }) => { const list = await openDeviceList(page, pane); const whenShort = (await list.boundingBox())!.height; - // The host grows the space Element Call is drawn in while the menu is open — - // a panel opening, a window dragged, a phone turned. A bound taken once on - // opening would still describe the smaller call. + // The host grows the call while the menu is open. await resizeContainer(container, { width: 900, height: 700 }); await expect .poll(async () => (await list.boundingBox())!.height) @@ -88,15 +73,13 @@ test("keeps every device reachable in a small container", async ({ page }) => { const pane = panes.first(); const container = pane.getByTestId("call-container"); - // Narrow as well as short, which is where entries get pushed out of reach. await resizeContainer(container, { width: 400, height: 360 }); await expect(pane.getByTestId("lobby_joinCall")).toBeVisible({ timeout: 60_000, }); const list = await openDeviceList(page, pane); - // More devices than the space allows, so the list has to scroll rather than - // put entries somewhere they cannot be got at. + // More devices than fit, so the list must scroll. expect(await list.evaluate((el) => el.scrollHeight > el.clientHeight)).toBe( true, ); @@ -104,9 +87,7 @@ test("keeps every device reachable in a small container", async ({ page }) => { const last = page.getByRole("menuitemradio", { name: "Fake Microphone 20" }); await last.scrollIntoViewIfNeeded(); await expect(last).toBeInViewport(); - // Reachable means usable, not merely painted: D11 accepts that the menu may - // be drawn outside the call area, so this asserts reach rather than - // containment. + // Reachable means clickable: the menu may be drawn outside the call area. await last.click(); await expect(last).toHaveAttribute("aria-checked", "true"); }); @@ -124,20 +105,13 @@ test("tracks the focus source of its own call, not the page", async ({ timeout: 60_000, }); await openDeviceList(page, pane); - // 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"); - // Asserted on the attribute rather than the painted ring, which cannot be - // read here: the menu is portalled outside the call root, and the component - // build scopes the stylesheet to it, so neither the ring nor the rule that - // 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. + // Asserted on the attribute: the component build's scoped stylesheet doesn't + // reach the portalled menu, so its ring can't be read here. 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. + // A key pressed in the other call says nothing about this menu. await other.evaluate((element) => element.dispatchEvent( new KeyboardEvent("keydown", { key: "ArrowDown", bubbles: true }), @@ -145,15 +119,11 @@ test("tracks the focus source of its own call, not the page", async ({ ); 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-source", "keyboard"); }); -/** - * Opens the microphone menu of one component and returns its scrolling device - * list, which lives outside the component: the menu is portalled to the page. - */ +/** Opens a component's microphone menu and returns its device list. */ async function openDeviceList(page: Page, pane: Locator): Promise { await pane .getByRole("button", { name: "Microphone" }) diff --git a/playwright/utils/fake-devices.ts b/playwright/utils/fake-devices.ts index c152955b0..e67f053bd 100644 --- a/playwright/utils/fake-devices.ts +++ b/playwright/utils/fake-devices.ts @@ -8,20 +8,10 @@ Please see LICENSE in the repository root for full details. import { type Page } from "@playwright/test"; /** - * Gives the browser more fake devices than it ships with. - * - * A headless browser's fake capture offers one microphone and one speaker, - * which is one short of what a device menu is for: with no choice to make, every - * entry renders disabled. These are synthetic entries on top of the real fake - * device, so the menu has a list to show and a selection to move, and the app - * runs its real device pipeline against them. - * - * What they do not do is route audio: every entry is backed by the same capture, - * and `setSinkId` is accepted rather than honoured. A test can prove that - * choosing a device changes the app's state and does not disturb the call. That - * a listener hears the change needs hardware, and stays a manual check. - * - * Must be called before the page navigates. + * Adds synthetic devices beside the browser's one fake microphone and speaker, + * so the menu has a choice to show. They share one capture and `setSinkId` is + * accepted but not honoured: routing still needs hardware and a manual check. + * Call before the page navigates. */ export async function installFakeDevices( page: Page, @@ -58,8 +48,7 @@ export async function installFakeDevices( ...synthetic("audiooutput", speakers, "Fake Speaker"), ]; - // Our ids name no hardware, so an exact-device constraint on one would be - // rejected. Drop it and let the one real fake device answer. + // Our ids name no hardware, so drop the exact-device constraint. const getUserMedia = devices.getUserMedia.bind(devices); devices.getUserMedia = async ( constraints?: MediaStreamConstraints, @@ -77,10 +66,8 @@ export async function installFakeDevices( return getUserMedia(constraints); }; - // Routing to a device that does not exist would reject, and the app - // treats that as a failed switch. Both sinks are patched: Element Call - // routes its own AudioContext as well as the media elements, and leaving - // that one alone logs a NotFoundError for every switch. + // Accept routing to our ids on both media elements and the AudioContext, + // which the app also routes. for (const proto of [ HTMLMediaElement.prototype, AudioContext.prototype, diff --git a/src/components/CallFooter.stories.tsx b/src/components/CallFooter.stories.tsx index b99bfa132..96e010d85 100644 --- a/src/components/CallFooter.stories.tsx +++ b/src/components/CallFooter.stories.tsx @@ -180,8 +180,7 @@ export const StaysWhileAMenuIsOpen: Story = { ...Default, args: { ...WithAudioAndVideoOptions.args, - // As it is in a short window, where the footer overlays the call and hides - // itself once nothing is happening. + // In a short window the footer overlays the call and hides itself. asOverlay: true, showFooter: true, }, @@ -194,18 +193,10 @@ export const StaysWhileAMenuIsOpen: Story = { ); await expect(document.body.querySelector('[role="menu"]')).not.toBeNull(); - // The call now decides to hide the footer, which is the class it does it - // with. The menu is portalled out of the footer, so the focus inside it is - // not something the footer can see: without the trigger's aria-expanded to - // go on, this would fade the footer out and take the menu's anchor with it. - // The call now decides to hide the footer, which is the class it does that - // with. The menu is portalled out of the footer, so the focus inside it is - // not something the footer can see: without the trigger's aria-expanded to - // go on, this fades the footer out and takes the menu's anchor with it. + // The call hides the footer with this class. footer.classList.add(styles.hidden); - // The footer fades over 0.15s, so a reading taken now is the value it - // started from whatever happens next. Let the transition finish first. + // Read after the 0.15s fade, not mid-transition. await new Promise((settled) => setTimeout(settled, 400)); await expect(getComputedStyle(footer).opacity).toBe("1"); }, diff --git a/src/components/CallFooter.tsx b/src/components/CallFooter.tsx index 45d346bab..6fdf13dda 100644 --- a/src/components/CallFooter.tsx +++ b/src/components/CallFooter.tsx @@ -99,7 +99,7 @@ export interface FooterState { /** Providing no options `[]` or `undefined` will imply that we dont have a audio fast switcher */ audioOptions: MenuOptions[]; - /** Output devices shown as their own section in the audio menu. */ + /** Output (speaker) devices. */ audioOutputOptions: MenuOptions[]; /** Providing no options `[]` or `undefined` will imply that we dont have a audio fast switcher */ videoOptions: MenuOptions[]; diff --git a/src/components/CallFooterViewModel.test.ts b/src/components/CallFooterViewModel.test.ts index df23cba22..0c02a2ba7 100644 --- a/src/components/CallFooterViewModel.test.ts +++ b/src/components/CallFooterViewModel.test.ts @@ -116,7 +116,6 @@ describe("createCallFooterViewModel", () => { it("is withheld where the platform cannot route audio to a chosen device", () => { outputSelectionMock.mockReturnValue(false); - // Undefined is what renders the speaker section disabled. expect(buildFooterVm().selectAudioOutputOption$.value).toBeUndefined(); }); @@ -145,9 +144,7 @@ describe("createCallFooterViewModel", () => { selected$: constant(undefined), select: vi.fn(), }, - // Safari enumerates no output devices whatsoever. Reproduced by the - // condition rather than by the browser, so it is checked on the - // Linux CI runners that have no Safari to check it with. + // As Safari: no outputs listed. audioOutput: { available$: constant(new Map()), selected$: constant(undefined), @@ -158,10 +155,7 @@ describe("createCallFooterViewModel", () => { { showControls: true, header: HeaderStyle.Standard }, ); - // Empty rather than undefined: undefined means this menu has no notion - // of outputs at all, as the camera menu has none, and hides the section. - // Empty means there are none to list, and the menu still shows the - // section with a default in it, disabled. + // Empty, not undefined: undefined would hide the section. expect(vm.audioOutputOptions$.value).toEqual([]); }); }); diff --git a/src/components/CallFooterViewModel.tsx b/src/components/CallFooterViewModel.tsx index 305b2a434..50efb42f9 100644 --- a/src/components/CallFooterViewModel.tsx +++ b/src/components/CallFooterViewModel.tsx @@ -104,8 +104,8 @@ function buildDeviceBehaviors( selectedAudioOutput$: scope.behavior( mediaDevices.audioOutput.selected$.pipe(map((s) => s?.id)), ), - // Safari and most Firefox builds cannot route audio to a chosen device at - // all. Withholding the callback is what renders the section disabled. + // Withheld where the platform can't route audio to a chosen device, which + // disables the speaker section. selectAudioOutputOption$: constant( supportsAudioOutputSelection() ? mediaDevices.audioOutput.select diff --git a/src/components/MediaMuteAndSwitchButton.module.css b/src/components/MediaMuteAndSwitchButton.module.css index 053dd1a93..1d5242689 100644 --- a/src/components/MediaMuteAndSwitchButton.module.css +++ b/src/components/MediaMuteAndSwitchButton.module.css @@ -41,55 +41,29 @@ Please see LICENSE in the repository root for full details. flex-direction: column; } -/* Only the device lists scroll; the level meter stays put beneath them. - The bound comes from the measured height of the call area, set by the - component: the menu is portalled outside the root, so neither a container - query nor a viewport unit describes the space it is allowed to fill. */ +/* Only the device lists scroll, bounded by the measured call area: the menu is + portalled out of the root, so CSS can't size it against the call. */ .deviceList { overflow-y: auto; min-block-size: 0; max-block-size: var(--device-list-max-height); - /* The heading stands over the head of this list and the level meter over its - foot. Scrolling a row flush to either edge would put it underneath one of - them, which is how a row reached by the keyboard ends up half-readable; - this keeps both their heights clear for anything the browser scrolls to. - Set by the component, which is the only thing that can measure them. */ + /* Keeps a row reached by keyboard clear of the sticky heading and meter. */ scroll-padding-block: var(--device-list-scroll-padding-start, 0) var(--device-list-scroll-padding-end, 0); } -/* Each section is headed by its own rule, which Compound draws under a menu - heading already. The heading spans the whole menu — the menu has no padding - of its own — so that rule runs edge to edge, stopping only where the frame - is, rather than inset as a separator between the sections would be. - - Compound's spacing around it is the design's too, bar one thing: the design - leaves a section's first device further below the rule than Compound does — - measured off the mock, 27px from the rule to the top of the control, against - the 16px that Compound's 8px margin and the row's own 8px padding give. The - difference goes below the rule, so the rule stays tight under its own text - where Compound put it. */ +/* Compound's heading rule heads each section; the design adds space below it. */ .menu h3 { margin-block-end: var(--cpd-space-5x); } -/* One section is set much further from the one above it than Compound's 8px - heading margin allows: measured off the mock, 41px from the last device's - control to the next heading's text, against the 22px we had. Only between - sections — the first heading keeps Compound's spacing, because the menu's - own padding is already above it. */ +/* And more above a heading that follows another section. */ .deviceList [role="group"] + [role="group"] h3 { margin-block-start: var(--cpd-space-7x); } -/* Each section's heading stays at the top of the scrollport while any of that - section is still in view, so a long list never leaves you wondering which - kind of device you are looking at. It leaves with its own section, because - sticky only holds while the group it belongs to is in view. - - Opaque, and held a border width clear of the sides, for the same reason as - the meter below: this is a positioned element, so it paints above the - outline the menu draws its frame with, and would swallow it. */ +/* Sticky within its own section. Opaque and inset a border width so it doesn't + paint over the menu's outline. */ .sectionHeading { position: sticky; inset-block-start: 0; @@ -98,39 +72,22 @@ Please see LICENSE in the repository root for full details. margin-block-start: var(--cpd-border-width-1); } -/* The meter belongs to the microphone section: it stays at the bottom of the - scrollport while that section is in view, and leaves with it when the list - is scrolled up to the speakers. The wrapper is deliberately unpositioned — - a positioned one paints above the menu's outline and swallows the frame - along this whole section. Sticky is resolved against the scroll container, - so it does not need one. */ - +/* Sticky at the foot of the microphone section; opaque and inset as above. */ .stickyMeter { position: sticky; inset-block-end: 0; - /* Opaque, so the list does not show through it as it scrolls past. The menu - draws its frame as an outline inset by one border width, and the device - rows are transparent at rest, so this is the only thing that can cover it: - hold it clear on the sides and the bottom. */ background: var(--cpd-color-bg-canvas-default); 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. - - Every kind of item the menu can focus, not only the device rows: the camera - menu's blur toggle is a checkbox item and a child of the menu rather than of - the list, and left out it kept the browser's own ring and followed the - pointer with it. */ +/* Radix focuses whatever the pointer is over, so the browser's ring would + follow the mouse. Replaced by one shown only for keyboard focus. */ .menu [role^="menuitem"]:focus, .menu [role^="menuitem"]:focus-visible { outline: none; } -/* Shown only when the keyboard is what moved the 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 fb9aa376b..d4965892f 100644 --- a/src/components/MediaMuteAndSwitchButton.stories.tsx +++ b/src/components/MediaMuteAndSwitchButton.stories.tsx @@ -21,24 +21,14 @@ const mediaDevices = new MediaDevices(globalScope, { controlledAudioDevices: false, }); -/** - * Gives these stories a microphone to read. - * - * The menu opens a capture of whichever device it has been told is selected, - * and the devices in a story are invented: asking for one by an id no hardware - * answers to fails, and the meter reports that — correctly — as there being no - * microphone. So the story provides one rather than borrowing the machine's: a - * wavering tone played into a real MediaStream, which the meter then runs its - * own analyser over. Nothing here stands in for the meter itself. - */ +/** Supplies a microphone, a wavering tone: a story's invented device ids match no hardware. */ const WithAMicrophone: FC<{ children: ReactNode }> = ({ children }) => { useEffect(() => { const context = new AudioContext(); const microphone = context.createMediaStreamDestination(); const tone = context.createOscillator(); const loudness = context.createGain(); - // Swinging between about a third and two thirds of the range, so the meter - // reads as something live rather than as a level someone pinned there. + // Wavers between a third and two thirds of full scale, so it reads as live. const swing = context.createOscillator(); const depth = context.createGain(); loudness.gain.value = 0.25; @@ -53,8 +43,7 @@ const WithAMicrophone: FC<{ children: ReactNode }> = ({ children }) => { const devices = navigator.mediaDevices; const openedForReal = devices.getUserMedia.bind(devices); const opened = Promise.resolve(microphone.stream); - // A fresh clone each time, so that a caller stopping its tracks when it is - // done does not take the microphone away from the next one. + // A fresh clone each time, so a caller stopping its tracks doesn't end the next. devices.getUserMedia = async (): Promise => (await opened).clone(); @@ -69,26 +58,14 @@ const WithAMicrophone: FC<{ children: ReactNode }> = ({ children }) => { return <>{children}; }; -/** - * Gives these stories the call area the menu belongs to. - * - * The menu sizes its device list against the space Element Call is drawn in, - * and takes that from a provider. Without one it falls back to the document - * body — which in a story is the whole of Storybook's frame, so the list is - * bounded by something far larger than the story it is drawn in and runs off - * the top of the canvas. Supplying a root is the same courtesy as supplying the - * devices: the story stands in for the call, so it has to say how big it is. - */ +/** Supplies a call-sized root. Without one the list is bounded by the whole Storybook frame. */ const WithACallArea: FC<{ children: ReactNode }> = ({ children }) => { const [callArea, setCallArea] = useState(null); return (
( `.${styles.deviceList}`, )!; await expect(list.scrollHeight).toBe(list.clientHeight); - // Each section is headed by its own rule, running the full width of the - // menu rather than inset — and nothing divides the sections besides. + // Each section is headed by its own edge-to-edge rule, with no separator. const menu = document.body.querySelector("[role='menu']")!; await expect( document.body.querySelectorAll("[role='separator']"), @@ -258,16 +229,14 @@ export const SpeakerAndMicrophoneSections: Story = { await expect( Number.parseFloat(getComputedStyle(rule).borderBottomWidth), ).toBeGreaterThan(0); - // Edge to edge, stopping only where the menu's frame is drawn. + // Edge to edge, within the frame. const box = rule.getBoundingClientRect(); await expect(box.left - frame.left).toBeLessThanOrEqual(2); await expect(frame.right - box.right).toBeLessThanOrEqual(2); } - // A section's first device sits further below the rule than it does from - // the menu's edge. Stated as the relationship rather than a number: what - // the design asks for is the asymmetry, and Compound's own heading margin - // alone would make the two equal. + // The first device sits further below the rule than from the menu's edge. + // Asserted as a relationship, not pixels: the asymmetry is the design. const control = document.body.querySelector("input[type='radio']")!; const ruleBottom = headings[0] .querySelector("h3")! @@ -275,8 +244,8 @@ export const SpeakerAndMicrophoneSections: Story = { const box = control.getBoundingClientRect(); await expect(box.top - ruleBottom).toBeGreaterThan(box.left - frame.left); - // And one section stands further from the one above it than a heading does - // from its own first device — again the relationship, not a number. + // And a section stands further from the one above than from its own first + // device. const groups = document.body.querySelectorAll("[role='group']"); const speakers = groups[0].querySelectorAll("input[type='radio']"); const lastSpeaker = speakers[speakers.length - 1].getBoundingClientRect(); @@ -331,7 +300,7 @@ export const OnlyOneDevice: Story = { const canvas = within(canvasElement); await userEvent.click(canvas.getByRole("button", { name: "Microphone" })); - // Shown rather than hidden, so the menu keeps its shape everywhere. + // Shown, disabled, rather than hidden. const only = await within(document.body).findByRole("menuitemradio", { name: "Microphone 1", }); @@ -339,10 +308,7 @@ export const OnlyOneDevice: Story = { }, }; -/** - * A device has been asked for and has not arrived. Nothing in either section - * can be picked until it does, so a second request cannot overtake the first. - */ +/** A requested device hasn't arrived: nothing in either section can be picked. */ export const SelectionSettling: Story = { args: { ...Default.args, @@ -365,8 +331,7 @@ export const SelectionSettling: Story = { await userEvent.click(canvas.getByRole("button", { name: "Microphone" })); const menu = within(document.body); - // The story never changes `selectedOption`, which is what a device that has - // not taken effect yet looks like from here. + // selectedOption never changes, so the request stays in flight. await userEvent.click( await menu.findByRole("menuitemradio", { name: "Microphone 2" }), ); @@ -378,12 +343,8 @@ 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-source`: the - * attribute is what the stylesheet keys off, so asserting it would pass even - * with the rule deleted. + * The ring shows for keyboard focus only. Asserted on the painted outline: + * asserting data-focus-source would still pass with the CSS rule deleted. */ export const KeyboardFocusRing: Story = { args: { @@ -418,10 +379,7 @@ export const KeyboardFocusRing: Story = { }, }; -/** - * More devices than the menu can show. The list scrolls, and the meter stays at - * the foot of the Microphone section rather than scrolling away with it. - */ +/** The list scrolls; the meter stays at the foot of the microphone section. */ export const ManyDevices: Story = { args: { ...Default.args, @@ -444,11 +402,8 @@ export const ManyDevices: Story = { await userEvent.click(canvas.getByRole("button", { name: "Microphone" })); const menu = within(document.body); - // The scroll container and the opaque sticky wrapper, named rather than - // walked: the nesting between them is layout, and it moves. The wrapper - // rather than the meter itself, because this story is about where the meter - // sits, not what it reads — without a fake microphone, as on WebKit, it - // says it has no permission instead of showing a level. + // The sticky wrapper rather than the meter: without a fake microphone (as on + // WebKit) the meter shows a message instead of a level. const list = document.body.querySelector( `.${styles.deviceList}`, )!; @@ -461,10 +416,8 @@ export const ManyDevices: Story = { }); await expect(list.scrollHeight).toBeGreaterThan(list.clientHeight); - // Scrolled so the Microphone section starts at the top of the scrollport. - // Its devices then run past the bottom, which is the position that tells a - // pinned meter from one that simply happens to be the last element: at the - // very bottom of the list the two look identical. + // Scrolled so the microphone section starts at the top: only there does a + // pinned meter differ from one that is simply last. const group = await menu.findByRole("group", { name: "Microphone" }); list.scrollTop += group.getBoundingClientRect().top - list.getBoundingClientRect().top; @@ -477,35 +430,27 @@ export const ManyDevices: Story = { await expect(pinned.bottom).toBeLessThanOrEqual(scrollport.bottom + 1); await expect(pinned.top).toBeGreaterThanOrEqual(scrollport.top - 1); - // The whole menu is on screen. It opens upward from the foot of the call, - // so a list bounded by something bigger than the call — the document, say — - // runs off the top and takes the speakers with it. + // The whole menu is on screen, not bounded by the document. const frame = document.body .querySelector("[role='menu']")! .getBoundingClientRect(); await expect(frame.top).toBeGreaterThanOrEqual(0); await expect(frame.bottom).toBeLessThanOrEqual(window.innerHeight + 1); - // The meter is the one opaque thing in the menu, so it is the one thing - // that can cover the frame. Its box has to stay inside the menu's own. The - // paint itself needs a screenshot; this pins the geometry that decides it. + // The meter is the menu's one opaque part, so it must stay inside the frame. await expect(pinned.left).toBeGreaterThan(frame.left); await expect(pinned.right).toBeLessThan(frame.right); }, }; -/** The painted outline width, in pixels, however the stylesheet spells it. */ +/** Painted outline width, in px. */ function outlineWidth(element: HTMLElement): number { const { outlineStyle, outlineWidth } = getComputedStyle(element); if (outlineStyle === "none") return 0; return Number.parseFloat(outlineWidth) || 0; } -/** - * A platform that enumerates no output devices and offers no way to choose one - * — Safari. The section still names where audio is going, disabled, rather than - * leaving a heading with nothing under it. - */ +/** Safari lists no outputs: a disabled, selected Default stands in. */ export const OutputNotEnumerated: Story = { args: { ...Default.args, @@ -531,15 +476,7 @@ export const OutputNotEnumerated: Story = { }, }; -/** - * The level meter's icon sits on the same centre line as the radio controls of - * the devices above it. - * - * Held here because it is a fact about two components side by side, and because - * layout decides it: the meter's row is inset to keep the menu's frame clear, - * and its icon is a different size from a radio control, so the padding that - * lines them up is arithmetic that would otherwise go stale in silence. - */ +/** The meter's icon shares the radio controls' centre line; only a real browser lays this out. */ export const MeterAlignsWithTheDeviceRows: Story = { args: { ...Default.args, @@ -568,28 +505,19 @@ export const MeterAlignsWithTheDeviceRows: Story = { }, }; -/** Where an element sits on the inline axis, at its middle. */ +/** Inline-axis centre of an element. */ function centre(element: Element): number { const box = element.getBoundingClientRect(); return box.left + box.width / 2; } -/** - * Walking the device list with the keyboard, all the way to the last entry. - * - * The level meter stands over the foot of the list, so a row scrolled flush to - * the bottom edge arrives underneath it and can only half be read. Nothing in - * the DOM says an element is covered, so this compares where the two were - * actually drawn. - */ +/** Every row the keyboard reaches is fully visible, not under a heading or the meter. */ export const KeyboardReachesEveryDevice: Story = { args: { ...Default.args, iconsAndLabels: "audio", enabled: true, - // Enough of them that the list scrolls well past its own height, so that - // arrowing back up has to scroll too — which is where the heading can hide - // a row, as the meter can on the way down. + // Enough to scroll both ways, so either end can hide a row. options: Array.from({ length: 20 }, (_, i) => ({ label: { type: "name" as const, name: `Microphone ${i + 1}` }, id: `mic${i + 1}`, @@ -614,8 +542,7 @@ export const KeyboardReachesEveryDevice: Story = { return element; }); - // Everything that is drawn over the scrolling list: a heading holds the top - // while its section is in view, the meter holds the foot. + // Drawn over the list: the sticky headings and the meter. const overlays = [ meter, ...document.body.querySelectorAll( @@ -624,28 +551,20 @@ export const KeyboardReachesEveryDevice: Story = { ]; const items = within(document.body).getAllByRole("menuitemradio"); - // Down to the last device, as someone reading the list would, and back up - // again: a row can be hidden at either end. + // Down to the last device and back up. for (const key of ["{ArrowDown}", "{ArrowUp}"]) for (let i = 0; i < items.length; i++) { await userEvent.keyboard(key); const focused = document.activeElement as HTMLElement; await expect(focused).toHaveRole("menuitemradio"); - // Nothing is drawn over the row the keyboard has just reached. Stated - // as overlap rather than as an edge, because whether a heading is in - // the way depends on whether its section is still on screen. + // Checked as overlap: a heading only covers rows while its section is on + // screen. await expect(overlapping(focused, overlays)).toBeLessThanOrEqual(1); } }, }; -/** - * A long list scrolled well into the microphones. - * - * The heading of the section you are in stays at the top of the list, so it is - * always clear which kind of device the rows below are. It leaves with its own - * section rather than stacking with the next one. - */ +/** A section's heading stays at the top while the section is in view, and leaves with it. */ export const HeadingsStayWhileScrolling: Story = { args: { ...Default.args, @@ -672,14 +591,12 @@ export const HeadingsStayWhileScrolling: Story = { `.${styles.deviceList}`, )!; const group = await menu.findByRole("group", { name: "Microphone" }); - // By class, not role: the heading is aria-hidden decoration, because the - // group it belongs to is what carries the name. + // By class: the heading is aria-hidden, and its group carries the name. const heading = group.querySelector( `.${styles.sectionHeading}`, )!; - // Far enough in that the heading's own place in the list is well above the - // top of the scrollport: it is only still on screen if it is stuck there. + // Far enough that the heading is only on screen if it is stuck there. list.scrollTop += group.getBoundingClientRect().top - list.getBoundingClientRect().top + 80; @@ -690,7 +607,7 @@ export const HeadingsStayWhileScrolling: Story = { await expect(heading.getBoundingClientRect().bottom).toBeGreaterThan( scrollport.top, ); - // And it keeps clear of the menu's frame, as the meter does. + // Clear of the menu's frame. const frame = document.body .querySelector("[role='menu']")! .getBoundingClientRect(); @@ -700,13 +617,7 @@ export const HeadingsStayWhileScrolling: Story = { }, }; -/** - * How far an element is covered, in pixels, by the most overlapping of others. - * - * Nothing in the DOM says an element is obscured, and an element scrolled flush - * to an edge of its container looks no different there from one a sticky - * heading is sitting on top of. The boxes are the only witness. - */ +/** How far the most overlapping of `overlays` covers `element`, in px. */ function overlapping(element: Element, overlays: Element[]): number { const box = element.getBoundingClientRect(); return overlays.reduce((worst, overlay) => { @@ -717,13 +628,7 @@ function overlapping(element: Element, overlays: Element[]): number { }, 0); } -/** - * The camera menu's blur toggle, which the keyboard reaches after the cameras. - * - * It is a checkbox item and a child of the menu rather than of the device list, - * so a focus ring hung on the list alone left it with the browser's own — - * which follows the pointer, and is what the ring exists to replace. - */ +/** The blur toggle gets the keyboard ring too: it is the menu's child, not the list's. */ export const FocusRingCoversTheBlurToggle: Story = { args: { ...VideoUnmute.args, @@ -738,16 +643,14 @@ export const FocusRingCoversTheBlurToggle: Story = { name: /Blur background/, }); - // Arrowed down past the cameras to the toggle, which is the last thing in - // the menu. + // Arrow down past the cameras to the toggle. for (let i = 0; i < 6 && document.activeElement !== toggle; i++) await userEvent.keyboard("{ArrowDown}"); await expect(document.activeElement).toBe(toggle); // The same ring the device rows get. await expect(outlineWidth(toggle)).toBeGreaterThan(0); - // And the pointer takes it away again, with the toggle still focused — so - // there is something to light up and it is not lit. + // And the pointer takes it away, with the toggle still focused. await userEvent.hover(toggle); await expect(document.activeElement).toBe(toggle); await expect(outlineWidth(toggle)).toBe(0); diff --git a/src/components/MediaMuteAndSwitchButton.test.tsx b/src/components/MediaMuteAndSwitchButton.test.tsx index 902a3bf29..b11b02d55 100644 --- a/src/components/MediaMuteAndSwitchButton.test.tsx +++ b/src/components/MediaMuteAndSwitchButton.test.tsx @@ -27,8 +27,8 @@ import { type MediaDevices } from "../state/MediaDevices"; import { restoreAudioCapture, stubAudioCapture } from "../utils/test"; import type * as MediaDevicesContextModule from "../MediaDevicesContext"; -// The menu reads the devices once per render and the meter never does, so -// counting these calls counts how often the menu itself re-rendered. +// The menu reads the devices on every render and the meter never does, so +// these calls count the menu's own renders. vi.mock("../MediaDevicesContext", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, useMediaDevices: vi.fn(actual.useMediaDevices) }; @@ -61,8 +61,7 @@ function renderComponent( } describe("MediaMuteAndSwitchButton", () => { - // Only one test stubs the capture, but leaving it stubbed would follow - // every later test in the run. + // Only one test stubs the capture; don't let it leak into the rest. afterEach(restoreAudioCapture); test("renders", () => { @@ -393,8 +392,7 @@ describe("MediaMuteAndSwitchButton", () => { screen.getByRole("menuitemradio", { name: "Microphone 2" }), ); - // In flight: nothing else can be picked, in either section, so a second - // request cannot overtake the first. + // In flight: nothing else can be picked, in either section. for (const name of ["Microphone 1", "Speakers", "Headset"]) { expect(screen.getByRole("menuitemradio", { name })).toHaveAttribute( "aria-disabled", @@ -407,7 +405,7 @@ describe("MediaMuteAndSwitchButton", () => { await promise; }); - // Settled: choosable again. + // Settled: selectable again. expect( screen.getByRole("menuitemradio", { name: "Microphone 1" }), ).not.toHaveAttribute("aria-disabled", "true"); @@ -418,9 +416,7 @@ describe("MediaMuteAndSwitchButton", () => { test("lets go of a device switch that never arrives", async () => { const user = userEvent.setup(); - // onSelect that never reports back is what a device removed mid-switch - // looks like from here: the selection falls back to the default, so what - // was asked for never becomes the selection. + // onSelect never reports back, as when a device is removed mid-switch. const { getByRole } = renderComponent( { await user.keyboard("{Escape}"); await user.click(getByRole("button", { name: "Microphone" })); - // Otherwise every device, in both sections, stays unselectable for the - // rest of the call. for (const name of ["Microphone 1", "Speakers", "Headset"]) { expect(screen.getByRole("menuitemradio", { name })).not.toHaveAttribute( "aria-disabled", @@ -485,20 +479,18 @@ describe("MediaMuteAndSwitchButton", () => { screen.getByRole("menuitemradio", { name: "Microphone 2" }), ); - // The second microphone is unplugged before the switch to it lands, so it - // is never going to become the selection. + // The second microphone is unplugged before the switch lands. rerender(withProviders(menu(mics.slice(0, 1)))); - // The speakers are choosable again without the menu being closed: a - // request for a device that is gone is not in flight, it is over. + // Selectable again without closing the menu. expect( screen.getByRole("menuitemradio", { name: "Headset" }), ).not.toHaveAttribute("aria-disabled", "true"); }); test("redraws the meter and not the device rows around it", async () => { - // A level arrives many times a second. Held in the menu it would reconcile - // every device row on its way to the bars, so it is held in the meter. + // The level is held by the meter, so a moving level doesn't re-render the + // menu. const capture = stubAudioCapture(); const user = userEvent.setup(); const menuRenders = vi.mocked(useMediaDevices); @@ -529,16 +521,13 @@ describe("MediaMuteAndSwitchButton", () => { const meter = await screen.findByRole("meter"); const settled = menuRenders.mock.calls.length; - // The meter follows elapsed time rather than frames, so hand-driven frames - // need a clock to move at all: a frame every 16ms, from where the capture - // started. + // The meter smooths by elapsed time, so hand-driven frames need a clock. let elapsed = performance.now(); const clock = vi .spyOn(performance, "now") .mockImplementation(() => (elapsed += 16)); - // One frame per task, as a browser delivers them: drawing them all inside - // one act would let React batch what it would not batch in a browser. + // One frame per task, as a browser delivers them: one act() would batch them. for (let step = 1; step <= 8; step++) { capture.speak(step / 8); await act(async () => { @@ -548,9 +537,7 @@ describe("MediaMuteAndSwitchButton", () => { } clock.mockRestore(); - // The level moved... expect(meter.getAttribute("aria-valuenow")).not.toBe("0"); - // ...and the menu around it did not render once on the way. expect(menuRenders.mock.calls.length - settled).toBe(0); }); @@ -572,10 +559,8 @@ describe("MediaMuteAndSwitchButton", () => { await user.click(getByRole("button", { name: "Camera" })); - // Same selection pattern as the microphone menu. screen.getByRole("menuitemradio", { name: "Camera 1", checked: true }); screen.getByRole("menuitemradio", { name: "Camera 2", checked: false }); - // And background blur is still reachable from here. expect( screen.getByRole("menuitemcheckbox", { name: "Blur background" }), ).toBeInTheDocument(); @@ -601,8 +586,7 @@ describe("MediaMuteAndSwitchButton", () => { .getByRole("menuitemradio", { name: "Microphone 1" }) .closest("[data-focus-source]"); - // The menu focuses whatever the pointer is over, so focus alone says - // nothing about how someone is navigating. + // The menu focuses whatever the pointer is over, so focus alone says nothing. expect(list).toHaveAttribute("data-focus-source", "pointer"); await user.keyboard("{ArrowDown}"); @@ -636,8 +620,7 @@ describe("MediaMuteAndSwitchButton", () => { .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. + // readOnly would paint the selected radio muted. expect(selected).not.toHaveAttribute("readonly"); }); @@ -664,8 +647,6 @@ describe("MediaMuteAndSwitchButton", () => { 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(); }); @@ -782,7 +763,7 @@ describe("MediaMuteAndSwitchButton", () => { await user.click(getByRole("button", { name: "Microphone" })); - // Shown rather than hidden, so the menu keeps its shape, but not choosable. + // Shown, but not selectable. const only = screen.getByRole("menuitemradio", { name: "Microphone 1" }); expect(only).toHaveAttribute("aria-disabled", "true"); }); @@ -799,8 +780,7 @@ describe("MediaMuteAndSwitchButton", () => { ]} selectedOption="mic1" onSelect={vi.fn()} - // Safari enumerates no output devices at all, and offers no way to - // choose one. + // As Safari: no outputs listed. outputOptions={[]} selectedOutputOption={undefined} onSelectOutput={undefined} @@ -809,8 +789,6 @@ describe("MediaMuteAndSwitchButton", () => { await user.click(getByRole("button", { name: "Microphone" })); - // A heading with nothing under it says the feature is broken. Audio is - // playing somewhere, so the section names that somewhere and disables it. const speakers = screen .getAllByRole("group") .find((group) => group.getAttribute("aria-label") === "Speaker")!; @@ -818,8 +796,6 @@ describe("MediaMuteAndSwitchButton", () => { expect(entries).toHaveLength(1); expect(entries[0]).toHaveAccessibleName("Default"); expect(entries[0]).toHaveAttribute("aria-disabled", "true"); - // And marked as the selection: it is where audio is going, so an unchecked - // lone entry would read as nothing being chosen at all. expect(entries[0]).toHaveAttribute("aria-checked", "true"); expect( within(entries[0]).getByRole("radio", { hidden: true }), @@ -843,7 +819,6 @@ describe("MediaMuteAndSwitchButton", () => { { label: { type: "name", name: "Headset" }, id: "spk2" }, ]} selectedOutputOption="spk1" - // No callback: nothing can be picked here. onSelectOutput={undefined} />, ); @@ -856,7 +831,6 @@ describe("MediaMuteAndSwitchButton", () => { 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 9b2065ce5..25342f001 100644 --- a/src/components/MediaMuteAndSwitchButton.tsx +++ b/src/components/MediaMuteAndSwitchButton.tsx @@ -62,10 +62,7 @@ export interface MediaMuteAndSwitchButtonProps { outputOptions?: MenuOptions[]; /** The output option currently rendered as selected */ selectedOutputOption?: string; - /** - * Called when an output device is picked. Undefined means no output can be - * chosen here, and the section renders disabled. - */ + /** Picks an output device. Undefined disables the speaker section. */ onSelectOutput?: (id: string) => void; videoBlurToggleClick?: () => void; videoBlurEnabled?: boolean; @@ -101,9 +98,8 @@ export const MediaMuteAndSwitchButton: FC = ({ videoBlurToggleClick, onSelect, }) => { - // Which device we have asked for but not yet been given. Carries the kind as - // well as the id, because an input and an output can share an id: "default" - // names both on Chrome. + // Requested but not yet selected. Keyed by kind too, since Chrome uses + // "default" for both an input and an output. const [plannedSelection, setPlannedSelection] = useState<{ kind: "input" | "output"; id: string; @@ -111,7 +107,7 @@ export const MediaMuteAndSwitchButton: FC = ({ const [menuOpen, setMenuOpen] = useState(false); const onOpenChange = useCallback((open: boolean): void => { setMenuOpen(open); - // A request that never arrived does not outlive the menu it was made in. + // Drop a request that never arrived. if (!open) setPlannedSelection(null); }, []); const isBusy = busy ?? false; @@ -144,18 +140,12 @@ export const MediaMuteAndSwitchButton: FC = ({ [], ); - // The menu is portalled outside the call root, so nothing in the stylesheets - // 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 on the call area: CSS can't size the portalled menu against it. const rootElement = useRootElement(); const [listMaxHeight, setListMaxHeight] = useState(); useEffect(() => { if (!menuOpen) return; - // Followed rather than measured once: a host can resize the space Element - // Call is drawn in while the menu is open — a panel animating, a window - // dragged, a phone turned — and a bound taken on opening then describes a - // call area that no longer exists. Quantised before it reaches React, so a - // resize re-renders only when the bound itself moves. + // Followed, since a host can resize the call while the menu is open. const subscription = observeElementSize$(rootElement) .pipe( map(({ height }) => @@ -167,23 +157,17 @@ export const MediaMuteAndSwitchButton: FC = ({ return (): void => subscription.unsubscribe(); }, [menuOpen, rootElement]); - // The meter sits over the foot of the scrolling list, so the list has to - // keep that much of itself clear: a row scrolled to by the keyboard would - // otherwise arrive underneath it, half-read. Its own height, measured, - // because the failure states are two lines where a level is one. + // Kept clear at the list's foot, so a row reached by keyboard isn't under + // the meter. const [meterHeight, meter] = useMeasuredHeight(); - // The headings stand over the head of the list, so it has to keep their - // height clear too — the same bargain as the meter, at the other end. One - // measurement serves both: the sections are headed alike. + // Likewise at its head, for the sticky headings. const [headingHeight, heading] = useMeasuredHeight(); useEffect(() => { if (menuOpen) devices.requestDeviceNames(); // No-op after the first call }, [menuOpen, devices]); - // The mute control differs between the two only in which button it is and - // what it is called; how it behaves is the same, and was worth saying once. const MuteButton = iconsAndLabels === "audio" ? MicButton : VideoButton; const button = ( = ({ /> ); - // Only the camera menu carries a toggle, and only when the caller offers one. const toggles = iconsAndLabels === "video" && videoBlurToggleClick !== undefined ? [ @@ -231,7 +214,6 @@ export const MediaMuteAndSwitchButton: FC = ({ break; } - /** The text shown for a device, whichever kind of label it carries. */ const labelText = ( label: MenuOptions["label"], numbered: (n: number) => string, @@ -252,13 +234,8 @@ export const MediaMuteAndSwitchButton: FC = ({ } }; - // A device we asked for that has not arrived yet. Until it does, nothing in - // the menu can be picked, so a second request cannot overtake the first. - // - // A request only counts as in flight while the device is still on offer. One - // that is removed before it takes effect never arrives — the selection falls - // back to the default instead — and waiting for it would leave every device - // in both sections unselectable for the rest of the call. + // Nothing can be picked while a requested device is on offer but not yet + // selected, so a second request can't overtake it. const plannedOutput = plannedSelection?.kind === "output"; const selectedOfPlannedKind = plannedOutput ? selectedOutputOption @@ -269,17 +246,12 @@ export const MediaMuteAndSwitchButton: FC = ({ plannedSelection.id !== selectedOfPlannedKind && offeredOfPlannedKind?.some(({ id }) => id === plannedSelection.id) === true; - // Safari enumerates no output devices at all, and offers no way to choose - // one, so the list arrives empty. The section is shown all the same — audio - // is playing somewhere — naming that somewhere and disabling it like any - // single entry. A heading with nothing beneath it reads as a broken feature, - // and leaves the menu a different shape on one browser. + // Safari lists no outputs: show a disabled, selected Default rather than an + // empty section. const noOutputsListed = outputOptions?.length === 0; const speakerOptions: MenuOptions[] | undefined = noOutputsListed ? [{ id: DEFAULT_OUTPUT_ID, label: { type: "default", name: null } }] : outputOptions; - // And it is the selection, not merely the only row: it is where audio is - // going. An unchecked lone entry reads as nothing being chosen at all. const selectedSpeaker = noOutputsListed ? DEFAULT_OUTPUT_ID : selectedOutputOption; @@ -292,30 +264,23 @@ export const MediaMuteAndSwitchButton: FC = ({ numbered: (n: number) => string, ): ReactElement[] => { const list = items ?? []; - // 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. + // Disabled rather than hidden, so the menu keeps its shape. const disabled = select === undefined || list.length <= 1 || settling; return list.map(({ label, id }) => ( {}} /> @@ -377,8 +342,7 @@ export const MediaMuteAndSwitchButton: FC = ({ >
= ({ {iconsAndLabels === "audio" && speakerOptions && ( <> {/* A menu may only contain items, separators and groups, so each - heading belongs to a group rather than sitting beside the - items it names. */} + heading is a hidden part of a named group. */}
- {/* The heading is decoration: the group carries the name, and - a menu may only contain items, separators and groups. */}
@@ -417,8 +378,7 @@ export const MediaMuteAndSwitchButton: FC = ({
- {/* The heading sits outside, so the meter can never ride up over it: - sticky only holds while this block is in view. */} + {/* Apart from the heading, so the sticky meter can't ride over it. */}
{deviceItems( "input", @@ -431,8 +391,7 @@ export const MediaMuteAndSwitchButton: FC = ({ @@ -457,12 +416,7 @@ export const MediaMuteAndSwitchButton: FC = ({ ); }; -/** - * Follows an element's height, for the two pieces of chrome that stand over the - * scrolling device list. Both have to keep their own height clear of it, and - * neither height is knowable in advance: a heading wraps, and the meter's - * failure states are two lines where a level is one. - */ +/** Follows an element's height. */ function useMeasuredHeight(): [ number | undefined, (element: HTMLElement | null) => (() => void) | undefined, diff --git a/src/components/MicrophoneLevelMeter.module.css b/src/components/MicrophoneLevelMeter.module.css index 25444da4f..83e440c55 100644 --- a/src/components/MicrophoneLevelMeter.module.css +++ b/src/components/MicrophoneLevelMeter.module.css @@ -6,25 +6,18 @@ Please see LICENSE in the repository root for full details. */ .meter { - /* The icon column, the same width as a device row's, so the bars start where - the device names start. */ + /* A device row's icon column, so the bars start where the names do. */ --meter-icon-size: 24px; - /* Compound's radio control, which the icon has to share a centre line with. */ + /* Compound's radio size, whose centre line the icon shares. */ --device-control-size: 20px; display: flex; align-items: center; gap: var(--cpd-space-3x); padding-block: var(--cpd-space-2x); - /* The icon shares a centre with the radio controls in the device rows above, - which sit one 4x padding in from the menu's edge. Two things pull this row - out of line with them, and both come off the padding: the row is inset by a - border width so that the menu's frame stays visible behind it (see - `.stickyMeter`), and the icon is wider than a radio control, so it starts - half that difference further left for the two to share a centre. Both are - measured in the MeterAlignsWithTheDeviceRows story rather than trusted: - jsdom lays nothing out, so only a real browser can hold this. */ + /* Centres the icon on the radio column: less the row's inset border and half + the icon's extra width. Checked in the MeterAlignsWithTheDeviceRows story. */ padding-inline-start: calc( var(--cpd-space-4x) - var(--cpd-border-width-1) - (var(--meter-icon-size) - var(--device-control-size)) / 2 @@ -40,27 +33,15 @@ Please see LICENSE in the repository root for full details. } .segments { - /* A bar and the space beside it are always the same size; what changes with - the width available is how many bars there are. Spreading a fixed number of - bars instead would make the meter a different shape in every place it is - used, and close them up into one solid block wherever the space ran short — - and a meter whose bars touch stops reading as a count, which is what - carries the level for anyone who cannot rely on the colour. - - Both sizes are set here and nowhere else: the component reads them back off - the rendered bars to work out how many fit. */ + /* Bars keep one size and the width decides how many; the component reads the + sizes back from here. */ flex: 1; - /* The width decides the count, never the other way round. Containment is - what enforces that: without it the bars are both a floor under the width - and the widest thing in the menu, so the meter would set the menu's width - and the device names — which are what a person is reading — would have to - fit around it. */ + /* So the bars never set the menu's width. */ contain: inline-size; min-inline-size: 0; display: flex; align-items: center; - /* Half again as wide as a bar, as the design has it: bars set as close as - their own width read as a solid block long before they touch. */ + /* Half again a bar's width, per the design. */ gap: var(--cpd-space-1-5x); } @@ -72,16 +53,13 @@ Please see LICENSE in the repository root for full details. background: var(--cpd-color-bg-subtle-primary); } -/* Filled segments carry the level by count as well as by colour, so the meter - stays readable without relying on hue. */ +/* The count of lit bars carries the level as well as their colour. */ .segmentLit { background: var(--cpd-color-bg-accent-rest); } .message { color: var(--cpd-color-text-secondary); - /* A paragraph brings a margin below it, which in a centred row does not push - the text down but lifts it: the margin box is what gets centred, so the - words end up above the middle and the icon beside them looks low. */ + /* A paragraph's margin would lift the text off centre. */ margin-block: 0; } diff --git a/src/components/MicrophoneLevelMeter.stories.tsx b/src/components/MicrophoneLevelMeter.stories.tsx index 8efbd374b..423472bc9 100644 --- a/src/components/MicrophoneLevelMeter.stories.tsx +++ b/src/components/MicrophoneLevelMeter.stories.tsx @@ -16,15 +16,7 @@ import { import styles from "./MicrophoneLevelMeter.module.css"; import { LEVEL_SCALE } from "../state/MicrophoneLevel"; -/** - * A width to show the meter at, close to the menu it lives in. - * - * Not a copy of the menu's width, and nothing depends on the two agreeing: a - * bar and the gap beside it are a fixed size now, so this only decides how many - * bars there is room for. Without a width at all the stories would shrink-wrap - * to almost nothing and show a meter two bars wide, which is no use to anyone - * looking at them. - */ +/** Roughly the menu's width. It only decides how many bars fit. */ const STORY_WIDTH = 256; const meta = { @@ -47,10 +39,7 @@ const meta = { export default meta; type Story = StoryObj; -/** - * A quiet room. Nothing is lit: room hiss below the noise floor must not read - * as "it can hear me". - */ +/** A quiet room: hiss below the noise floor lights nothing. */ export const Silent: Story = { args: { state: { type: "level", level: 0 } }, play: async ({ canvasElement }) => { @@ -65,11 +54,7 @@ export const QuietSpeech: Story = { export const NormalSpeech: Story = { args: { state: { type: "level", level: 12 } }, play: async ({ canvasElement }) => { - // Shown at something like the width of the menu, so the meter in a story - // reads like the meter in a call rather than like a handful of bars. The - // design's own mock has sixteen of them at this width; a floor rather than - // a count, because the number follows from the bar and gap sizes and those - // are the design's to change. + // A floor, not a count: the count follows from the design's bar and gap sizes. await expect( canvasElement.getElementsByClassName(styles.segment).length, ).toBeGreaterThanOrEqual(15); @@ -80,10 +65,7 @@ export const LoudSpeech: Story = { args: { state: { type: "level", level: LEVEL_SCALE } }, }; -/** - * The three volumes differ by how many bars are lit, so the level survives - * greyscale and a screen reader as well as it survives colour. - */ +/** The three volumes differ in how many bars are lit, not only in colour. */ export const VolumesAreDistinguishable: Story = { args: { state: { type: "level", level: 5 } }, play: async ({ canvasElement, mount }) => { @@ -96,17 +78,12 @@ export const VolumesAreDistinguishable: Story = { String(level), ); } - // Three different counts, rising: the level is carried by how many bars - // are lit, not by their colour alone. await expect(new Set(lit).size).toBe(lit.length); await expect(lit).toEqual([...lit].sort((a, b) => a - b)); }, }; -/** - * Permission refused. A message with a next action, never a still meter that - * reads as silence. - */ +/** Permission refused: a message with a next action, not a still meter. */ export const PermissionDenied: Story = { args: { state: { type: "permission-denied" } }, play: async ({ canvasElement }) => { @@ -118,7 +95,7 @@ export const PermissionDenied: Story = { }, }; -/** No input device at all, told apart from a refusal. */ +/** No input device, told apart from a refusal. */ export const NoDevice: Story = { args: { state: { type: "no-device" } }, play: async ({ canvasElement }) => { @@ -126,9 +103,7 @@ export const NoDevice: Story = { await expect(canvas.queryByRole("meter")).toBeNull(); await expect(canvas.getByText(/No microphone found/)).toBeVisible(); - // The icon sits on the middle of the words, however many lines they run to. - // A paragraph's own margin would centre its margin box instead, leaving the - // text high and the icon looking low beside it. + // The icon sits on the middle of the text, however many lines it runs to. const middle = (element: Element): number => { const box = element.getBoundingClientRect(); return box.top + box.height / 2; @@ -139,15 +114,7 @@ export const NoDevice: Story = { }, }; -/** - * The same meter at two widths. - * - * A bar and the space beside it are always the same size; what changes is how - * many bars there are. Spreading a fixed number of bars instead would make the - * meter a different shape in every place it is used, and close the bars up into - * one block wherever the space ran short — and bars that touch cannot be - * counted, which is what carries the level without colour. - */ +/** The same meter at two widths: the bars keep their size and only their count changes. */ export const ShapeStaysTheSameAtAnyWidth: Story = { args: { state: { type: "level", level: 12 } }, play: async ({ mount, args }) => { @@ -157,17 +124,17 @@ export const ShapeStaysTheSameAtAnyWidth: Story = { await expect(narrow.bar).toBe(wide.bar); await expect(narrow.gap).toBe(wide.gap); await expect(narrow.count).toBeLessThan(wide.count); - // And the bars are still bars, not one run of colour. + // The bars are still separate, not one run of colour. await expect(narrow.gap).toBeGreaterThan(0); }, }; -/** How many bars are painted as carrying level, rather than as empty. */ +/** How many bars are lit. */ function litSegments(canvasElement: HTMLElement): number { return canvasElement.getElementsByClassName(styles.segmentLit).length; } -/** Renders the meter at one width and reports the shape of its bars. */ +/** Renders the meter at one width and reports its bars' shape. */ async function measureAt( mount: (ui: JSX.Element) => Promise, args: MicrophoneLevelMeterProps, diff --git a/src/components/MicrophoneLevelMeter.test.tsx b/src/components/MicrophoneLevelMeter.test.tsx index b215f39e9..8723791ae 100644 --- a/src/components/MicrophoneLevelMeter.test.tsx +++ b/src/components/MicrophoneLevelMeter.test.tsx @@ -25,7 +25,6 @@ describe("MicrophoneLevelMeter", () => { const denied = render( , ); - // A next action, not a flat meter that reads as silence. expect( denied.getByText(/Microphone access is blocked/), ).toBeInTheDocument(); diff --git a/src/components/MicrophoneLevelMeter.tsx b/src/components/MicrophoneLevelMeter.tsx index 38516b20a..0382aa55f 100644 --- a/src/components/MicrophoneLevelMeter.tsx +++ b/src/components/MicrophoneLevelMeter.tsx @@ -28,12 +28,8 @@ export interface LiveMicrophoneLevelMeterProps { } /** - * The level meter, wired to a live capture of a microphone. - * - * A component of its own so that a level which does change redraws the meter - * and not whatever is rendered beside it. A moving level arrives many times a - * second; held in the menu, it would reconcile every device row on the way to - * the bars. + * The meter, wired to a live capture. Its own component so a changing level + * re-renders the meter, not its parent. */ export const LiveMicrophoneLevelMeter: FC = ({ deviceId, @@ -48,27 +44,18 @@ export const LiveMicrophoneLevelMeter: FC = ({ export interface MicrophoneLevelMeterProps { state: MicrophoneState; className?: string; - /** - * The meter's own element. Its height is what a scroll container has to keep - * clear to stop the meter covering the row it has just scrolled to. - */ ref?: Ref; } -/** - * The live input level of the selected microphone, shown beneath it, or the - * reason there is no level to show. - */ +/** The input level of a microphone, or why there is none. */ export const MicrophoneLevelMeter: FC = ({ state, className, ref, }) => { const { t } = useTranslation(); - // How many bars there is room for. The bars never change size, so this is - // what absorbs a change of width. Starts at the full count so that the first - // paint is a meter rather than a single bar, and so that a renderer with no - // layout at all — jsdom — still draws the whole thing. + // Bars keep one size, so their count follows the width. Starts full so the + // first paint, and jsdom, draw a whole meter. const [barCount, setBarCount] = useState(LEVEL_SCALE); const track = useCallback( (element: HTMLDivElement | null): (() => void) | undefined => { @@ -112,8 +99,7 @@ export const MicrophoneLevelMeter: FC = ({ key={i} aria-hidden className={classNames(styles.segment, { - // The level is a share of the scale, not a count of bars: how - // many stand for it depends on how many there are. + // The level is a share of the scale, not a bar count. [styles.segmentLit]: i < Math.round((state.level / LEVEL_SCALE) * barCount), })} @@ -125,15 +111,11 @@ export const MicrophoneLevelMeter: FC = ({ ); }; -/** - * How many bars fit across `width`, measured off a rendered one rather than - * told: a bar's size is a design question, settled in the stylesheet, and - * reading it back is what keeps it from being settled twice. - */ +/** How many bars fit across `width`, measured so the stylesheet alone sets their size. */ function barsThatFit(track: HTMLElement, width: number): number { const gap = Number.parseFloat(getComputedStyle(track).columnGap); const bar = track.firstElementChild?.getBoundingClientRect().width ?? 0; - // A renderer that lays nothing out tells us nothing; keep the full count. + // No layout (jsdom): keep the full count. if (!(bar > 0) || !(gap >= 0)) return LEVEL_SCALE; return Math.max(1, Math.floor((width + gap) / (bar + gap))); } diff --git a/src/components/useMicrophoneLevel.test.tsx b/src/components/useMicrophoneLevel.test.tsx index 1258bfeea..2c651457a 100644 --- a/src/components/useMicrophoneLevel.test.tsx +++ b/src/components/useMicrophoneLevel.test.tsx @@ -11,9 +11,7 @@ import { renderHook } from "@testing-library/react"; import { useMicrophoneLevel } from "./useMicrophoneLevel"; import { restoreAudioCapture, stubAudioCapture } from "../utils/test"; -// The capture itself, and releasing it, belong to observeMicrophoneState$ and -// are covered in src/state/MicrophoneLevel.test.ts. What is left here is the -// bridging: when the hook watches, and what it reports before it has an answer. +// Capture and release are covered in MicrophoneLevel.test.ts; this covers the bridge. describe("useMicrophoneLevel", () => { afterEach(restoreAudioCapture); @@ -36,7 +34,7 @@ describe("useMicrophoneLevel", () => { rerender({ id: "mic2" }); - // No level carried over: the meter reads the new device or nothing at all. + // No level carried over from the previous device. expect(result.current).toEqual({ type: "level", level: 0 }); }); }); diff --git a/src/components/useMicrophoneLevel.ts b/src/components/useMicrophoneLevel.ts index 6e295d3e4..f357744db 100644 --- a/src/components/useMicrophoneLevel.ts +++ b/src/components/useMicrophoneLevel.ts @@ -14,14 +14,7 @@ import { const IDLE: MicrophoneState = { type: "level", level: 0 }; -/** - * Reads the live input level of a microphone, while `active`. - * - * A bridge and nothing else: the capture, its lifetime and the maths belong to - * {@link observeMicrophoneState$}. Scoped to `active` so the device is held - * only while whatever shows the meter is on screen, rather than for the length - * of a call. - */ +/** The live level of a microphone, captured only while `active`. */ export function useMicrophoneLevel( deviceId: string | undefined, active: boolean, @@ -30,8 +23,7 @@ export function useMicrophoneLevel( useEffect(() => { if (!active) return; - // Idle first, so a new device starts from nothing rather than from the - // level the previous one was reading. + // Idle first, so a new device doesn't start from the previous level. setState(IDLE); const subscription = observeMicrophoneState$(deviceId).subscribe(setState); return (): void => subscription.unsubscribe(); diff --git a/src/room/LobbyView.test.tsx b/src/room/LobbyView.test.tsx index a75dc1c79..3aa4f136f 100644 --- a/src/room/LobbyView.test.tsx +++ b/src/room/LobbyView.test.tsx @@ -191,7 +191,6 @@ describe("LobbyView microphone level", () => { afterEach(() => { vi.unstubAllGlobals(); - // Put navigator back, or every later test in the run inherits the stub. if (realMediaDevices === undefined) { Reflect.deleteProperty(navigator, "mediaDevices"); } else { @@ -199,7 +198,6 @@ describe("LobbyView microphone level", () => { } }); - /** Just enough of the Web Audio and capture APIs for the meter to run. */ function stubAudioCapture(): void { vi.stubGlobal( "AudioContext", @@ -217,8 +215,7 @@ describe("LobbyView microphone level", () => { public close(): void {} }, ); - // Only this property: replacing navigator wholesale drops the getters on - // its prototype, such as userAgent. + // Only this property: replacing navigator loses getters like userAgent. Object.defineProperty(navigator, "mediaDevices", { configurable: true, value: { @@ -241,8 +238,7 @@ describe("LobbyView microphone level", () => { }, } 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. + // Pre-join reaches the meter through the same chevron as a call. await user.click(getByRole("button", { name: "Microphone" })); expect( diff --git a/src/state/MediaDevices.test.ts b/src/state/MediaDevices.test.ts index 739f6d196..69c23765c 100644 --- a/src/state/MediaDevices.test.ts +++ b/src/state/MediaDevices.test.ts @@ -14,7 +14,6 @@ vi.mock("../Platform", () => ({ }, isFirefox: (): boolean => false, })); -// One observer per device kind, so a test can add and remove hardware. const observers = vi.hoisted( () => new Map void }>(), ); @@ -75,7 +74,7 @@ function device(deviceId: string, label: string, groupId = deviceId): object { return { deviceId, label, groupId, kind: "audioinput" }; } -/** Replaces the hardware of one kind, as the browser would report it. */ +/** Replaces the devices of one kind, as the browser reports them. */ function setDevices(kind: string, devices: object[]): void { const observer = observers.get(kind); if (observer === undefined) throw new Error(`nothing observing ${kind}`); @@ -103,7 +102,7 @@ describe("MediaDevices selection", () => { devices.audioInput.select("mic2"); - // A later call on the same machine reads the same stored preference. + // A later call reads the same stored preference. expect(newMediaDevices().audioInput.selected$.value?.id).toBe("mic2"); }); @@ -112,7 +111,6 @@ describe("MediaDevices selection", () => { setDevices("audioinput", [device("mic1", "Microphone 1")]); expect([...devices.audioInput.available$.value.keys()]).toEqual(["mic1"]); - // A headset is plugged in. setDevices("audioinput", [ device("mic1", "Microphone 1"), device("mic2", "Headset"), @@ -122,7 +120,6 @@ describe("MediaDevices selection", () => { "mic2", ]); - // And unplugged again. setDevices("audioinput", [device("mic1", "Microphone 1")]); expect([...devices.audioInput.available$.value.keys()]).toEqual(["mic1"]); }); @@ -136,7 +133,6 @@ describe("MediaDevices selection", () => { devices.audioInput.select("mic2"); expect(devices.audioInput.selected$.value?.id).toBe("mic2"); - // The headset is unplugged mid-call. setDevices("audioinput", [device("mic1", "Microphone 1")]); expect(devices.audioInput.selected$.value?.id).toBe("mic1"); @@ -145,7 +141,6 @@ describe("MediaDevices selection", () => { test("falls back when the remembered device is absent", () => { const devices = newMediaDevices(); setDevices("audioinput", [device("mic1", "Microphone 1")]); - // Remembered from a previous call, on hardware that is not here now. devices.audioInput.select("a-device-from-last-time"); expect(devices.audioInput.selected$.value?.id).toBe("mic1"); @@ -153,7 +148,7 @@ describe("MediaDevices selection", () => { test("falls back to numbered labels when labels are unavailable", () => { const devices = newMediaDevices(); - // The browser withholds names until permission has been granted. + // Names are withheld until permission is granted. setDevices("audioinput", [device("mic1", ""), device("mic2", "")]); expect([...devices.audioInput.available$.value.values()]).toEqual([ @@ -167,10 +162,7 @@ describe("MediaDevices selection", () => { setDevices("audiooutput", [device("spk1", "Speakers")]); const available = devices.audioOutput.available$.value; - // Default follows the operating system and re-points when it changes, so - // it is its own choice rather than an alias for the device it resolves to. - // It carries no name of its own precisely because which device it resolves - // to is not knowable from here. + // Default is its own entry, unnamed: which device it resolves to isn't knowable. expect(available.get("spk1")).toEqual({ type: "name", name: "Speakers" }); expect(available.get("")).toEqual({ type: "default", name: null }); }); diff --git a/src/state/MicrophoneLevel.test.ts b/src/state/MicrophoneLevel.test.ts index 79c849782..b99988f81 100644 --- a/src/state/MicrophoneLevel.test.ts +++ b/src/state/MicrophoneLevel.test.ts @@ -23,8 +23,6 @@ describe("segmentsForVolume", () => { }); test("shows nothing for the hiss of a quiet room", () => { - // Without a noise floor these light the first segments permanently, which - // reads as "it can hear me" when nobody is speaking. expect(segmentsForVolume(0.005)).toBe(0); expect(segmentsForVolume(0.015)).toBe(0); }); @@ -40,8 +38,7 @@ describe("segmentsForVolume", () => { }); test("moves the meter visibly for normal speech", () => { - // Ordinary speech should reach the middle of the meter, not scrape along - // the floor: a meter that barely moves reads as a broken microphone. + // Ordinary speech reaches the middle of the meter. expect(segmentsForVolume(0.2)).toBeGreaterThanOrEqual(LEVEL_SCALE / 4); }); @@ -65,16 +62,14 @@ describe("smoothVolume", () => { }); test("registers a syllable as it starts", () => { - // Most of the way there within one attack time constant, so speech does - // not lag the speaker. + // Most of the way within one attack time constant. expect(smoothVolume(0, 1, ATTACK_MS)).toBeGreaterThan(0.6); }); test("rides over the gaps between words", () => { - // A pause of a few tens of milliseconds should not collapse the meter, or - // it flickers rather than reading as a level. + // A short pause doesn't collapse the meter... expect(smoothVolume(1, 0, 30)).toBeGreaterThan(0.7); - // A real silence still brings it down. + // ...but a real silence brings it down. expect(smoothVolume(1, 0, RELEASE_MS * 3)).toBeLessThan(0.1); }); @@ -98,8 +93,7 @@ describe("observeMicrophoneState$", () => { const capture = stubAudioCapture(); const subscription = observeMicrophoneState$("mic1").subscribe(); - // The user gives up on the permission prompt and closes the menu, and only - // then does the browser hand the microphone over. + // The menu closes before the browser hands the microphone over. subscription.unsubscribe(); capture.grant(); await vi.waitFor(() => expect(capture.track.stop).toHaveBeenCalled()); @@ -120,8 +114,7 @@ describe("observeMicrophoneState$", () => { test("gives the microphone back when the audio graph fails to build", async () => { const capture = stubAudioCapture(); - // The graph fails only after getUserMedia has handed the device over, so - // there is a live capture to lose. + // Fails only after getUserMedia has granted the device. vi.stubGlobal( "AudioContext", class { @@ -138,8 +131,6 @@ describe("observeMicrophoneState$", () => { capture.grant(); await vi.waitFor(() => expect(seen).toContain("no-device")); - // Without this the microphone stays open, and its in-use light on, behind - // a meter that says it is unavailable. expect(capture.track.stop).toHaveBeenCalled(); subscription.unsubscribe(); @@ -173,8 +164,7 @@ describe("observeMicrophoneState$", () => { capture.grant(); await vi.waitFor(() => expect(emissions).toBe(1)); - // The analyser is read every animation frame, but the meter has only - // LEVEL_SCALE steps: a steady signal must not redraw the meter. + // Read every frame, but a steady signal emits once. capture.drawFrames(20); expect(emissions).toBe(1); @@ -182,7 +172,7 @@ describe("observeMicrophoneState$", () => { }); }); -/** An error with the `name` the browser would give it, not just a message. */ +/** An error carrying the browser's `name`. */ function named(error: Error, name: string): Error { error.name = name; return error; diff --git a/src/state/MicrophoneLevel.ts b/src/state/MicrophoneLevel.ts index 43f57ec29..8e88a5042 100644 --- a/src/state/MicrophoneLevel.ts +++ b/src/state/MicrophoneLevel.ts @@ -8,37 +8,19 @@ Please see LICENSE in the repository root for full details. import { distinctUntilChanged, Observable } from "rxjs"; import { logger } from "matrix-js-sdk/lib/logger"; -/** - * What the microphone selector can say about the input, beyond its level. - * - * Silence and a broken microphone look identical on a meter, so the states a - * user has to act on are named rather than drawn as a flat bar. - */ +/** What the microphone picks up, or why it can't be read. */ export type MicrophoneState = | { type: "level"; level: number } | { type: "permission-denied" } | { type: "no-device" }; -/** - * The scale a level is reported on: 0 means silence, this means full scale. - * - * Fixed, and deliberately not the number of bars drawn — that follows the - * width available. A scale that moved with the width would announce the same - * loudness as different numbers in different places, and would make this layer - * depend on how wide something is drawn. - */ +/** Scale a level is reported on. Fixed rather than the bar count, so a level reads the same at any width. */ export const LEVEL_SCALE = 24; /** - * Observes what a microphone is picking up, as the meter should show it. - * - * - Held for exactly as long as something is watching: subscribing opens the - * device, unsubscribing releases it. The menu, not the call. - * - Its own capture rather than the track the call holds, by design. The - * pre-join screen freezes that track to the device selected when it mounted, - * so a meter fed from it could not follow the picker. - * - Says whether the microphone hears anything, not whether anyone hears the - * user: it keeps reading while muted, and the mute control carries that. + * The level of a microphone, captured while subscribed. Its own capture rather + * than the call's track, because pre-join freezes that track to the device + * selected at mount. */ export function observeMicrophoneState$( deviceId: string | undefined, @@ -48,8 +30,7 @@ export function observeMicrophoneState$( let context: AudioContext | undefined; let frame: number | undefined; - // Safe to call more than once: unsubscribing runs it, and `start` runs it - // again for anything the browser handed over after that. + // Idempotent: teardown and start can both call it. const release = (): void => { if (frame !== undefined) cancelAnimationFrame(frame); stream?.getTracks().forEach((track) => track.stop()); @@ -64,16 +45,11 @@ export function observeMicrophoneState$( audio: deviceId === undefined ? true : { deviceId: { exact: deviceId } }, }); - // A permission prompt outlives the subscription that asked for it, so by - // now nobody may be watching — and `release` ran while `stream` was still - // undefined. Nothing will call it again, so release here or the device - // stays held, with the indicator lit and no meter on screen. + // Unsubscribed while the permission prompt was open. if (subscriber.closed) return release(); context = new AudioContext(); - // Chrome starts the context suspended unless it was created during a - // gesture; opening the menu is one, but resume explicitly so the meter - // cannot silently sit at zero. + // Starts suspended outside a user gesture, which would read as silence. if (context.state === "suspended") await context.resume(); if (subscriber.closed) return release(); @@ -86,8 +62,7 @@ export function observeMicrophoneState$( const read = (): void => { analyser.getByteTimeDomainData(samples); - // Root mean square of the waveform around its centre, which is the - // loudness a listener perceives rather than the tallest spike. + // RMS: perceived loudness rather than the peak. let sum = 0; for (const sample of samples) { const centred = (sample - 128) / 128; @@ -107,19 +82,14 @@ export function observeMicrophoneState$( }; start().catch((e: unknown) => { - // Building the graph can fail after getUserMedia has already resolved, - // and the capture would then outlive its own failure: the microphone - // open and its in-use light on, behind a meter reporting it as - // unavailable. Releasing is its own step, which teardown and this path - // both take. + // Building the graph can fail after the device was granted. release(); subscriber.next(stateForFailure(e)); }); return release; }).pipe( - // Read every animation frame, but quantised to a whole number of segments, - // so most frames say nothing new and should not reach React. + // Frames that don't move the quantised level don't reach React. distinctUntilChanged( (a, b) => a.type === b.type && @@ -139,41 +109,24 @@ function stateForFailure(e: unknown): MicrophoneState { return { type: "no-device" }; } -/** - * Loudness below which the microphone counts as hearing nothing. A quiet room - * is never digitally silent, and without a floor that hiss lights the first - * bars permanently — which reads as "it can hear me" when nobody is speaking. - */ +/** Below this counts as silence, so a quiet room's hiss doesn't light the first bars. */ const NOISE_FLOOR = 0.02; -/** - * Quantises a 0..1 volume onto {@link LEVEL_SCALE}. Exported for the tests: - * this mapping is what decides whether quiet, normal and loud look different. - */ +/** Quantises a 0..1 volume onto {@link LEVEL_SCALE}. */ export function segmentsForVolume(volume: number): number { if (!Number.isFinite(volume) || volume <= NOISE_FLOOR) return 0; - // Volume arrives as amplitude, where speech occupies a small part of the top - // of the range. A square root spreads that out, so ordinary speech moves the - // meter through its middle rather than barely leaving the floor. + // Square root, so ordinary speech reaches the middle of the scale. const aboveFloor = (Math.min(volume, 1) - NOISE_FLOOR) / (1 - NOISE_FLOOR); return Math.min(LEVEL_SCALE, Math.ceil(Math.sqrt(aboveFloor) * LEVEL_SCALE)); } -/** Time constant for a rise. Short, so a syllable registers as it starts. */ +/** Rise time constant: short, so a syllable registers as it starts. */ export const ATTACK_MS = 50; -/** - * Time constant for a fall. Longer than the attack: speech is full of gaps a - * few tens of milliseconds long, and tracking them exactly would flicker. - */ +/** Fall time constant: longer, so the gaps between words don't flicker. */ export const RELEASE_MS = 120; -/** - * Moves a displayed level towards a new reading, fast up and slowly down. - * - * In elapsed time rather than frames, so it behaves the same at 60Hz and - * 120Hz, and does not jump when a frame is dropped. - */ +/** Eases towards a reading, by elapsed time so the frame rate doesn't matter. */ export function smoothVolume( displayed: number, reading: number, diff --git a/src/utils/test.ts b/src/utils/test.ts index d8964d6ac..405e5947b 100644 --- a/src/utils/test.ts +++ b/src/utils/test.ts @@ -594,26 +594,19 @@ export class MockConnection extends Connection { export interface StubbedCapture { getUserMedia: Mock; - /** Hands the microphone over, as the browser does once permission is given. */ + /** Grants the microphone, as the browser does once permission is given. */ grant: () => void; track: { stop: Mock }; contexts: { close: Mock }[]; - /** Runs the animation frames the level meter reads on, in order. */ + /** Runs the pending animation frames, in order. */ drawFrames: (count: number) => void; - /** - * Sets how loud the microphone is, 0 for silence and 1 for full scale. - * Takes effect on the next frame drawn. - */ + /** Sets the microphone's loudness from the next frame, 0 to 1. */ speak: (amplitude: number) => void; } /** - * Stubs just enough of the capture and Web Audio APIs for a microphone level to - * be read, with the grant held back so a test decides when — or whether — it - * lands, and with animation frames driven by hand rather than by a clock. - * - * Call {@link restoreAudioCapture} afterwards, or every later test in the run - * inherits the stub. + * Stubs the capture and Web Audio APIs, with the grant and the animation + * frames driven by the test. Undo with {@link restoreAudioCapture}. */ export function stubAudioCapture(): StubbedCapture { const track = { stop: vi.fn() }; @@ -643,8 +636,7 @@ export function stubAudioCapture(): StubbedCapture { return { fftSize: 1024, getByteTimeDomainData: (samples: Uint8Array): void => { - // Digital silence is the midpoint of the range and not zero: a - // buffer left at zero reads as a full-scale waveform. + // Silence is the midpoint of the range; a zeroed buffer reads as full scale. if (amplitude <= 0) { samples.fill(128); return; @@ -660,8 +652,7 @@ export function stubAudioCapture(): StubbedCapture { } }, ); - // Only this property: replacing navigator wholesale drops the getters on its - // prototype, such as userAgent. + // Only this property: replacing navigator loses getters like userAgent. Object.defineProperty(navigator, "mediaDevices", { configurable: true, value: { getUserMedia: vi.fn().mockReturnValue(granted) },