chore: add optional parent boundary clamping for popover trigger positioning by jsilll · Pull Request #4344 · cloudscape-design/components

jsilll · 2026-03-11T13:37:41Z

Summary of Changes

Adds optional parent boundary clamping for popover trigger positioning. This allows popovers to respect parent container boundaries when positioning their arrows.

New Props:

Added parentRef?: React.RefObject<HTMLElement> to ChartPopover and PopoverContainer components

Implementation Details:

Introduced getClampedTrackRect() function in use-popover-position.ts that:
- Clamps the track center point to parent boundaries when parentRef is provided
- Calculates adjusted track position while preserving original dimensions
- Uses logical properties (insetInlineStart, blockSize, etc.) for RTL/writing-mode support
- Falls back to normal behavior when parentRef is absent (backward compatible)

Screen.Recording.2026-03-11.at.14.08.39.mov

…ger positioning

codecov · 2026-03-11T14:27:23Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.45%. Comparing base (41acf98) to head (620c8e5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4344   +/-   ##
=======================================
  Coverage   97.44%   97.45%           
=======================================
  Files         901      901           
  Lines       26419    26435   +16     
  Branches     9521     9524    +3     
=======================================
+ Hits        25745    25761   +16     
- Misses        631      668   +37     
+ Partials       43        6   -37

☔ 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.

jperals · 2026-03-11T15:34:44Z

I updated the PR title to fit the format accepted by our release notes script (just feat: instead of feat(popover):)

jperals · 2026-03-11T15:26:12Z

src/popover/use-popover-position.ts

+  const parentBlockEnd = parentRect.insetBlockStart + parentRect.blockSize;
+
+  // Find where the arrow should point (closest visible point to actual track center)
+  const trackCenterInline = trackRect.insetInlineStart + trackRect.inlineSize / 2;


Why do we need to take the center of the target into account? Wouldn't it be enough to return the rectangle resulting from intersecting the track and the parent?

I made it in such a way that the popover arrow always points to the closest visible point to the actual track center. I think we could do that yes but it would behave slightly differently.

I'll try to make a much simpler version of this

jperals · 2026-03-11T15:29:52Z

src/popover/use-popover-position.ts

+  return Math.max(min, Math.min(value, max));
+}
+
+function getClampedTrackRect(track: HTMLElement | SVGElement, parentRef?: HTMLElement | null) {


Can you add unit testing coverage for this new logic?

Yes, sure! I just wanted to get your feedback on the implementation first.

Thanks! Note, Codecov is still complaining about line 280 below not being covered. You might want to either add coverage for getClampedTrackRect specifically, or some case(s) for the popover component using a value for parent (if not too complicated, I'd personally prefer the latter, because it would test the integration of the logic in the component rather than in isolation).

I agree with the latter approach as well!

Thanks for adding the test, although Codecov still complains about line 280 not being covered. Could this be because by asserting

expect(usePopoverPositionSpy.mock.calls[0][0].parentRef?.current).toBe(parentRef);

you are only ever checking the first call, which corresponds to when the ref is null?

jperals · 2026-03-12T07:30:17Z

src/popover/utils/positions.ts

  return ['top', 'bottom'].includes(internalPosition.split('-')[0]);
 }

+export function clampRectStart<T extends BoundingBox>(rect: T, parentRect: BoundingBox): T {


Why the generic T? Can we be more specific?

Oh I missed the already defined Rect type

Thanks! Now the tests complain about the requiring insetBlockEnd and insetInlineEnd not being passed (see, TypeScript doing its job 😛)

jperals · 2026-03-12T07:30:38Z

src/popover/utils/positions.ts

+    ...rect,
+    insetInlineStart: Math.max(parentRect.insetInlineStart, Math.min(rect.insetInlineStart, parentInlineEnd)),
+    insetBlockStart: Math.max(parentRect.insetBlockStart, Math.min(rect.insetBlockStart, parentBlockEnd)),
+  } as T;


Can we get rid of the as T for better type safety?

jperals · 2026-03-12T07:34:53Z

src/popover/utils/positions.ts

+export function clampRectStart<T extends BoundingBox>(rect: T, parentRect: BoundingBox): T {
+  const parentInlineEnd = parentRect.insetInlineStart + parentRect.inlineSize;
+  const parentBlockEnd = parentRect.insetBlockStart + parentRect.blockSize;
+  return {


Although in the case we are trying to solve we only care about the "inline start" side, and the new API is internal, nothing in it indicates that the parent will be used to clamp only from that side. Unless it adds considerable complexity, would it be possible to clamp all sides?

We're already clamping both the inline and block directions. While working on this I found out that doing it for the block direction actually fixes a problem in the current version of the chart-components library.

Screen.Recording.2026-03-12.at.09.35.21.mov

In this new simplified implementation I'm not clamping inlineSize or blockSize which means the center of the track can still overflow by a very tiny amount outside of the container in some cases. I think this is small enough issue for us to ignore.

What I meant is that we are clamping on the "start" (of both inline and block directions) but not on the "end".

Thanks for the changes! Should the function be renamed accordingly, e.g., to just clampRect? :)

jperals · 2026-03-12T07:50:41Z

src/popover/use-popover-position.ts

+  return Math.max(min, Math.min(value, max));
+}
+
+function getClampedTrackRect(track: HTMLElement | SVGElement, parentRef?: HTMLElement | null) {


Thanks! Note, Codecov is still complaining about line 280 below not being covered. You might want to either add coverage for getClampedTrackRect specifically, or some case(s) for the popover component using a value for parent (if not too complicated, I'd personally prefer the latter, because it would test the integration of the logic in the component rather than in isolation).

…ampRectStart

…Start

jperals · 2026-03-12T11:16:27Z

src/popover/utils/positions.ts

+export function clampRectStart(rect: Rect, bounds: BoundingBox) {
+  const parentInlineEnd = bounds.insetInlineStart + bounds.inlineSize;
+  const parentBlockEnd = bounds.insetBlockStart + bounds.blockSize;
+  rect.insetInlineStart = Math.max(bounds.insetInlineStart, Math.min(rect.insetInlineStart, parentInlineEnd));


Any particular reason to modify the original passed-in object rather than creating a new one?

Looking again at the function I preferred to have it mutate the argument instead of creating a new one. I know this is mostly a matter of personal preference so if you prefer to create the new object I'm also fine with that.

In general it is preferred to not mutate objects, because from within the function you don't know what else is the object is used for, so its mutation might be unexpected to a user outside of it, leading to hard-to-debug issues.

jperals · 2026-03-16T08:59:14Z

src/popover/container.tsx

  trackKey?: string | number;
  minVisibleBlockSize?: number;
+  /** Optional parent element to clamp popover position within its bounds */
+  parentRef?: React.RefObject<HTMLElement>;


Not blocking, but this prop name does not convey very well what it is used for. What about something like triggerClipRef, triggerBoundaryRef or triggerClampRef?

Let's go with triggerClampRef :) Thanks for the suggestion!

jperals · 2026-03-16T11:05:06Z

src/popover/__tests__/positions.test.ts

+import { Rect } from '../../../lib/components/popover/interfaces';
 import {
  calculatePosition,
+  clampRectStart,


This needs to be updated to the new function name

Sorry for missing this

jperals · 2026-03-16T11:05:38Z

src/popover/__tests__/positions.test.ts

  });
 });
+
+describe('clampStart', () => {


Also update the function name in this test description

jperals · 2026-03-16T11:06:28Z

Actually I'd suggest prefixing the PR title with chore: instead of feat: because this is not a public feature

…pRect in tests

…tioning (#4344)

feat(popover): add optional parent boundary clamping for popover trig…

f99b7a0

…ger positioning

jsilll requested a review from a team as a code owner March 11, 2026 13:37

jsilll requested review from srungta08 and removed request for a team March 11, 2026 13:37

refactor(popover): simplify track rect clamping logic

3661f47

jperals changed the title ~~feat(popover): add optional parent boundary clamping for popover trigger positioning~~ feat: add optional parent boundary clamping for popover trigger positioning Mar 11, 2026

jperals reviewed Mar 11, 2026

View reviewed changes

jsilll added 2 commits March 11, 2026 16:12

fix(popover): simplify track rect clamping logic

868ecf7

refactor(popover): extract rect clamping logic and add tests

a6b5a9e

jperals reviewed Mar 12, 2026

View reviewed changes

jsilll added 3 commits March 12, 2026 09:51

refactor(popover): improve type safety and mutate rect directly in cl…

7899dc1

…ampRectStart

test(popover): add test for parentRef passed to usePopoverPosition

4a5ea43

refactor(popover): rename parentRect parameter to bounds in clampRect…

562cb5e

…Start

jperals reviewed Mar 12, 2026

View reviewed changes

jsilll added 3 commits March 13, 2026 13:56

refactor(popover): make clampRectStart function immutable

88dff5c

fix: improve clampRectStart to clamp both position and size

843e241

test: refactor clampRectStart tests with helper function

ad2c82d

jperals reviewed Mar 16, 2026

View reviewed changes

jsilll added 2 commits March 16, 2026 10:27

refactor(popover): make bounds parameter optional in clampRect function

f22680b

refactor(popover): rename parentRef to triggerClampRef

4f229bd

jperals reviewed Mar 16, 2026

View reviewed changes

jsilll changed the title ~~feat: add optional parent boundary clamping for popover trigger positioning~~ chore: add optional parent boundary clamping for popover trigger positioning Mar 16, 2026

jsilll and others added 2 commits March 16, 2026 11:16

chore: rename parentRef to triggerClampRef and clampRectStart to clam…

8cd39b8

…pRect in tests

Merge branch 'main' into main

620c8e5

jperals approved these changes Mar 16, 2026

View reviewed changes

jperals added this pull request to the merge queue Mar 16, 2026

Merged via the queue into cloudscape-design:main with commit a3d7548 Mar 16, 2026
82 of 84 checks passed

amanabiy pushed a commit that referenced this pull request Mar 20, 2026

chore: add optional parent boundary clamping for popover trigger posi…

176453e

…tioning (#4344)

Conversation

jsilll commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Uh oh!

codecov bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jperals commented Mar 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jperals commented Mar 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jsilll commented Mar 11, 2026 •

edited

Loading

codecov bot commented Mar 11, 2026 •

edited

Loading