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

Commit 930f077

Browse files
authored
[PositionedOverlay][Tooltip] Fix tooltip scroll bug (#7899)
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? Closes https://github.com/Shopify/core-workflows/issues/663 This PR fixes a bug with `Tooltip` and other consumers of `PositionedOverlay` where the position of the overlay does not correctly update when the overlay's closest `Scrollable` ancestor is **_not_** being scrolled, but some more distant ancestor node **_is_** being scrolled. In such cases, the overlay position moves with the more distant ancestor node rather than the correct `Scrollable` node. (The bug specifically occurs `Tooltip` instances rendered in a `Scrollable`, because the `PositionedOverlay` from the tooltip is rendered in a `PortalContainer`, which is a descendant of the `body` rather than the tooltip's `Scrollable`.) ### WHAT is this pull request doing? <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> This PR ensures that `scroll` event handlers are added to all scrollable ancestors of the component, rather than just the nearest scrollable ancestor. This fixes a bug where the tooltip "sticks" to the page body rather than the correct scrollable container. This video shows the correct behaviour: https://user-images.githubusercontent.com/1948799/210642458-ec0f7a77-9021-46c9-8283-8e7665a05ade.mov ### How to 🎩 Tophatting URL: https://admin.web.tooltip-polaris.jerad-gallinger.us.spin.dev/store/shop1 - Open the admin - Resize your browser window to be tall enough for the left nav to not scroll - In the left-hand nav, hover over `Online Store`. The tooltip should appear as usual. - While still hovering over the nav item, scroll the page (with up/down arrow keys, trackpad gestures, or scroll wheel). When the main page content scrolls, the tooltip should reposition so that it still appears above the nav item (rather than scrolling with the main page content). - Resize the height of the browser so that the left nav content overflows the height of the nav and becomes scrollable. - Scroll the left nav to the bottom of its contents. Hover over the `Online Store` item so the tooltip appears. - While still hovering over the nav item, scroll the page down (with up/down arrow keys, trackpad gestures, or scroll wheel). When the main page content scrolls, the tooltip should reposition so that it still appears above the nav item (rather than scrolling with the main page content). ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [X] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
1 parent 1233828 commit 930f077

File tree

2 files changed

+52
-15
lines changed

2 files changed

+52
-15
lines changed

.changeset/fair-queens-sniff.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/polaris': patch
3+
---
4+
5+
Fixed a bug where Tooltips nested in Scrollable containers sometimes don't update their positions correctly

polaris-react/src/components/PositionedOverlay/PositionedOverlay.tsx

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ export class PositionedOverlay extends PureComponent<
8383
};
8484

8585
private overlay: HTMLElement | null = null;
86-
private scrollableContainer: HTMLElement | Document | null = null;
86+
private scrollableContainers: (HTMLElement | Document)[] = [];
8787
private observer: MutationObserver;
8888

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

9595
componentDidMount() {
96-
this.scrollableContainer = Scrollable.forNode(this.props.activator);
97-
if (this.scrollableContainer && !this.props.fixed) {
98-
this.scrollableContainer.addEventListener(
99-
'scroll',
100-
this.handleMeasurement,
101-
);
96+
this.setScrollableContainers();
97+
98+
if (this.scrollableContainers.length && !this.props.fixed) {
99+
this.registerScrollHandlers();
102100
}
101+
103102
this.handleMeasurement();
104103
}
105104

106105
componentWillUnmount() {
107106
this.observer.disconnect();
108-
if (this.scrollableContainer && !this.props.fixed) {
109-
this.scrollableContainer.removeEventListener(
110-
'scroll',
111-
this.handleMeasurement,
112-
);
107+
108+
if (this.scrollableContainers.length && !this.props.fixed) {
109+
this.unregisterScrollHandlers();
113110
}
114111
}
115112

@@ -160,6 +157,10 @@ export class PositionedOverlay extends PureComponent<
160157
);
161158
}
162159

160+
get firstScrollableContainer(): HTMLElement | Document | null {
161+
return this.scrollableContainers[0] ?? null;
162+
}
163+
163164
forceUpdatePosition() {
164165
// Wait a single animation frame before re-measuring.
165166
// Consumer's may also need to setup their own timers for
@@ -186,6 +187,37 @@ export class PositionedOverlay extends PureComponent<
186187
this.overlay = node;
187188
};
188189

190+
private setScrollableContainers = () => {
191+
const containers: (HTMLElement | Document)[] = [];
192+
let scrollableContainer = Scrollable.forNode(this.props.activator);
193+
194+
if (scrollableContainer) {
195+
containers.push(scrollableContainer);
196+
197+
while (scrollableContainer?.parentElement) {
198+
scrollableContainer = Scrollable.forNode(
199+
scrollableContainer.parentElement,
200+
);
201+
202+
containers.push(scrollableContainer);
203+
}
204+
}
205+
206+
this.scrollableContainers = containers;
207+
};
208+
209+
private registerScrollHandlers = () => {
210+
this.scrollableContainers.forEach((node) => {
211+
node.addEventListener('scroll', this.handleMeasurement);
212+
});
213+
};
214+
215+
private unregisterScrollHandlers = () => {
216+
this.scrollableContainers.forEach((node) => {
217+
node.removeEventListener('scroll', this.handleMeasurement);
218+
});
219+
};
220+
189221
private handleMeasurement = () => {
190222
const {lockPosition, top} = this.state;
191223

@@ -201,7 +233,7 @@ export class PositionedOverlay extends PureComponent<
201233
measuring: true,
202234
}),
203235
() => {
204-
if (this.overlay == null || this.scrollableContainer == null) {
236+
if (this.overlay == null || this.firstScrollableContainer == null) {
205237
return;
206238
}
207239

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

224256
const currentOverlayRect = getRectForNode(this.overlay);
225-
const scrollableElement = isDocument(this.scrollableContainer)
257+
const scrollableElement = isDocument(this.firstScrollableContainer)
226258
? document.body
227-
: this.scrollableContainer;
259+
: this.firstScrollableContainer;
228260
const scrollableContainerRect = getRectForNode(scrollableElement);
229261

230262
const overlayRect = fullWidth

0 commit comments

Comments
 (0)