fix: keep the composer's colour and table menus on screen and let Escape close only them

This commit is contained in:
Linus Rath
2026-11-03 05:30:37 +01:00
parent 8931e4ad48
commit 1069c9fac0
4 changed files with 65 additions and 2 deletions
@@ -528,6 +528,16 @@ describe('composer with the @ recipient list', () => {
expect(onClose).not.toHaveBeenCalled();
});
it('lets Escape in a formatting menu close only the menu', async () => {
const { onClose } = await renderComposer();
fireEvent.click(screen.getByTitle('text_color'));
const menu = await screen.findByRole('menu', { name: 'text_color' });
fireEvent.keyDown(menu.querySelector('button')!, { key: 'Escape' });
expect(screen.queryByRole('menu', { name: 'text_color' })).toBeNull();
expect(screen.queryByRole('alertdialog')).toBeNull();
expect(onClose).not.toHaveBeenCalled();
});
// The directory picker is drawn inside the composer itself, not portaled.
it('lets Escape in the directory picker leave the composer alone', async () => {
// Every other capability answers nothing.
@@ -0,0 +1,22 @@
import { describe, expect, it } from 'vitest';
import { popoverShift } from '../rich-text-editor';
describe('popoverShift', () => {
const bounds = { left: 0, right: 320 };
it('leaves a popover that fits where it is', () => {
expect(popoverShift({ left: 20, right: 200 }, bounds)).toBe(0);
});
it('pulls one past the end back inside, with the margin', () => {
expect(popoverShift({ left: 180, right: 350 }, bounds)).toBe(-38);
});
it('pushes one past the start back inside, for right-to-left', () => {
expect(popoverShift({ left: -30, right: 140 }, bounds)).toBe(38);
});
it('keeps the start edge inside when it is wider than the bounds', () => {
expect(popoverShift({ left: 100, right: 500 }, bounds)).toBe(-92);
});
});
+4 -2
View File
@@ -4206,8 +4206,10 @@ export function EmailComposer({
// through React's tree although the dialog portals to the body: they
// belong to that dialog, and Escape must close only it.
if (!(e.target instanceof Node) || !e.currentTarget.contains(e.target)) return;
// Same for a dialog drawn inside the composer (the directory picker).
const targetDialog = e.target instanceof Element ? e.target.closest('[role="dialog"],[role="alertdialog"]') : null;
// Same for what is drawn inside the composer: the directory picker and the
// formatting toolbar's menus, which close on Escape themselves. This
// handler runs in the capture phase, so they would never see it.
const targetDialog = e.target instanceof Element ? e.target.closest('[role="dialog"],[role="alertdialog"],[role="menu"]') : null;
if (targetDialog && e.currentTarget.contains(targetDialog)) return;
const isPlainEscape = e.key === 'Escape' && !e.ctrlKey && !e.metaKey && !e.altKey && !e.shiftKey;
+29
View File
@@ -205,6 +205,22 @@ const ToolbarButton = React.forwardRef<HTMLButtonElement, {
);
});
/**
* How far a popover has to move sideways to stay `margin` px inside `bounds`.
* When it is wider than the bounds its start edge wins, so the first entries
* stay reachable.
*/
export function popoverShift(
popover: { left: number; right: number },
bounds: { left: number; right: number },
margin = 8,
): number {
const overEnd = popover.right - (bounds.right - margin);
const shift = overEnd > 0 ? -overEnd : 0;
const overStart = bounds.left + margin - (popover.left + shift);
return overStart > 0 ? shift + overStart : shift;
}
/**
* A toolbar popover (font size, colours, table) that works from the
* keyboard: it takes the focus when it opens (the checked entry, else the
@@ -239,6 +255,19 @@ function ToolbarPopover({
(checked ?? list[0])?.focus();
}, []);
// Anchored to its button, it would run off a narrow composer's edge.
useLayoutEffect(() => {
const el = ref.current;
const toolbar = el?.closest('[role="toolbar"]');
if (!el || !toolbar) return;
const box = toolbar.getBoundingClientRect();
const shift = popoverShift(el.getBoundingClientRect(), {
left: Math.max(box.left, 0),
right: Math.min(box.right, window.innerWidth),
});
if (shift !== 0) el.style.transform = `translateX(${shift}px)`;
}, []);
const close = (returnFocus: boolean) => {
onClose();
if (returnFocus) triggerRef.current?.focus();