From ecb25ceb11cb5fe653d4876c04fdb36c315d0810 Mon Sep 17 00:00:00 2001
From: Robin <robin@robin.town>
Date: Thu, 5 Sep 2024 15:33:28 -0400
Subject: [PATCH] Allow tooltips to label non-interactive triggers

The combination of tooltip labels and non-interactive triggers was problematic: The tooltip component would place the label on the span element that it creates, rather than your provided trigger element. If your trigger was an icon or image, then accessibility tooling would flag it as missing a label. You might try putting aria-hidden on the element, but then you would still end up with the label being on an element that lacks a role (which is illegal).

The solution I've implemented here is to move the aria-labelledby and aria-describedby attributes to the trigger.
---
 src/components/Tooltip/Tooltip.stories.tsx |  2 +-
 src/components/Tooltip/Tooltip.test.tsx    | 16 +++++-
 src/components/Tooltip/Tooltip.tsx         | 61 +++++++++++++++++-----
 3 files changed, 63 insertions(+), 16 deletions(-)

diff --git a/src/components/Tooltip/Tooltip.stories.tsx b/src/components/Tooltip/Tooltip.stories.tsx
index ece8232a..a90bff88 100644
--- a/src/components/Tooltip/Tooltip.stories.tsx
+++ b/src/components/Tooltip/Tooltip.stories.tsx
@@ -177,7 +177,7 @@ export const NonInteractiveTrigger = {
   args: {
     isTriggerInteractive: false,
     description: "Shown without delay",
-    children: "Just some text",
+    children: <span>Just some text</span>,
   },
 };
 
diff --git a/src/components/Tooltip/Tooltip.test.tsx b/src/components/Tooltip/Tooltip.test.tsx
index 291cca79..b935b925 100644
--- a/src/components/Tooltip/Tooltip.test.tsx
+++ b/src/components/Tooltip/Tooltip.test.tsx
@@ -21,6 +21,9 @@ import React from "react";
 import * as stories from "./Tooltip.stories";
 import { composeStories, composeStory } from "@storybook/react";
 import userEvent from "@testing-library/user-event";
+import { TooltipProvider } from "./TooltipProvider";
+import { Tooltip } from "./Tooltip";
+import { UserIcon } from "@vector-im/compound-design-tokens/assets/web/icons";
 
 const {
   Default,
@@ -79,7 +82,7 @@ describe("Tooltip", () => {
     expect(screen.queryByRole("tooltip")).toBe(null);
     await user.tab();
     // trigger focused, tooltip shown
-    expect(screen.getByText("Just some text")).toHaveFocus();
+    expect(screen.getByText("Just some text").parentElement).toHaveFocus();
     screen.getByRole("tooltip");
   });
 
@@ -125,4 +128,15 @@ describe("Tooltip", () => {
     screen.getByRole("tooltip", { name: "Employer Identification Number" });
     expect(screen.queryByRole("button", { name: "EIN" })).toBe(null);
   });
+
+  it("labels an image", async () => {
+    render(
+      <TooltipProvider>
+        <Tooltip isTriggerInteractive={false} label="User profile">
+          <UserIcon role="image" width={24} height={24} />
+        </Tooltip>
+      </TooltipProvider>,
+    );
+    screen.getByRole("image", { name: "User profile" });
+  });
 });
diff --git a/src/components/Tooltip/Tooltip.tsx b/src/components/Tooltip/Tooltip.tsx
index f0acec9f..28bf65c6 100644
--- a/src/components/Tooltip/Tooltip.tsx
+++ b/src/components/Tooltip/Tooltip.tsx
@@ -27,6 +27,9 @@ import React, {
   isValidElement,
   cloneElement,
   useMemo,
+  ReactNode,
+  FC,
+  ReactElement,
 } from "react";
 
 import classNames from "classnames";
@@ -72,12 +75,11 @@ export function Tooltip({
 
   return (
     <TooltipContext.Provider value={context}>
-      <TooltipAnchor>
-        {isTriggerInteractive ? (
-          children
-        ) : (
-          <span tabIndex={nonInteractiveTriggerTabIndex}>{children}</span>
-        )}
+      <TooltipAnchor
+        isTriggerInteractive={isTriggerInteractive}
+        nonInteractiveTriggerTabIndex={nonInteractiveTriggerTabIndex}
+      >
+        {children}
       </TooltipAnchor>
       <TooltipContent>
         <span id={context.labelId}>
@@ -156,11 +158,21 @@ function TooltipContent({
   );
 }
 
+interface TooltipAnchorProps {
+  children: ReactNode;
+  isTriggerInteractive: boolean;
+  nonInteractiveTriggerTabIndex?: number;
+}
+
 /**
  * The anchor of the tooltip
  * @param children
  */
-function TooltipAnchor({ children }: Readonly<PropsWithChildren>): JSX.Element {
+const TooltipAnchor: FC<TooltipAnchorProps> = ({
+  children,
+  isTriggerInteractive,
+  nonInteractiveTriggerTabIndex,
+}) => {
   const context = useTooltipContext();
 
   // The children can have a ref and we don't want to discard it
@@ -168,19 +180,40 @@ function TooltipAnchor({ children }: Readonly<PropsWithChildren>): JSX.Element {
   const childrenRef = (children as unknown as { ref?: Ref<HTMLElement> })?.ref;
   const ref = useMergeRefs([context.refs.setReference, childrenRef]);
 
-  // We need to check `isValidElement` to infer the type of `children`
-  const childrenProps = isValidElement(children) && children.props;
-
   const element = useMemo(() => {
     if (!isValidElement(children)) return;
 
-    const props = context.getReferenceProps({ ref, ...childrenProps });
-    return cloneElement(children, props);
-  }, [context, ref, children, childrenProps]);
+    if (isTriggerInteractive) {
+      const props = context.getReferenceProps({ ref, ...children.props });
+      return cloneElement(children, props);
+    } else {
+      // For a non-interactive trigger, we want most of the props to go on the
+      // span element that we provide, since that's what receives focus, but it
+      // should still be the trigger that receives the label/description. It
+      // would be wrong to label the span element, as it lacks a role.
+      const props = context.getReferenceProps({
+        ref,
+        tabIndex: nonInteractiveTriggerTabIndex,
+      });
+      const {
+        "aria-labelledby": labelId,
+        "aria-describedby": descriptionId,
+        ...spanProps
+      } = props;
+      return (
+        <span tabIndex={nonInteractiveTriggerTabIndex} {...spanProps}>
+          {cloneElement(children as ReactElement, {
+            "aria-labelledby": labelId,
+            "aria-describedby": descriptionId,
+          })}
+        </span>
+      );
+    }
+  }, [context, ref, children]);
 
   if (!element) {
     throw new Error("Tooltip anchor must be a single valid React element");
   }
 
   return element;
-}
+};