feat(tooltip): Enable custom portal container for tooltip #1567

fbouquet · 2022-09-14T16:08:30Z

🚀 Enhancements

Enables setting a custom container for the tooltip portal when using useTooltipInPortal, via the portalContainer property.

Currently, when using useTooltipInPortal, the tooltip renders at the root of the document. My team is running into an issue where we are sometimes rendering a dropdown in a static tooltip, but the dropdown menu appears behind the tooltip. Setting hacky z indexes would not robustly fix the issue if there is another overlay layer that is supposed to be under the tooltip, and we figured that if we could tell the tooltip portal where to render, we could fix the bug in a clean way.

I've updated the tooltip example to show that use case:

Screen.Recording.2022-09-14.at.12.07.07.PM.mov

Screen.Recording.2022-09-14.at.12.07.34.PM.mov

williaster

Thanks for the contribution and adding tests/updating the docs @fbouquet! 🙏

I had a couple of comments but overall looks great.

williaster · 2022-09-15T22:00:06Z

packages/visx-demo/src/sandboxes/visx-tooltip/Example.tsx

+  placeAfterTooltipInDom?: boolean;
+};
+
+const OverlayLayer = function OverlayLayer({


nit – this could be simplified slightly

Suggested change

const OverlayLayer = function OverlayLayer({

function OverlayLayer({

williaster · 2022-09-16T17:40:09Z

packages/visx-tooltip/src/Portal.tsx

      document.body.removeChild(this.node);
      delete this.node;
    }
  }

  render() {
+    if (!this.node && this.props.container) {
+      this.node = this.props.container;
+      if (this.props.zIndex != null) this.node.style.zIndex = `${this.props.zIndex}`;


if someone is providing their own container, it seems a bit strange for us to handle setting styles on it. I guess this is fine tho and consumers can always set it to null.

That's a good point, I had set this up for the zIndex to still do something in this case too, but actually I don't see a point in doing it that way, because consumers can just directly set the z-index of the portal container in their styles directly. 🤷‍♂️ I'll go ahead and remove this line.

williaster · 2022-09-16T17:44:02Z

packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx

          </Portal>
        );
      },
-    [detectBoundsOption, zIndexOption, containerBounds.left, containerBounds.top],
+    [containerBounds, detectBoundsOption, portalContainerOption, zIndexOption],


I think it's ideal to have this reference containerBounds.left/top explicitly since the object containerBounds might not be referentially equal across renders.

Totally fair, I assumed that would never be the case based on the type of containerBounds being RectReadOnly where left and top are readonly properties, but better safe than sorry. 👍

Ha, actually, eslint is yelling at me saying that containerBounds.left and containerBounds.top are unnecessary dependencies when I add them. 🙈 Do you think there's actually any reasonable change for the readonly modifier on left and top to be violated on this object?

williaster · 2022-09-16T17:48:31Z

packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx

        zIndex: zIndexProp, // allow override at the component-level
        ...tooltipProps
      }: TooltipInPortalProps) => {
        const detectBounds = detectBoundsProp == null ? detectBoundsOption : detectBoundsProp;
+        const portalContainer =
+          portalContainerProp == null ? portalContainerOption : portalContainerProp;
+        const portalContainerRect = portalContainer?.getBoundingClientRect();


this can be quite expensive to call each render which is why we opted to use useMeasure for the container.

Is it necessary to support the portalContainerProp override or can it be passed in as an option? If the latter is fine we could use useMeasure for the portalContainer as well. that's tricky though because it may not always update when an element resize happens etc. (which is what forceRefreshBounds is for)

if this simplifies things we could leave it for now.

Oh that's a good catch. 🤔 I don't think we necessarily need to support the override at the component level since typically a portal container doesn't move in the screen. I'll drop this prop at the tooltip level and just keep it on useTooltipInPortal.

However, I'll keep passing in the HTMLDivElement directly to useTooltipInPortal and memoize the bounding rect instead of using useMeasure for this if that's OK, because having to deal with the old fashioned ref useMeasure returns makes things harder on the consumer side in my opinion.

williaster · 2022-09-16T17:53:31Z

packages/visx-tooltip/test/Portal.test.tsx

+
+  it('should render children at the document root', async () => {
+    render(
+      <>


would this make more sense as a test setup since you wouldn't expect to find element-in-portal inside inner-div if it's a sibling?

Suggested change

<>

<div data-testid="inner-div">

<Portal>

<div data-testid="element-in-portal" />

</Portal>

</div>

That's a great point!

williaster · 2022-09-16T17:56:28Z

packages/visx-tooltip/test/useTooltipInPortal.test.tsx

+      const elementInPortal = await screen.findByTestId('element-in-tooltip');
+      expect(elementInPortal).toBeInTheDocument();
+      expect(
+        within(screen.getByTestId('inner-div')).queryByTestId('element-in-tooltip'),


I guess same question on this test since element-in-tooltip is a sibling / wouldn't be expected to be a child of inner-div

williaster · 2022-09-16T17:56:46Z

packages/visx-tooltip/test/Portal.test.tsx

 describe('Portal', () => {
  test('it should be defined', () => {
    expect(Portal).toBeDefined();
  });
+
+  it('should render children at the document root', async () => {


thanks for adding such great tests! 🙌

williaster · 2022-09-16T18:05:08Z

happo diff looks good (sorry this isn't visible to contributors)

fbouquet · 2022-09-16T22:08:01Z

Thanks a lot for the quick code review @williaster !

I went ahead and updated the code based on your comments. I'll be away from keyboard for the next two weeks but if there's anything else that need to be changed I'll be happy to look into it when I'm back.

fbouquet · 2022-10-04T19:49:02Z

Hi @williaster ! Just wanted to double check if there's anything else needed from me on this PR or if it's all good to go?

williaster

Hey @fbouquet thanks again for the addition and your patience getting this in. I have one big concern with this as it stands.

williaster · 2022-10-05T21:00:05Z

packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx

  zIndex: zIndexOption,
  ...useMeasureOptions
 }: UseTooltipPortalOptions | undefined = {}): UseTooltipInPortal {
  const [containerRef, containerBounds, forceRefreshBounds] = useMeasure(useMeasureOptions);

+  const portalContainerRect = useMemo(


hmm, I don't think this will necessarily update the way we want, it's just computed once and if the container changes sizes or moves this will be stale/wrong.

this is the advantage of useMeasure, I'm a bit apprehensive of adding this if it's potentially going to result in issues of it being stale / incorrectly positioned.

This was a really good point indeed! I was to simulate containers moving / changing sizes in the demo page and to confirm that my prior implementation didn't handle that gracefully.

Unfortunately useMeasure wouldn't work here because of the legacy type of ref it generates, which doesn't let us get the actual HTML element from the ref when we need it in Portal. That is a known issue of this library and it turns out that the library is not maintained anymore. In the issue, there are recommendations to use @react-hookz/web instead, but I couldn't find any way to target a specific hook in the library and importing the whole library just to use one hook didn't feel like a good idea.

Instead, I've opted for using @react-hook/resize-observer, and I was able to verify that my solution works as expected when containers move and change sizes.

This was before the fix, with randomly changing container positions and sizes:

Screen.Recording.2022-11-10.at.2.25.27.PM.mov

And this is after the fix:

Screen.Recording.2022-11-10.at.2.26.52.PM.Shorter.mov

williaster · 2022-10-05T21:00:50Z

packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx

-        const portalLeft = containerLeft + (containerBounds.left || 0) + window.scrollX;
-        const portalTop = containerTop + (containerBounds.top || 0) + window.scrollY;
+        const portalLeft = portalContainer
+          ? tooltipLeft - (portalContainerRect?.left || 0) + (containerBounds.left || 0)


is window.scrollX/Y not relevant for these?

They are not indeed. I don't know enough about the internals of React portals to explain exactly why, but in the case when a custom portal container is used, including window.scrollX/Y in the calculation of the tooltip position leads to bugs like this:

Screen.Recording.2022-11-10.at.2.28.47.PM.mov

ah maybe when using a custom container, the window x/y is already accounted for. thanks for the video!

fbouquet · 2022-10-12T18:36:53Z

Hey @fbouquet thanks again for the addition and your patience getting this in. I have one big concern with this as it stands.

Hi @williaster ! Thank you, those are good points, I'll be looking into this within the next 2 weeks and will get back to you. 🙂

fbouquet · 2022-11-10T20:09:09Z

packages/visx-tooltip/src/tooltips/TooltipWithBounds.tsx

+    /**
+     * When the tooltip is in a portal, this is the position of the portal
+     * container to be used for offsetting calculations around bounds as a consequence.
+     */
+    portalContainerPosition?: { left: number; top: number };
+    /**
+     * When the tooltip is in a portal, the portal container becomes the direct parent of the tooltip.
+     * Often the portal is not what we want the tooltip to be bound to. Another visual parent can be specified
+     * by specifying its dimensions here.
+     */
+    visualParentRect?: { width: number; height: number; left: number; top: number };


The TooltipWithBounds case did not work at all for tooltips in a portal, because it wasn't considering the right visual parent for the tooltip.

I did not change the behavior for the case with no custom portal container for backward compatibility concerns, but I did get it to work as expected in the case when a custom portal container is provided and when the bounds options are on. I think this will help my team fix a bug where the bottom of our tooltip gets cropped sometimes.

Fixing it was not super intuitive at first, I hope my comments and the variable names make sufficient sense to understand what's going on here.

fbouquet · 2022-11-10T20:13:55Z

@williaster Apologies for the additional delay on this, the last few weeks have been busier than I had expected. I went ahead and updated my code to correctly handle container changes in position and size, and I also fixed the TooltipWithBounds case when a custom portal is specified.

Unfortunately that behavior is hard to cover in automated tests but I was able to manually ensure that things are working well with my changes, as you can see in the screen recording above. 🙂

fbouquet · 2022-11-29T16:21:51Z

Hi @williaster ! 👋 Just wanted to send you a reminder about this PR; anything I should change before it's ready to go?

williaster

Thanks for another great iteratio here @fbouquet 🙏

I had a couple remaining comments, some to simplify the API and some to simplify the example. Maybe we can try to land this before EOY! I've been busy so haven't had a lot of time but will try to keep an eye on it.

williaster · 2022-12-15T21:24:16Z

packages/visx-demo/src/sandboxes/visx-tooltip/Example.tsx

@@ -28,11 +29,17 @@ const tooltipStyles = {

 export default function Example({ width, height, showControls = true }: TooltipProps) {
  const [tooltipShouldDetectBounds, setTooltipShouldDetectBounds] = useState(true);
+  const [tooltipPortalShouldUseCustomContainer, setTooltipPortalShouldUseCustomContainer] =


simplifying a bit

Suggested change

const [tooltipPortalShouldUseCustomContainer, setTooltipPortalShouldUseCustomContainer] =

const [tooltipShouldUseCustomContainer, setTooltipShouldUseCustomContainer] =

williaster · 2022-12-15T21:31:05Z

packages/visx-demo/src/sandboxes/visx-tooltip/Example.tsx

+  placeAfterTooltipInDom?: boolean;
+};
+
+function OverlayLayer({ className, container, placeAfterTooltipInDom, text }: OverlayLayerProps) {


maybe I'm not fully following the nuance, but I find that these make the example much more complex to understand / they don't directly relate to the tooltip API that we want to demonstrate. wdyt about removing them?

I see your point about this making the example code more complex, but on the other hand I feel like having these elements in the example helps understand what the portalContainer prop in useTooltipInPortal can achieve and I'm worried that the point may be missed if we removed this from the example. 🤔 Plus, I think it's a nice way to visually make sure that nothing is broken when making changes to the tooltip module. I won't fight too hard if you feel strongly about trimming the example though. 🤷‍♂️

williaster · 2022-12-15T21:34:12Z

packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx

-        const portalLeft = containerLeft + (containerBounds.left || 0) + window.scrollX;
-        const portalTop = containerTop + (containerBounds.top || 0) + window.scrollY;
+        const portalLeft = portalContainer
+          ? tooltipLeft - (portalContainerRect?.left || 0) + (containerBounds.left || 0)


ah maybe when using a custom container, the window x/y is already accounted for. thanks for the video!

williaster · 2022-12-15T21:37:15Z

packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx

+                  left: portalContainerRect?.left || 0,
+                  top: portalContainerRect?.top || 0,
+                },
+                visualParentRect: {


do you think this should always be passed as props even without a portal?

I'd say yes ideally, for consistency and because it would fix the bound detection for the case of tooltips in the default portal.

That being said, since we're dealing with a library here and the bound detection has been broken since the beginning when rendering the tooltip in the default portal, I'm worried that people may have worked under the assumption that it would stay that way, and that fixing this bound detection retroactively may break the tooltip position in their respective apps. That's the reason why I only applied this prop to the case of a custom portal container.

But if you feel confident that we could do this without inadvertently breaking other users' tooltips, I'd be down for doing it, just let me know!

williaster · 2022-12-15T21:39:47Z

packages/visx-tooltip/src/tooltips/TooltipWithBounds.tsx

@@ -20,19 +33,26 @@ function TooltipWithBounds({
  top: initialTop = 0,
  unstyled = false,
  nodeRef,
+  portalContainerPosition,
+  visualParentRect: visualParentBounds = parentBounds,


rather than introducing a new prop (this component is getting super complex), could TooltipInPortal simply overwrite parentRect and it's always assumed to be the visual parent?

I did try to overwrite the parentRect prop when I worked on this, but whatever I passed from here was being overwritten by the withBoundingRects decorator, which is what sets the parentRect prop. The only way I see around this would be to refactor the TooltipWithBounds/withBoundingRects couple, but that feels risky and beyond the scope of this PR. 😕

fbouquet · 2023-01-18T16:55:39Z

Thank you for the code review @williaster and apologies for the long delay of my response, the end of year has been pretty busy. I'll be dealing with your comments in the near future and will ping you when it's ready for another round. 🙂

williaster · 2023-01-18T20:40:09Z

Sounds great @fbouquet ! Will keep an eye out for your ping when you have time 👍

fbouquet · 2023-01-25T16:40:20Z

@williaster I requested a new review last week but just realized I didn't ping you, so I'm doing that now to make sure that you see the notification. No big rush though!

fbouquet · 2023-03-02T21:40:27Z

@williaster Just a reminder about this PR for whenever you get a chance to look into it. 🙂

TSIA-SN · 2023-11-22T14:00:26Z

Ran into an issue today with tooltips in portals due to the fact that we're using Tailwind with the important config option set to the __next div, so TooltipInPortal is unable to be styled by Tailwind due to the fact that the portal is rendering in the root of the DOM. Would love to simply be able to specify a selector for the hook to use in the Portal, similar to what's done here: https://github.com/vercel/next.js/blob/canary/examples/with-portals/components/ClientOnlyPortal.js. Came across this PR, looks like it's been stale for a while though, any plans for it to be merged soon or was it handled by a different update since the last comment?

fbouquet changed the title ~~feat(tooltip) Enable custom portal container for tooltip~~ feat(tooltip): Enable custom portal container for tooltip Sep 14, 2022

williaster added 🚀enhancement @visx/tooltip minor-release labels Sep 15, 2022

williaster reviewed Sep 16, 2022

View reviewed changes

williaster reviewed Oct 5, 2022

View reviewed changes

fbouquet force-pushed the fb-tooltip-custom-portal-container branch from 0601c34 to ab3353f Compare November 10, 2022 19:21

fbouquet commented Nov 10, 2022

View reviewed changes

fbouquet requested a review from williaster November 10, 2022 20:09

williaster reviewed Dec 15, 2022

View reviewed changes

Florent Bouquet added 7 commits January 18, 2023 19:54

Enable custom portal container for tooltip

ac66f31

Prettier + Readme update

ae56a47

Fix linting

3fbf8a9

Code review changes

7a5e3b3

Handle portal container resizing

99066ae

Fix TooltipWithBounds for tooltips in portal

6aeb835

Fix linting

0ec63f5

fbouquet force-pushed the fb-tooltip-custom-portal-container branch from ab3353f to 0ec63f5 Compare January 19, 2023 01:38

Simplify variable name

ec94a27

fbouquet requested a review from williaster January 19, 2023 02:09

williaster mentioned this pull request Feb 21, 2024

TooltipInPortal used inside a microfrontend obtains an unstyled (and unstylable) tooltip content #1802

Open

hshoff added the needs review label May 24, 2024

	const OverlayLayer = function OverlayLayer({
	function OverlayLayer({

-      <>
+      <div data-testid="inner-div">
+        <Portal>
+          <div data-testid="element-in-portal" />
+        </Portal>
+      </div>

	const [tooltipPortalShouldUseCustomContainer, setTooltipPortalShouldUseCustomContainer] =
	const [tooltipShouldUseCustomContainer, setTooltipShouldUseCustomContainer] =

feat(tooltip): Enable custom portal container for tooltip #1567

Are you sure you want to change the base?

feat(tooltip): Enable custom portal container for tooltip #1567

Conversation

fbouquet commented Sep 14, 2022

🚀 Enhancements

williaster left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster commented Sep 16, 2022

fbouquet commented Sep 16, 2022

fbouquet commented Oct 4, 2022

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbouquet commented Oct 12, 2022

Choose a reason for hiding this comment

fbouquet commented Nov 10, 2022

fbouquet commented Nov 29, 2022

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbouquet commented Jan 18, 2023

williaster commented Jan 18, 2023

fbouquet commented Jan 25, 2023

fbouquet commented Mar 2, 2023

TSIA-SN commented Nov 22, 2023 • edited Loading

williaster left a comment •

edited

Loading

TSIA-SN commented Nov 22, 2023 •

edited

Loading