fix: Dismiss chart tooltip on ESC key press#167
Conversation
Who-is-PS
requested review from
amanabiy
and removed request for
a team and
amanabiy
Who-is-PS
changed the title
| const handleKeyDown = (event: KeyboardEvent) => { | ||
| // Skip if keyboard navigation is handling this (event originated from application element) | ||
| const target = event.target as Element; | ||
| if (context.keyboardNavigationEnabled && target?.closest('[role="application"]')) { |
There was a problem hiding this comment.
Why do wen need to check for target?.closest('[role="application"]')?
There was a problem hiding this comment.
Prevents double-handling. The navigation handler in chart-extra-navigation.ts already handles Escape for keyboard nav, this check skips the global handler when navigation is active.
| ].includes(event.keyCode) | ||
| ) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
Does this break other event handlers? E.g. if I am currently in the keyboard navigation, and I have an event listener on the parent element, does it still get triggered?
There was a problem hiding this comment.
Yes, intentionally. Standard role="application" behavior, captures all keys to prevent parent handlers (page scrolling, modal closing, etc.) from interfering with chart navigation.
There was a problem hiding this comment.
What if this role is not set? Or can't that happen?
There was a problem hiding this comment.
This should not happen in practice because, the role="application" is hardcoded in the ChartApplication component (it's not configurable). The application element is only rendered when keyboardNavigation={true}.
But if hypothetically it wasn't set: the global Escape handler would run during keyboard navigation, causing tooltip to dismiss when user meant to exit navigation.
2 participants
Description
Users can now press ESC to dismiss hover tooltips. Keyboard navigation remains unaffected - ESC moves focus up the hierarchy when navigating with keyboard.
Related links, issue #, if available:
AWSUI-61678How has this been tested?
Did run through my pipeline and checked it on the website
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.