mirror of
https://github.com/penpot/penpot.git
synced 2026-10-29 05:50:35 +00:00
✨ Close search and shortcuts panels with Escape (#12374)
* ✨ Close search and shortcuts panels with Escape The workspace search and shortcuts panels could only be dismissed with the mouse, which interrupts keyboard-first workflows. A new use-escape-to-close hook gives each panel an on-key-down handler that closes it on Escape while the user is working in it: the latest pointer press or focus change must have landed inside the panel, or the panel must hold focus when it opens (a panel that autofocuses its search box, such as Shortcuts, is closed by the first Escape). Focus left behind in a panel after the user moved on to the canvas does not count, so Escape keeps its usual meaning there. The handler never stops the event. The global Escape shortcut still runs and finishes in-progress operations such as path drawing; an earlier version stopped propagation and left a half-drawn path that failed shape validation on the next commit. In the search panel an open filters menu takes the first Escape and the panel closes on the second. The history panel needs no handler: Escape already closes it through the :interrupt it emits. Closes #9540 AI-assisted-by: claude-opus-5-5 AI-assisted-by: claude-sonnet-5-5 Signed-off-by: Filip Sajdak <filip.sajdak@siili.com> * 📎 Add e2e test for closing panels with Escape Cover the Escape handling added for the layers search and shortcuts panels with four Playwright cases on the mocked workspace: - Escape closes the layers search panel. - With the search filters menu open, the first Escape closes only the menu and the second closes the panel. - Escape closes the shortcuts panel right after it opens with `?`. The panel autofocuses its search box, so the first Escape must close it instead of only blurring the box. - A path drawn while focus sits in the shortcuts panel is finished by Escape, which proves the panel does not swallow the global shortcut. The first three cases fail on develop. The shortcuts case also fails when the hook does not seed its tracker from the focused element. AI-assisted-by: claude-sonnet-5-5 Signed-off-by: Filip Sajdak <filip.sajdak@siili.com> * 🐛 Close History with Escape when opened by shortcut or menu Only the header button opened History through initialize-history, which removes the :document-history flag on the next :interrupt. The Ctrl/Cmd+Alt+H shortcut and the "Version history" menu item toggled the flag directly, so the global Escape left the panel open. Both now emit initialize-history, like the header, and keep their own event origin. Two e2e cases cover the shortcut and the menu; both fail without this change. AI-assisted-by: claude-sonnet-5-5 Signed-off-by: Filip Sajdak <filip.sajdak@siili.com> * ♻️ Apply review suggestions to Escape-to-close Use dom/child? instead of .contains in the Escape hook and drop the redundant nil checks. Drive the path test through clickAt so its clicks wait for the path tool. Add a negative Search case: Escape reaches the panel after the last click landed on the viewport, and the panel stays open. That case fails when the containment check is removed. AI-assisted-by: claude-sonnet-5-5 Signed-off-by: Filip Sajdak <filip.sajdak@siili.com> --------- Signed-off-by: Filip Sajdak <filip.sajdak@siili.com>
This commit is contained in:
parent
e24e9dc747
commit
576bf4ed3d
138
frontend/playwright/ui/specs/escape-closes-panels.spec.js
Normal file
138
frontend/playwright/ui/specs/escape-closes-panels.spec.js
Normal file
@ -0,0 +1,138 @@
|
||||
import { test, expect } from "@playwright/test";
|
||||
import { WorkspacePage } from "../pages/WorkspacePage";
|
||||
|
||||
const VIEWPORT_POINTS = [
|
||||
[200, 200],
|
||||
[400, 220],
|
||||
[320, 420],
|
||||
];
|
||||
|
||||
const openSearch = async (page) => {
|
||||
await page.getByRole("button", { name: "Search" }).click();
|
||||
const findInput = page.getByRole("textbox", { name: /^Find/ });
|
||||
await expect(findInput).toBeVisible();
|
||||
return findInput;
|
||||
};
|
||||
|
||||
const openShortcuts = async (page) => {
|
||||
await page.keyboard.press("?");
|
||||
const searchInput = page.getByPlaceholder("Search shortcuts");
|
||||
await expect(searchInput).toBeVisible();
|
||||
return searchInput;
|
||||
};
|
||||
|
||||
const saveVersionButton = (page) =>
|
||||
page.getByRole("button", { name: "Save version" });
|
||||
|
||||
const openHistoryAssertion = (page) =>
|
||||
expect(saveVersionButton(page)).toBeVisible();
|
||||
|
||||
test.beforeEach(async ({ page }) => {
|
||||
await WorkspacePage.init(page);
|
||||
const workspacePage = new WorkspacePage(page);
|
||||
await workspacePage.setupEmptyFile();
|
||||
await workspacePage.mockRPC(
|
||||
"get-file-snapshots?file-id=*",
|
||||
"workspace/versions-snapshot-1.json",
|
||||
);
|
||||
await workspacePage.goToWorkspace();
|
||||
});
|
||||
|
||||
test("Escape closes the layers search panel", async ({ page }) => {
|
||||
const findInput = await openSearch(page);
|
||||
|
||||
await page.keyboard.press("Escape");
|
||||
|
||||
await expect(findInput).toHaveCount(0);
|
||||
});
|
||||
|
||||
test("Escape closes the search filters menu before the search panel", async ({
|
||||
page,
|
||||
}) => {
|
||||
const findInput = await openSearch(page);
|
||||
await page.getByRole("button", { name: "Filter" }).click();
|
||||
const menuItem = page.getByText("Masks", { exact: true });
|
||||
await expect(menuItem).toBeVisible();
|
||||
|
||||
await page.keyboard.press("Escape");
|
||||
await expect(menuItem).toHaveCount(0);
|
||||
await expect(findInput).toBeVisible();
|
||||
|
||||
await page.keyboard.press("Escape");
|
||||
await expect(findInput).toHaveCount(0);
|
||||
});
|
||||
|
||||
test("Escape keeps the search panel open after clicking the viewport", async ({
|
||||
page,
|
||||
}) => {
|
||||
const workspacePage = new WorkspacePage(page);
|
||||
const findInput = await openSearch(page);
|
||||
|
||||
await workspacePage.clickAt(...VIEWPORT_POINTS[0]);
|
||||
await page.keyboard.press("Escape");
|
||||
await expect(findInput).toBeVisible();
|
||||
|
||||
// The click moves focus out of the panel, so a real key press would
|
||||
// never reach its handler. Dispatch Escape on the input instead, to
|
||||
// exercise the case the hook exists for: the event reaches the panel
|
||||
// although the user's last action was on the canvas.
|
||||
await findInput.dispatchEvent("keydown", {
|
||||
key: "Escape",
|
||||
code: "Escape",
|
||||
keyCode: 27,
|
||||
bubbles: true,
|
||||
});
|
||||
|
||||
await expect(findInput).toBeVisible();
|
||||
});
|
||||
|
||||
test("Escape closes the shortcuts panel right after it opens", async ({
|
||||
page,
|
||||
}) => {
|
||||
const searchInput = await openShortcuts(page);
|
||||
|
||||
await page.keyboard.press("Escape");
|
||||
|
||||
await expect(searchInput).toHaveCount(0);
|
||||
});
|
||||
|
||||
test("Escape closes History opened with its shortcut", async ({ page }) => {
|
||||
await page.keyboard.press("ControlOrMeta+Alt+h");
|
||||
await openHistoryAssertion(page);
|
||||
|
||||
await page.keyboard.press("Escape");
|
||||
|
||||
await expect(saveVersionButton(page)).toHaveCount(0);
|
||||
});
|
||||
|
||||
test("Escape closes History opened from the main menu", async ({ page }) => {
|
||||
await page.getByRole("button", { name: "Main menu" }).click();
|
||||
await page.getByText("file").last().click();
|
||||
await page.locator("#file-menu-show-version-history").click();
|
||||
await openHistoryAssertion(page);
|
||||
|
||||
await page.keyboard.press("Escape");
|
||||
|
||||
await expect(saveVersionButton(page)).toHaveCount(0);
|
||||
});
|
||||
|
||||
test("Escape finishes a path drawn with the shortcuts panel open", async ({
|
||||
page,
|
||||
}) => {
|
||||
const workspacePage = new WorkspacePage(page);
|
||||
const searchInput = await openShortcuts(page);
|
||||
await page.getByRole("button", { name: "BASICS" }).click();
|
||||
|
||||
await page.keyboard.press("p");
|
||||
for (const [x, y] of VIEWPORT_POINTS) {
|
||||
await workspacePage.clickAt(x, y);
|
||||
}
|
||||
await page.keyboard.press("Escape");
|
||||
|
||||
// The shortcuts panel replaces the layers tree, so the new path is
|
||||
// observed through the design tab of the selected shape.
|
||||
await expect(
|
||||
page.getByRole("region", { name: "Stroke section" }),
|
||||
).toBeVisible();
|
||||
await expect(searchInput).toBeVisible();
|
||||
});
|
||||
@ -19,6 +19,7 @@
|
||||
[app.main.data.workspace.comments :as dwcm]
|
||||
[app.main.data.workspace.drawing :as dwd]
|
||||
[app.main.data.workspace.drawing.common :as dwdc]
|
||||
[app.main.data.workspace.history :as dwh]
|
||||
[app.main.data.workspace.layers :as dwly]
|
||||
[app.main.data.workspace.libraries :as dwl]
|
||||
[app.main.data.workspace.shape-layout :as dwsl]
|
||||
@ -655,7 +656,8 @@
|
||||
:subsections [:panels]
|
||||
:section [:workspace]
|
||||
:fn #(emit-when-no-readonly
|
||||
(dw/toggle-layout-flag :document-history))}
|
||||
(-> (dwh/initialize-history)
|
||||
(vary-meta assoc ::ev/origin "workspace:shortcuts")))}
|
||||
|
||||
:toggle-colorpalette {:tooltip (ds/alt "P")
|
||||
:label (fn [] (tr "shortcuts.toggle-colorpalette"))
|
||||
|
||||
@ -16,6 +16,7 @@
|
||||
[app.main.store :as st]
|
||||
[app.util.dom :as dom]
|
||||
[app.util.dom.dnd :as dnd]
|
||||
[app.util.keyboard :as kbd]
|
||||
[app.util.storage :as storage]
|
||||
[app.util.timers :as ts]
|
||||
[app.util.webapi :as wapi]
|
||||
@ -597,3 +598,44 @@
|
||||
(rx/dispose! sub))))
|
||||
|
||||
[rowref limit])))
|
||||
|
||||
(defn use-escape-to-close
|
||||
"Returns an `on-key-down` handler that closes a panel with Escape.
|
||||
|
||||
The panel only closes while the user is working in it: the latest
|
||||
pointer press or focus change must have landed inside the element
|
||||
that owns the handler, or that element must hold focus when it
|
||||
mounts. Focus left behind in a panel after the user moved on to the
|
||||
canvas does not count, so Escape keeps its usual meaning there.
|
||||
|
||||
The event is never stopped: the global Escape shortcut still runs
|
||||
and finishes any in-progress operation.
|
||||
|
||||
`dismiss-nested`, when given, is called first; it returns true when
|
||||
it consumed the Escape (an open menu, an inline rename), and then
|
||||
the panel stays open."
|
||||
([on-close]
|
||||
(use-escape-to-close on-close nil))
|
||||
([on-close dismiss-nested]
|
||||
(let [last-target-ref (mf/use-ref nil)]
|
||||
(mf/with-effect []
|
||||
(let [track (fn [event]
|
||||
(mf/set-ref-val! last-target-ref (dom/get-target event)))]
|
||||
;; A panel that autofocuses its input on open has already
|
||||
;; received its `focusin` before this effect runs.
|
||||
(mf/set-ref-val! last-target-ref (dom/get-active))
|
||||
(.addEventListener js/document "pointerdown" track true)
|
||||
(.addEventListener js/document "focusin" track true)
|
||||
#(do (.removeEventListener js/document "pointerdown" track true)
|
||||
(.removeEventListener js/document "focusin" track true))))
|
||||
|
||||
(mf/use-fn
|
||||
(mf/deps on-close dismiss-nested)
|
||||
(fn [event]
|
||||
(let [panel (dom/get-current-target event)
|
||||
last-target (mf/ref-val last-target-ref)]
|
||||
(when (and (kbd/esc? event)
|
||||
(dom/child? last-target panel)
|
||||
(not (and (fn? dismiss-nested) (dismiss-nested))))
|
||||
(dom/blur! (dom/get-target event))
|
||||
(on-close))))))))
|
||||
|
||||
@ -23,6 +23,7 @@
|
||||
[app.main.data.shortcuts :as scd]
|
||||
[app.main.data.workspace :as dw]
|
||||
[app.main.data.workspace.comments :as dwcm]
|
||||
[app.main.data.workspace.history :as dwh]
|
||||
[app.main.data.workspace.libraries :as dwl]
|
||||
[app.main.data.workspace.mcp :as mcp]
|
||||
[app.main.data.workspace.shortcuts :as sc]
|
||||
@ -617,9 +618,9 @@
|
||||
|
||||
on-show-version-history
|
||||
(mf/use-fn
|
||||
(mf/deps file-id)
|
||||
(fn [_]
|
||||
(st/emit! (dw/toggle-layout-flag :document-history))))
|
||||
(st/emit! (-> (dwh/initialize-history)
|
||||
(vary-meta assoc ::ev/origin "workspace:menu")))))
|
||||
|
||||
on-show-version-history-key-down
|
||||
(mf/use-fn
|
||||
|
||||
@ -386,6 +386,21 @@
|
||||
(st/emit! dw/close-layers-search)
|
||||
(st/emit! (dw/open-layers-search :find {:force? true}))))))
|
||||
|
||||
close-search
|
||||
(mf/use-fn
|
||||
#(st/emit! dw/close-layers-search))
|
||||
|
||||
dismiss-filters-menu
|
||||
(mf/use-fn
|
||||
(mf/deps show-menu? hide-menu)
|
||||
(fn []
|
||||
(when ^boolean show-menu?
|
||||
(hide-menu)
|
||||
true)))
|
||||
|
||||
on-search-key-down
|
||||
(hooks/use-escape-to-close close-search dismiss-filters-menu)
|
||||
|
||||
remove-filter
|
||||
(mf/use-fn
|
||||
(fn [event]
|
||||
@ -587,7 +602,8 @@
|
||||
#(mf/html
|
||||
(if show-search?
|
||||
[:*
|
||||
[:div {:class (stl/css :tool-window-bar)}
|
||||
[:div {:class (stl/css :tool-window-bar)
|
||||
:on-key-down on-search-key-down}
|
||||
[:> search-bar* {:input-ref search-input-ref
|
||||
:class (stl/css :search-item)
|
||||
:on-change update-search-text
|
||||
@ -612,7 +628,8 @@
|
||||
:on-click toggle-search
|
||||
:icon i/close}]]
|
||||
|
||||
[:div {:class (stl/css :replace-wrapper)}
|
||||
[:div {:class (stl/css :replace-wrapper)
|
||||
:on-key-down on-search-key-down}
|
||||
(when ^boolean find-replace-mode?
|
||||
[:div {:class (stl/css :replace-row)}
|
||||
[:> input* {:type "text"
|
||||
|
||||
@ -23,6 +23,7 @@
|
||||
[app.main.ui.components.search-bar :refer [search-bar*]]
|
||||
[app.main.ui.ds.foundations.assets.icon :as i :refer [icon*]]
|
||||
[app.main.ui.ds.product.panel-title :refer [panel-title*]]
|
||||
[app.main.ui.hooks :as hooks]
|
||||
[app.main.ui.shortcuts :as ss]
|
||||
[app.util.dom :as dom]
|
||||
[app.util.i18n :refer [tr]]
|
||||
@ -60,7 +61,9 @@
|
||||
filter-term* (mf/use-state "")
|
||||
filter-term (deref filter-term*)
|
||||
|
||||
close-fn #(st/emit! (dw/toggle-layout-flag :shortcuts))
|
||||
close-fn (mf/use-fn #(st/emit! (dw/toggle-layout-flag :shortcuts)))
|
||||
|
||||
on-key-down (hooks/use-escape-to-close close-fn)
|
||||
|
||||
{:keys [all-shortcuts all-sc-names all-sub-names all-section-names]}
|
||||
(ss/build-all-shortcuts workspace-shortcuts dashboard-shortcuts viewer-shortcuts)
|
||||
@ -127,7 +130,8 @@
|
||||
(mf/use-fn
|
||||
#(st/emit! (rt/nav :settings-shortcuts {} {::rt/new-window true})))]
|
||||
|
||||
[:div {:class (dm/str class " " (stl/css :shortcuts))}
|
||||
[:div {:class (dm/str class " " (stl/css :shortcuts))
|
||||
:on-key-down on-key-down}
|
||||
[:> panel-title* {:class (stl/css :shortcuts-title)
|
||||
:text (tr "shortcuts.title")
|
||||
:on-close close-fn}]
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user