mirror of
https://github.com/vector-im/element-call.git
synced 2026-09-22 22:29:30 +00:00
Show the focus border for the keyboard alone
The menu focuses whatever the pointer is over, so `:focus-visible` was being asked a question it cannot answer. Chromium calls every focus after any key press keyboard-driven, so one Escape left the border following the mouse for the rest of the session; Firefox never calls a programmatic focus keyboard-driven, so the border never appeared there at all. Neither is what D13 asks for. The menu now notes which of the two reached the current item, and the border follows that. Nothing existing covers this: Compound exports no such helper, the repo has no react-aria, and Radix marks both modalities alike. `:focus-visible:not(:hover)` would hide the border under the cursor but still leave Firefox without one. Two acceptance criteria named checks that could not fail for the reason they exist. AC24 was manual and now names an e2e test that reads the painted outline as the pointer and keyboard take turns. AC15 asserted the attribute its stylesheet keys off rather than the greying, and now names the meter's own story; deleting the CSS rule fails that story and not the unit test. Both edits made with the owner's authorisation. Spec: FEATURES_SPEC/2026-09_Audio_Quick_Menu.md — AC15, AC24. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -112,7 +112,10 @@ Test names are the anchor; tests are created with exactly these names.
|
||||
- check: `pnpm vitest run --project=unit -t "audio menu hints when microphone permission is denied"`
|
||||
- AC15 [D7] — When the selected microphone cannot be opened because another application holds
|
||||
it, the indicator renders in a greyed-out state.
|
||||
- check: `pnpm vitest run --project=unit -t "level indicator greys out when the microphone is unavailable"`
|
||||
- check: `pnpm vitest run --project=storybook -t "Microphone Unavailable"`, which reads
|
||||
the greying a browser applies. The unit test
|
||||
`-t "level indicator greys out when the microphone is unavailable"` covers the state
|
||||
the stylesheet keys off, and cannot fail on the greying itself.
|
||||
- AC16 [FR-012] — Moving the sound-effects slider changes the level at which the next sound
|
||||
effect plays.
|
||||
- check: `pnpm vitest run --project=unit -t "sound effects volume from the menu applies to the next effect"`
|
||||
@@ -132,15 +135,17 @@ Test names are the anchor; tests are created with exactly these names.
|
||||
confirm the announced state changes with the signal.
|
||||
- AC22 [D11] — With more devices than fit on screen, the heading and the sound-effect slider
|
||||
stay visible while the device lists scroll.
|
||||
- check: `pnpm vitest run --project=unit -t "audio menu keeps its title and volume slider out of the scrolling area"`
|
||||
- check: `pnpm vitest run --project=storybook -t "With Many Devices"`, which measures the
|
||||
layout in a browser with the device count fixed. The unit test
|
||||
`-t "audio menu keeps its title and volume slider out of the scrolling area"` and the
|
||||
e2e `-g "audio menu stays inside a short window"` cover the structure and the window
|
||||
bound; neither can fail on layout alone.
|
||||
- AC23 [D12] — The level indicator stays with the microphone list and never sits among the
|
||||
output controls.
|
||||
- check: `pnpm vitest run --project=unit -t "level indicator stays with the microphone list rather than the speakers"`
|
||||
- AC24 [D13] — Moving through the menu by keyboard marks the current item with a border;
|
||||
moving over it with a pointer marks it with a background and no border.
|
||||
- check: manual, open the menu and tab through the device rows, confirming a border marks
|
||||
the focused row; then move the pointer across the rows, confirming a background appears
|
||||
and no border does.
|
||||
- check: `pnpm test:playwright --project=chromium --project=firefox -g "the focus border follows the keyboard and not the pointer"`
|
||||
- AC25 [SC-001] — Selecting an output from the menu during a call leaves the other
|
||||
participants visible throughout.
|
||||
- check: `pnpm test:playwright --project=chromium -g "audio menu leaves participants visible while switching output"`
|
||||
@@ -310,6 +315,33 @@ is what holds it there.
|
||||
- Added as AC28 and the product spec's edge case reworded to match, both with the owner's
|
||||
authorisation, since acceptance criteria and the product spec are human-owned.
|
||||
|
||||
### 2026-09-11 — finding: `:focus-visible` cannot express D13 (#4254)
|
||||
- The menu moves focus onto whichever item the pointer is over, so the browser decides the
|
||||
modality for a focus it did not see the user cause. Chromium calls every focus after any
|
||||
key press keyboard-driven, so one Escape left the border following the mouse for the rest
|
||||
of the session; Firefox never calls a programmatic focus keyboard-driven, so the border
|
||||
never appeared there at all. Neither matches D13.
|
||||
- The menu now tracks which way each item was reached — a key press on the trigger or on an
|
||||
item, against pointer movement over one — and the border follows that. Verified in both
|
||||
browsers: the border appears only after a key press and leaves again on the next pointer
|
||||
move. AC24's manual check could now name a unit test; owner's call.
|
||||
|
||||
### 2026-09-11 — acceptance checks that could not fail (#4254)
|
||||
- Raised in review: AC22's check ran in jsdom, which lays nothing out, so it passed whatever
|
||||
the layout did — the defect it exists for went unseen until a screenshot. Its check now
|
||||
names the `CallFooter` "With Many Devices" story, which measures the geometry in a browser
|
||||
with the device count fixed. The unit test and the short-window e2e stay as they are; the
|
||||
criterion names the one that would fail.
|
||||
- AC24 was a manual check, and the border it describes turned out to be wrong in both
|
||||
browsers for different reasons. Its check now names an e2e test that reads the painted
|
||||
outline as the pointer and the keyboard take turns.
|
||||
- The same audit found AC15: its check asserted the attribute the stylesheet keys off, not
|
||||
the greying, so losing the CSS rule would not have failed it. Re-anchored to the meter's
|
||||
own story, which reads the applied opacity.
|
||||
- Every edit to `## Acceptance criteria` was made with the owner's authorisation. The
|
||||
remaining manual checks are AC6, which needs Safari, and AC21, which needs a screen
|
||||
reader.
|
||||
|
||||
## PRs
|
||||
|
||||
- #4254 — draft, one commit per slice — AC1–AC27 (AC6, AC21, AC24 manual by the reviewer;
|
||||
|
||||
@@ -172,6 +172,40 @@ test("audio menu stays inside a short window", async ({ browser }) => {
|
||||
.toBe(true);
|
||||
});
|
||||
|
||||
test("the focus border follows the keyboard and not the pointer", async ({
|
||||
browser,
|
||||
}) => {
|
||||
const context = await browser.newContext({ reducedMotion: "reduce" });
|
||||
const page = await context.newPage();
|
||||
await page.goto("/");
|
||||
await SpaHelpers.createCall(page, "Focus", "Focus border", true);
|
||||
await page.getByTestId("videoTile").first().waitFor();
|
||||
|
||||
const chevron = page.getByRole("button", { name: "Microphone" });
|
||||
const rows = page.getByRole("menu").getByRole("menuitemradio");
|
||||
const borderOfFocused = async (): Promise<string> =>
|
||||
page.evaluate(() => {
|
||||
const el = document.activeElement;
|
||||
return el === null ? "none" : getComputedStyle(el).outlineStyle;
|
||||
});
|
||||
|
||||
// Reached with the pointer: the hover background carries it, no border.
|
||||
await chevron.click();
|
||||
await page.getByRole("menu").waitFor();
|
||||
await rows.nth(1).hover();
|
||||
expect(await borderOfFocused()).toBe("none");
|
||||
|
||||
// Reached with the keyboard: a border marks where the keyboard is. The menu
|
||||
// moves focus to whatever the pointer is over, so the browser cannot tell
|
||||
// these two apart on its own.
|
||||
await page.keyboard.press("ArrowDown");
|
||||
expect(await borderOfFocused()).toBe("solid");
|
||||
|
||||
// And back, on the next movement of the pointer.
|
||||
await rows.nth(0).hover();
|
||||
expect(await borderOfFocused()).toBe("none");
|
||||
});
|
||||
|
||||
async function firstUncheckedIndex(
|
||||
rows: Locator,
|
||||
count: number,
|
||||
|
||||
@@ -47,11 +47,16 @@ export const Loud: Story = {
|
||||
args: { state: { type: "active", level: 0.95 } },
|
||||
};
|
||||
|
||||
export const Unavailable: Story = {
|
||||
/** Named apart from the footer's own "Unavailable Media Devices" story. */
|
||||
export const MicrophoneUnavailable: Story = {
|
||||
args: { state: { type: "unavailable" } },
|
||||
play: async ({ canvasElement }) => {
|
||||
const meter = within(canvasElement).getByRole("meter");
|
||||
await expect(meter).toHaveAttribute("data-unavailable", "true");
|
||||
// The greying is the point, and it is done in CSS: an attribute alone
|
||||
// would still be there with the rule gone.
|
||||
const bar = meter.querySelector("span");
|
||||
await expect(Number(getComputedStyle(bar!).opacity)).toBeLessThan(1);
|
||||
},
|
||||
};
|
||||
|
||||
|
||||
@@ -113,14 +113,14 @@ Please see LICENSE in the repository root for full details.
|
||||
flex: 1;
|
||||
}
|
||||
|
||||
/* Radix moves DOM focus onto whichever item the pointer is over, so the
|
||||
/* The menu moves focus onto whichever item the pointer is over, so the
|
||||
browser's own focus ring shows up during mouse use. Keyboard navigation
|
||||
gets the border; the pointer gets the hover background and nothing else. */
|
||||
.menu [role^="menuitem"]:focus {
|
||||
outline: none;
|
||||
}
|
||||
|
||||
.menu [role^="menuitem"]:focus-visible {
|
||||
.keyboardNav [role^="menuitem"]:focus {
|
||||
outline: var(--cpd-border-width-2) solid var(--cpd-color-border-focused);
|
||||
outline-offset: calc(-1 * var(--cpd-border-width-2));
|
||||
}
|
||||
|
||||
@@ -555,6 +555,26 @@ describe("audio menu", () => {
|
||||
);
|
||||
});
|
||||
|
||||
test("the focus border marks keyboard use and never the pointer", async () => {
|
||||
const user = await openAudioMenu();
|
||||
const menu = screen.getByRole("menu");
|
||||
const rows = screen.getAllByRole("menuitemradio");
|
||||
|
||||
// Opened and driven by pointer: the hover background alone.
|
||||
await user.hover(rows[1]);
|
||||
expect(menu.className).not.toMatch(/keyboardNav/);
|
||||
|
||||
// One arrow key, and the border marks where the keyboard is.
|
||||
await user.keyboard("[ArrowDown]");
|
||||
expect(menu.className).toMatch(/keyboardNav/);
|
||||
|
||||
// Back to the pointer, and the border goes with it: the browser calls
|
||||
// every focus after a key press keyboard-driven, so this cannot be left
|
||||
// to :focus-visible.
|
||||
await user.hover(rows[0]);
|
||||
expect(menu.className).not.toMatch(/keyboardNav/);
|
||||
});
|
||||
|
||||
test("audio menu is fully operable from the keyboard", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onSelect = vi.fn();
|
||||
|
||||
@@ -123,6 +123,13 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
|
||||
}) => {
|
||||
const [plannedSelection, setPlannedSelection] = useState<string | null>(null);
|
||||
const [menuOpen, setMenuOpen] = useState(false);
|
||||
// Which of the two reached the current item, since `:focus-visible` cannot
|
||||
// tell: the menu focuses whatever the pointer is over.
|
||||
const [keyboardNav, setKeyboardNav] = useState(false);
|
||||
const modality = {
|
||||
onKeyDown: (): void => setKeyboardNav(true),
|
||||
onPointerMove: (): void => setKeyboardNav(false),
|
||||
};
|
||||
const isBusy = busy ?? false;
|
||||
const { t } = useTranslation();
|
||||
const devices = useMediaDevices();
|
||||
@@ -226,6 +233,7 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
|
||||
key={id}
|
||||
role="menuitemradio"
|
||||
aria-checked={selectedOption === id}
|
||||
{...modality}
|
||||
>
|
||||
{selectedOption === id && (
|
||||
<CheckIcon
|
||||
@@ -256,7 +264,9 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
|
||||
{/* The mute button lives inside */}
|
||||
{button}
|
||||
<Menu
|
||||
className={styles.menu}
|
||||
className={classNames(styles.menu, {
|
||||
[styles.keyboardNav]: keyboardNav,
|
||||
})}
|
||||
title={title}
|
||||
showTitle={true}
|
||||
open={menuOpen}
|
||||
@@ -273,6 +283,8 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
|
||||
kind={"tertiary"}
|
||||
size="lg"
|
||||
aria-label={optionsButtonLabel}
|
||||
onKeyDown={() => setKeyboardNav(true)}
|
||||
onPointerDown={() => setKeyboardNav(false)}
|
||||
/>
|
||||
}
|
||||
>
|
||||
@@ -295,7 +307,7 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
|
||||
microphone list rather than sitting among the output
|
||||
controls; pinned to the foot of the scroll port, it stays on
|
||||
screen for as long as any microphone is. */}
|
||||
<div className={styles.stickyMeter}>
|
||||
<div className={styles.stickyMeter} {...modality}>
|
||||
{/* The capture is bound to the menu being open, so the
|
||||
microphone is only ever held while the user is looking at
|
||||
the level. */}
|
||||
@@ -310,6 +322,7 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
|
||||
options={audioControls.outputOptions}
|
||||
selected={audioControls.selectedOutput}
|
||||
onSelect={audioControls.onSelectOutput}
|
||||
modality={modality}
|
||||
/>
|
||||
</div>
|
||||
) : (
|
||||
@@ -321,12 +334,14 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
|
||||
<SoundEffectVolume
|
||||
volume={audioControls.soundEffectVolume}
|
||||
onCommit={audioControls.onSoundEffectVolumeCommit}
|
||||
onPointerMove={modality.onPointerMove}
|
||||
/>
|
||||
</>
|
||||
)}
|
||||
{(toggles?.length ?? 0) > 0 && <hr />}
|
||||
{toggles?.map((toggle) => (
|
||||
<ToggleMenuItem
|
||||
{...modality}
|
||||
label={toggle.label}
|
||||
onSelect={(e) => {
|
||||
videoBlurToggleClick?.();
|
||||
@@ -345,6 +360,11 @@ interface SpeakerSectionProps {
|
||||
options: OutputMenuOptions[];
|
||||
selected: string | undefined;
|
||||
onSelect: (id: string) => void;
|
||||
/** Reports whether a row was reached by pointer or by keyboard. */
|
||||
modality: {
|
||||
onKeyDown: () => void;
|
||||
onPointerMove: () => void;
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -359,6 +379,7 @@ function SpeakerSection({
|
||||
options,
|
||||
selected,
|
||||
onSelect,
|
||||
modality,
|
||||
}: SpeakerSectionProps): JSX.Element {
|
||||
const { t } = useTranslation();
|
||||
const labelText = (label: AudioOutputDeviceLabel): string => {
|
||||
@@ -412,6 +433,7 @@ function SpeakerSection({
|
||||
}}
|
||||
role="menuitemradio"
|
||||
aria-checked={selected === id}
|
||||
{...modality}
|
||||
>
|
||||
{selected === id && <CheckIcon width={24} height={24} aria-hidden />}
|
||||
</MenuItem>
|
||||
@@ -423,12 +445,14 @@ function SpeakerSection({
|
||||
interface SoundEffectVolumeProps {
|
||||
volume: number;
|
||||
onCommit: (volume: number) => void;
|
||||
onPointerMove: () => void;
|
||||
}
|
||||
|
||||
/** The sound-effect volume slider: the same stored value as in settings. */
|
||||
function SoundEffectVolume({
|
||||
volume,
|
||||
onCommit,
|
||||
onPointerMove,
|
||||
}: SoundEffectVolumeProps): JSX.Element {
|
||||
const { t } = useTranslation();
|
||||
const label = t("settings.audio_tab.effect_volume_label");
|
||||
@@ -451,6 +475,7 @@ function SoundEffectVolume({
|
||||
data-testid="sound_effect_volume"
|
||||
role="group"
|
||||
aria-label={label}
|
||||
onPointerMove={onPointerMove}
|
||||
onKeyDown={(e) => {
|
||||
if (SLIDER_KEYS.has(e.key)) e.stopPropagation();
|
||||
else keepTabInsideMenu(e);
|
||||
|
||||
Reference in New Issue
Block a user