Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/internal/components/chart-popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ export interface ChartPopoverProps extends PopoverProps {
trackKey?: string | number;
minVisibleBlockSize?: number;

/** Optional element to clamp the popover trigger position within its bounds */
triggerClampRef?: React.RefObject<HTMLElement>;

/** Optional container element that prevents any clicks in there from dismissing the popover */
container: Element | null;

Expand Down Expand Up @@ -74,6 +77,7 @@ function ChartPopover(
trackKey,
onDismiss,
container,
triggerClampRef,
minVisibleBlockSize,

onMouseEnter,
Expand Down Expand Up @@ -137,6 +141,7 @@ function ChartPopover(
trackRef={trackRef}
getTrack={getTrack}
trackKey={trackKey}
triggerClampRef={triggerClampRef}
minVisibleBlockSize={minVisibleBlockSize}
arrow={position => (
<div className={clsx(popoverStyles.arrow, popoverStyles[`arrow-position-${position}`])}>
Expand Down
14 changes: 14 additions & 0 deletions src/popover/__tests__/popover-container.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,17 @@ test.each([null, document.createElement('div')])('accepts track element with get
const getTrack = usePopoverPositionSpy.mock.calls[0][0].getTrack;
expect(getTrack()).toBe(track);
});

test.each([null, document.createElement('div')])('passes parentRef=%s to usePopoverPosition', parentRef => {
render(
<PopoverContainer
{...defaultProps}
triggerClampRef={{ current: parentRef }}
trackRef={{ current: document.createElement('div') }}
>
content
</PopoverContainer>
);

expect(usePopoverPositionSpy.mock.calls[0][0].triggerClampRef?.current).toBe(parentRef);
});
55 changes: 55 additions & 0 deletions src/popover/__tests__/positions.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
import { Rect } from '../../../lib/components/popover/interfaces';
import {
calculatePosition,
clampRect,
intersectRectangles,
isCenterOutside,
PRIORITY_MAPPING,
Expand Down Expand Up @@ -280,3 +282,56 @@ describe('isCenterOutside', () => {
).toBe(false);
});
});

describe('clampRect', () => {
const parent = { insetInlineStart: 100, insetBlockStart: 100, inlineSize: 400, blockSize: 300 };
function rect(insetInlineStart: number, insetBlockStart: number, inlineSize: number, blockSize: number): Rect {
return {
insetInlineStart,
insetBlockStart,
inlineSize,
blockSize,
insetInlineEnd: insetInlineStart + inlineSize,
insetBlockEnd: insetBlockStart + blockSize,
};
}

test('returns rect unchanged when bounds is not provided', () => {
const testRect = rect(150, 200, 100, 80);
expect(clampRect(testRect)).toEqual(testRect);
});

test('returns rect unchanged when fully inside parent', () => {
expect(clampRect(rect(200, 200, 50, 50), parent)).toEqual(rect(200, 200, 50, 50));
});

test('clamps start to parent start when rect is before parent', () => {
const result = clampRect(rect(50, 30, 20, 20), parent);
expect(result.insetInlineStart).toBe(100);
expect(result.insetBlockStart).toBe(100);
});

test('clamps start to parent end when rect is past parent', () => {
const result = clampRect(rect(600, 500, 20, 20), parent);
expect(result.insetInlineStart).toBe(500);
expect(result.insetBlockStart).toBe(400);
});

test('clamps size when rect would overflow parent end', () => {
const result = clampRect(rect(400, 350, 200, 100), parent);
expect(result.inlineSize).toBe(100);
expect(result.blockSize).toBe(50);
});

test('reduces size to zero when start is clamped to parent end', () => {
const result = clampRect(rect(600, 500, 50, 50), parent);
expect(result.inlineSize).toBe(0);
expect(result.blockSize).toBe(0);
});

test('computes insetInlineEnd and insetBlockEnd correctly', () => {
const result = clampRect(rect(150, 200, 100, 80), parent);
expect(result.insetInlineEnd).toBe(250);
expect(result.insetBlockEnd).toBe(280);
});
});
4 changes: 4 additions & 0 deletions src/popover/container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ interface PopoverContainerProps {
*/
trackKey?: string | number;
minVisibleBlockSize?: number;
/** Optional parent element to clamp popover position within its bounds */
triggerClampRef?: React.RefObject<HTMLElement>;
position: PopoverProps.Position;
zIndex?: React.CSSProperties['zIndex'];
arrow: (position: InternalPosition | null) => React.ReactNode;
Expand All @@ -52,6 +54,7 @@ export default function PopoverContainer({
trackRef,
getTrack: externalGetTrack,
trackKey,
triggerClampRef,
minVisibleBlockSize,
arrow,
children,
Expand Down Expand Up @@ -90,6 +93,7 @@ export default function PopoverContainer({
popoverRef,
bodyRef,
arrowRef,
triggerClampRef,
getTrack: getTrack.current,
contentRef,
allowScrollToFit,
Expand Down
17 changes: 13 additions & 4 deletions src/popover/use-popover-position.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ import {
scrollRectangleIntoView,
} from '../internal/utils/scrollable-containers';
import { BoundingBox, InternalPosition, Offset, PopoverProps, Rect } from './interfaces';
import { calculatePosition, getDimensions, getOffsetDimensions, isCenterOutside } from './utils/positions';
import { calculatePosition, clampRect, getDimensions, getOffsetDimensions, isCenterOutside } from './utils/positions';

export default function usePopoverPosition({
popoverRef,
bodyRef,
arrowRef,
getTrack,
triggerClampRef,
contentRef,
allowScrollToFit,
allowVerticalOverflow,
Expand All @@ -34,6 +35,7 @@ export default function usePopoverPosition({
arrowRef: React.RefObject<HTMLDivElement | null>;
getTrack: () => null | HTMLElement | SVGElement;
contentRef: React.RefObject<HTMLDivElement | null>;
triggerClampRef?: React.RefObject<HTMLElement>;
allowScrollToFit?: boolean;
allowVerticalOverflow?: boolean;
preferredPosition: PopoverProps.Position;
Expand Down Expand Up @@ -88,7 +90,7 @@ export default function usePopoverPosition({
// Get rects representing key elements
// Use getComputedStyle for arrowRect to avoid modifications made by transform
const viewportRect = getViewportRect(document.defaultView!);
const trackRect = getLogicalBoundingClientRect(track);
const trackRect = getClampedTrackRect(track, triggerClampRef?.current);
const arrowRect = getDimensions(arrow);
const { containingBlock, boundary } = findUpUntilMultiple({
startElement: popover,
Expand Down Expand Up @@ -183,7 +185,7 @@ export default function usePopoverPosition({
if (!track) {
return;
}
const trackRect = getLogicalBoundingClientRect(track);
const trackRect = getClampedTrackRect(track, triggerClampRef?.current);

const newTrackOffset = toRelativePosition(
trackRect,
Expand All @@ -209,13 +211,14 @@ export default function usePopoverPosition({
bodyRef,
contentRef,
arrowRef,
triggerClampRef,
keepPosition,
preferredPosition,
renderWithPortal,
allowVerticalOverflow,
minVisibleBlockSize,
allowScrollToFit,
hideOnOverscroll,
minVisibleBlockSize,
]
);
return { updatePositionHandler, popoverStyle, internalPosition, positionHandlerRef, isOverscrolling };
Expand Down Expand Up @@ -262,3 +265,9 @@ function isBoundary(element: HTMLElement) {
const computedStyle = getComputedStyle(element);
return !!computedStyle.clipPath && computedStyle.clipPath !== 'none';
}

function getClampedTrackRect(track: HTMLElement | SVGElement, parentRef?: HTMLElement | null) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add unit testing coverage for this new logic?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with the latter approach as well!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

const rect = getLogicalBoundingClientRect(track);
const bounds = parentRef ? getLogicalBoundingClientRect(parentRef) : undefined;
return clampRect(rect, bounds);
}
25 changes: 25 additions & 0 deletions src/popover/utils/positions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,31 @@ function isTopOrBottom(internalPosition: InternalPosition) {
return ['top', 'bottom'].includes(internalPosition.split('-')[0]);
}

export function clampRect(rect: Rect, bounds?: BoundingBox) {
if (!bounds) {
return rect;
}
const parentInlineEnd = bounds.insetInlineStart + bounds.inlineSize;
const parentBlockEnd = bounds.insetBlockStart + bounds.blockSize;

const clampedInsetInlineStart = Math.max(bounds.insetInlineStart, Math.min(rect.insetInlineStart, parentInlineEnd));
const clampedInsetBlockStart = Math.max(bounds.insetBlockStart, Math.min(rect.insetBlockStart, parentBlockEnd));

const maxInlineSize = parentInlineEnd - clampedInsetInlineStart;
const maxBlockSize = parentBlockEnd - clampedInsetBlockStart;
const clampedInlineSize = Math.min(rect.inlineSize, maxInlineSize);
const clampedBlockSize = Math.min(rect.blockSize, maxBlockSize);

return {
insetInlineStart: clampedInsetInlineStart,
insetBlockStart: clampedInsetBlockStart,
inlineSize: clampedInlineSize,
blockSize: clampedBlockSize,
insetInlineEnd: clampedInsetInlineStart + clampedInlineSize,
insetBlockEnd: clampedInsetBlockStart + clampedBlockSize,
};
}

export function isCenterOutside(child: Rect, parent: Rect) {
const childCenter = child.insetBlockStart + child.blockSize / 2;
const overflowsBlockStart = childCenter < parent.insetBlockStart;
Expand Down
Loading