Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

[PositionedOverlay][Tooltip] Fix tooltip scroll bug #7899

Merged
merged 1 commit into from
Jan 5, 2023
Merged

[PositionedOverlay][Tooltip] Fix tooltip scroll bug #7899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 .changeset/fair-queens-sniff.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': patch
---

Fixed a bug where Tooltips nested in Scrollable containers sometimes don't update their positions correctly
62 changes: 47 additions & 15 deletions polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export class PositionedOverlay extends PureComponent<
};

private overlay: HTMLElement | null = null;
private scrollableContainer: HTMLElement | Document | null = null;
private scrollableContainers: (HTMLElement | Document)[] = [];
private observer: MutationObserver;

constructor(props: PositionedOverlayProps) {
Expand All @@ -93,23 +93,20 @@ export class PositionedOverlay extends PureComponent<
}

componentDidMount() {
this.scrollableContainer = Scrollable.forNode(this.props.activator);
if (this.scrollableContainer && !this.props.fixed) {
this.scrollableContainer.addEventListener(
'scroll',
this.handleMeasurement,
);
this.setScrollableContainers();

if (this.scrollableContainers.length && !this.props.fixed) {
this.registerScrollHandlers();
}

this.handleMeasurement();
}

componentWillUnmount() {
this.observer.disconnect();
if (this.scrollableContainer && !this.props.fixed) {
this.scrollableContainer.removeEventListener(
'scroll',
this.handleMeasurement,
);

if (this.scrollableContainers.length && !this.props.fixed) {
this.unregisterScrollHandlers();
}
}

Expand Down Expand Up @@ -160,6 +157,10 @@ export class PositionedOverlay extends PureComponent<
);
}

get firstScrollableContainer(): HTMLElement | Document | null {
return this.scrollableContainers[0] ?? null;
}

forceUpdatePosition() {
// Wait a single animation frame before re-measuring.
// Consumer's may also need to setup their own timers for
Expand All @@ -186,6 +187,37 @@ export class PositionedOverlay extends PureComponent<
this.overlay = node;
};

private setScrollableContainers = () => {
const containers: (HTMLElement | Document)[] = [];
let scrollableContainer = Scrollable.forNode(this.props.activator);

if (scrollableContainer) {
containers.push(scrollableContainer);

while (scrollableContainer?.parentElement) {
scrollableContainer = Scrollable.forNode(
scrollableContainer.parentElement,
);

containers.push(scrollableContainer);
}
}

this.scrollableContainers = containers;
};

private registerScrollHandlers = () => {
this.scrollableContainers.forEach((node) => {
node.addEventListener('scroll', this.handleMeasurement);
});
};

private unregisterScrollHandlers = () => {
this.scrollableContainers.forEach((node) => {
node.removeEventListener('scroll', this.handleMeasurement);
});
};

private handleMeasurement = () => {
const {lockPosition, top} = this.state;

Expand All @@ -201,7 +233,7 @@ export class PositionedOverlay extends PureComponent<
measuring: true,
}),
() => {
if (this.overlay == null || this.scrollableContainer == null) {
if (this.overlay == null || this.firstScrollableContainer == null) {
Copy link
Contributor

@henryyi henryyi Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We're registering scroll events for all scrollable containers but this measurement is only ever using the first one as a reference. Would that result in the correct positioning?

Copy link
Contributor Author

@jeradg jeradg Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, it results in the correct positioning.

In main (i.e., without the bugfix) the tooltip is always positioned correctly when first rendered, regardless of what container the trigger activator is in. The bug isn't with the calculation of the position, it's that we're not triggering this calculation for all relevant scroll events.

With this fix, we still want to use the first scrollable container to determine the position of the tooltip, because that's the container the trigger activator element is within (and the tooltip is visually positioned relative to its trigger activator).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup looks like it's working. The scroll container dimensions are only used for adjusting the tooltip position (above, below, etc).

return;
}

Expand All @@ -222,9 +254,9 @@ export class PositionedOverlay extends PureComponent<
const activatorRect = getRectForNode(preferredActivator);

const currentOverlayRect = getRectForNode(this.overlay);
const scrollableElement = isDocument(this.scrollableContainer)
const scrollableElement = isDocument(this.firstScrollableContainer)
? document.body
: this.scrollableContainer;
: this.firstScrollableContainer;
const scrollableContainerRect = getRectForNode(scrollableElement);

const overlayRect = fullWidth
Expand Down