-
Notifications
You must be signed in to change notification settings - Fork 73
fix(Modal): allow Escape to close ContextualMenu before closing Modal #1339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
koushik717
wants to merge
8
commits into
canonical:main
Choose a base branch
from
koushik717:fix/contextual-menu-escape-in-modal
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
433441c
fix(Modal): allow Escape to close ContextualMenu before closing Modal
koushik717 0b0d300
fix(Modal): replace CSS-selector guard with a global LIFO escape-key …
koushik717 9ab706d
Merge branch 'main' into fix/contextual-menu-escape-in-modal
koushik717 e531964
Merge branch 'main' into fix/contextual-menu-escape-in-modal
koushik717 16813f7
fix(useEscapeStack): address Copilot review feedback
koushik717 82eb54b
fix(useEscapeStack): align naming, fix Navigation/SearchAndFilter sta…
koushik717 99f88d1
fix(useOnEscapePressed): revert to pre-PR implementation, no breaking…
koushik717 d5bc53b
fix(useEscapeStack): make non-Modal overlays non-exclusive on the esc…
koushik717 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| 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(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /** | ||
| * 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. | ||
| */ | ||
|
|
||
| import { useEffect } from "react"; | ||
|
koushik717 marked this conversation as resolved.
Outdated
|
||
|
|
||
| const handlers: Array<() => void> = []; | ||
|
|
||
| function onKeyDown(e: KeyboardEvent): void { | ||
| if (e.key !== "Escape" || handlers.length === 0) return; | ||
| e.stopImmediatePropagation(); | ||
| handlers[handlers.length - 1](); | ||
| } | ||
|
|
||
| /** | ||
| * 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. | ||
| */ | ||
| export function pushEscapeHandler(handler: () => void): () => void { | ||
| if (handlers.length === 0) { | ||
| document.addEventListener("keydown", onKeyDown); | ||
| } | ||
| handlers.push(handler); | ||
| return () => { | ||
| const idx = handlers.lastIndexOf(handler); | ||
| if (idx !== -1) handlers.splice(idx, 1); | ||
| if (handlers.length === 0) { | ||
| document.removeEventListener("keydown", onKeyDown); | ||
| } | ||
| }; | ||
| } | ||
|
koushik717 marked this conversation as resolved.
Outdated
koushik717 marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * React hook that registers an Escape-key handler on the global LIFO stack | ||
| * for the lifetime of the component (or while `isActive` is true). | ||
| * | ||
| * The most recently registered handler always fires first, so nested overlays | ||
| * naturally dismiss in the correct order regardless of DOM structure. | ||
| * | ||
| * @param handler - Callback invoked when Escape is pressed and this handler | ||
| * is at the top of the stack. | ||
| * @param options.isActive - When `false` the handler is not registered | ||
| * (defaults to `true`). | ||
| */ | ||
| export const useEscapeStack = ( | ||
| handler: () => void, | ||
| { isActive } = { isActive: true }, | ||
|
koushik717 marked this conversation as resolved.
Outdated
|
||
| ): void => { | ||
| useEffect(() => { | ||
| if (!isActive) return undefined; | ||
| return pushEscapeHandler(handler); | ||
| }, [handler, isActive]); | ||
| }; | ||
|
koushik717 marked this conversation as resolved.
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,26 +1,18 @@ | ||
| import { useCallback, useEffect } from "react"; | ||
| import { useEffect } from "react"; | ||
| import { pushEscapeHandler } from "./useEscapeStack"; | ||
|
|
||
| /** | ||
| * Handle the escape key pressed. | ||
| * Registers the callback on the global escape-key stack so that nested | ||
| * overlays (modals, dropdowns, drawers, …) always dismiss in the correct | ||
| * LIFO order, regardless of DOM position or portal placement. | ||
| */ | ||
| export const useOnEscapePressed = ( | ||
| onEscape: () => void, | ||
| { isEnabled } = { isEnabled: true }, | ||
| ) => { | ||
| const keyDown = useCallback( | ||
| (evt: KeyboardEvent) => { | ||
| if (evt.code === "Escape") { | ||
| onEscape(); | ||
| } | ||
| }, | ||
| [onEscape], | ||
| ); | ||
| useEffect(() => { | ||
| if (isEnabled) { | ||
| document.addEventListener("keydown", keyDown); | ||
| } | ||
| return () => { | ||
| document.removeEventListener("keydown", keyDown); | ||
| }; | ||
| }, [keyDown, isEnabled]); | ||
| if (!isEnabled) return undefined; | ||
| return pushEscapeHandler(onEscape); | ||
| }, [onEscape, isEnabled]); | ||
| }; | ||
|
koushik717 marked this conversation as resolved.
Outdated
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.