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

jeradg · 2022-12-13T00:04:47Z

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?

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:

tooltip-bugfix.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
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide

github-actions · 2022-12-13T20:07:36Z

size-limit report 📦

Path	Size
polaris-react-cjs	211.43 KB (+0.06% 🔺)
polaris-react-esm	136.61 KB (+0.09% 🔺)
polaris-react-esnext	192.39 KB (+0.07% 🔺)
polaris-react-css	42.07 KB (0%)

jeradg · 2022-12-16T20:36:10Z

/snapit

github-actions · 2022-12-16T20:40:12Z

🫰✨ Thanks @jeradg! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221216203819

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221216203819

yarn add @shopify/polaris@0.0.0-snapshot-release-20221216203819

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20221216203819

henryyi · 2023-01-05T16:01:39Z

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

      }),
      () => {
-        if (this.overlay == null || this.scrollableContainer == null) {
+        if (this.overlay == null || this.firstScrollableContainer == null) {


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?

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

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

henryyi

Top hat and changes look good!

kushp

Code + 🎩 looks good to me, I like the little bounce on the tooltip when main page scrolls ahah oddly nice. Good work!!

@aveline

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@10.18.0 ### Minor Changes - [#7992](#7992) [`e8f74f4cd`](e8f74f4) Thanks [@aveline](https://github.com/aveline)! - Added support for `outline` to `Box` ### Patch Changes - [#7988](#7988) [`382784f4e`](382784f) Thanks [@kyledurand](https://github.com/kyledurand)! - Reduced spacing on ChoiceList children - [#7899](#7899) [`930f077eb`](930f077) Thanks [@jeradg](https://github.com/jeradg)! - Fixed a bug where Tooltips nested in Scrollable containers sometimes don't update their positions correctly - [#7831](#7831) [`47487ee0c`](47487ee) Thanks [@acmertz](https://github.com/acmertz)! - Updated the focus helper functions to no longer treat buttons with `aria-disabled="true"` and `tabindex="-1" (but no`disabled\` attribute) as focusable. ## @shopify/plugin-polaris@0.0.25 ## polaris.shopify.com@0.28.2 ### Patch Changes - Updated dependencies \[[`382784f4e`](382784f), [`930f077eb`](930f077), [`47487ee0c`](47487ee), [`e8f74f4cd`](e8f74f4)]: - @shopify/polaris@10.18.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

### 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?  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

@aveline

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@10.18.0 ### Minor Changes - [Shopify#7992](Shopify#7992) [`e8f74f4cd`](Shopify@e8f74f4) Thanks [@aveline](https://github.com/aveline)! - Added support for `outline` to `Box` ### Patch Changes - [Shopify#7988](Shopify#7988) [`382784f4e`](Shopify@382784f) Thanks [@kyledurand](https://github.com/kyledurand)! - Reduced spacing on ChoiceList children - [Shopify#7899](Shopify#7899) [`930f077eb`](Shopify@930f077) Thanks [@jeradg](https://github.com/jeradg)! - Fixed a bug where Tooltips nested in Scrollable containers sometimes don't update their positions correctly - [Shopify#7831](Shopify#7831) [`47487ee0c`](Shopify@47487ee) Thanks [@acmertz](https://github.com/acmertz)! - Updated the focus helper functions to no longer treat buttons with `aria-disabled="true"` and `tabindex="-1" (but no`disabled\` attribute) as focusable. ## @shopify/plugin-polaris@0.0.25 ## polaris.shopify.com@0.28.2 ### Patch Changes - Updated dependencies \[[`382784f4e`](Shopify@382784f), [`930f077eb`](Shopify@930f077), [`47487ee0c`](Shopify@47487ee), [`e8f74f4cd`](Shopify@e8f74f4)]: - @shopify/polaris@10.18.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

jeradg changed the title ~~Fix tooltip scroll bug~~ [PositionedOverlay][Tooltip] Fix tooltip scroll bug Dec 13, 2022

jeradg force-pushed the fix-tooltip-scroll-bug branch from c49543f to deb41ab Compare December 13, 2022 20:03

jeradg force-pushed the fix-tooltip-scroll-bug branch 2 times, most recently from b347a57 to 43c616c Compare December 16, 2022 20:29

Fix tooltip scrolling bug

52c7534

jeradg force-pushed the fix-tooltip-scroll-bug branch from 43c616c to 52c7534 Compare December 16, 2022 20:40

jeradg marked this pull request as ready for review January 4, 2023 21:52

jeradg self-assigned this Jan 4, 2023

henryyi reviewed Jan 5, 2023

View reviewed changes

jeradg requested a review from henryyi January 5, 2023 17:14

henryyi approved these changes Jan 5, 2023

View reviewed changes

kushp approved these changes Jan 5, 2023

View reviewed changes

jeradg merged commit 930f077 into main Jan 5, 2023

jeradg deleted the fix-tooltip-scroll-bug branch January 5, 2023 22:16

shopify-github-actions-access bot mentioned this pull request Jan 5, 2023

Version Packages #7995

Merged

gwyneplaine mentioned this pull request Feb 14, 2023

8248/fix toc layout #8338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

jeradg commented Dec 13, 2022 •

edited

Loading

Uh oh!

github-actions bot commented Dec 13, 2022 •

edited

Loading

Uh oh!

jeradg commented Dec 16, 2022

Uh oh!

github-actions bot commented Dec 16, 2022

Uh oh!

henryyi Jan 5, 2023 •

edited

Loading

Uh oh!

jeradg Jan 5, 2023 •

edited

Loading

Uh oh!

henryyi Jan 5, 2023

Uh oh!

henryyi left a comment

Uh oh!

kushp left a comment

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

jeradg commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

jeradg commented Dec 16, 2022

Uh oh!

github-actions bot commented Dec 16, 2022

Uh oh!

henryyi Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeradg Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henryyi Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

henryyi left a comment

Choose a reason for hiding this comment

Uh oh!

kushp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeradg commented Dec 13, 2022 •

edited

Loading

github-actions bot commented Dec 13, 2022 •

edited

Loading

henryyi Jan 5, 2023 •

edited

Loading

jeradg Jan 5, 2023 •

edited

Loading