Skip to content

Commit 1d254f4

Browse files
ParkreinerParkreiner
authored
fix: add tests and improve accessibility for useClickable (#12218)
1 parent a827185 commit 1d254f4

File tree

6 files changed

+246
-47
lines changed

6 files changed

+246
-47
lines changed

‎.vscode/settings.json

+1
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@
114114
"Signup",
115115
"slogtest",
116116
"sourcemapped",
117+
"spinbutton",
117118
"Srcs",
118119
"stdbuf",
119120
"stretchr",

‎site/src/components/FileUpload/FileUpload.tsx

+4-7
Original file line numberDiff line numberDiff line change
@@ -61,14 +61,11 @@ export const FileUpload: FC<FileUploadProps> = ({
6161
extension,
6262
fileTypeRequired,
6363
}) => {
64-
const inputRef = useRef<HTMLInputElement>(null);
6564
const tarDrop = useFileDrop(onUpload, fileTypeRequired);
66-
67-
const clickable = useClickable<HTMLDivElement>(() => {
68-
if (inputRef.current) {
69-
inputRef.current.click();
70-
}
71-
});
65+
const inputRef = useRef<HTMLInputElement>(null);
66+
const clickable = useClickable<HTMLDivElement>(
67+
() => inputRef.current?.click(),
68+
);
7269

7370
if (!isUploading && file) {
7471
return (

‎site/src/hooks/useClickable.ts

+44-26
Original file line numberDiff line numberDiff line change
@@ -10,44 +10,62 @@ type ClickableElement = {
1010
click: () => void;
1111
};
1212

13-
export interface UseClickableResult<
14-
T extends ClickableElement = ClickableElement,
15-
> {
16-
ref: RefObject<T>;
13+
/**
14+
* May be worth adding support for the 'spinbutton' role at some point, but that
15+
* will change the structure of the return result in a big way. Better to wait
16+
* until we actually need it.
17+
*
18+
* @see {@link https://www.w3.org/WAI/ARIA/apg/patterns/spinbutton/}
19+
*/
20+
export type ClickableAriaRole = "button" | "switch";
21+
22+
export type UseClickableResult<
23+
TElement extends ClickableElement = ClickableElement,
24+
TRole extends ClickableAriaRole = ClickableAriaRole,
25+
> = Readonly<{
26+
ref: RefObject<TElement>;
1727
tabIndex: 0;
18-
role: "button";
19-
onClick: MouseEventHandler<T>;
20-
onKeyDown: KeyboardEventHandler<T>;
21-
}
28+
role: TRole;
29+
onClick: MouseEventHandler<TElement>;
30+
onKeyDown: KeyboardEventHandler<TElement>;
31+
onKeyUp: KeyboardEventHandler<TElement>;
32+
}>;
2233

2334
/**
24-
* Exposes props to add basic click/interactive behavior to HTML elements that
25-
* don't traditionally have support for them.
35+
* Exposes props that let you turn traditionally non-interactive elements into
36+
* buttons.
2637
*/
2738
export const useClickable = <
28-
// T doesn't have a default type on purpose; the hook should error out if it
29-
// doesn't have an explicit type, or a type it can infer from onClick
30-
T extends ClickableElement,
39+
TElement extends ClickableElement,
40+
TRole extends ClickableAriaRole = ClickableAriaRole,
3141
>(
32-
// Even though onClick isn't used in any of the internal calculations, it's
33-
// still a required argument, just to make sure that useClickable can't
34-
// accidentally be called in a component without also defining click behavior
35-
onClick: MouseEventHandler<T>,
36-
): UseClickableResult<T> => {
37-
const ref = useRef<T>(null);
42+
onClick: MouseEventHandler<TElement>,
43+
role?: TRole,
44+
): UseClickableResult<TElement, TRole> => {
45+
const ref = useRef<TElement>(null);
3846

3947
return {
4048
ref,
41-
tabIndex: 0,
42-
role: "button",
4349
onClick,
50+
tabIndex: 0,
51+
role: (role ?? "button") as TRole,
4452

45-
// Most interactive elements automatically make Space/Enter trigger onClick
46-
// callbacks, but you explicitly have to add it for non-interactive elements
53+
/*
54+
* Native buttons are programmed to handle both space and enter, but they're
55+
* each handled via different event handlers.
56+
*
57+
* 99% of the time, you shouldn't be able to tell the difference, but one
58+
* edge case behavior is that holding down Enter will continually fire
59+
* events, while holding down Space won't fire anything until you let go.
60+
*/
4761
onKeyDown: (event) => {
48-
if (event.key === "Enter" || event.key === " ") {
49-
// Can't call onClick from here because onKeydown's keyboard event isn't
50-
// compatible with mouse events. Have to use a ref to simulate a click
62+
if (event.key === "Enter") {
63+
ref.current?.click();
64+
event.stopPropagation();
65+
}
66+
},
67+
onKeyUp: (event) => {
68+
if (event.key === " ") {
5169
ref.current?.click();
5270
event.stopPropagation();
5371
}

‎site/src/hooks/useClickableTableRow.test.tsx

+154
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
import {
2+
type ElementType,
3+
type FC,
4+
type MouseEventHandler,
5+
type PropsWithChildren,
6+
} from "react";
7+
8+
import { type ClickableAriaRole, useClickable } from "./useClickable";
9+
import { render, screen } from "@testing-library/react";
10+
import userEvent from "@testing-library/user-event";
11+
12+
/**
13+
* Since the point of the hook is to take a traditionally non-interactive HTML
14+
* element and make it interactive, it made the most sense to test against an
15+
* element directly.
16+
*/
17+
type NonNativeButtonProps<TElement extends HTMLElement = HTMLElement> =
18+
Readonly<
19+
PropsWithChildren<{
20+
as?: Exclude<ElementType, "button">;
21+
role?: ClickableAriaRole;
22+
onInteraction: MouseEventHandler<TElement>;
23+
}>
24+
>;
25+
26+
const NonNativeButton: FC<NonNativeButtonProps<HTMLElement>> = ({
27+
as,
28+
onInteraction,
29+
children,
30+
role = "button",
31+
}) => {
32+
const clickableProps = useClickable(onInteraction, role);
33+
const Component = as ?? "div";
34+
return <Component {...clickableProps}>{children}</Component>;
35+
};
36+
37+
describe(useClickable.name, () => {
38+
it("Always defaults to role 'button'", () => {
39+
render(<NonNativeButton onInteraction={jest.fn()} />);
40+
expect(() => screen.getByRole("button")).not.toThrow();
41+
});
42+
43+
it("Overrides the native role of any element that receives the hook result (be very careful with this behavior)", () => {
44+
const anchorText = "I'm a button that's secretly a link!";
45+
render(
46+
<NonNativeButton as="a" role="button" onInteraction={jest.fn()}>
47+
{anchorText}
48+
</NonNativeButton>,
49+
);
50+
51+
const linkButton = screen.getByRole("button", {
52+
name: anchorText,
53+
});
54+
55+
expect(linkButton).toBeInstanceOf(HTMLAnchorElement);
56+
});
57+
58+
it("Always returns out the same role override received via arguments", () => {
59+
const placeholderCallback = jest.fn();
60+
const roles = [
61+
"button",
62+
"switch",
63+
] as const satisfies readonly ClickableAriaRole[];
64+
65+
for (const role of roles) {
66+
const { unmount } = render(
67+
<NonNativeButton role={role} onInteraction={placeholderCallback} />,
68+
);
69+
70+
expect(() => screen.getByRole(role)).not.toThrow();
71+
unmount();
72+
}
73+
});
74+
75+
it("Allows an element to receive keyboard focus", async () => {
76+
const user = userEvent.setup();
77+
const mockCallback = jest.fn();
78+
79+
render(<NonNativeButton role="button" onInteraction={mockCallback} />, {
80+
wrapper: ({ children }) => (
81+
<>
82+
<button>Native button</button>
83+
{children}
84+
</>
85+
),
86+
});
87+
88+
await user.keyboard("[Tab][Tab]");
89+
await user.keyboard(" ");
90+
expect(mockCallback).toBeCalledTimes(1);
91+
});
92+
93+
it("Allows an element to respond to clicks and Space/Enter, following all rules for native Button element interactions", async () => {
94+
const mockCallback = jest.fn();
95+
const user = userEvent.setup();
96+
render(<NonNativeButton role="button" onInteraction={mockCallback} />);
97+
98+
await user.click(document.body);
99+
await user.keyboard(" ");
100+
await user.keyboard("[Enter]");
101+
expect(mockCallback).not.toBeCalled();
102+
103+
const button = screen.getByRole("button");
104+
await user.click(button);
105+
await user.keyboard(" ");
106+
await user.keyboard("[Enter]");
107+
expect(mockCallback).toBeCalledTimes(3);
108+
});
109+
110+
it("Will keep firing events if the Enter key is held down", async () => {
111+
const mockCallback = jest.fn();
112+
const user = userEvent.setup();
113+
render(<NonNativeButton role="button" onInteraction={mockCallback} />);
114+
115+
// Focus over to element, hold down Enter for specified keydown period
116+
// (count determined by browser/library), and then release the Enter key
117+
const keydownCount = 5;
118+
await user.keyboard(`[Tab]{Enter>${keydownCount}}{/Enter}`);
119+
expect(mockCallback).toBeCalledTimes(keydownCount);
120+
});
121+
122+
it("Will NOT keep firing events if the Space key is held down", async () => {
123+
const mockCallback = jest.fn();
124+
const user = userEvent.setup();
125+
render(<NonNativeButton role="button" onInteraction={mockCallback} />);
126+
127+
// Focus over to element, and then hold down Space for 100 keydown cycles
128+
await user.keyboard("[Tab]{ >100}");
129+
expect(mockCallback).not.toBeCalled();
130+
131+
// Then explicitly release the space bar
132+
await user.keyboard(`{/ }`);
133+
expect(mockCallback).toBeCalledTimes(1);
134+
});
135+
136+
test("If focus is lost while Space is held down, then releasing the key will do nothing", async () => {
137+
const mockCallback = jest.fn();
138+
const user = userEvent.setup();
139+
140+
render(<NonNativeButton role="button" onInteraction={mockCallback} />, {
141+
wrapper: ({ children }) => (
142+
<>
143+
{children}
144+
<button>Native button</button>
145+
</>
146+
),
147+
});
148+
149+
// Focus over to element, hold down Space for an indefinite amount of time,
150+
// move focus away from element, and then release Space
151+
await user.keyboard("[Tab]{ >}[Tab]{/ }");
152+
expect(mockCallback).not.toBeCalled();
153+
});
154+
});

‎site/src/hooks/useClickableTableRow.ts

+41-13
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,60 @@
1-
import { useTheme, type CSSObject } from "@emotion/react";
1+
/**
2+
* @file 2024-02-19 - MES - Sadly, even though this hook aims to make elements
3+
* more accessible, it's doing the opposite right now. Per axe audits, the
4+
* current implementation will create a bunch of critical-level accessibility
5+
* violations:
6+
*
7+
* 1. Nesting interactive elements (e.g., workspace table rows having checkboxes
8+
* inside them)
9+
* 2. Overriding the native element's role (in this case, turning a native table
10+
* row into a button, which means that screen readers lose the ability to
11+
* announce the row's data as part of a larger table)
12+
*
13+
* It might not make sense to test this hook until the underlying design
14+
* problems are fixed.
15+
*/
216
import { type MouseEventHandler } from "react";
17+
import { type CSSObject, useTheme } from "@emotion/react";
18+
19+
import {
20+
type ClickableAriaRole,
21+
type UseClickableResult,
22+
useClickable,
23+
} from "./useClickable";
324
import { type TableRowProps } from "@mui/material/TableRow";
4-
import { useClickable, type UseClickableResult } from "./useClickable";
525

6-
type UseClickableTableRowResult = UseClickableResult<HTMLTableRowElement> &
26+
type UseClickableTableRowResult<
27+
TRole extends ClickableAriaRole = ClickableAriaRole,
28+
> = UseClickableResult<HTMLTableRowElement, TRole> &
729
TableRowProps & {
830
css: CSSObject;
931
hover: true;
1032
onAuxClick: MouseEventHandler<HTMLTableRowElement>;
1133
};
1234

1335
// Awkward type definition (the hover preview in VS Code isn't great, either),
14-
// but this basically takes all click props from TableRowProps, but makes
15-
// onClick required, and adds an optional onMiddleClick
16-
type UseClickableTableRowConfig = {
36+
// but this basically extracts all click props from TableRowProps, but makes
37+
// onClick required, and adds additional optional props (notably onMiddleClick)
38+
type UseClickableTableRowConfig<TRole extends ClickableAriaRole> = {
1739
[Key in keyof TableRowProps as Key extends `on${string}Click`
1840
? Key
19-
: never]: UseClickableTableRowResult[Key];
41+
: never]: UseClickableTableRowResult<TRole>[Key];
2042
} & {
43+
role?: TRole;
2144
onClick: MouseEventHandler<HTMLTableRowElement>;
2245
onMiddleClick?: MouseEventHandler<HTMLTableRowElement>;
2346
};
2447

25-
export const useClickableTableRow = ({
48+
export const useClickableTableRow = <
49+
TRole extends ClickableAriaRole = ClickableAriaRole,
50+
>({
51+
role,
2652
onClick,
27-
onAuxClick: externalOnAuxClick,
2853
onDoubleClick,
2954
onMiddleClick,
30-
}: UseClickableTableRowConfig): UseClickableTableRowResult => {
31-
const clickableProps = useClickable(onClick);
55+
onAuxClick: externalOnAuxClick,
56+
}: UseClickableTableRowConfig<TRole>): UseClickableTableRowResult<TRole> => {
57+
const clickableProps = useClickable(onClick, (role ?? "button") as TRole);
3258
const theme = useTheme();
3359

3460
return {
@@ -49,12 +75,14 @@ export const useClickableTableRow = ({
4975
hover: true,
5076
onDoubleClick,
5177
onAuxClick: (event) => {
78+
// Regardless of which callback gets called, the hook won't stop the event
79+
// from bubbling further up the DOM
5280
const isMiddleMouseButton = event.button === 1;
5381
if (isMiddleMouseButton) {
5482
onMiddleClick?.(event);
83+
} else {
84+
externalOnAuxClick?.(event);
5585
}
56-
57-
externalOnAuxClick?.(event);
5886
},
5987
};
6088
};

‎site/src/pages/WorkspacesPage/WorkspacesTable.tsx

+2-1
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,6 @@ const WorkspacesRow: FC<WorkspacesRowProps> = ({
255255

256256
const workspacePageLink = `/@${workspace.owner_name}/${workspace.name}`;
257257
const openLinkInNewTab = () => window.open(workspacePageLink, "_blank");
258-
259258
const clickableProps = useClickableTableRow({
260259
onMiddleClick: openLinkInNewTab,
261260
onClick: (event) => {
@@ -272,7 +271,9 @@ const WorkspacesRow: FC<WorkspacesRowProps> = ({
272271
}
273272
},
274273
});
274+
275275
const bgColor = checked ? theme.palette.action.hover : undefined;
276+
276277
return (
277278
<TableRow
278279
{...clickableProps}

0 commit comments

Comments
 (0)