diff --git a/playwright/background-effects.spec.ts b/playwright/background-effects.spec.ts index b7a384f28..fb0a7516c 100644 --- a/playwright/background-effects.spec.ts +++ b/playwright/background-effects.spec.ts @@ -326,6 +326,33 @@ test.describe("a background of one's own", () => { await addImage(page, red); await expect(mine).toBeVisible(); }); + + test("a removed image stays removed", async ({ page }) => { + test.setTimeout(120_000); + await page.goto("/"); + await SpaHelpers.createCall( + page, + "Removing user", + "Own background removed", + ); + await addImage(page, await redImage(page)); + const mine = page + .getByRole("group", { name: "Background effects" }) + .getByRole("menuitemradio", { name: "Background 3" }); + await expect(mine).toHaveAttribute("aria-keyshortcuts", "Delete"); + await mine.focus(); + await page.keyboard.press("Delete"); + await expect(mine).toHaveCount(0); + + await page.reload(); + await expect(page.getByTestId("lobby_joinCall")).toBeVisible(); + await page.getByRole("button", { name: "Camera", exact: true }).click(); + await expect( + page.getByRole("group", { name: "Background effects" }), + ).toBeVisible(); + await expect(mine).toHaveCount(0); + expect(await keptImages(page)).toEqual([]); + }); }); test.describe("what can be added", () => { diff --git a/src/components/BackgroundEffectGrid.module.css b/src/components/BackgroundEffectGrid.module.css index 472772c15..70c514a9e 100644 --- a/src/components/BackgroundEffectGrid.module.css +++ b/src/components/BackgroundEffectGrid.module.css @@ -90,3 +90,42 @@ Please see LICENSE in the repository root for full details. text-align: center; overflow-wrap: anywhere; } + +.removable { + position: relative; + display: flex; + min-inline-size: 0; +} + +.removable > .tile { + flex: 1; +} + +/* Clear of the target for choosing the tile. */ +.remove { + position: absolute; + inset-block-start: 0; + inset-inline-end: 0; + translate: 20% -20%; + display: none; + align-items: center; + justify-content: center; + inline-size: 28px; + block-size: 28px; + border-radius: 50%; + background: var(--cpd-color-bg-canvas-default); + color: var(--cpd-color-icon-critical-primary); + box-shadow: 0 1.2px 2.4px 0 rgb(0 0 0 / 15%); + cursor: pointer; +} + +.remove:hover { + background: var(--cpd-color-bg-critical-subtle-hovered); + box-shadow: 0 0 0 var(--cpd-border-width-1) + var(--cpd-color-border-critical-subtle); +} + +.removable:hover .remove, +.removable:focus-within .remove { + display: flex; +} diff --git a/src/components/BackgroundEffectGrid.tsx b/src/components/BackgroundEffectGrid.tsx index 66fcfb94d..83d1c0cd8 100644 --- a/src/components/BackgroundEffectGrid.tsx +++ b/src/components/BackgroundEffectGrid.tsx @@ -5,13 +5,19 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { type FC, type ReactNode, type ReactElement } from "react"; +import { + type FC, + type KeyboardEvent, + type ReactNode, + type ReactElement, +} from "react"; import { VisuallyHidden } from "@radix-ui/react-visually-hidden"; -import { InlineSpinner, MenuItem } from "@vector-im/compound-web"; +import { InlineSpinner, MenuItem, Tooltip } from "@vector-im/compound-web"; import { BlockIcon, BlurIcon, CheckCircleSolidIcon, + CloseIcon, PlusIcon, } from "@vector-im/compound-design-tokens/assets/web/icons"; @@ -25,6 +31,8 @@ export interface BackgroundEffectOption { kind: "none" | "blur" | "image"; /** For kind "image". */ imageUrl?: string; + /** Whether it is the user's to remove. */ + removable?: boolean; } interface Props { @@ -42,6 +50,9 @@ interface Props { /** Offers to add a background of one's own. Omit to leave that tile out. */ onAdd?: () => void; addLabel?: string; + /** Removes a removable one, never the one in force. Omit to offer none. */ + onRemove?: (id: string) => void; + removeLabel?: string; } /** @@ -58,6 +69,8 @@ export const BackgroundEffectGrid: FC = ({ describedBy, onAdd, addLabel, + onRemove, + removeLabel, }) => { const choose = (id: string): void => { if (id !== selected) onSelect?.(id); @@ -65,7 +78,29 @@ export const BackgroundEffectGrid: FC = ({ const tiles = effects.map((effect) => { const checked = effect.id === selected; const disabled = onSelect === undefined && effect.kind !== "none"; - return ( + const remove = + effect.removable && !checked && onRemove !== undefined + ? (): void => onRemove(effect.id) + : undefined; + const removeProps = + remove === undefined + ? {} + : { + "aria-keyshortcuts": "Delete", + onKeyDown: (e: KeyboardEvent): void => { + if (e.key !== "Delete" && e.key !== "Backspace") return; + e.preventDefault(); + remove(); + }, + }; + const content = ( + + ); + const tile = ( = ({ choose(effect.id); } } + {...removeProps} > - + {content} ); + if (remove === undefined) return tile; + // Beside the item, not in it: inside, the item takes the press first and + // the tile is chosen instead. + return ( +
+ {tile} + + {/* Not a control of its own, so the menu keeps one keyboard model: + the keyboard reaches the same action through Delete. */} + + + + +
+ ); }); // A command rather than a choice, so an item beside the radios. + const addContent = ( + <> + + + + {addLabel} + + ); const addTile = onAdd === undefined ? null : ( = ({ onAdd(); }} > - - - - {addLabel} + {addContent} ); diff --git a/src/components/CallFooter.stories.tsx b/src/components/CallFooter.stories.tsx index 6d3a72d1e..767aec39a 100644 --- a/src/components/CallFooter.stories.tsx +++ b/src/components/CallFooter.stories.tsx @@ -127,18 +127,20 @@ export const Default: Story = { selectBackgroundEffect: fn(), backgroundEffect: "blur", backgroundEffects: [ - { id: "none", kind: "none" }, - { id: "blur", kind: "blur" }, + { id: "none", kind: "none", removable: false }, + { id: "blur", kind: "blur", removable: false }, ...shippedBackgrounds.map((background) => ({ id: `image:${background.id}`, kind: "image" as const, imageUrl: background.imagePath, + removable: false, })), ], backgroundEffectNotice: undefined, backgroundEffectSettling: false, backgroundImageRefusal: undefined, addBackgroundImage: fn(), + removeBackgroundEffect: fn(), hangup: fn(), buttonSize: "lg", showFooter: true, diff --git a/src/components/CallFooter.tsx b/src/components/CallFooter.tsx index 0d8ba24fc..d49771bc8 100644 --- a/src/components/CallFooter.tsx +++ b/src/components/CallFooter.tsx @@ -45,6 +45,8 @@ export interface BackgroundEffectChoice { id: string; kind: "none" | "blur" | "image"; imageUrl?: string; + /** One the user added, which they can take away again. */ + removable: boolean; } export interface AudioOutputSwitcher { @@ -75,6 +77,8 @@ export interface FooterActions { selectBackgroundEffect: ((id: string) => void) | undefined; /** Undefined where no background can be added. */ addBackgroundImage: ((file: File) => void) | undefined; + /** Removes an added background by its option id; never the one in force. */ + removeBackgroundEffect: ((id: string) => void) | undefined; toggleScreenSharing: (() => void) | undefined; /** Also controls if the settings button is visible */ openSettings: (() => void) | undefined; @@ -186,6 +190,7 @@ export const CallFooter: FC = ({ const backgroundEffectNotice = useBehavior(vm.backgroundEffectNotice$); const backgroundEffectSettling = useBehavior(vm.backgroundEffectSettling$); const addBackgroundImage = useBehavior(vm.addBackgroundImage$); + const removeBackgroundEffect = useBehavior(vm.removeBackgroundEffect$); const refusal = useBehavior(vm.backgroundImageRefusal$); const refusalMessage = useMemo(() => { switch (refusal?.reason) { @@ -270,6 +275,7 @@ export const CallFooter: FC = ({ onSelectBackgroundEffect={selectBackgroundEffect} backgroundEffectSettling={backgroundEffectSettling} onAddBackgroundImage={addBackgroundImage} + onRemoveBackgroundEffect={removeBackgroundEffect} backgroundImageRefusal={refusalMessage} backgroundEffectNotice={ backgroundEffectNotice === "unavailable" diff --git a/src/components/CallFooterViewModel.test.ts b/src/components/CallFooterViewModel.test.ts index 38ca2be10..f6e2aa24d 100644 --- a/src/components/CallFooterViewModel.test.ts +++ b/src/components/CallFooterViewModel.test.ts @@ -51,6 +51,7 @@ vi.mock("@livekit/track-processors", () => ({ const store = vi.hoisted(() => ({ add: vi.fn<(file: Blob) => Promise>(), + remove: vi.fn<(id: string) => Promise>(), })); vi.mock("../livekit/backgroundImages", async (original) => { const { BehaviorSubject } = await import("rxjs"); @@ -59,6 +60,7 @@ vi.mock("../livekit/backgroundImages", async (original) => { addedBackgrounds: { added$: new BehaviorSubject([]), add: store.add, + remove: store.remove, }, }; }); @@ -324,21 +326,27 @@ describe("createCallFooterViewModel", () => { expect(lobbyOn("desktop").backgroundEffectNotice$.value).toBeUndefined(); }); - it("offers every effect in order", () => { + it("offers every effect in order, the added ones removable", () => { sdkSupportMock.mockReturnValue(true); const added$ = addedBackgrounds.added$ as BehaviorSubject< AddedBackground[] | undefined >; added$.next([{ id: "mine", url: "blob:mine" }]); expect(lobbyOn("desktop").backgroundEffects$.value).toEqual([ - { id: "none", kind: "none" }, - { id: "blur", kind: "blur" }, + { id: "none", kind: "none", removable: false }, + { id: "blur", kind: "blur", removable: false }, ...shippedBackgrounds.map((background) => ({ id: `image:${background.id}`, kind: "image", imageUrl: background.imagePath, + removable: false, })), - { id: "added:mine", kind: "image", imageUrl: "blob:mine" }, + { + id: "added:mine", + kind: "image", + imageUrl: "blob:mine", + removable: true, + }, ]); added$.next([]); }); @@ -412,6 +420,17 @@ describe("createCallFooterViewModel", () => { expect(vm.backgroundImageRefusal$.value).toBeUndefined(); }); + it("never removes the background in force", () => { + sdkSupportMock.mockReturnValue(true); + store.remove.mockResolvedValue(); + const vm = lobbyOn("desktop"); + backgroundEffectSetting.setValue("added:worn"); + vm.removeBackgroundEffect$.value!("added:worn"); + expect(store.remove).not.toHaveBeenCalled(); + vm.removeBackgroundEffect$.value!("added:spare"); + expect(store.remove).toHaveBeenCalledWith("spare"); + }); + it("availability is the same before and during a call", () => { for (const supported of [true, false]) { sdkSupportMock.mockReturnValue(supported); diff --git a/src/components/CallFooterViewModel.tsx b/src/components/CallFooterViewModel.tsx index 4c4834595..4e5d2aa67 100644 --- a/src/components/CallFooterViewModel.tsx +++ b/src/components/CallFooterViewModel.tsx @@ -109,6 +109,7 @@ function buildDeviceBehaviors( | "backgroundEffectSettling$" | "addBackgroundImage$" | "backgroundImageRefusal$" + | "removeBackgroundEffect$" > { const options$ = ( available$: Behavior>, @@ -210,6 +211,25 @@ function buildDeviceBehaviors( ), ), backgroundImageRefusal$: refusal$, + // Never the one in force: taking it away would leave nothing chosen. + removeBackgroundEffect$: scope.behavior( + offered$.pipe( + map((offered) => + offered + ? (optionId: string): void => { + const effect = parseEffect(optionId); + if (effect.kind !== "added") return; + if (backgroundEffectSetting.getValue() === optionId) return; + addedBackgrounds + .remove(effect.id) + .catch((e) => + logger.warn("Could not remove that background", e), + ); + } + : undefined, + ), + ), + ), }; } @@ -381,17 +401,19 @@ function backgroundEffectChoices( added: AddedBackground[], ): BackgroundEffectChoice[] { return [ - { id: serializeEffect({ kind: "none" }), kind: "none" }, - { id: serializeEffect({ kind: "blur" }), kind: "blur" }, + { id: serializeEffect({ kind: "none" }), kind: "none", removable: false }, + { id: serializeEffect({ kind: "blur" }), kind: "blur", removable: false }, ...shippedBackgrounds.map((background) => ({ id: serializeEffect({ kind: "shipped", id: background.id }), kind: "image" as const, imageUrl: background.imagePath, + removable: false, })), ...added.map((background) => ({ id: serializeEffect({ kind: "added", id: background.id }), kind: "image" as const, imageUrl: background.url, + removable: true, })), ]; } diff --git a/src/components/MediaMuteAndSwitchButton.stories.tsx b/src/components/MediaMuteAndSwitchButton.stories.tsx index 2b5faae4f..c58eb1464 100644 --- a/src/components/MediaMuteAndSwitchButton.stories.tsx +++ b/src/components/MediaMuteAndSwitchButton.stories.tsx @@ -374,6 +374,126 @@ export const BackgroundImageRefused: Story = { }, }; +/** A flat picture, for backgrounds of one's own. */ +function swatch(colour: string): string { + return `data:image/svg+xml,${encodeURIComponent( + ``, + )}`; +} + +/** One's own backgrounds are removable, never the one in force. */ +export const AddedBackgroundsCanBeRemoved: Story = { + args: { + ...VideoUnmute.args, + backgroundEffects: [ + ...effects, + { + id: "added:one", + label: "Background 3", + kind: "image", + imageUrl: swatch("#c2410c"), + removable: true, + }, + { + id: "added:two", + label: "Background 4", + kind: "image", + imageUrl: swatch("#1d4ed8"), + 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); + + const inForce = await body.findByRole("menuitemradio", { + name: "Background 3", + }); + await expect(inForce).not.toHaveAttribute("aria-keyshortcuts"); + + const other = body.getByRole("menuitemradio", { name: "Background 4" }); + await expect(other).toHaveAttribute("aria-keyshortcuts", "Delete"); + other.focus(); + await userEvent.keyboard("{Delete}"); + await expect(args.onRemoveBackgroundEffect).toHaveBeenCalledWith( + "added:two", + ); + + const wrapper = other.parentElement!; + await userEvent.hover(wrapper); + const cross = wrapper.querySelector( + "[aria-hidden]:last-child", + )!; + const probe = document.createElement("span"); + probe.style.color = "var(--cpd-color-icon-critical-primary)"; + cross.append(probe); + const critical = getComputedStyle(probe).color; + probe.remove(); + await expect(getComputedStyle(cross).color).toBe(critical); + const tile = other.getBoundingClientRect(); + const box = cross.getBoundingClientRect(); + await expect(box.right).toBeGreaterThan(tile.right); + await expect(box.top).toBeLessThan(tile.top); + + await userEvent.hover(cross); + await waitFor(async () => + expect( + [...document.body.querySelectorAll("div")].some( + (d) => d.textContent === "Remove" && d.offsetParent !== null, + ), + ).toBe(true), + ); + }, +}; + +/** Removing by pointer while the selection really changes. */ +export const RemovingWithLiveSelection: Story = { + args: AddedBackgroundsCanBeRemoved.args, + render: function WithLiveSelection(args): JSX.Element { + const [selected, setSelected] = useState("none"); + const [offered, setOffered] = useState(args.backgroundEffects ?? []); + return ( + + setOffered((current) => current.filter((o) => o.id !== id)) + } + /> + ); + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole("button", { name: "Camera" })); + const body = within(document.body); + + const tile = await body.findByRole("menuitemradio", { + name: "Background 4", + }); + await userEvent.hover(tile); + const cross = tile.parentElement!.querySelector( + "[aria-hidden]:last-child", + )!; + await expect(cross).toBeVisible(); + await userEvent.click(cross); + + await waitFor(async () => + expect( + body.queryByRole("menuitemradio", { name: "Background 4" }), + ).toBeNull(), + ); + await expect( + body.getByRole("menuitemradio", { name: "None" }), + ).toHaveAttribute("aria-checked", "true"); + }, +}; + /** In a short call the effects scroll into view with the list. */ export const BackgroundEffectsScrollWhenTheyDoNotFit: Story = { args: { diff --git a/src/components/MediaMuteAndSwitchButton.tsx b/src/components/MediaMuteAndSwitchButton.tsx index 5ebd032f0..21e58ef6f 100644 --- a/src/components/MediaMuteAndSwitchButton.tsx +++ b/src/components/MediaMuteAndSwitchButton.tsx @@ -83,6 +83,8 @@ export interface MediaMuteAndSwitchButtonProps { backgroundEffectSettling?: boolean; /** Called with the file chosen from the add tile. Omit to leave it out. */ onAddBackgroundImage?: (file: File) => void; + /** Removes a removable effect, never the one in force. */ + onRemoveBackgroundEffect?: (id: string) => void; /** Why the last file offered couldn't be used; a new object each time. */ backgroundImageRefusal?: { text: string }; /** @@ -120,6 +122,7 @@ export const MediaMuteAndSwitchButton: FC = ({ backgroundEffectNotice, backgroundEffectSettling, onAddBackgroundImage, + onRemoveBackgroundEffect, backgroundImageRefusal, onSelect, }) => { @@ -492,6 +495,8 @@ export const MediaMuteAndSwitchButton: FC = ({ } } addLabel={t("action.add_background_image")} + onRemove={onRemoveBackgroundEffect} + removeLabel={t("action.remove")} /> )} {/* In the list, so the menu grows no taller for it. */} diff --git a/src/livekit/backgroundImages.test.ts b/src/livekit/backgroundImages.test.ts index 6e52a7105..a67728c3c 100644 --- a/src/livekit/backgroundImages.test.ts +++ b/src/livekit/backgroundImages.test.ts @@ -81,6 +81,23 @@ describe("AddedBackgrounds", () => { expect(await storage.list()).toHaveLength(maxAddedBackgrounds); }); + it("forgets a removed image, and lets its picture go", async () => { + const revoke = vi + .spyOn(URL, "revokeObjectURL") + .mockImplementation(() => {}); + const storage = memoryStorage(); + const store = new AddedBackgrounds(storage, asGiven); + await flushPromises(); + const first = await store.add(new Blob(["a"])); + const second = await store.add(new Blob(["b"])); + const url = store.added$.value![0].url; + + await store.remove(first); + expect(store.added$.value?.map((a) => a.id)).toEqual([second]); + expect((await storage.list()).map((k) => k.id)).toEqual([second]); + expect(revoke).toHaveBeenCalledWith(url); + }); + it("offers nothing where the browser keeps nothing", async () => { const store = new AddedBackgrounds(null); await flushPromises(); @@ -116,7 +133,7 @@ describe("prepareImage", () => { }); describe("IndexedDBImageStorage", () => { - it("keeps images across sessions", async () => { + it("keeps images across sessions until they are deleted", async () => { const indexedDB = new IDBFactory(); const image = new Blob(["x"], { type: "image/webp" }); const first = new IndexedDBImageStorage(indexedDB); @@ -131,6 +148,8 @@ describe("IndexedDBImageStorage", () => { ["a", 1], ["b", 2], ]); + await second.delete("a"); + expect((await first.list()).map(({ id }) => id)).toEqual(["b"]); }); }); @@ -144,5 +163,12 @@ function memoryStorage(kept: KeptImage[] = []): BackgroundImageStorage { kept.push(image); return Promise.resolve(); }, + delete: async (id): Promise => { + kept.splice( + kept.findIndex((k) => k.id === id), + 1, + ); + return Promise.resolve(); + }, }; } diff --git a/src/livekit/backgroundImages.ts b/src/livekit/backgroundImages.ts index 8f48abdf3..f20e2f53b 100644 --- a/src/livekit/backgroundImages.ts +++ b/src/livekit/backgroundImages.ts @@ -42,6 +42,7 @@ export interface KeptImage { export interface BackgroundImageStorage { list(): Promise; put(image: KeptImage): Promise; + delete(id: string): Promise; } /** The backgrounds this device keeps, which never leave it. */ @@ -96,6 +97,14 @@ export class AddedBackgrounds { ]); return kept.id; } + + public async remove(id: string): Promise { + await this.storage?.delete(id); + const current = this.subject$.value ?? []; + const gone = current.find((a) => a.id === id); + if (gone) URL.revokeObjectURL(gone.url); + this.subject$.next(current.filter((a) => a.id !== id)); + } } /** @@ -190,6 +199,10 @@ export class IndexedDBImageStorage implements BackgroundImageStorage { public async put(image: KeptImage): Promise { await this.run("readwrite", (s) => s.put(image)); } + + public async delete(id: string): Promise { + await this.run("readwrite", (s) => s.delete(id)); + } } function openStorage(): BackgroundImageStorage | null {