Skip to content
Open
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
29 changes: 29 additions & 0 deletions src/components/ContextualMenu/ContextualMenu.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -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 <div>other</div>;
};

const user = userEvent.setup();
render(
<>
<ContextualMenu
toggleLabel="Open menu"
links={[{ children: "Item 1" }]}
/>
<Other />
</>,
);

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(<ContextualMenu data-testid="menu" links={[]} position="right" />);
const menu = screen.getByTestId("menu");
Expand Down
40 changes: 38 additions & 2 deletions src/components/Modal/Modal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 ", () => {
Expand Down Expand Up @@ -104,15 +105,19 @@ 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,
}: {
onEscPress: () => void;
}): React.JSX.Element => {
useEffect(() => {
const handleEscPress = (e: KeyboardEvent) => {
if (e.code === "Escape") {
if (e.key === "Escape") {
onEscPress();
}
};
Expand Down Expand Up @@ -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(
<Modal title="Test" close={handleCloseModal}>
<ContextualMenu
toggleLabel="Open menu"
links={[{ children: "Item 1" }]}
/>
</Modal>,
);

// 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();

Expand Down
26 changes: 5 additions & 21 deletions src/components/Modal/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<
{
Expand Down Expand Up @@ -88,20 +89,7 @@ export const Modal = ({
}
};

const handleEscKey = (
event: KeyboardEvent | React.KeyboardEvent<HTMLDivElement>,
) => {
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) {
Expand All @@ -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<HTMLDivElement>);
}
};

document.addEventListener("keydown", keyDown, true);
Expand Down
37 changes: 37 additions & 0 deletions src/components/Navigation/Navigation.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 <div>Mock component with event on Esc key press</div>;
};

render(
<>
<Navigation
logo={<img src="" alt="" />}
searchProps={{ onSearch: jest.fn() }}
/>
<MockEscEventComponent />
</>,
);

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(
<Navigation
Expand Down
2 changes: 1 addition & 1 deletion src/components/Navigation/Navigation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ const Navigation = ({
}
};
// Hide the searchbox when the escape key is pressed.
useOnEscapePressed(() => toggleSearch(false));
useOnEscapePressed(() => toggleSearch(false), { isEnabled: searchOpen });

useEffect(() => {
if (searchOpen) {
Expand Down
57 changes: 57 additions & 0 deletions src/components/SearchAndFilter/SearchAndFilter.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<SearchAndFilter
data-testid="searchandfilter"
filterPanelData={sampleData}
returnSearchData={returnSearchData}
/>,
);
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 <div>Mock component with event on Esc key press</div>;
};

render(
<>
<SearchAndFilter
filterPanelData={sampleData}
returnSearchData={returnSearchData}
/>
<MockEscEventComponent />
</>,
);

expect(getPanel()).toHaveAttribute("aria-hidden", "true");
await userEvent.keyboard("{Escape}");
expect(onEscPress).toHaveBeenCalledTimes(1);
});
});
2 changes: 1 addition & 1 deletion src/components/SearchAndFilter/SearchAndFilter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 27 additions & 11 deletions src/external/usePortal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
RefObject,
MouseEvent,
} from "react";
import { pushEscapeHandler } from "../hooks/useEscapeStack";
import { createPortal } from "react-dom";
import { useSSR } from "./useSSR";

Expand Down Expand Up @@ -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<HTMLElement, Event>)
: undefined,
[closeOnEsc, closePortal],
);

const handleOutsideMouseClick = useCallback(
(e: MouseEvent): void => {
const containsTarget = (target: RefObject<HTMLElement>) =>
Expand Down Expand Up @@ -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 }) => {
Expand Down
1 change: 1 addition & 0 deletions src/hooks/index.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down
Loading