Skip to content

fix(Modal): call close when pressing Esc#1374

Open
kimanhou wants to merge 1 commit into
canonical:mainfrom
kimanhou:WD-36511-fix-modal-esc
Open

fix(Modal): call close when pressing Esc#1374
kimanhou wants to merge 1 commit into
canonical:mainfrom
kimanhou:WD-36511-fix-modal-esc

Conversation

@kimanhou
Copy link
Copy Markdown
Contributor

@kimanhou kimanhou commented May 8, 2026

Done

  • fix(Modal): call close when pressing Esc

QA

Pinging @canonical/react-library-maintainers for a review.

Storybook

To see rendered examples of all react-components, run:

yarn start

QA in your project

from react-components run:

yarn build
npm pack

Install the resulting tarball in your project with:

yarn add <path-to-tarball>

QA steps

  • Steps for QA.

Percy steps

  • List any expected visual change in Percy, or write something like "No visual changes expected" if none is expected.

Fixes

Fixes: # .

Signed-off-by: Kim Anh Nguyen <4783194+kimanhou@users.noreply.github.com>
@kimanhou kimanhou force-pushed the WD-36511-fix-modal-esc branch from 04a5d95 to 9f231dc Compare May 8, 2026 13:32
Comment on lines 116 to +132
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.key === "Escape") {
handleEscKey(event);
}

if (event.key === "Tab") {
handleTabKey(event as unknown as React.KeyboardEvent<HTMLDivElement>);
}
};

document.addEventListener("keydown", keyDown);
document.addEventListener("keydown", keyDown, true);

return () => {
document.removeEventListener("keydown", keyDown);
document.removeEventListener("keydown", keyDown, true);
};
});
}, [close]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this logically equivalent to what was there before? Only change I see is the additional true parameter on the addEventListener and removeEventListener calls and the close dependency. If those two are the fix, shall we keep the rest as it was?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants