From fd8b0dac45ffee8b465b0714d31281e5927202c0 Mon Sep 17 00:00:00 2001
From: Julien Wajsberg <felash@gmail.com>
Date: Mon, 22 Nov 2021 15:42:50 +0100
Subject: [PATCH] Fixes an edge case when the tooltip wouldn't move closer to
 the mouse cursor in some situations

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.
---
 src/components/tooltip/Tooltip.js   |  9 +++++-
 src/test/components/Tooltip.test.js | 44 ++++++++++++++++++++++-------
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/src/components/tooltip/Tooltip.js b/src/components/tooltip/Tooltip.js
index 7c01426720..38295645c2 100644
--- a/src/components/tooltip/Tooltip.js
+++ b/src/components/tooltip/Tooltip.js
@@ -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(
diff --git a/src/test/components/Tooltip.test.js b/src/test/components/Tooltip.test.js
index 17d0c6247b..53e314aecb 100644
--- a/src/test/components/Tooltip.test.js
+++ b/src/test/components/Tooltip.test.js
@@ -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`,
@@ -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`,
@@ -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`,
@@ -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`,
@@ -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`,
@@ -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`,
+      });
     });
   });
 });
@@ -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>