diff --git a/src/components/MediaMuteAndSwitchButton.module.css b/src/components/MediaMuteAndSwitchButton.module.css index 5fd10d5cd..9dda265e3 100644 --- a/src/components/MediaMuteAndSwitchButton.module.css +++ b/src/components/MediaMuteAndSwitchButton.module.css @@ -63,6 +63,29 @@ Please see LICENSE in the repository root for full details. 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. */ + scroll-padding-block: var(--device-list-scroll-padding-start, 0) + var(--device-list-scroll-padding-end, 0); +} + +/* 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. */ +.sectionHeading { + position: sticky; + inset-block-start: 0; + background: var(--cpd-color-bg-canvas-default); + margin-inline: var(--cpd-border-width-1); + margin-block-start: var(--cpd-border-width-1); } /* The meter belongs to the microphone section: it stays at the bottom of the diff --git a/src/components/MediaMuteAndSwitchButton.stories.tsx b/src/components/MediaMuteAndSwitchButton.stories.tsx index 1bfa7ecd4..26eda4a9a 100644 --- a/src/components/MediaMuteAndSwitchButton.stories.tsx +++ b/src/components/MediaMuteAndSwitchButton.stories.tsx @@ -5,11 +5,12 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { fn, userEvent, within, expect } from "storybook/test"; +import { fn, userEvent, waitFor, within, expect } from "storybook/test"; import { type JSX } from "react"; import type { Meta, StoryObj } from "@storybook/react-vite"; import { MediaMuteAndSwitchButton } from "./MediaMuteAndSwitchButton"; +import styles from "./MediaMuteAndSwitchButton.module.css"; import { MediaDevicesContext } from "../MediaDevicesContext"; import { MediaDevices } from "../state/MediaDevices"; import { globalScope } from "../state/ObservableScope"; @@ -228,3 +229,148 @@ export const OutputNotEnumerated: Story = { await expect(speakers).toHaveAttribute("aria-disabled", "true"); }, }; + +/** + * 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. + */ +export const KeyboardReachesEveryDevice: Story = { + args: { + ...Default.args, + title: "Microphone", + iconsAndLabels: "audio", + enabled: true, + // Enough of them that the list scrolls well past its own height, so that + // arrowing back up has to scroll too — which is where the heading can hide + // a row, as the meter can on the way down. + options: Array.from({ length: 20 }, (_, i) => ({ + label: { type: "name" as const, name: `Microphone ${i + 1}` }, + id: `mic${i + 1}`, + })), + selectedOption: "mic1", + outputOptions: Array.from({ length: 4 }, (_, i) => ({ + label: { type: "name" as const, name: `Speaker ${i + 1}` }, + id: `spk${i + 1}`, + })), + selectedOutputOption: "spk1", + onSelectOutput: fn(), + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole("button", { name: "Microphone" })); + + const meter = await waitFor(() => { + const element = document.body.querySelector( + `.${styles.stickyMeter}`, + ); + if (element === null) throw new Error("the meter has not rendered yet"); + 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. + const overlays = [ + meter, + ...document.body.querySelectorAll( + `.${styles.sectionHeading}`, + ), + ]; + 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. + 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. + 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. + */ +export const HeadingsStayWhileScrolling: Story = { + args: { + ...Default.args, + title: "Microphone", + iconsAndLabels: "audio", + enabled: true, + options: Array.from({ length: 20 }, (_, i) => ({ + label: { type: "name" as const, name: `Microphone ${i + 1}` }, + id: `mic${i + 1}`, + })), + selectedOption: "mic1", + outputOptions: Array.from({ length: 4 }, (_, i) => ({ + label: { type: "name" as const, name: `Speaker ${i + 1}` }, + id: `spk${i + 1}`, + })), + selectedOutputOption: "spk1", + onSelectOutput: fn(), + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole("button", { name: "Microphone" })); + const menu = within(document.body); + + const list = document.body.querySelector( + `.${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. + 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. + list.scrollTop += + group.getBoundingClientRect().top - list.getBoundingClientRect().top + 80; + + const scrollport = list.getBoundingClientRect(); + await expect(heading.getBoundingClientRect().top).toBeLessThanOrEqual( + scrollport.top + 2, + ); + await expect(heading.getBoundingClientRect().bottom).toBeGreaterThan( + scrollport.top, + ); + // And it keeps clear of the menu's frame, as the meter does. + const frame = document.body + .querySelector("[role='menu']")! + .getBoundingClientRect(); + await expect(heading.getBoundingClientRect().left).toBeGreaterThan( + frame.left, + ); + }, +}; + +/** + * 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. + */ +function overlapping(element: Element, overlays: Element[]): number { + const box = element.getBoundingClientRect(); + return overlays.reduce((worst, overlay) => { + const over = overlay.getBoundingClientRect(); + const shared = + Math.min(box.bottom, over.bottom) - Math.max(box.top, over.top); + return Math.max(worst, shared); + }, 0); +} diff --git a/src/components/MediaMuteAndSwitchButton.tsx b/src/components/MediaMuteAndSwitchButton.tsx index 7879d965e..44111c194 100644 --- a/src/components/MediaMuteAndSwitchButton.tsx +++ b/src/components/MediaMuteAndSwitchButton.tsx @@ -6,6 +6,7 @@ Please see LICENSE in the repository root for full details. */ import { + useCallback, useState, type CSSProperties, type FC, @@ -191,6 +192,43 @@ export const MediaMuteAndSwitchButton: FC = ({ // Only while the menu is open, so nothing holds a second capture of the // microphone for the length of a call. + // 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. + const [meterHeight, setMeterHeight] = useState(); + const meter = useCallback( + (element: HTMLDivElement | null): (() => void) | undefined => { + if (element === null) return; + const subscription = observeElementSize$(element) + .pipe( + map(({ height }) => height), + distinctUntilChanged(), + ) + .subscribe(setMeterHeight); + return (): void => subscription.unsubscribe(); + }, + [], + ); + + // 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. + const [headingHeight, setHeadingHeight] = useState(); + const heading = useCallback( + (element: HTMLDivElement | null): (() => void) | undefined => { + if (element === null) return; + const subscription = observeElementSize$(element) + .pipe( + map(({ height }) => height), + distinctUntilChanged(), + ) + .subscribe(setHeadingHeight); + return (): void => subscription.unsubscribe(); + }, + [], + ); + const microphoneState = useMicrophoneLevel( selectedOption, menuOpen && iconsAndLabels === "audio", @@ -409,6 +447,10 @@ export const MediaMuteAndSwitchButton: FC = ({ { "--device-list-max-height": listMaxHeight === undefined ? undefined : `${listMaxHeight}px`, + "--device-list-scroll-padding-end": + meterHeight === undefined ? undefined : `${meterHeight}px`, + "--device-list-scroll-padding-start": + headingHeight === undefined ? undefined : `${headingHeight}px`, } as CSSProperties } > @@ -420,7 +462,7 @@ export const MediaMuteAndSwitchButton: FC = ({
{/* The heading is decoration: the group carries the name, and a menu may only contain items, separators and groups. */} -
+
{deviceItems( @@ -435,7 +477,7 @@ export const MediaMuteAndSwitchButton: FC = ({ )}
-
+
{/* The heading sits outside, so the meter can never ride up over it: @@ -450,6 +492,7 @@ export const MediaMuteAndSwitchButton: FC = ({ )} {iconsAndLabels === "audio" && ( diff --git a/src/components/MicrophoneLevelMeter.tsx b/src/components/MicrophoneLevelMeter.tsx index ec5c823a2..25c39b10b 100644 --- a/src/components/MicrophoneLevelMeter.tsx +++ b/src/components/MicrophoneLevelMeter.tsx @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { useCallback, useState, type FC } from "react"; +import { useCallback, useState, type FC, type Ref } from "react"; import { Text } from "@vector-im/compound-web"; import { MicOnIcon } from "@vector-im/compound-design-tokens/assets/web/icons"; import classNames from "classnames"; @@ -20,6 +20,11 @@ import { observeElementSize$ } from "../utils/elementSize"; 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; } /** @@ -32,6 +37,7 @@ export interface MicrophoneLevelMeterProps { 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 @@ -55,7 +61,7 @@ export const MicrophoneLevelMeter: FC = ({ if (state.type !== "level") return ( -
+
{state.type === "permission-denied" @@ -66,7 +72,7 @@ export const MicrophoneLevelMeter: FC = ({ ); return ( -
+