From 3ac42a735b9bea69b1b3cb282d38cfe9c44bfd3c Mon Sep 17 00:00:00 2001 From: fkwp Date: Thu, 17 Sep 2026 18:45:22 +0200 Subject: [PATCH] Make the cross remove by mouse, not just by keyboard - The cross now sits beside the tile rather than inside it. Inside, the menu reached the item before any handler on the cross, whichever phase it was in: the tile was chosen, became the one in force, and the cross was taken away before it had acted - That is why the keyboard worked and the mouse did not, and why the static stories missed it: with selection fixed by an arg, nothing re-rendered and the click survived - A story with live selection covers it now: the cross removes the tile and does not make it the one in force on the way out Co-Authored-By: Claude Opus 5 (1M context) --- .../MediaMuteAndSwitchButton.module.css | 18 +- .../MediaMuteAndSwitchButton.stories.tsx | 54 +++++ src/components/MediaMuteAndSwitchButton.tsx | 188 +++++++++--------- 3 files changed, 166 insertions(+), 94 deletions(-) diff --git a/src/components/MediaMuteAndSwitchButton.module.css b/src/components/MediaMuteAndSwitchButton.module.css index bf9a5e62a..47c349991 100644 --- a/src/components/MediaMuteAndSwitchButton.module.css +++ b/src/components/MediaMuteAndSwitchButton.module.css @@ -352,8 +352,20 @@ Please see LICENSE in the repository root for full details. cursor: pointer; } -.effectGrid .effectTile:hover .effectRemove, -.effectGrid .effectTile:focus-within .effectRemove, -.menu[data-focus-modality="keyboard"] .effectTile:focus .effectRemove { +.effectGrid .effectTileWrap:hover .effectRemove, +.effectGrid .effectTileWrap:focus-within .effectRemove { display: flex; } + +/* Holds a tile that can be removed, so the cross can sit over it without + being inside the menu item. */ +.effectTileWrap { + position: relative; + display: flex; + min-inline-size: 0; +} + +.effectGrid .effectTileWrap > .effectTile { + flex: 1; + min-inline-size: 0; +} diff --git a/src/components/MediaMuteAndSwitchButton.stories.tsx b/src/components/MediaMuteAndSwitchButton.stories.tsx index 6371126c9..8bb945424 100644 --- a/src/components/MediaMuteAndSwitchButton.stories.tsx +++ b/src/components/MediaMuteAndSwitchButton.stories.tsx @@ -960,3 +960,57 @@ export const AddedBackgroundsCanBeRemoved: Story = { ); }, }; + +/** + * Removal where choosing a tile really changes what is in force, as it does in + * a call. + * + * The static stories cannot catch this: pressing the cross also reaches the + * tile, and with live selection that made the tile the one in force, which + * took the cross away before its own click landed. By mouse nothing happened; + * by keyboard it worked, because the keyboard never touches the cross. + */ +export const RemovingWithLiveSelection: Story = { + args: { ...AddedBackgroundsCanBeRemoved.args }, + render: function WithLiveSelection(args): JSX.Element { + const [selected, setSelected] = useState("none"); + const [effects, setEffects] = useState(args.backgroundEffects ?? []); + return ( + + setEffects((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); + // Beside the tile, not inside it: a control within a menu item would be + // invalid, and the item would take the click first. + const cross = tile.parentElement!.querySelector( + `.${styles.effectRemove}`, + )!; + await expect(cross).toBeVisible(); + await userEvent.click(cross); + + // Gone, and it did not make itself the one in force on the way out. + await waitFor(async () => + expect( + body.queryByRole("menuitemradio", { name: "Background 4" }), + ).toBeNull(), + ); + const none = await body.findByRole("menuitemradio", { name: "None" }); + await expect(none).toHaveAttribute("aria-checked", "true"); + }, +}; diff --git a/src/components/MediaMuteAndSwitchButton.tsx b/src/components/MediaMuteAndSwitchButton.tsx index c10760c4b..8139bb801 100644 --- a/src/components/MediaMuteAndSwitchButton.tsx +++ b/src/components/MediaMuteAndSwitchButton.tsx @@ -488,99 +488,105 @@ export const MediaMuteAndSwitchButton: FC = ({ 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, - })} - label={effect.label} - // An image is its own label, so its name is carried for assistive - // technology alone rather than drawn over the picture. - labelProps={{ - className: - effect.kind === "image" - ? styles.effectLabelUnseen - : styles.effectLabel, - }} - Icon={ - - {effect.kind === "image" && ( - - )} - {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(); + const tiles = list.map((effect) => { + const tile = ( + { + if (e.key !== "Delete" && e.key !== "Backspace") return; + e.preventDefault(); 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 - // it. - - ) : ( - <> - {effect.kind === "none" && } - {effect.kind === "blur" && ( - - )} - - )} + } + : undefined + } + className={classNames(styles.effectTile, { + [styles.effectTileSelected]: selectedBackgroundEffect === effect.id, + })} + label={effect.label} + // An image is its own label, so its name is carried for assistive + // technology alone rather than drawn over the picture. + labelProps={{ + className: + effect.kind === "image" + ? styles.effectLabelUnseen + : styles.effectLabel, + }} + Icon={ + + {effect.kind === "image" && ( + + )} + {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 + // it. + + ) : ( + <> + {effect.kind === "none" && ( + + )} + {effect.kind === "blur" && ( + + )} + + )} + + } + onSelect={(e) => { + e.preventDefault(); + if (effect.id === selectedBackgroundEffect) return; + onSelectBackgroundEffect?.(effect.id); + }} + key={effect.id} + role="menuitemradio" + aria-checked={selectedBackgroundEffect === effect.id} + /> + ); + // The cross sits beside the item rather than inside it. Inside, its + // press and its click both reach the item first however they are + // handled, so the tile was chosen and the cross taken away before it + // could act: by mouse nothing happened, while the keyboard, which never + // touches it, worked. + return canRemove(effect) ? ( +
+ {tile} + {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, + jsx-a11y/no-static-element-interactions -- + Deliberately not a control: aria-hidden, so assistive technology + never meets it, and the keyboard reaches the same action through + Delete on the tile. A role and a tab stop here would add a stop + the menu's arrow keys know nothing about. */} + onRemoveBackgroundEffect?.(effect.id)} + > + - } - onSelect={(e) => { - e.preventDefault(); - if (effect.id === selectedBackgroundEffect) return; - onSelectBackgroundEffect?.(effect.id); - }} - key={effect.id} - role="menuitemradio" - aria-checked={selectedBackgroundEffect === effect.id} - /> - )); +
+ ) : ( + tile + ); + }); // Adding is a command, not a choice, so it is a plain item among the tiles // rather than another radio. if (onAddBackgroundImage !== undefined)