mirror of
https://github.com/vector-im/element-call.git
synced 2026-10-10 23:25:52 +00:00
Keep only comments that give a reason the code can't
- Cut placement, surrounding context, design measurements and decision history from comments this branch added, across code, styles, tests and stories. - Correct one that claimed most Firefox builds can't route audio. - Drop a comment left duplicated in the footer story. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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<number> {
|
||||
if ((await item.count()) === 0) return 0;
|
||||
return item.first().evaluate((element) => {
|
||||
|
||||
@@ -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<Locator> {
|
||||
await pane
|
||||
.getByRole("button", { name: "Microphone" })
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user