From e3aaad8615b83e0276b8de8ca69f1821ddc48dc5 Mon Sep 17 00:00:00 2001 From: fkwp Date: Thu, 17 Sep 2026 18:34:34 +0200 Subject: [PATCH] Let a user remove a background they added - A cross where the tick would be, on hovering one of their own images - Not offered on the one in force: that is what they are wearing, and taking it away would leave them with nothing chosen (FR-025) - Delete or Backspace on a focused tile does the same, announced through aria-keyshortcuts. The cross is drawn rather than focusable: a control inside a menu item is invalid, and the item is what the arrows walk - Covered by a story: the one in force offers no removal, the others say Delete and answer to it Co-Authored-By: Claude Opus 5 (1M context) --- src/components/CallFooter.tsx | 16 +++++- .../MediaMuteAndSwitchButton.module.css | 29 ++++++++++ .../MediaMuteAndSwitchButton.stories.tsx | 51 ++++++++++++++++++ src/components/MediaMuteAndSwitchButton.tsx | 53 +++++++++++++++++++ 4 files changed, 148 insertions(+), 1 deletion(-) diff --git a/src/components/CallFooter.tsx b/src/components/CallFooter.tsx index 632b26148..5a62443ba 100644 --- a/src/components/CallFooter.tsx +++ b/src/components/CallFooter.tsx @@ -30,6 +30,7 @@ import { type MenuOptions, } from "./MediaMuteAndSwitchButton"; import { + parseEffect, serializeEffect, shippedBackgrounds, } from "../livekit/backgroundEffects"; @@ -170,7 +171,18 @@ export const CallFooter: FC = ({ const selectVideoButtonOption = useBehavior(vm.selectVideoButtonOption$); const backgroundEffect = useBehavior(vm.backgroundEffect$); const selectBackgroundEffect = useBehavior(vm.selectBackgroundEffect$); - const { added, addBackground } = useAddedBackgrounds(); + const { added, addBackground, removeBackground } = useAddedBackgrounds(); + + const onRemoveBackgroundEffect = useCallback( + (id: string): void => { + const effect = parseEffect(id); + if (effect.kind !== "added") return; + removeBackground(effect.id).catch((e) => + logger.warn("Could not remove that background", e), + ); + }, + [removeBackground], + ); const onAddBackgroundImage = useCallback( (file: File): void => { @@ -212,6 +224,7 @@ export const CallFooter: FC = ({ ...added.map((background, i) => ({ id: serializeEffect({ kind: "added", id: background.id }), kind: "image" as const, + removable: true, label: t("action.background_effect_numbered", { n: shippedBackgrounds.length + i + 1, }), @@ -291,6 +304,7 @@ export const CallFooter: FC = ({ ? onAddBackgroundImage : undefined } + onRemoveBackgroundEffect={onRemoveBackgroundEffect} />, ); } else { diff --git a/src/components/MediaMuteAndSwitchButton.module.css b/src/components/MediaMuteAndSwitchButton.module.css index 4a326f310..bf9a5e62a 100644 --- a/src/components/MediaMuteAndSwitchButton.module.css +++ b/src/components/MediaMuteAndSwitchButton.module.css @@ -328,3 +328,32 @@ Please see LICENSE in the repository root for full details. .effectGrid .effectTile[aria-disabled="true"] .effectThumb { filter: grayscale(1); } + +/* + * The cross that removes one of the user's own backgrounds. + * + * Shown on hover, and whenever the tile has the keyboard's attention, so the + * two ways of reaching it look the same. It carries its own ground for the + * same reason the tick does: it sits on a picture nobody chose. + */ +.effectRemove { + position: absolute; + inset-block-start: 50%; + inset-inline-start: 50%; + translate: -50% -50%; + display: none; + align-items: center; + justify-content: center; + inline-size: 24px; + block-size: 24px; + border-radius: 50%; + background: var(--cpd-color-bg-canvas-default); + color: var(--cpd-color-icon-primary); + cursor: pointer; +} + +.effectGrid .effectTile:hover .effectRemove, +.effectGrid .effectTile:focus-within .effectRemove, +.menu[data-focus-modality="keyboard"] .effectTile:focus .effectRemove { + display: flex; +} diff --git a/src/components/MediaMuteAndSwitchButton.stories.tsx b/src/components/MediaMuteAndSwitchButton.stories.tsx index 61d866640..6371126c9 100644 --- a/src/components/MediaMuteAndSwitchButton.stories.tsx +++ b/src/components/MediaMuteAndSwitchButton.stories.tsx @@ -909,3 +909,54 @@ export const BackgroundEffectsWithALongDeviceName: Story = { await expect(getComputedStyle(grid).gridTemplateColumns.split(" ")).toHaveLength(3); }, }; + +/** + * Backgrounds the user added are theirs to remove — except the one in force, + * which is what they are wearing. + */ +export const AddedBackgroundsCanBeRemoved: Story = { + args: { + ...BackgroundEffects.args, + backgroundEffects: [ + ...backgroundEffects, + { + id: "added:one", + label: "Background 3", + kind: "image" as const, + imageUrl: swatch("#c2410c", "#7c2d12"), + removable: true, + }, + { + id: "added:two", + label: "Background 4", + kind: "image" as const, + imageUrl: swatch("#1d4ed8", "#172554"), + removable: true, + }, + ], + selectedBackgroundEffect: "added:one", + onRemoveBackgroundEffect: fn(), + }, + play: async ({ args, canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole("button", { name: "Camera" })); + const body = within(document.body); + + // The one in force offers no removal. + const inForce = await body.findByRole("menuitemradio", { + name: "Background 3", + }); + await expect(inForce).not.toHaveAttribute("aria-keyshortcuts"); + + // The other does, and says so, and Delete reaches it from the keyboard. + const other = await body.findByRole("menuitemradio", { + name: "Background 4", + }); + await expect(other).toHaveAttribute("aria-keyshortcuts", "Delete"); + other.focus(); + await userEvent.keyboard("{Delete}"); + await expect(args.onRemoveBackgroundEffect).toHaveBeenCalledWith( + "added:two", + ); + }, +}; diff --git a/src/components/MediaMuteAndSwitchButton.tsx b/src/components/MediaMuteAndSwitchButton.tsx index 6f7eb9af9..c10760c4b 100644 --- a/src/components/MediaMuteAndSwitchButton.tsx +++ b/src/components/MediaMuteAndSwitchButton.tsx @@ -29,6 +29,7 @@ import { BlockIcon, PlusIcon, CheckCircleSolidIcon, + CloseIcon, } from "@vector-im/compound-design-tokens/assets/web/icons"; import classNames from "classnames"; import { useTranslation } from "react-i18next"; @@ -59,6 +60,8 @@ export interface BackgroundEffectOption { kind: "none" | "blur" | "image"; /** The thumbnail, for kind "image". Blur and no effect draw their own. */ imageUrl?: string; + /** Whether this one is the user's to remove. */ + removable?: boolean; } export interface MediaMuteAndSwitchButtonProps { @@ -111,6 +114,12 @@ export interface MediaMuteAndSwitchButtonProps { * opening it takes the focus, which would otherwise dismiss the menu. */ onAddBackgroundImage?: (file: File) => void; + /** + * Called to remove one of the user's own backgrounds. Omit to offer no + * removal. The one in force is never offered: it is what the user is + * wearing, and taking it away would leave them with nothing chosen. + */ + onRemoveBackgroundEffect?: (id: string) => void; /** * For any toggle and option this method will be called. * So toggles need to be implemented by listening here and setting the right toggle item to `enabled` @@ -162,6 +171,7 @@ export const MediaMuteAndSwitchButton: FC = ({ selectedBackgroundEffect, onSelectBackgroundEffect, onAddBackgroundImage, + onRemoveBackgroundEffect, onSelect, }) => { // Which device we have asked for but not yet been given. Carries the kind as @@ -471,11 +481,31 @@ export const MediaMuteAndSwitchButton: FC = ({ // exception: it needs no processing, so it stays choosable and stays the // one in force. const unavailable = onSelectBackgroundEffect === undefined; + // The one in force is never offered for removal: FR-025. Everything else + // the user added is theirs to take away. + const canRemove = (effect: BackgroundEffectOption): boolean => + effect.removable === true && + onRemoveBackgroundEffect !== undefined && + selectedBackgroundEffect !== effect.id; + const tiles = list.map((effect) => ( { + if (e.key !== "Delete" && e.key !== "Backspace") return; + e.preventDefault(); + onRemoveBackgroundEffect?.(effect.id); + } + : undefined + } className={classNames(styles.effectTile, { [styles.effectTileSelected]: selectedBackgroundEffect === effect.id, })} @@ -497,6 +527,29 @@ export const MediaMuteAndSwitchButton: FC = ({ alt="" /> )} + {canRemove(effect) && ( + // Stands where the tick would on the one in force, so the corner + // of a tile means the same thing throughout: what this tile is + // doing, or what you can do to it. + /* eslint-disable-next-line jsx-a11y/click-events-have-key-events, + jsx-a11y/no-static-element-interactions -- + Deliberately not a control. It sits inside the swatch, which + is aria-hidden, so assistive technology never meets it; the + keyboard reaches the same action through Delete on the item + itself. Giving it a role and a tab stop would put a control + inside a menu item, which is invalid, and would add a stop the + arrow keys do not know about. */ + e.stopPropagation()} + onClick={(e): void => { + e.stopPropagation(); + onRemoveBackgroundEffect?.(effect.id); + }} + > + + + )} {selectedBackgroundEffect === effect.id ? ( // The tick stands where the glyph would, and on a picture it // carries its own ground so it reads against whatever is behind