fix(Popover): popover content now closes on ESC key press #4557
+70
−35
Conversation
This file contains 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
|
Storybook staging is available at https://kiwicom-orbit-sarka-popover-esc-close.surge.sh |
|
Size Change: +111 B (+0.02%) Total Size: 459 kB
ℹ️ View Unchanged
|
sarkaaa
force-pushed
the
sarka/popover-esc-close
branch
from
7e5c738 to
fcb64e7
Compare
Deploying orbit with
|
| Latest commit: |
a386339
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8bf26751.orbit.pages.dev |
| Branch Preview URL: | https://sarka-popover-esc-close.orbit.pages.dev |
![callstackai[bot]](https://avatars.githubusercontent.com/in/1040204?s=60&v=4){kind=small}
Comment on lines
69
to
82
| } else { | ||
| setShown(false); | ||
| } |
There was a problem hiding this comment.
🐛 Possible Bug
The new else block in handleOut will close the popover when clicking inside it, which breaks the expected behavior. The popover should only close when clicking outside (!ref.current.contains(ev.target)) or when explicitly triggered. The current implementation will make the popover close when interacting with any content inside it, including buttons or form elements.
Suggested change
| } else { | |
| setShown(false); | |
| } | |
| } else if (typeof opened === "undefined" && onClose) { | |
| onClose(); | |
| } |
sarkaaa
force-pushed
the
sarka/popover-esc-close
branch
3 times, most recently
from
a565143 to
002b225
Compare
sarkaaa
changed the title
| @@ -66,6 +66,9 @@ const Popover = ({ | |||
| setRenderWithTimeout(false); | |||
| resolveCallback(false); | |||
| } else if (onClose) onClose(); | |||
| } else { | |||
| setShown(false); | |||
| resolveCallback(false); | |||
There was a problem hiding this comment.
I was checking Storybook and there are several issues now:
- On Popover open, both
onOpen&onCloseevents are fired (on mouse click, for keyboard only on second focus) - When I close the Popover with ESC,
onCloseevent is firing on every ESC click even if the Popover has been closed - When I used
openedprop and provided my ownonOpen&onClosefunctions, using keyboard I was able to close the Popover with ESC but I couldn't open it afterwards on the second focus. BothonOpen&onCloseevents been firing though. When I go back once more (shift + tab) to the first focus, I could open it normally. It worked with mouse click but then theonCloseevent was firing twice. Some issues are present on production already, but adding ESC functionality is adding more mess 😅
I am not sure if fixing also the current issues (double focus) is in the scope of this MR but maybe we should expand the scope if all the issues are connected 🤔
sarkaaa
force-pushed
the
sarka/popover-esc-close
branch
from
002b225 to
8c88931
Compare
Comment on lines
58
to
69
| const handleEsc = React.useCallback( | ||
| (ev: { key: string }) => { | ||
| if (ev.key === "Escape") { | ||
| setShown(false); | ||
| setRenderWithTimeout(false); | ||
| document.removeEventListener("keydown", handleEsc); | ||
| } | ||
| }, | ||
| [setShown, setRenderWithTimeout], | ||
| ); |
There was a problem hiding this comment.
🐛 Possible Bug
The event listener is only removed when the Escape key is pressed, but not when the popover is closed through other means (clicking outside or programmatically). This can lead to multiple event listeners being added and memory leaks.
Suggested change
| const handleEsc = React.useCallback( | |
| (ev: { key: string }) => { | |
| if (ev.key === "Escape") { | |
| setShown(false); | |
| setRenderWithTimeout(false); | |
| document.removeEventListener("keydown", handleEsc); | |
| } | |
| }, | |
| [setShown, setRenderWithTimeout], | |
| ); | |
| const handleEsc = React.useCallback( | |
| (ev: { key: string }) => { | |
| if (ev.key === "Escape") { | |
| setShown(false); | |
| setRenderWithTimeout(false); | |
| } | |
| }, | |
| [setShown, setRenderWithTimeout], | |
| ); |
sarkaaa
force-pushed
the
sarka/popover-esc-close
branch
from
8c88931 to
5349315
Compare
Comment on lines
58
to
69
| const handleEsc = React.useCallback( | ||
| (ev: { key: string }) => { | ||
| if (ev.key === "Escape") { | ||
| setShown(false); | ||
| resolveCallback(false); | ||
| setRenderWithTimeout(false); | ||
| document.removeEventListener("keydown", handleEsc); | ||
| } | ||
| }, | ||
| [setShown, setRenderWithTimeout], | ||
| ); |
There was a problem hiding this comment.
🐛 Possible Bug
The handleEsc callback is missing resolveCallback in its dependency array. This can lead to stale closures where the callback uses an outdated version of resolveCallback when the escape key is pressed.
Suggested change
| const handleEsc = React.useCallback( | |
| (ev: { key: string }) => { | |
| if (ev.key === "Escape") { | |
| setShown(false); | |
| resolveCallback(false); | |
| setRenderWithTimeout(false); | |
| document.removeEventListener("keydown", handleEsc); | |
| } | |
| }, | |
| [setShown, setRenderWithTimeout], | |
| ); | |
| const handleEsc = React.useCallback( | |
| (ev: { key: string }) => { | |
| if (ev.key === "Escape") { | |
| setShown(false); | |
| resolveCallback(false); | |
| setRenderWithTimeout(false); | |
| document.removeEventListener("keydown", handleEsc); | |
| } | |
| }, | |
| [setShown, setRenderWithTimeout, resolveCallback], | |
| ); |
sarkaaa
force-pushed
the
sarka/popover-esc-close
branch
from
5349315 to
3efb26a
Compare
Comment on lines
+58
to
+69
| const handleEsc = React.useCallback( | ||
| (ev: { key: string }) => { | ||
| if (ev.key === "Escape") { | ||
| setShown(false); | ||
| clearShownTimeout(); | ||
| resolveCallback(false); | ||
| setRenderWithTimeout(false); | ||
| document.removeEventListener("keydown", handleEsc); | ||
| } | ||
| }, | ||
| [setShown, clearShownTimeout, resolveCallback, setRenderWithTimeout], | ||
| ); |
There was a problem hiding this comment.
🐛 Possible Bug
The 'keydown' event listener is only removed when the Escape key is pressed, but not when the popover is closed through other means (like clicking outside or programmatically). This can lead to memory leaks and multiple event listeners being attached if the popover is opened/closed repeatedly.
Suggested change
| const handleEsc = React.useCallback( | |
| (ev: { key: string }) => { | |
| if (ev.key === "Escape") { | |
| setShown(false); | |
| clearShownTimeout(); | |
| resolveCallback(false); | |
| setRenderWithTimeout(false); | |
| document.removeEventListener("keydown", handleEsc); | |
| } | |
| }, | |
| [setShown, clearShownTimeout, resolveCallback, setRenderWithTimeout], | |
| ); | |
| const handleEsc = React.useCallback( | |
| (ev: { key: string }) => { | |
| if (ev.key === "Escape") { | |
| setShown(false); | |
| clearShownTimeout(); | |
| resolveCallback(false); | |
| setRenderWithTimeout(false); | |
| } | |
| }, | |
| [setShown, clearShownTimeout, resolveCallback, setRenderWithTimeout], | |
| ); |
sarkaaa
force-pushed
the
sarka/popover-esc-close
branch
from
3efb26a to
a37e4d7
Compare
sarkaaa
commented
Jan 22, 2025
| ev.stopPropagation(); | ||
| setIsOpenedPopover(true); | ||
| title="Card title" | ||
| actions={ |
There was a problem hiding this comment.
I moved component to an actions prop. After CardSection changes, the children part of CardSection component is not clickable, so we couldn't demonstrate opening modal on wrapper click.
sarkaaa
force-pushed
the
sarka/popover-esc-close
branch
from
a37e4d7 to
a386339
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
closes https://kiwicom.atlassian.net/browse/FEPLT-2185
This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.ts✨
Description by Callstackai
This PR adds functionality to close the Popover component when the ESC key is pressed, along with visual regression tests for this feature.
Diagrams of code changes
sequenceDiagram participant User participant Popover participant Document participant EventListener User->>Popover: Opens popover Popover->>Document: Adds ESC key event listener User->>Document: Presses ESC key Document->>EventListener: Triggers keydown event EventListener->>Popover: Handles ESC key press Popover->>Popover: setShown(false) Popover->>Popover: clearShownTimeout() Popover->>Popover: resolveCallback(false) Popover->>Popover: setRenderWithTimeout(false) Popover->>Document: Removes ESC key event listener Popover->>User: Closes popoverFiles Changed