Skip to content

Conversation

@pan-kot
Copy link
Member

@pan-kot pan-kot commented Oct 22, 2025

Description

The popover includes code that should recompute its position when there is a layout shift on the page, e.g. when a split-panel opens, causing the popover target to change. The keepPosition property is only used for charts popover, and for some reason the popovers with keepPosition=true were not updated. However, this causes the charts popover position to be off if the layout shift occurs.

See how legacy charts reacted to a split panel open before the change:

Screen.Recording.2025-10-22.at.14.06.33.mov

With the change (removing keepPosition check), the popover position is correctly adjusted.

Screen.Recording.2025-10-22.at.14.07.30.mov

The fix only works well when the layout change does not cause the chart to change size. If that happens, then the popover (even if pinned) is closed in area- and pie charts. In mixed charts (incl. bar- and line charts, too), the popover is not closed, but the track marker is not updated, causing both the marker and the popover to have incorrect position. That is an existing bug, reproducible with both old and new code, and it is not addressed by this change.

Screen.Recording.2025-10-22.at.14.11.29.mov

The fix also works in the new cartesian and pie charts. There, the positions of the tooltip and the marker are correctly recomputed.

Screen.Recording.2025-10-22.at.14.12.57.mov

Rel: AWSUI-61377

How has this been tested?

  • Manual tests using the new test pages for legacy and new charts
  • Removed the now irrelevant integ test
Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

// Recalculate position when the DOM changes.
// istanbul ignore next - tested via integration tests
usePositionObserver(trackRef, trackKey, () => {
usePositionObserver(getTrack.current, trackKey, () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

getTrack.current represents one of the two supported ways to provide the track element, the second one is used by the charts components.

const popoverOffset = popoverRef.current && getLogicalBoundingClientRect(popoverRef.current);

if (
keepPosition ||
Copy link
Member Author

Choose a reason for hiding this comment

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

We only use keepPosition=true in charts. The need of preventing the position update in this case is not clear though: when a layout shift occurs, the chart popover can be misplaced, too.

@codecov
Copy link

codecov bot commented Oct 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.17%. Comparing base (71f259e) to head (da6b352).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3958   +/-   ##
=======================================
  Coverage   97.17%   97.17%           
=======================================
  Files         854      854           
  Lines       24983    24983           
  Branches     8803     8802    -1     
=======================================
  Hits        24276    24276           
- Misses        659      701   +42     
+ Partials       48        6   -42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pan-kot pan-kot force-pushed the fix-popover-pos-observer branch from f90c0a8 to da6b352 Compare October 22, 2025 10:16
@pan-kot pan-kot marked this pull request as ready for review October 22, 2025 12:14
@pan-kot pan-kot requested a review from a team as a code owner October 22, 2025 12:14
@pan-kot pan-kot requested review from ClaudioGSDB and removed request for a team October 22, 2025 12:14
@pan-kot pan-kot changed the title fix: (WIP) Fixes popover position recomputation for charts fix: Fixes popover position recomputation for charts Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant