Fix tooltip (#121)

* Minor clean up of EventTooltip.js * Fix root cause of #47 and improve tooltip styling Tooltips would go offscreen because of the way `top` is defined. This commit fixes that root cause by changing CanvasPage to position fixed so that tooltips will be positioned relative to the DOM rather than the CanvasPage div. Undoes some of the changes made in #78 as this commit fixes the root cause of the bug. * Disable component name truncation Although this was Brian's recommendation (see #67), I don't think component names will be very long because the source code for those will be really annoying. * Set mouse location on every interaction to fix #87 * Introduce 768-char component name length limit Covers the unlikely edge case that a component name is unreasonably long.
MLH-Fellowship · Aug 10, 2020 · d1b1b50 · d1b1b50
1 parent 09fbff4
commit d1b1b50
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 95 deletions.
diff --git a/src/CanvasPage.css b/src/CanvasPage.css
@@ -4,5 +4,4 @@
   bottom: 0.5rem;
   left: 0.5rem;
   right: 0.5rem;
-  overflow: hidden;
 }
diff --git a/src/CanvasPage.js b/src/CanvasPage.js
@@ -250,25 +250,19 @@ function AutoSizedCanvas({data, height, width}: AutoSizedCanvasProps) {
 
   const interactor = useCallback(
     interaction => {
-      if (
-        hoveredEvent &&
-        (hoveredEvent.event ||
-          hoveredEvent.measure ||
-          hoveredEvent.flamechartStackFrame ||
-          hoveredEvent.userTimingMark)
-      ) {
-        setMouseLocation({
-          x: interaction.payload.event.x,
-          y: interaction.payload.event.y,
-        });
-      }
       if (canvasRef.current === null) {
         return;
       }
       surfaceRef.current.handleInteraction(interaction);
-      surfaceRef.current.displayIfNeeded();
+      // Defer drawing to canvas until React's commit phase, to avoid drawing
+      // twice and to ensure that both the canvas and DOM elements managed by
+      // React are in sync.
+      setMouseLocation({
+        x: interaction.payload.event.x,
+        y: interaction.payload.event.y,
+      });
     },
-    [surfaceRef, hoveredEvent, setMouseLocation],
+    [surfaceRef, canvasRef, hoveredEvent, setMouseLocation],
   );
 
   useCanvasInteraction(canvasRef, interactor);
@@ -387,10 +381,7 @@ function AutoSizedCanvas({data, height, width}: AutoSizedCanvasProps) {
     hoveredEvent,
   ]);
 
-  // When React component renders, rerender surface.
-  // TODO: See if displaying on rAF would make more sense since we're somewhat
-  // decoupled from React and we don't want to render canvas multiple times per
-  // frame.
+  // Draw to canvas in React's commit phase
   useLayoutEffect(() => {
     surfaceRef.current.displayIfNeeded();
   });

diff --git a/src/EventTooltip.css b/src/EventTooltip.css
@@ -1,5 +1,5 @@
 .Tooltip {
-  position: absolute;
+  position: fixed;
   display: inline-block;
   border-radius: 0.125rem;
   max-width: 300px;
@@ -31,12 +31,20 @@
 }
 
 .DetailsGridURL {
+  word-break: break-all;
+  max-height: 50vh;
   overflow: hidden;
 }
 
+.FlamechartStackFrameName {
+  word-break: break-word;
+  margin-left: 0.4rem;
+}
+
 .ComponentName {
   font-weight: bold;
-  word-wrap: break-word;
+  word-break: break-word;
+  margin-right: 0.4rem;
 }
 
 .ComponentStack {
@@ -61,3 +69,11 @@
   white-space: pre;
   --gradient-height: 5em;
 }
+
+.ReactMeasureLabel {
+  margin-left: 0.4rem;
+}
+
+.UserTimingLabel {
+  word-break: break-word;
+}
diff --git a/src/EventTooltip.js b/src/EventTooltip.js
@@ -32,14 +32,14 @@ function formatDuration(ms) {
   return prettyMilliseconds(ms, {millisecondsDecimalDigits: 3});
 }
 
-function trimComponentName(name) {
-  if (name.length > 128) {
-    return name.substring(0, 127) + '...';
+function trimmedString(string: string, length: number): string {
+  if (string.length > length) {
+    return `${string.substr(0, length - 1)}…`;
   }
-  return name;
+  return string;
 }
 
-function getReactEventLabel(type) {
+function getReactEventLabel(type): string | null {
   switch (type) {
     case 'schedule-render':
       return 'render scheduled';
@@ -58,7 +58,25 @@ function getReactEventLabel(type) {
   }
 }
 
-function getReactMeasureLabel(type: string) {
+function getReactEventColor(event: ReactEvent): string | null {
+  switch (event.type) {
+    case 'schedule-render':
+      return COLORS.REACT_SCHEDULE_HOVER;
+    case 'schedule-state-update':
+    case 'schedule-force-update':
+      return event.isCascading
+        ? COLORS.REACT_SCHEDULE_CASCADING_HOVER
+        : COLORS.REACT_SCHEDULE_HOVER;
+    case 'suspense-suspend':
+    case 'suspense-resolved':
+    case 'suspense-rejected':
+      return COLORS.REACT_SUSPEND_HOVER;
+    default:
+      return null;
+  }
+}
+
+function getReactMeasureLabel(type): string | null {
   switch (type) {
     case 'commit':
       return 'commit';
@@ -85,65 +103,18 @@ export default function EventTooltip({data, hoveredEvent, origin}: Props) {
     return null;
   }
 
-  const {event, flamechartStackFrame, measure, userTimingMark} = hoveredEvent;
+  const {event, measure, flamechartStackFrame, userTimingMark} = hoveredEvent;
 
   if (event !== null) {
-    switch (event.type) {
-      case 'schedule-render':
-        return (
-          <TooltipReactEvent
-            color={COLORS.REACT_SCHEDULE_HOVER}
-            data={data}
-            event={event}
-            tooltipRef={tooltipRef}
-          />
-        );
-      case 'schedule-state-update': // eslint-disable-line no-case-declarations
-      case 'schedule-force-update':
-        const color = event.isCascading
-          ? COLORS.REACT_SCHEDULE_CASCADING_HOVER
-          : COLORS.REACT_SCHEDULE_HOVER;
-        return (
-          <TooltipReactEvent
-            color={color}
-            data={data}
-            event={event}
-            tooltipRef={tooltipRef}
-          />
-        );
-      case 'suspense-suspend':
-      case 'suspense-resolved':
-      case 'suspense-rejected':
-        return (
-          <TooltipReactEvent
-            color={COLORS.REACT_SUSPEND_HOVER}
-            data={data}
-            event={event}
-            tooltipRef={tooltipRef}
-          />
-        );
-      default:
-        console.warn(`Unexpected event type "${event.type}"`);
-        break;
-    }
+    return <TooltipReactEvent event={event} tooltipRef={tooltipRef} />;
   } else if (measure !== null) {
-    switch (measure.type) {
-      case 'commit':
-      case 'render-idle':
-      case 'render':
-      case 'layout-effects':
-      case 'passive-effects':
-        return (
-          <TooltipReactMeasure
-            data={data}
-            measure={measure}
-            tooltipRef={tooltipRef}
-          />
-        );
-      default:
-        console.warn(`Unexpected measure type "${measure.type}"`);
-        break;
-    }
+    return (
+      <TooltipReactMeasure
+        data={data}
+        measure={measure}
+        tooltipRef={tooltipRef}
+      />
+    );
   } else if (flamechartStackFrame !== null) {
     return (
       <TooltipFlamechartNode
@@ -186,7 +157,8 @@ const TooltipFlamechartNode = ({
   } = stackFrame;
   return (
     <div className={styles.Tooltip} ref={tooltipRef}>
-      {formatDuration(duration)} {trimComponentName(name)}
+      {formatDuration(duration)}
+      <span className={styles.FlamechartStackFrameName}>{name}</span>
       <div className={styles.DetailsGrid}>
         <div className={styles.DetailsGridLabel}>Timestamp:</div>
         <div>{formatTimestamp(timestamp)}</div>
@@ -210,24 +182,28 @@ const TooltipFlamechartNode = ({
 };
 
 const TooltipReactEvent = ({
-  color,
   event,
   tooltipRef,
 }: {
-  color: string,
   event: ReactEvent,
   tooltipRef: Return<typeof useRef>,
 }) => {
-  const {componentName, componentStack, timestamp, type} = event;
-  const label = getReactEventLabel(type);
+  const label = getReactEventLabel(event.type);
+  const color = getReactEventColor(event);
+  if (!label || !color) {
+    console.warn(`Unexpected event type "${event.type}"`);
+    return null;
+  }
+
+  const {componentName, componentStack, timestamp} = event;
 
   return (
     <div className={styles.Tooltip} ref={tooltipRef}>
       {componentName && (
         <span className={styles.ComponentName} style={{color}}>
-          {trimComponentName(componentName)}
+          {trimmedString(componentName, 768)}
         </span>
-      )}{' '}
+      )}
       {label}
       <div className={styles.Divider} />
       <div className={styles.DetailsGrid}>
@@ -255,13 +231,19 @@ const TooltipReactMeasure = ({
   measure: ReactMeasure,
   tooltipRef: Return<typeof useRef>,
 }) => {
-  const {batchUID, duration, timestamp, type, lanes} = measure;
-  const label = getReactMeasureLabel(type);
+  const label = getReactMeasureLabel(measure.type);
+  if (!label) {
+    console.warn(`Unexpected measure type "${measure.type}"`);
+    return null;
+  }
+
+  const {batchUID, duration, timestamp, lanes} = measure;
   const [startTime, stopTime] = getBatchRange(batchUID, data);
 
   return (
     <div className={styles.Tooltip} ref={tooltipRef}>
-      {formatDuration(duration)} {label}
+      {formatDuration(duration)}
+      <span className={styles.ReactMeasureLabel}>{label}</span>
       <div className={styles.Divider} />
       <div className={styles.DetailsGrid}>
         <div className={styles.DetailsGridLabel}>Timestamp:</div>
@@ -287,7 +269,7 @@ const TooltipUserTimingMark = ({
   const {name, timestamp} = mark;
   return (
     <div className={styles.Tooltip} ref={tooltipRef}>
-      {name}
+      <span className={styles.UserTimingLabel}>{name}</span>
       <div className={styles.Divider} />
       <div className={styles.DetailsGrid}>
         <div className={styles.DetailsGridLabel}>Timestamp:</div>

diff --git a/src/utils/useSmartTooltip.js b/src/utils/useSmartTooltip.js
@@ -57,7 +57,7 @@ export default function useSmartTooltip({
         element.style.left = `${mouseX + TOOLTIP_OFFSET}px`;
       }
     }
-  });
+  }, [mouseX, mouseY, ref]);
 
   return ref;
 }