Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/components/ai-edition/ColorField.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @vitest-environment jsdom
import "@testing-library/jest-dom";
import { fireEvent, render, screen } from "@testing-library/react";
import { fireEvent, render, screen, waitFor } from "@testing-library/react";
import { beforeAll, describe, expect, it, vi } from "vitest";

// Le traducteur renvoie la cl茅 : une assertion lit mieux contre une cl茅 que contre une phrase qui
Expand Down Expand Up @@ -99,4 +99,15 @@ describe("ColorField", () => {
fireEvent.keyDown(document.body, { key: "Escape" });
expect(onCommit).toHaveBeenCalledTimes(1);
});

it("closes on Escape and hands focus back to the swatch (#1015)", async () => {
const { trigger } = renderField();
fireEvent.click(trigger);
// Radix moves focus into the picker as it opens; the key is pressed from there.
const inside = document.activeElement as HTMLElement;
expect(screen.getByLabelText("annotation.colorPalette").parentElement).toContainElement(inside);
fireEvent.keyDown(inside, { key: "Escape" });
expect(screen.queryByLabelText("annotation.colorPalette")).not.toBeInTheDocument();
await waitFor(() => expect(trigger).toHaveFocus());
});
});
8 changes: 8 additions & 0 deletions src/components/ai-edition/v4/EditorShellV4.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@
-webkit-app-region: no-drag;
app-region: no-drag;
}
/* While a menu hanging off the bar is open (the wordmark's, the presets'), the bar stops being a
titlebar. The OS takes a press on a drag region as a caption click and the page never sees it,
so the outside-click that closes the menu missed every click on the empty bar (#1009). That
click now closes the menu; the bar drags again from the next one. */
.topbar:has([aria-expanded="true"]) {
-webkit-app-region: no-drag;
app-region: no-drag;
}
.sep {
width: 1px;
height: 18px;
Expand Down
17 changes: 17 additions & 0 deletions src/components/ai-edition/v4/EditorTopBar.stylePresets.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// catalog: the bridge, the settings hook, the platform and the toaster are the only fakes.

import "@testing-library/jest-dom";
import { readFileSync } from "node:fs";
import path from "node:path";
import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react";
import { afterEach, beforeEach, describe, expect, it, type Mock, vi } from "vitest";
import { TooltipProvider } from "@/components/ui/tooltip";
Expand Down Expand Up @@ -469,3 +471,18 @@ describe("Presets menu in the editor top bar", () => {
expect(state.set).not.toHaveBeenCalled();
});
});

// The other menu hanging off the bar: while it is open the bar stops being a window-drag region
// too, or a click on the empty bar never reaches the page to close it (#1009).
describe("Presets menu and the bar's drag region", () => {
const css = readFileSync(path.join(__dirname, "EditorShellV4.module.css"), "utf8");
const [, condition = ""] = css.match(/\n\.topbar(:has\([^{]*\))\s*\{/) ?? [];

it("puts the bar under the no-drag rule while it is open", async () => {
renderPane();
const bar = screen.getByRole("button", { name: "Presets" }).closest("header") as HTMLElement;
expect(bar.matches(condition)).toBe(false);
await openMenu();
expect(bar.matches(condition)).toBe(true);
});
});
61 changes: 58 additions & 3 deletions src/components/ai-edition/v4/EditorTopBar.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ vi.mock("@/hooks/useTheme", () => ({
useTheme: () => ({ theme: "dark", toggle: toggleTheme }),
}));

import styles from "./EditorShellV4.module.css";
import { EditorTopBar } from "./EditorTopBar";

const noop = () => {};
Expand Down Expand Up @@ -301,11 +302,40 @@ describe("AppMenu", () => {
);
});

it("closes on Escape", () => {
it("closes on Escape and hands focus back to the trigger", () => {
renderTopBar("Demo Project");
fireEvent.click(screen.getByRole("button", { name: /OpenScreen/ }));
fireEvent.keyDown(screen.getByRole("menu"), { key: "Escape" });
const trigger = screen.getByRole("button", { name: /OpenScreen/ });
fireEvent.click(trigger);
// Pressed on the first row, where the menu put focus.
expect(document.activeElement).toHaveAttribute("role", "menuitem");
fireEvent.keyDown(document.activeElement as HTMLElement, { key: "Escape" });
expect(screen.queryByRole("menu")).not.toBeInTheDocument();
expect(trigger).toHaveFocus();
});

// A press on the menu's padding or a separator drops focus to <body>, outside the menu, where
// a listener on the menu itself never heard the key (#1015).
it("closes on Escape when focus is no longer in the menu", () => {
renderTopBar("Demo Project");
const trigger = screen.getByRole("button", { name: /OpenScreen/ });
fireEvent.click(trigger);
act(() => (document.activeElement as HTMLElement).blur());
fireEvent.keyDown(document.body, { key: "Escape" });
expect(screen.queryByRole("menu")).not.toBeInTheDocument();
expect(trigger).toHaveFocus();
});

it("keeps the Escape that closed it from the window's shortcut handlers", () => {
const onWindowKeyDown = vi.fn();
window.addEventListener("keydown", onWindowKeyDown);
try {
renderTopBar("Demo Project");
fireEvent.click(screen.getByRole("button", { name: /OpenScreen/ }));
fireEvent.keyDown(document.activeElement as HTMLElement, { key: "Escape" });
expect(onWindowKeyDown).not.toHaveBeenCalled();
} finally {
window.removeEventListener("keydown", onWindowKeyDown);
}
});

it("hides Check for Updates when the install channel owns updates", async () => {
Expand Down Expand Up @@ -475,3 +505,28 @@ describe("AppMenu sizing (issue #969)", () => {
expect(version).toMatch(/flex-shrink:\s*0/);
});
});

// jsdom has no window manager, so it cannot play the caption click that swallowed the press. What
// it can pin is the mechanism: the rule that drops the bar's drag region exists, and it matches the
// bar exactly while a menu hanging off it is open (#1009).
describe("top bar drag region while a menu is open (issue #1009)", () => {
const css = readFileSync(path.join(__dirname, "EditorShellV4.module.css"), "utf8");
const [, condition = "", body = ""] = css.match(/\n\.topbar(:has\([^{]*\))\s*\{([^}]*)\}/) ?? [];

it("declares the bar no-drag under that condition", () => {
expect(condition).not.toBe("");
expect(body).toMatch(/-webkit-app-region:\s*no-drag/);
expect(body).toMatch(/(^|\s)app-region:\s*no-drag/);
});

it("puts the bar under it only while the wordmark menu is open", () => {
renderTopBar("Demo Project");
const bar = document.querySelector(`header.${styles.topbar}`) as HTMLElement;
expect(bar.matches(condition)).toBe(false);
fireEvent.click(screen.getByRole("button", { name: /OpenScreen/ }));
expect(bar.matches(condition)).toBe(true);
fireEvent.mouseDown(bar);
expect(screen.queryByRole("menu")).not.toBeInTheDocument();
expect(bar.matches(condition)).toBe(false);
});
});
34 changes: 19 additions & 15 deletions src/components/ai-edition/v4/EditorTopBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -335,8 +335,23 @@ function AppMenu({ actions }: { actions: TopBarActions }) {
const onDocMouseDown = (e: MouseEvent) => {
if (ref.current && !ref.current.contains(e.target as Node)) setOpen(false);
};
// On the document, as the dialogs do (Modals.tsx), not on the menu: a press on the menu's
// padding or a separator drops focus to <body>, where a listener on the menu never hears
// the key (#1015). Escape hands focus back to the trigger, and goes no further, so nothing
// else bound to it acts on the same press.
const onDocKeyDown = (e: KeyboardEvent) => {
if (e.key !== "Escape") return;
e.preventDefault();
e.stopPropagation();
setOpen(false);
triggerRef.current?.focus();
};
document.addEventListener("mousedown", onDocMouseDown);
return () => document.removeEventListener("mousedown", onDocMouseDown);
document.addEventListener("keydown", onDocKeyDown);
return () => {
document.removeEventListener("mousedown", onDocMouseDown);
document.removeEventListener("keydown", onDocKeyDown);
};
}, [open]);

// Focus the first item as the menu appears, so it is operable from the keyboard without a
Expand All @@ -346,20 +361,7 @@ function AppMenu({ actions }: { actions: TopBarActions }) {
menuRef.current?.querySelector<HTMLButtonElement>('[role="menuitem"]')?.focus();
}, [open]);

const close = (restoreFocus: boolean) => {
setOpen(false);
// Escape and Tab-out hand focus back to the trigger; a click does not, because the
// pointer user did not come from there and a focus ring appearing under the cursor
// reads as a bug.
if (restoreFocus) triggerRef.current?.focus();
};

const onMenuKeyDown = (e: ReactKeyboardEvent<HTMLDivElement>) => {
if (e.key === "Escape") {
e.preventDefault();
close(true);
return;
}
if (e.key !== "ArrowDown" && e.key !== "ArrowUp") return;
e.preventDefault();
const items = Array.from(
Expand All @@ -375,7 +377,9 @@ function AppMenu({ actions }: { actions: TopBarActions }) {
};

const run = (action: () => void) => () => {
close(false);
// Unlike Escape, a click does not hand focus back to the trigger: the pointer user did not
// come from there, and a focus ring appearing under the cursor reads as a bug.
setOpen(false);
action();
};

Expand Down
30 changes: 30 additions & 0 deletions src/components/launch/LaunchWindow.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1510,6 +1510,22 @@ describe("LaunchWindow popover dismissal", () => {
expect(i18nState.value.setLocale).not.toHaveBeenCalled();
});

it("hands focus back to the language button when Escape closes its menu", async () => {
renderLaunchWindow();
const menu = await openLanguageMenu();
// Tabbed into the list: unmounting it must not leave focus on <body>.
const item = within(menu).getAllByRole("menuitemradio")[0];
act(() => item.focus());

fireEvent.keyDown(item, { key: "Escape" });

await waitFor(() => {
expect(screen.queryByTestId("hud-language-menu")).not.toBeInTheDocument();
});
expect(screen.getByRole("button", { name: "Language: English" })).toHaveFocus();
expect(i18nState.value.setLocale).not.toHaveBeenCalled();
});

it("closes the device-settings panel on Escape", async () => {
renderLaunchWindow();
await openDeviceSettings();
Expand All @@ -1521,6 +1537,20 @@ describe("LaunchWindow popover dismissal", () => {
});
});

it("hands focus back to the gear when Escape closes the device-settings panel", async () => {
renderLaunchWindow();
const panel = await openDeviceSettings();
const close = within(panel).getByRole("button", { name: "Close" });
act(() => close.focus());

fireEvent.keyDown(close, { key: "Escape" });

await waitFor(() => {
expect(screen.queryByTestId("hud-device-settings")).not.toBeInTheDocument();
});
expect(screen.getByTestId("launch-device-settings-button")).toHaveFocus();
});

it("closes the device-settings panel on a pointerdown outside the trigger and the panel", async () => {
renderLaunchWindow();
await openDeviceSettings();
Expand Down
5 changes: 4 additions & 1 deletion src/components/launch/LaunchWindow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,9 @@ export function LaunchWindow() {

const handleEscape = (event: KeyboardEvent) => {
if (event.key === "Escape") {
// Back to the control that opened it, as the editor's menus do: with focus inside
// the popover, unmounting it would drop focus to <body>.
(isLanguageMenuOpen ? languageTriggerRef : settingsTriggerRef).current?.focus();
closePopovers();
}
};
Expand All @@ -353,7 +356,7 @@ export function LaunchWindow() {
window.removeEventListener("keydown", handleEscape);
window.removeEventListener("blur", closePopovers);
};
}, [closePopovers, isPopoverOpen]);
}, [closePopovers, isPopoverOpen, isLanguageMenuOpen]);

// ---------------------------------------------------------------------------
// Overlay window sizing
Expand Down
Loading