From 1d254f4680fcf8cc06d313d1f078a1ea5fb7cae7 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 21 Feb 2024 10:59:13 -0500 Subject: [PATCH] fix: add tests and improve accessibility for useClickable (#12218) --- .vscode/settings.json | 1 + site/src/components/FileUpload/FileUpload.tsx | 11 +- site/src/hooks/useClickable.ts | 70 +++++--- site/src/hooks/useClickableTableRow.test.tsx | 154 ++++++++++++++++++ site/src/hooks/useClickableTableRow.ts | 56 +++++-- .../pages/WorkspacesPage/WorkspacesTable.tsx | 3 +- 6 files changed, 247 insertions(+), 48 deletions(-) create mode 100644 site/src/hooks/useClickableTableRow.test.tsx diff --git a/.vscode/settings.json b/.vscode/settings.json index 107031e5b0..1f47d7ecf9 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -114,6 +114,7 @@ "Signup", "slogtest", "sourcemapped", + "spinbutton", "Srcs", "stdbuf", "stretchr", diff --git a/site/src/components/FileUpload/FileUpload.tsx b/site/src/components/FileUpload/FileUpload.tsx index c8d29220ad..534fc05a01 100644 --- a/site/src/components/FileUpload/FileUpload.tsx +++ b/site/src/components/FileUpload/FileUpload.tsx @@ -61,14 +61,11 @@ export const FileUpload: FC = ({ extension, fileTypeRequired, }) => { - const inputRef = useRef(null); const tarDrop = useFileDrop(onUpload, fileTypeRequired); - - const clickable = useClickable(() => { - if (inputRef.current) { - inputRef.current.click(); - } - }); + const inputRef = useRef(null); + const clickable = useClickable( + () => inputRef.current?.click(), + ); if (!isUploading && file) { return ( diff --git a/site/src/hooks/useClickable.ts b/site/src/hooks/useClickable.ts index f9d065cabe..0ea4d7115d 100644 --- a/site/src/hooks/useClickable.ts +++ b/site/src/hooks/useClickable.ts @@ -10,44 +10,62 @@ type ClickableElement = { click: () => void; }; -export interface UseClickableResult< - T extends ClickableElement = ClickableElement, -> { - ref: RefObject; +/** + * 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 + * 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; tabIndex: 0; - role: "button"; - onClick: MouseEventHandler; - onKeyDown: KeyboardEventHandler; -} + role: TRole; + onClick: MouseEventHandler; + onKeyDown: KeyboardEventHandler; + onKeyUp: KeyboardEventHandler; +}>; /** - * Exposes props to add basic click/interactive behavior to HTML elements that - * don't traditionally have support for them. + * Exposes props that let you turn traditionally non-interactive elements into + * buttons. */ export const useClickable = < - // T doesn't have a default type on purpose; the hook should error out if it - // doesn't have an explicit type, or a type it can infer from onClick - T extends ClickableElement, + TElement extends ClickableElement, + TRole extends ClickableAriaRole = ClickableAriaRole, >( - // Even though onClick isn't used in any of the internal calculations, it's - // still a required argument, just to make sure that useClickable can't - // accidentally be called in a component without also defining click behavior - onClick: MouseEventHandler, -): UseClickableResult => { - const ref = useRef(null); + onClick: MouseEventHandler, + role?: TRole, +): UseClickableResult => { + const ref = useRef(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 === " ") { - // Can't call onClick from here because onKeydown's keyboard event isn't - // compatible with mouse events. Have to use a ref to simulate a click + if (event.key === "Enter") { + ref.current?.click(); + event.stopPropagation(); + } + }, + onKeyUp: (event) => { + if (event.key === " ") { ref.current?.click(); event.stopPropagation(); } diff --git a/site/src/hooks/useClickableTableRow.test.tsx b/site/src/hooks/useClickableTableRow.test.tsx new file mode 100644 index 0000000000..cab5e8833a --- /dev/null +++ b/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 = + Readonly< + PropsWithChildren<{ + as?: Exclude; + role?: ClickableAriaRole; + onInteraction: MouseEventHandler; + }> + >; + +const NonNativeButton: FC> = ({ + as, + onInteraction, + children, + role = "button", +}) => { + const clickableProps = useClickable(onInteraction, role); + const Component = as ?? "div"; + return {children}; +}; + +describe(useClickable.name, () => { + it("Always defaults to role 'button'", () => { + render(); + 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( + + {anchorText} + , + ); + + 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( + , + ); + + 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(, { + wrapper: ({ children }) => ( + <> + + {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(); + + 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(); + + // 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(); + + // 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(, { + wrapper: ({ children }) => ( + <> + {children} + + + ), + }); + + // 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(); + }); +}); diff --git a/site/src/hooks/useClickableTableRow.ts b/site/src/hooks/useClickableTableRow.ts index 95a20b8597..106dd43ebd 100644 --- a/site/src/hooks/useClickableTableRow.ts +++ b/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 { useClickable, type UseClickableResult } from "./useClickable"; +import { type CSSObject, useTheme } from "@emotion/react"; -type UseClickableTableRowResult = UseClickableResult & +import { + type ClickableAriaRole, + type UseClickableResult, + useClickable, +} from "./useClickable"; +import { type TableRowProps } from "@mui/material/TableRow"; + +type UseClickableTableRowResult< + TRole extends ClickableAriaRole = ClickableAriaRole, +> = UseClickableResult & TableRowProps & { css: CSSObject; hover: true; @@ -11,24 +33,28 @@ type UseClickableTableRowResult = UseClickableResult & }; // Awkward type definition (the hover preview in VS Code isn't great, either), -// but this basically takes all click props from TableRowProps, but makes -// onClick required, and adds an optional onMiddleClick -type UseClickableTableRowConfig = { +// but this basically extracts all click props from TableRowProps, but makes +// onClick required, and adds additional optional props (notably onMiddleClick) +type UseClickableTableRowConfig = { [Key in keyof TableRowProps as Key extends `on${string}Click` ? Key - : never]: UseClickableTableRowResult[Key]; + : never]: UseClickableTableRowResult[Key]; } & { + role?: TRole; onClick: MouseEventHandler; onMiddleClick?: MouseEventHandler; }; -export const useClickableTableRow = ({ +export const useClickableTableRow = < + TRole extends ClickableAriaRole = ClickableAriaRole, +>({ + role, onClick, - onAuxClick: externalOnAuxClick, onDoubleClick, onMiddleClick, -}: UseClickableTableRowConfig): UseClickableTableRowResult => { - const clickableProps = useClickable(onClick); + onAuxClick: externalOnAuxClick, +}: UseClickableTableRowConfig): UseClickableTableRowResult => { + 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); }, }; }; diff --git a/site/src/pages/WorkspacesPage/WorkspacesTable.tsx b/site/src/pages/WorkspacesPage/WorkspacesTable.tsx index 90d071c501..f5a43babc2 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesTable.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesTable.tsx @@ -255,7 +255,6 @@ const WorkspacesRow: FC = ({ 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 = ({ } }, }); + const bgColor = checked ? theme.palette.action.hover : undefined; + return (