Skip to content

Commit

Permalink
Fixes an edge case when the tooltip wouldn't move closer to the mouse…
Browse files Browse the repository at this point in the history
… cursor in some situations (PR #3663)

This would happen in this situation:
* The user hovers an element, whose tooltip is too big for the available
  space: the tooltip would align with the window's left edge.
* Then the user hovers another element whose tooltip would have space to
  go either at the right or left of the cursor. In that case we reuse the
  previous position, so that the tooltip stays stable.
  But if the previous position was the window-edge, then the tooltip
  would stay at that location and could be far from the mouse cursor.
  • Loading branch information
julienw committed Nov 24, 2021
1 parent 040c16f commit 98493ee
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 11 deletions.
9 changes: 8 additions & 1 deletion src/components/tooltip/Tooltip.js
Expand Up @@ -83,7 +83,14 @@ export class Tooltip extends React.PureComponent<Props> {
newPosition = possiblePositions[0];
break;
case 2:
newPosition = previousPosition;
// In case both positions before/after work, let's reuse the previous
// position if it's one of those, for more stability. If the previous
// position was window-edge, before-mouse looks more appropriate.
// Ideally we would try to keep the tooltip below the cursor.
newPosition =
previousPosition !== 'window-edge'
? previousPosition
: 'before-mouse';
break;
default:
throw new Error(
Expand Down
44 changes: 34 additions & 10 deletions src/test/components/Tooltip.test.js
Expand Up @@ -52,7 +52,7 @@ describe('shared/Tooltip', () => {

mouseX = 50;
mouseY = 70;
rerender({ x: mouseX, y: mouseY });
rerender({ mouse: { x: mouseX, y: mouseY } });

expect(getTooltipStyle()).toEqual({
left: `${mouseX + MOUSE_OFFSET}px`,
Expand All @@ -63,7 +63,7 @@ describe('shared/Tooltip', () => {
// below/right and above/left will still render the tooltip below/right.
mouseX = 510;
mouseY = 310;
rerender({ x: mouseX, y: mouseY });
rerender({ mouse: { x: mouseX, y: mouseY } });
expect(getTooltipStyle()).toEqual({
left: `${mouseX + MOUSE_OFFSET}px`,
top: `${mouseY + MOUSE_OFFSET}px`,
Expand All @@ -73,7 +73,7 @@ describe('shared/Tooltip', () => {
// available will move the tooltip left/above.
mouseX = 800;
mouseY = 700;
rerender({ x: mouseX, y: mouseY });
rerender({ mouse: { x: mouseX, y: mouseY } });
expect(getTooltipStyle()).toEqual({
left: `${mouseX - MOUSE_OFFSET - tooltipWidth}px`,
top: `${mouseY - MOUSE_OFFSET - tooltipHeight}px`,
Expand Down Expand Up @@ -102,7 +102,7 @@ describe('shared/Tooltip', () => {
// below/right and above/left will still render the tooltip above/left.
mouseX = 510;
mouseY = 310;
rerender({ x: mouseX, y: mouseY });
rerender({ mouse: { x: mouseX, y: mouseY } });
expect(getTooltipStyle()).toEqual({
left: `${expectedLeft()}px`,
top: `${expectedTop()}px`,
Expand All @@ -112,7 +112,7 @@ describe('shared/Tooltip', () => {
// available will move the tooltip right/below.
mouseX = 50;
mouseY = 30;
rerender({ x: mouseX, y: mouseY });
rerender({ mouse: { x: mouseX, y: mouseY } });
expect(getTooltipStyle()).toEqual({
left: `${mouseX + MOUSE_OFFSET}px`,
top: `${mouseY + MOUSE_OFFSET}px`,
Expand All @@ -121,17 +121,36 @@ describe('shared/Tooltip', () => {

it('is rendered at the left and top of the window if the space is missing elsewhere', () => {
// The jsdom window size is 1024x768.
const { getTooltipStyle } = setup({
box: { width: 700, height: 500 },
mouse: { x: 500, y: 300 },
let mouseX = 500;
let mouseY = 300;
let tooltipWidth = 700;
const tooltipHeight = 500;
const { getTooltipStyle, rerender } = setup({
box: { width: tooltipWidth, height: tooltipHeight },
mouse: { x: mouseX, y: mouseY },
});

const expectedLeft = VISUAL_MARGIN;
let expectedLeft = VISUAL_MARGIN;
const expectedTop = VISUAL_MARGIN;
expect(getTooltipStyle()).toEqual({
left: `${expectedLeft}px`,
top: `${expectedTop}px`,
});

// We change the size of the box and move the mouse slightly to trigger a render.
tooltipWidth = 300;
mouseX = 510;
mouseY = 310;
rerender({
box: { width: tooltipWidth, height: tooltipHeight },
mouse: { x: mouseX, y: mouseY },
});

expectedLeft = mouseX - MOUSE_OFFSET - tooltipWidth;
expect(getTooltipStyle()).toEqual({
left: `${expectedLeft}px`,
top: `${expectedTop}px`,
});
});
});
});
Expand Down Expand Up @@ -181,7 +200,12 @@ function setup({ box, mouse }: Setup) {
}

return {
rerender: (mouse: Position) => {
rerender: ({ mouse, box: newBox }: $Shape<Setup>) => {
if (newBox) {
box.width = newBox.width;
box.height = newBox.height;
}

rerender(
<Tooltip mouseX={mouse.x} mouseY={mouse.y}>
<p>Lorem ipsum</p>
Expand Down

0 comments on commit 98493ee

Please sign in to comment.