diff --git a/src/components/ContextualMenu/ContextualMenu.test.tsx b/src/components/ContextualMenu/ContextualMenu.test.tsx index 5fd5122f7..05d7157ce 100644 --- a/src/components/ContextualMenu/ContextualMenu.test.tsx +++ b/src/components/ContextualMenu/ContextualMenu.test.tsx @@ -3,6 +3,7 @@ import React from "react"; import userEvent from "@testing-library/user-event"; import Button from "../Button"; +import { useOnEscapePressed } from "hooks"; import ContextualMenu, { Label } from "./ContextualMenu"; import { Label as DropdownLabel } from "./ContextualMenuDropdown/ContextualMenuDropdown"; @@ -16,6 +17,34 @@ describe("ContextualMenu ", () => { expect(screen.getByTestId("menu")).toMatchSnapshot(); }); + it("does not block other Escape handlers elsewhere on the page when open", async () => { + // Regression test: a standalone ContextualMenu (i.e. not nested inside a + // Modal) registers itself as a non-exclusive entry on the global escape + // stack. Opening it should not prevent unrelated useOnEscapePressed-based + // components elsewhere on the page from also responding to Escape. + const onEscape = jest.fn(); + const Other = (): React.JSX.Element => { + useOnEscapePressed(onEscape); + return
other
; + }; + + const user = userEvent.setup(); + render( + <> + + + , + ); + + await user.click(screen.getByRole("button", { name: /open menu/i })); + await user.keyboard("{Escape}"); + + expect(onEscape).toHaveBeenCalledTimes(1); + }); + it("can be aligned to the right", () => { render(); const menu = screen.getByTestId("menu"); diff --git a/src/components/Modal/Modal.test.tsx b/src/components/Modal/Modal.test.tsx index 7a423235e..b9d18d00b 100644 --- a/src/components/Modal/Modal.test.tsx +++ b/src/components/Modal/Modal.test.tsx @@ -4,6 +4,7 @@ import React, { FC, useEffect, useState } from "react"; import Button from "components/Button"; import Input from "components/Input"; +import ContextualMenu from "components/ContextualMenu"; import Modal from "./Modal"; describe("Modal ", () => { @@ -104,7 +105,11 @@ describe("Modal ", () => { expect(closeButton).toHaveFocus(); }); - it("should stop immediate Esc press propagation", async () => { + it("should stop immediate Esc press propagation to other document listeners", async () => { + // A sibling component that registers a raw document keydown listener. + // It should NOT fire when the Modal handles Escape, because the global + // escape stack calls stopImmediatePropagation() before any other + // document listeners run. const MockEscEventComponent = ({ onEscPress, }: { @@ -112,7 +117,7 @@ describe("Modal ", () => { }): React.JSX.Element => { useEffect(() => { const handleEscPress = (e: KeyboardEvent) => { - if (e.code === "Escape") { + if (e.key === "Escape") { onEscPress(); } }; @@ -223,6 +228,37 @@ describe("Modal ", () => { expect(input).toHaveValue("delete item1"); }); + it("should allow Escape to close an open overlay inside the modal before closing the modal", async () => { + // Regression test for https://github.com/canonical/react-components/issues/1305 + // + // Both Modal and ContextualMenu register on the global escape-key stack. + // The menu is opened *after* the modal mounts (via a click), so its handler + // sits on top of the stack and must be called first (LIFO order). + const user = userEvent.setup(); + const handleCloseModal = jest.fn(); + + render( + + + , + ); + + // Open the ContextualMenu after the modal is already mounted — + // this pushes its escape handler on top of the modal's in the LIFO stack. + await user.click(screen.getByRole("button", { name: /open menu/i })); + + // First Escape — should dismiss the ContextualMenu, not the Modal + await user.keyboard("{Escape}"); + expect(handleCloseModal).not.toHaveBeenCalled(); + + // Second Escape — menu is gone, so the Modal should now close + await user.keyboard("{Escape}"); + expect(handleCloseModal).toHaveBeenCalledTimes(1); + }); + it("updates focusable elements when an initially disabled button becomes enabled", async () => { const user = userEvent.setup(); diff --git a/src/components/Modal/Modal.tsx b/src/components/Modal/Modal.tsx index c346372d1..24cc4cb6e 100644 --- a/src/components/Modal/Modal.tsx +++ b/src/components/Modal/Modal.tsx @@ -2,6 +2,7 @@ import classNames from "classnames"; import React, { useId, useRef, useEffect } from "react"; import type { HTMLProps, ReactNode, RefObject } from "react"; import { ClassName, PropsWithSpread } from "types"; +import { useEscapeStack } from "hooks"; export type Props = PropsWithSpread< { @@ -88,20 +89,7 @@ export const Modal = ({ } }; - const handleEscKey = ( - event: KeyboardEvent | React.KeyboardEvent, - ) => { - if ("nativeEvent" in event && event.nativeEvent.stopImmediatePropagation) { - event.nativeEvent.stopImmediatePropagation(); - } else if ("stopImmediatePropagation" in event) { - event.stopImmediatePropagation(); - } else if (event.stopPropagation) { - event.stopPropagation(); - } - if (close) { - close(); - } - }; + useEscapeStack(() => close?.(), { isEnabled: !!close }); useEffect(() => { if (focusRef?.current) { @@ -114,14 +102,10 @@ export const Modal = ({ }, [focusRef]); useEffect(() => { - const keyListenersMap = new Map([ - ["Escape", handleEscKey], - ["Tab", handleTabKey], - ]); - const keyDown = (event: KeyboardEvent) => { - const listener = keyListenersMap.get(event.code); - return listener && listener(event); + if (event.code === "Tab") { + handleTabKey(event as unknown as React.KeyboardEvent); + } }; document.addEventListener("keydown", keyDown, true); diff --git a/src/components/Navigation/Navigation.test.tsx b/src/components/Navigation/Navigation.test.tsx index ff3f66d69..94419167e 100644 --- a/src/components/Navigation/Navigation.test.tsx +++ b/src/components/Navigation/Navigation.test.tsx @@ -275,6 +275,43 @@ it("closes the search form when the escape key is pressed", async () => { expect(screen.queryByRole("searchbox")).not.toBeInTheDocument(); }); +it("does not occupy the escape stack while the search box is closed", async () => { + // Regression test: Navigation must only join the global escape-key stack + // while its search box is open. Otherwise it can sit on top of the LIFO + // stack indefinitely and swallow Escape presses meant for another overlay + // (e.g. a SearchAndFilter panel) opened elsewhere on the page. + const onEscPress = jest.fn(); + + const MockEscEventComponent = (): React.JSX.Element => { + React.useEffect(() => { + const handleEscPress = (e: KeyboardEvent) => { + if (e.key === "Escape") { + onEscPress(); + } + }; + document.addEventListener("keydown", handleEscPress); + return () => { + document.removeEventListener("keydown", handleEscPress); + }; + }); + return
Mock component with event on Esc key press
; + }; + + render( + <> + } + searchProps={{ onSearch: jest.fn() }} + /> + + , + ); + + expect(screen.queryByRole("searchbox")).not.toBeInTheDocument(); + fireEvent.keyDown(document, { key: "Escape", code: "Escape" }); + expect(onEscPress).toHaveBeenCalledTimes(1); +}); + it("closes the search form when opening the mobile menu", async () => { render( toggleSearch(false)); + useOnEscapePressed(() => toggleSearch(false), { isEnabled: searchOpen }); useEffect(() => { if (searchOpen) { diff --git a/src/components/SearchAndFilter/SearchAndFilter.test.tsx b/src/components/SearchAndFilter/SearchAndFilter.test.tsx index b918ccb30..83493d723 100644 --- a/src/components/SearchAndFilter/SearchAndFilter.test.tsx +++ b/src/components/SearchAndFilter/SearchAndFilter.test.tsx @@ -394,4 +394,61 @@ describe("Search and filter", () => { expect(onPanelToggle).not.toHaveBeenCalled(); }); + + it("closes the filter panel when escape is pressed", async () => { + const returnSearchData = jest.fn(); + render( + , + ); + await userEvent.click(screen.getByTestId("searchandfilter")); + expect(getPanel()).toHaveAttribute("aria-hidden", "false"); + + await userEvent.keyboard("{Escape}"); + expect(getPanel()).toHaveAttribute("aria-hidden", "true"); + }); + + it("does not occupy the escape stack while the panel is closed", async () => { + // Regression test for https://github.com/canonical/react-components/pull/1339#discussion_r1234 + // + // SearchAndFilter must only join the global escape-key stack while its + // panel is open. Otherwise, if it is mounted alongside another overlay + // (e.g. Navigation's search box) and registers unconditionally, it can + // sit on top of the LIFO stack and swallow Escape presses meant for that + // other overlay even though its own panel is closed. + const returnSearchData = jest.fn(); + const onEscPress = jest.fn(); + + const MockEscEventComponent = (): React.JSX.Element => { + React.useEffect(() => { + const handleEscPress = (e: KeyboardEvent) => { + if (e.key === "Escape") { + onEscPress(); + } + }; + document.addEventListener("keydown", handleEscPress); + return () => { + document.removeEventListener("keydown", handleEscPress); + }; + }); + return
Mock component with event on Esc key press
; + }; + + render( + <> + + + , + ); + + expect(getPanel()).toHaveAttribute("aria-hidden", "true"); + await userEvent.keyboard("{Escape}"); + expect(onEscPress).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/components/SearchAndFilter/SearchAndFilter.tsx b/src/components/SearchAndFilter/SearchAndFilter.tsx index 1323f0b71..8cc004737 100644 --- a/src/components/SearchAndFilter/SearchAndFilter.tsx +++ b/src/components/SearchAndFilter/SearchAndFilter.tsx @@ -92,7 +92,7 @@ const SearchAndFilter = ({ const closePanel = () => { setFilterPanelHidden(true); }; - useOnEscapePressed(() => closePanel()); + useOnEscapePressed(() => closePanel(), { isEnabled: !filterPanelHidden }); // This useEffect sets up listeners so the panel will close if user clicks // anywhere else on the page or hits the escape key diff --git a/src/external/usePortal.ts b/src/external/usePortal.ts index 0b30cc9d8..64eef99e8 100644 --- a/src/external/usePortal.ts +++ b/src/external/usePortal.ts @@ -13,6 +13,7 @@ import { RefObject, MouseEvent, } from "react"; +import { pushEscapeHandler } from "../hooks/useEscapeStack"; import { createPortal } from "react-dom"; import { useSSR } from "./useSSR"; @@ -126,14 +127,6 @@ export const usePortal = ({ [closePortal, openPortal], ); - const handleKeydown = useCallback( - (e: KeyboardEvent): void => - e.key === "Escape" && closeOnEsc - ? closePortal(e as unknown as SyntheticEvent) - : undefined, - [closeOnEsc, closePortal], - ); - const handleOutsideMouseClick = useCallback( (e: MouseEvent): void => { const containsTarget = (target: RefObject) => @@ -179,21 +172,44 @@ export const usePortal = ({ const node = portal.current; elToMountTo.appendChild(portal.current); - document.addEventListener("keydown", handleKeydown); document.addEventListener( "mousedown", handleMouseDown as unknown as EventListener, ); return () => { - document.removeEventListener("keydown", handleKeydown); document.removeEventListener( "mousedown", handleMouseDown as unknown as EventListener, ); elToMountTo.removeChild(node); }; - }, [isServer, handleOutsideMouseClick, handleKeydown, elToMountTo, portal]); + }, [isServer, handleOutsideMouseClick, elToMountTo, portal]); + + // Keep a ref to closePortal so the escape-stack effect below does not need + // it as a dependency. This prevents closePortal identity changes (e.g. when + // onClose prop changes) from re-registering the handler and incorrectly + // moving an already-open portal to the top of the LIFO stack. + const closePortalRef = useRef(closePortal); + useEffect(() => { + closePortalRef.current = closePortal; + }, [closePortal]); + + // Register on the global escape-key stack only while the portal is open. + // LIFO ordering ensures the most recently opened overlay always handles + // Escape first, regardless of component type or DOM structure. + // + // Registered as non-exclusive: this portal closes itself on Escape, but + // does not stop the event from propagating to unrelated `document` + // keydown listeners (e.g. useOnEscapePressed-based components elsewhere + // on the page). Exclusive ownership of Escape (e.g. while a Modal is open) + // is reserved for entries that opt into it explicitly. + useEffect(() => { + if (isServer || !closeOnEsc || !isOpen) return undefined; + return pushEscapeHandler(() => closePortalRef.current(), { + exclusive: false, + }); + }, [isOpen, closeOnEsc, isServer]); const Portal = useCallback( ({ children }: { children: ReactNode }) => { diff --git a/src/hooks/index.ts b/src/hooks/index.ts index 2d5a700f0..4a0cd7fee 100644 --- a/src/hooks/index.ts +++ b/src/hooks/index.ts @@ -1,4 +1,5 @@ export { useOnClickOutside, useClickOutside } from "./useOnClickOutside"; +export { useEscapeStack } from "./useEscapeStack"; export { useId } from "./useId"; export { useListener } from "./useListener"; export { useOnEscapePressed } from "./useOnEscapePressed"; diff --git a/src/hooks/useEscapeStack.test.ts b/src/hooks/useEscapeStack.test.ts new file mode 100644 index 000000000..58c32bfa3 --- /dev/null +++ b/src/hooks/useEscapeStack.test.ts @@ -0,0 +1,134 @@ +import { pushEscapeHandler } from "./useEscapeStack"; + +const fireEscape = () => + document.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape" })); + +afterEach(() => { + // Each test unregisters its own handlers via the returned cleanup functions, + // so nothing to do here — this guard is just for safety. +}); + +describe("pushEscapeHandler", () => { + it("calls the registered handler when Escape is pressed", () => { + const handler = jest.fn(); + const unregister = pushEscapeHandler(handler); + fireEscape(); + expect(handler).toHaveBeenCalledTimes(1); + unregister(); + }); + + it("does not call the handler after it is unregistered", () => { + const handler = jest.fn(); + const unregister = pushEscapeHandler(handler); + unregister(); + fireEscape(); + expect(handler).not.toHaveBeenCalled(); + }); + + it("calls only the most recently pushed handler (LIFO order)", () => { + const first = jest.fn(); + const second = jest.fn(); + const unregisterFirst = pushEscapeHandler(first); + const unregisterSecond = pushEscapeHandler(second); + + fireEscape(); + expect(second).toHaveBeenCalledTimes(1); + expect(first).not.toHaveBeenCalled(); + + unregisterSecond(); + unregisterFirst(); + }); + + it("falls back to the previous handler once the top handler is removed", () => { + const first = jest.fn(); + const second = jest.fn(); + const unregisterFirst = pushEscapeHandler(first); + const unregisterSecond = pushEscapeHandler(second); + + unregisterSecond(); + fireEscape(); + expect(first).toHaveBeenCalledTimes(1); + expect(second).not.toHaveBeenCalled(); + + unregisterFirst(); + }); + + it("stops propagation so other document keydown listeners do not fire", () => { + const outsideListener = jest.fn(); + // Register an external listener AFTER the stack listener would be added. + const unregister = pushEscapeHandler(jest.fn()); + document.addEventListener("keydown", outsideListener); + + fireEscape(); + expect(outsideListener).not.toHaveBeenCalled(); + + document.removeEventListener("keydown", outsideListener); + unregister(); + }); + + it("removes the correct entry when the same handler reference is pushed twice", () => { + // Regression test: unregistering must not rely on the handler's + // identity, since the same function reference could be passed to + // multiple registrations. + const shared = jest.fn(); + const unregisterFirst = pushEscapeHandler(shared); + const unregisterSecond = pushEscapeHandler(shared); + + // Unregister the first (bottom) entry — the second (top) entry should + // remain on the stack and still fire. + unregisterFirst(); + fireEscape(); + expect(shared).toHaveBeenCalledTimes(1); + + unregisterSecond(); + shared.mockClear(); + fireEscape(); + expect(shared).not.toHaveBeenCalled(); + }); + + describe("non-exclusive entries", () => { + it("calls the handler but does not stop propagation to other listeners", () => { + const handler = jest.fn(); + const outsideListener = jest.fn(); + const unregister = pushEscapeHandler(handler, { exclusive: false }); + document.addEventListener("keydown", outsideListener); + + fireEscape(); + expect(handler).toHaveBeenCalledTimes(1); + expect(outsideListener).toHaveBeenCalledTimes(1); + + document.removeEventListener("keydown", outsideListener); + unregister(); + }); + + it("is skipped in favour of a non-exclusive entry above it, without stopping propagation", () => { + // Mirrors a ContextualMenu (non-exclusive) opened inside a Modal + // (exclusive): the first Escape press should be handled by the + // ContextualMenu only, and should not be silenced for other listeners. + const exclusiveHandler = jest.fn(); + const nonExclusiveHandler = jest.fn(); + const outsideListener = jest.fn(); + + const unregisterExclusive = pushEscapeHandler(exclusiveHandler); + const unregisterNonExclusive = pushEscapeHandler(nonExclusiveHandler, { + exclusive: false, + }); + document.addEventListener("keydown", outsideListener); + + fireEscape(); + expect(nonExclusiveHandler).toHaveBeenCalledTimes(1); + expect(exclusiveHandler).not.toHaveBeenCalled(); + expect(outsideListener).toHaveBeenCalledTimes(1); + + // Once the non-exclusive entry is gone, the exclusive one underneath + // takes over and reclaims exclusive ownership of Escape. + unregisterNonExclusive(); + fireEscape(); + expect(exclusiveHandler).toHaveBeenCalledTimes(1); + expect(outsideListener).toHaveBeenCalledTimes(1); + + document.removeEventListener("keydown", outsideListener); + unregisterExclusive(); + }); + }); +}); diff --git a/src/hooks/useEscapeStack.ts b/src/hooks/useEscapeStack.ts new file mode 100644 index 000000000..44c80d8ab --- /dev/null +++ b/src/hooks/useEscapeStack.ts @@ -0,0 +1,127 @@ +/** + * A module-level LIFO stack of Escape-key handlers. + * + * Any overlay component (Modal, ContextualMenu, Drawer, …) can register a + * handler here. Only the most recently registered handler fires when Escape + * is pressed, so nested overlays always dismiss in the correct order + * regardless of their DOM position or portal placement. + * + * Each entry can be `exclusive` (the default) or not: + * - Exclusive entries (e.g. Modal) claim Escape entirely while they are on + * top of the stack — `stopImmediatePropagation()` is called, so no other + * `document` keydown listener (including non-stack ones, such as + * `useOnEscapePressed`) sees the event. This preserves Modal's long + * standing behaviour of owning Escape while it is open. + * - Non-exclusive entries (e.g. portal-based overlays such as + * ContextualMenu) handle Escape themselves but let the event continue to + * propagate, so they don't silence unrelated listeners elsewhere on the + * page when used on their own. + * + * When a non-exclusive entry sits on top of an exclusive one (e.g. a + * ContextualMenu opened inside a Modal), the first Escape press is handled + * by the non-exclusive entry without claiming the event, and the exclusive + * entry below it is skipped for that press — giving correct two-step + * dismissal (innermost overlay first). + */ + +import { useEffect, useRef } from "react"; + +type EscapeStackEntry = { + onEscape: () => void; + exclusive: boolean; +}; + +const stack: EscapeStackEntry[] = []; + +// Use capture phase so the stack fires before any bubble-phase listeners +// and cannot be silenced by stopPropagation() on a focused child element. +function onKeyDown(e: KeyboardEvent): void { + if (e.key !== "Escape" || stack.length === 0) return; + const top = stack[stack.length - 1]; + if (top.exclusive) { + e.stopImmediatePropagation(); + } + top.onEscape(); +} + +/** + * Push a callback onto the global escape-key stack. + * Returns a cleanup function that removes the handler (suitable for + * use as a `useEffect` cleanup return). + * Handlers are invoked in LIFO order — the last one pushed runs first, + * mirroring the visual stacking of overlays. + * + * @param onEscape - Callback invoked when Escape is pressed and this entry + * is at the top of the stack. + * @param options.exclusive - When `true` (the default), this entry claims + * the Escape press entirely while on top of the stack, preventing any + * other `document` keydown listener from seeing it. Set to `false` for + * overlays that should not silence unrelated Escape handlers when used on + * their own (e.g. portal-based overlays). + * + * Safe to call in SSR environments — it no-ops when `document` is unavailable. + * + * This is the imperative primitive behind `useEscapeStack` and is not part + * of the public package API. React consumers should use `useEscapeStack`. + */ +export function pushEscapeHandler( + onEscape: () => void, + { exclusive = true }: { exclusive?: boolean } = {}, +): () => void { + if (typeof document === "undefined") return () => undefined; + // Wrap in a fresh object so each registration has a unique identity, even + // if the same callback reference is pushed more than once. + const entry: EscapeStackEntry = { onEscape, exclusive }; + if (stack.length === 0) { + // capture: true — fires before bubble-phase listeners on any descendant + document.addEventListener("keydown", onKeyDown, true); + } + stack.push(entry); + return () => { + const idx = stack.indexOf(entry); + if (idx !== -1) stack.splice(idx, 1); + if (stack.length === 0) { + document.removeEventListener("keydown", onKeyDown, true); + } + }; +} + +/** + * React hook that registers an Escape-key handler on the global LIFO stack + * for the lifetime of the component (or while `isEnabled` is true). + * + * The most recently registered handler always fires first, so nested overlays + * naturally dismiss in the correct order regardless of DOM structure. + * + * A stable wrapper is kept in a ref so that inline callbacks passed by callers + * do not cause the handler to be popped/pushed on every re-render, which would + * incorrectly move the overlay to the top of the stack. + * + * @param handler - Callback invoked when Escape is pressed and this handler + * is at the top of the stack. + * @param options.isEnabled - When `false` the handler is not registered + * (defaults to `true`). + * @param options.exclusive - See `pushEscapeHandler`. Defaults to `true`. + */ +export type UseEscapeStackOptions = { + isEnabled?: boolean; + exclusive?: boolean; +}; + +export const useEscapeStack = ( + handler: () => void, + { isEnabled = true, exclusive = true }: UseEscapeStackOptions = {}, +): void => { + // Always keep the ref pointing at the latest handler without changing + // the registered stable wrapper. + const handlerRef = useRef(handler); + useEffect(() => { + handlerRef.current = handler; + }, [handler]); + + useEffect(() => { + if (!isEnabled) return undefined; + // Register a stable wrapper; the ref always calls the latest handler. + return pushEscapeHandler(() => handlerRef.current(), { exclusive }); + }, [isEnabled, exclusive]); +}; diff --git a/src/index.ts b/src/index.ts index 50d9ec989..092094305 100644 --- a/src/index.ts +++ b/src/index.ts @@ -223,6 +223,7 @@ export type { } from "./components/CustomSelect"; export { + useEscapeStack, useOnClickOutside, useClickOutside, useId,