[TooltipWithBounds] fix : overflowing its parent on small screens

[LethalPants]

🐛 Bug Fix

• TooltipWithBounds overflowing its parent on small screens #466 : change logic to calculate position

[coveralls]

Pull Request Test Coverage Report for Build 297437244

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 14 (0.0%) changed or added relevant lines in 1 file are covered.
• 41 unchanged lines in 8 files lost coverage.
• Overall coverage increased (+1.8%) to 57.086%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/visx-tooltip/src/tooltips/TooltipWithBounds.tsx	0	14	0.0%

Files with Coverage Reduction	New Missed Lines	%
packages/visx-pattern/src/patterns/Lines.tsx	1	85.0%
packages/visx-xychart/src/components/XYChart.tsx	2	92.5%
packages/visx-tooltip/src/hooks/useTooltip.ts	3	44.44%
packages/visx-responsive/src/enhancers/withParentSize.tsx	5	58.06%
packages/visx-xychart/src/components/series/BarSeries.tsx	5	58.44%
packages/visx-xychart/src/components/series/LineSeries.tsx	5	40.63%
packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx	7	0.0%
packages/visx-responsive/src/components/ParentSize.tsx	13	0.0%

Totals
Change from base Build 283122090:	1.8%
Covered Lines:	1285
Relevant Lines:	2184

---

💛 - Coveralls

[williaster]

Hey @LethalPants thanks for the PR! In playing with this locally there are a couple of issues:

• leftOffset/topOffset are not applied correctly when the tooltip is positioned to the bottom or the right (they are flush with the lines but there should be 10px of offset padding)

• to tooltipInPortal is always positioned to the bottom left regardless of its position

[LethalPants]

Okay I'll take a look at it and make the changes. Thanks!

[joshxyzhimself]

Hi @LethalPants this looks gangsta, just curious do you think this is also related to #839?

[LethalPants]

to tooltipInPortal is always positioned to the bottom left regardless of its position

I don't think this and the overflow issue my commit fixes are related as it is persistent in the current version too, just that the tooltip is in the bottom right not sure if that was planned.

Unrelated but the tooltip is under the plot in the published docs. 👀

[LethalPants]

I've changed the logic so that the position of the tooltip is change when the gap is less than 1% of the width of parentBounds
Which eliminated the offset issue mentioned by @williaster. As for the issue it's not entirely possible to fix it with a fixed width of the tooltip even if it's placed in the largest quadrant. We should probably make the tooltip responsive.

[williaster]

@LethalPants sorry for the delay, thanks for iterating on this! After playing with this version, I think something like the following might be the ideal fix:

// parentBounds.width = 0 when rendered in a `Portal`
const placeTooltipLeft = parentBounds.width
  ? left + offsetLeft > parentBounds.width / 2
  : left + offsetLeft > window.innerWidth / 2;

const placeTooltipUp = parentBounds.height
  ? top + offsetTop > parentBounds.height / 2
  : top + offsetTop > window.innerHeight / 2;

I think this has two benefits versus the 1% implementation:

1. This always puts the tooltip in the quadrant with the most space (even if it's clipped; to avoid this I agree the Tooltip needs to have a dynamic width). With the 1% logic, if the tooltip will overflow on either side, it will still choose the side with less space (which is the bug report)

1% logic (current)

largest quadrant logic (proposed)

2. This logic doesn't depend on ownBounds.left/right, which actually is a great performance win. If you check the example code, we often use key={Math.random()} for the tooltip to force update ownBounds, but that re-mounts the component on every mouse move. If we don't need ownBounds, we can remove that and improve perf pretty significantly.

[williaster]

Thanks for the 🐛 report for the z-index issue in the airbnb.io/visx site, I noticed this as well and created an issue #850

[williaster]

Sorry, playing with my suggested code more also isn't 100% perfect for the Portal case because parentBounds is basically the entire page. So if you do > parentBounds.[width or height] / 2 it has weird effects depending where on the page the tooltip is used 🤦 .

I think this is better, but it isn't as robust to not needing the key={Math.random} (ownBounds.width/height is more stable than ownBounds.left/top/right/bottom would be, and we still avoid referencing the latter):

const placeTooltipLeft = parentBounds.width
  ? left + offsetLeft > parentBounds.width / 2
  : left + offsetLeft + ownBounds.width > window.innerWidth;
const placeTooltipUp = parentBounds.height
  ? top + offsetTop + ownBounds.height > parentBounds.height
  : top + offsetTop + ownBounds.height > window.innerHeight;

[LethalPants]

My initial solution used left > parentBounds.width / 2 which seemed to work almost fine but seeing the tooltip shift position at the halfway mark didn't feel natural 😅

So if you do > parentBounds.[width or height] / 2 it has weird effects depending where on the page the tooltip is used 🤦.

Can you please elaborate on this, I couldn't recreate the issue you mentioned.

I'm not sure whether the tooltip is supposed to stay in the bounds of the plot but that isn't the case in the code snippet you pasted.

[williaster]

@LethalPants yeah I agree that the shift at the midway point doesn't feel as great UX wise. okay what about this, it doesn't rely on left/right/top/bottom, swaps position only when it encounters the right/bottom edge, and swaps to the side where the fewest px will be clipped (the original bug). for very wide tooltips that don't fit on either side this will swap the position at the parent's mid-width point (in this case the user should render the Tooltip in a Portal, or make its width responsive)

let placeTooltipLeft = false;

if (parentBounds.width) {
  const rightPlacementClippedPx = left + offsetLeft + ownBounds.width - parentBounds.width;
  const leftPlacementClippedPx = ownBounds.width - left - offsetLeft;
  placeTooltipLeft =
    rightPlacementClippedPx > 0 && rightPlacementClippedPx > leftPlacementClippedPx;
} else {
  placeTooltipLeft = left + offsetLeft + ownBounds.width > window.innerWidth;
}

const placeTooltipUp = parentBounds.height
  ? top + offsetTop + ownBounds.height > parentBounds.height
  : top + offsetTop + ownBounds.height > window.innerHeight;

I'm not sure whether the tooltip is supposed to stay in the bounds of the plot but that isn't the case in the code snippet you pasted.

When Portal is enabled, the tooltip is rendered outside of the container, and so its true parent container is <body />. So in this case it's not supposed to detect its immediate container's bounds, instead it should detect the page bounds (the above solution should work for this).

So if you do > parentBounds.[width or height] / 2 it has weird effects depending where on the page the tooltip is used 🤦.

Can you please elaborate on this, I couldn't recreate the issue you mentioned.

This is confusing, sorry. It only applies to the Portal case where parentRect is the <body />, in some of the solutions we discussed where we flipped position based on parent width/height, there was weird behavior on /gallery and /docs/tooltip where the tooltip example is further down the page. I think we don't have to worry about this if we go with the above solution. sorry this turned out to be more complicated than initially anticipated 🙉 .

[LethalPants]

Sorry for the delay.

what about this, it doesn't rely on left/right/top/bottom, swaps position only when it encounters the right/bottom edge, and swaps to the side where the fewest px will be clipped (the original bug).

This seems fine.

for very wide tooltips that don't fit on either side this will swap the position at the parent's mid-width point (in this case the user should render the Tooltip in a Portal, or make its width responsive)

I think changing to Portal would be a better option, incase the tooltip is wider than parentBounds.width * 0.75 we can switch it to Portal, sounds good?

I've also pushed changes for detecting the edge with the snippet you posted.

[williaster]

@LethalPants this looks great 👌 going to go ahead and merge, thanks for all the iteration on this!

I think changing to Portal would be a better option, incase the tooltip is wider than parentBounds.width * 0.75 we can switch it to Portal, sounds good?

I think auto-switching is pretty tricky at the moment because rendering in the Portal currently requires a ResizeObserver polyfill so it'd be a breaking change. I think in most cases a Portal is actually more ideal, and useTooltipInPortal is compatible with TooltipWithBounds so users can leverage these enhancements.