fix: add tests and improve accessibility for useClickable (#12218)

.vscode/settings.json

@@ -114,6 +114,7 @@
     "Signup",
     "slogtest",
     "sourcemapped",
+    "spinbutton",
     "Srcs",
     "stdbuf",
     "stretchr",

site/src/components/FileUpload/FileUpload.tsx

@@ -61,14 +61,11 @@ export const FileUpload: FC<FileUploadProps> = ({
   extension,
   fileTypeRequired,
 }) => {
-  const inputRef = useRef<HTMLInputElement>(null);
   const tarDrop = useFileDrop(onUpload, fileTypeRequired);
-
+  const inputRef = useRef<HTMLInputElement>(null);
-  const clickable = useClickable<HTMLDivElement>(() => {
+  const clickable = useClickable<HTMLDivElement>(
-    if (inputRef.current) {
+    () => inputRef.current?.click(),
-      inputRef.current.click();
+  );
-    }
-  });
 
   if (!isUploading && file) {
     return (

site/src/hooks/useClickable.ts

@@ -10,44 +10,62 @@ type ClickableElement = {
   click: () => void;
 };
 
-export interface UseClickableResult<
+/**
-  T extends ClickableElement = ClickableElement,
+ * May be worth adding support for the 'spinbutton' role at some point, but that
-> {
+ * will change the structure of the return result in a big way. Better to wait
-  ref: RefObject<T>;
+ * until we actually need it.
+ *
+ * @see {@link https://www.w3.org/WAI/ARIA/apg/patterns/spinbutton/}
+ */
+export type ClickableAriaRole = "button" | "switch";
+
+export type UseClickableResult<
+  TElement extends ClickableElement = ClickableElement,
+  TRole extends ClickableAriaRole = ClickableAriaRole,
+> = Readonly<{
+  ref: RefObject<TElement>;
   tabIndex: 0;
-  role: "button";
+  role: TRole;
-  onClick: MouseEventHandler<T>;
+  onClick: MouseEventHandler<TElement>;
-  onKeyDown: KeyboardEventHandler<T>;
+  onKeyDown: KeyboardEventHandler<TElement>;
-}
+  onKeyUp: KeyboardEventHandler<TElement>;
+}>;
 
 /**
- * Exposes props to add basic click/interactive behavior to HTML elements that
+ * Exposes props that let you turn traditionally non-interactive elements into
- * don't traditionally have support for them.
+ * buttons.
  */
 export const useClickable = <
-  // T doesn't have a default type on purpose; the hook should error out if it
+  TElement extends ClickableElement,
-  // doesn't have an explicit type, or a type it can infer from onClick
+  TRole extends ClickableAriaRole = ClickableAriaRole,
-  T extends ClickableElement,
 >(
-  // Even though onClick isn't used in any of the internal calculations, it's
+  onClick: MouseEventHandler<TElement>,
-  // still a required argument, just to make sure that useClickable can't
+  role?: TRole,
-  // accidentally be called in a component without also defining click behavior
+): UseClickableResult<TElement, TRole> => {
-  onClick: MouseEventHandler<T>,
+  const ref = useRef<TElement>(null);
-): UseClickableResult<T> => {
-  const ref = useRef<T>(null);
 
   return {
     ref,
-    tabIndex: 0,
-    role: "button",
     onClick,
+    tabIndex: 0,
+    role: (role ?? "button") as TRole,
 
-    // Most interactive elements automatically make Space/Enter trigger onClick
+    /*
-    // callbacks, but you explicitly have to add it for non-interactive elements
+     * Native buttons are programmed to handle both space and enter, but they're
+     * each handled via different event handlers.
+     *
+     * 99% of the time, you shouldn't be able to tell the difference, but one
+     * edge case behavior is that holding down Enter will continually fire
+     * events, while holding down Space won't fire anything until you let go.
+     */
     onKeyDown: (event) => {
-      if (event.key === "Enter" || event.key === " ") {
+      if (event.key === "Enter") {
-        // Can't call onClick from here because onKeydown's keyboard event isn't
+        ref.current?.click();
-        // compatible with mouse events. Have to use a ref to simulate a click
+        event.stopPropagation();
+      }
+    },
+    onKeyUp: (event) => {
+      if (event.key === " ") {
         ref.current?.click();
         event.stopPropagation();
       }

site/src/hooks/useClickableTableRow.test.tsx

@@ -0,0 +1,154 @@
+import {
+  type ElementType,
+  type FC,
+  type MouseEventHandler,
+  type PropsWithChildren,
+} from "react";
+
+import { type ClickableAriaRole, useClickable } from "./useClickable";
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+
+/**
+ * Since the point of the hook is to take a traditionally non-interactive HTML
+ * element and make it interactive, it made the most sense to test against an
+ * element directly.
+ */
+type NonNativeButtonProps<TElement extends HTMLElement = HTMLElement> =
+  Readonly<
+    PropsWithChildren<{
+      as?: Exclude<ElementType, "button">;
+      role?: ClickableAriaRole;
+      onInteraction: MouseEventHandler<TElement>;
+    }>
+  >;
+
+const NonNativeButton: FC<NonNativeButtonProps<HTMLElement>> = ({
+  as,
+  onInteraction,
+  children,
+  role = "button",
+}) => {
+  const clickableProps = useClickable(onInteraction, role);
+  const Component = as ?? "div";
+  return <Component {...clickableProps}>{children}</Component>;
+};
+
+describe(useClickable.name, () => {
+  it("Always defaults to role 'button'", () => {
+    render(<NonNativeButton onInteraction={jest.fn()} />);
+    expect(() => screen.getByRole("button")).not.toThrow();
+  });
+
+  it("Overrides the native role of any element that receives the hook result (be very careful with this behavior)", () => {
+    const anchorText = "I'm a button that's secretly a link!";
+    render(
+      <NonNativeButton as="a" role="button" onInteraction={jest.fn()}>
+        {anchorText}
+      </NonNativeButton>,
+    );
+
+    const linkButton = screen.getByRole("button", {
+      name: anchorText,
+    });
+
+    expect(linkButton).toBeInstanceOf(HTMLAnchorElement);
+  });
+
+  it("Always returns out the same role override received via arguments", () => {
+    const placeholderCallback = jest.fn();
+    const roles = [
+      "button",
+      "switch",
+    ] as const satisfies readonly ClickableAriaRole[];
+
+    for (const role of roles) {
+      const { unmount } = render(
+        <NonNativeButton role={role} onInteraction={placeholderCallback} />,
+      );
+
+      expect(() => screen.getByRole(role)).not.toThrow();
+      unmount();
+    }
+  });
+
+  it("Allows an element to receive keyboard focus", async () => {
+    const user = userEvent.setup();
+    const mockCallback = jest.fn();
+
+    render(<NonNativeButton role="button" onInteraction={mockCallback} />, {
+      wrapper: ({ children }) => (
+        <>
+          <button>Native button</button>
+          {children}
+        </>
+      ),
+    });
+
+    await user.keyboard("[Tab][Tab]");
+    await user.keyboard(" ");
+    expect(mockCallback).toBeCalledTimes(1);
+  });
+
+  it("Allows an element to respond to clicks and Space/Enter, following all rules for native Button element interactions", async () => {
+    const mockCallback = jest.fn();
+    const user = userEvent.setup();
+    render(<NonNativeButton role="button" onInteraction={mockCallback} />);
+
+    await user.click(document.body);
+    await user.keyboard(" ");
+    await user.keyboard("[Enter]");
+    expect(mockCallback).not.toBeCalled();
+
+    const button = screen.getByRole("button");
+    await user.click(button);
+    await user.keyboard(" ");
+    await user.keyboard("[Enter]");
+    expect(mockCallback).toBeCalledTimes(3);
+  });
+
+  it("Will keep firing events if the Enter key is held down", async () => {
+    const mockCallback = jest.fn();
+    const user = userEvent.setup();
+    render(<NonNativeButton role="button" onInteraction={mockCallback} />);
+
+    // Focus over to element, hold down Enter for specified keydown period
+    // (count determined by browser/library), and then release the Enter key
+    const keydownCount = 5;
+    await user.keyboard(`[Tab]{Enter>${keydownCount}}{/Enter}`);
+    expect(mockCallback).toBeCalledTimes(keydownCount);
+  });
+
+  it("Will NOT keep firing events if the Space key is held down", async () => {
+    const mockCallback = jest.fn();
+    const user = userEvent.setup();
+    render(<NonNativeButton role="button" onInteraction={mockCallback} />);
+
+    // Focus over to element, and then hold down Space for 100 keydown cycles
+    await user.keyboard("[Tab]{ >100}");
+    expect(mockCallback).not.toBeCalled();
+
+    // Then explicitly release the space bar
+    await user.keyboard(`{/ }`);
+    expect(mockCallback).toBeCalledTimes(1);
+  });
+
+  test("If focus is lost while Space is held down, then releasing the key will do nothing", async () => {
+    const mockCallback = jest.fn();
+    const user = userEvent.setup();
+
+    render(<NonNativeButton role="button" onInteraction={mockCallback} />, {
+      wrapper: ({ children }) => (
+        <>
+          {children}
+          <button>Native button</button>
+        </>
+      ),
+    });
+
+    // Focus over to element, hold down Space for an indefinite amount of time,
+    // move focus away from element, and then release Space
+    await user.keyboard("[Tab]{ >}[Tab]{/ }");
+    expect(mockCallback).not.toBeCalled();
+  });
+});

site/src/hooks/useClickableTableRow.ts

@@ -1,9 +1,31 @@
-import { useTheme, type CSSObject } from "@emotion/react";
+/**
+ * @file 2024-02-19 - MES - Sadly, even though this hook aims to make elements
+ * more accessible, it's doing the opposite right now. Per axe audits, the
+ * current implementation will create a bunch of critical-level accessibility
+ * violations:
+ *
+ * 1. Nesting interactive elements (e.g., workspace table rows having checkboxes
+ *    inside them)
+ * 2. Overriding the native element's role (in this case, turning a native table
+ *    row into a button, which means that screen readers lose the ability to
+ *    announce the row's data as part of a larger table)
+ *
+ * It might not make sense to test this hook until the underlying design
+ * problems are fixed.
+ */
 import { type MouseEventHandler } from "react";
-import { type TableRowProps } from "@mui/material/TableRow";
+import { type CSSObject, useTheme } from "@emotion/react";
-import { useClickable, type UseClickableResult } from "./useClickable";
 
-type UseClickableTableRowResult = UseClickableResult<HTMLTableRowElement> &
+import {
+  type ClickableAriaRole,
+  type UseClickableResult,
+  useClickable,
+} from "./useClickable";
+import { type TableRowProps } from "@mui/material/TableRow";
+
+type UseClickableTableRowResult<
+  TRole extends ClickableAriaRole = ClickableAriaRole,
+> = UseClickableResult<HTMLTableRowElement, TRole> &
   TableRowProps & {
     css: CSSObject;
     hover: true;
@@ -11,24 +33,28 @@ type UseClickableTableRowResult = UseClickableResult<HTMLTableRowElement> &
   };
 
 // Awkward type definition (the hover preview in VS Code isn't great, either),
-// but this basically takes all click props from TableRowProps, but makes
+// but this basically extracts all click props from TableRowProps, but makes
-// onClick required, and adds an optional onMiddleClick
+// onClick required, and adds additional optional props (notably onMiddleClick)
-type UseClickableTableRowConfig = {
+type UseClickableTableRowConfig<TRole extends ClickableAriaRole> = {
   [Key in keyof TableRowProps as Key extends `on${string}Click`
     ? Key
-    : never]: UseClickableTableRowResult[Key];
+    : never]: UseClickableTableRowResult<TRole>[Key];
 } & {
+  role?: TRole;
   onClick: MouseEventHandler<HTMLTableRowElement>;
   onMiddleClick?: MouseEventHandler<HTMLTableRowElement>;
 };
 
-export const useClickableTableRow = ({
+export const useClickableTableRow = <
+  TRole extends ClickableAriaRole = ClickableAriaRole,
+>({
+  role,
   onClick,
-  onAuxClick: externalOnAuxClick,
   onDoubleClick,
   onMiddleClick,
-}: UseClickableTableRowConfig): UseClickableTableRowResult => {
+  onAuxClick: externalOnAuxClick,
-  const clickableProps = useClickable(onClick);
+}: UseClickableTableRowConfig<TRole>): UseClickableTableRowResult<TRole> => {
+  const clickableProps = useClickable(onClick, (role ?? "button") as TRole);
   const theme = useTheme();
 
   return {
@@ -49,12 +75,14 @@ export const useClickableTableRow = ({
     hover: true,
     onDoubleClick,
     onAuxClick: (event) => {
+      // Regardless of which callback gets called, the hook won't stop the event
+      // from bubbling further up the DOM
       const isMiddleMouseButton = event.button === 1;
       if (isMiddleMouseButton) {
         onMiddleClick?.(event);
+      } else {
+        externalOnAuxClick?.(event);
       }
-
-      externalOnAuxClick?.(event);
     },
   };
 };

site/src/pages/WorkspacesPage/WorkspacesTable.tsx

@@ -255,7 +255,6 @@ const WorkspacesRow: FC<WorkspacesRowProps> = ({
 
   const workspacePageLink = `/@${workspace.owner_name}/${workspace.name}`;
   const openLinkInNewTab = () => window.open(workspacePageLink, "_blank");
-
   const clickableProps = useClickableTableRow({
     onMiddleClick: openLinkInNewTab,
     onClick: (event) => {
@@ -272,7 +271,9 @@ const WorkspacesRow: FC<WorkspacesRowProps> = ({
       }
     },
   });
+
   const bgColor = checked ? theme.palette.action.hover : undefined;
+
   return (
     <TableRow
       {...clickableProps}