mirror of
https://github.com/vector-im/element-call.git
synced 2026-09-22 22:29:30 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
@@ -30,6 +30,7 @@ import {
|
||||
type MenuOptions,
|
||||
} from "./MediaMuteAndSwitchButton";
|
||||
import {
|
||||
parseEffect,
|
||||
serializeEffect,
|
||||
shippedBackgrounds,
|
||||
} from "../livekit/backgroundEffects";
|
||||
@@ -170,7 +171,18 @@ export const CallFooter: FC<FooterProps> = ({
|
||||
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<FooterProps> = ({
|
||||
...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<FooterProps> = ({
|
||||
? onAddBackgroundImage
|
||||
: undefined
|
||||
}
|
||||
onRemoveBackgroundEffect={onRemoveBackgroundEffect}
|
||||
/>,
|
||||
);
|
||||
} else {
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
},
|
||||
};
|
||||
|
||||
@@ -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<MediaMuteAndSwitchButtonProps> = ({
|
||||
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<MediaMuteAndSwitchButtonProps> = ({
|
||||
// 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) => (
|
||||
<MenuItem
|
||||
as="div"
|
||||
hideChevron
|
||||
disabled={unavailable && effect.kind !== "none"}
|
||||
// The cross is drawn, not focusable: a control inside a menu item is
|
||||
// invalid, and the item is what the arrow keys walk. Delete reaches
|
||||
// the same action from the keyboard, announced by aria-keyshortcuts.
|
||||
aria-keyshortcuts={canRemove(effect) ? "Delete" : undefined}
|
||||
onKeyDown={
|
||||
canRemove(effect)
|
||||
? (e: React.KeyboardEvent): void => {
|
||||
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<MediaMuteAndSwitchButtonProps> = ({
|
||||
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. */
|
||||
<span
|
||||
className={styles.effectRemove}
|
||||
onPointerDown={(e): void => e.stopPropagation()}
|
||||
onClick={(e): void => {
|
||||
e.stopPropagation();
|
||||
onRemoveBackgroundEffect?.(effect.id);
|
||||
}}
|
||||
>
|
||||
<CloseIcon width={16} height={16} />
|
||||
</span>
|
||||
)}
|
||||
{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
|
||||
|
||||
Reference in New Issue
Block a user