-
Notifications
You must be signed in to change notification settings - Fork 204
fix: Fixes chart popover click outside when in iframe #4005
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
Conversation
17b9e1c to
db0ca4e
Compare
|
|
||
| const popoverRef = useMergeRefs(popoverObjectRef, ref); | ||
|
|
||
| const clickFrameId = useRef<number | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the same implementation in the internal popover component. The chart popover cannot use it because it imports the popover container instead.
| ) { | ||
| // Dismiss popover unless there was a click inside within the last animation frame. | ||
| // Ignore clicks inside the chart as those are handled separately. | ||
| if (clickFrameId.current === null && !nodeContains(container, event.target as Element)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The !nodeContains(container, event.target as Element) is a check that is chart-popover specific, and is taken from the prev. implementation - not from the internal popover. Its purpose is to prevent dismiss action in case the click is done within the chart's SVG so that the chart itself can define the appropriate popover behaviour in that case.
| const blurTarget = event.relatedTarget || event.target; | ||
| if ( | ||
| blurTarget === null || | ||
| !(blurTarget instanceof Element) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The blurTarget instanceof Element fails if the target belongs to a different document. As result, this if condition always resolves to true and thus causes the popover to be dismissed unexpectedly.
See:
Screen.Recording.2025-11-04.at.10.18.49.mov
Replacing the check with !isElement(blurTarget) fixes the problem.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4005 +/- ##
==========================================
+ Coverage 97.20% 97.25% +0.05%
==========================================
Files 858 859 +1
Lines 25362 25390 +28
Branches 9016 9046 +30
==========================================
+ Hits 24652 24693 +41
+ Misses 704 691 -13
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
db0ca4e to
b3c960f
Compare
bd1d394 to
71e792d
Compare
|
|
||
| // Pins popover on the first point. | ||
| await page.click('h2'); | ||
| await page.keys(['Tab', 'ArrowRight', 'Enter']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gethinwebster I changed popover activation from pointer to keyboard because the former approach did not work with React 16 test pages: the page.hoverElement() did not help with offsetting the click. I did test the feature manually on both React versions.
Description
Rel: [AWSUI-61421]
How has this been tested?
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.