Skip to content

Commit 0778055

Browse files
committed
fix(webapp): give icon-only tooltip buttons an accessible name
Radix tooltip content is not the accessible name of its trigger, so the icon-only ask-agent button and the two deploy docs links announced as unnamed controls. Name them explicitly and pass asChild so the tooltip trigger stops wrapping them in a second button. Adds a source scan that fails on the next SimpleTooltip with an unnamed or double-wrapped control, with the pre-existing sites baselined.
1 parent eef4131 commit 0778055

4 files changed

Lines changed: 285 additions & 14 deletions

File tree

apps/webapp/app/components/BlankStatePanels.tsx

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -65,24 +65,36 @@ function DeployDocsLinks() {
6565
return (
6666
<>
6767
<SimpleTooltip
68+
asChild
69+
tabbable
6870
button={
69-
<LinkButton
70-
variant="small-menu-item"
71-
LeadingIcon={BookOpenIcon}
72-
leadingIconClassName="text-blue-500"
73-
to={docsPath("deployment/overview")}
74-
/>
71+
// Span wrapper: LinkButton drops the pointer-event props Radix injects via asChild, so
72+
// the tooltip trigger has to be a plain element (same pattern as FavoritePageButton).
73+
<span className="flex">
74+
<LinkButton
75+
variant="small-menu-item"
76+
LeadingIcon={BookOpenIcon}
77+
leadingIconClassName="text-blue-500"
78+
to={docsPath("deployment/overview")}
79+
aria-label="Deploy docs"
80+
/>
81+
</span>
7582
}
7683
content="Deploy docs"
7784
/>
7885
<SimpleTooltip
86+
asChild
87+
tabbable
7988
button={
80-
<LinkButton
81-
variant="small-menu-item"
82-
LeadingIcon={QuestionMarkCircleIcon}
83-
leadingIconClassName="text-blue-500"
84-
to={docsPath("troubleshooting#deployment")}
85-
/>
89+
<span className="flex">
90+
<LinkButton
91+
variant="small-menu-item"
92+
LeadingIcon={QuestionMarkCircleIcon}
93+
leadingIconClassName="text-blue-500"
94+
to={docsPath("troubleshooting#deployment")}
95+
aria-label="Troubleshooting docs"
96+
/>
97+
</span>
8698
}
8799
content="Troubleshooting docs"
88100
/>

apps/webapp/app/components/dashboard-agent/AskAgentButton.tsx

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,23 @@ export function AskAgentButton({
3131
LeadingIcon={AgentIcon}
3232
leadingIconClassName={AGENT_ICON_ACCENT_CLASS}
3333
className={className}
34+
aria-label={iconOnly ? label : undefined}
3435
onClick={() => requestDashboardAgent(prompt)}
3536
>
3637
{iconOnly ? undefined : label}
3738
</Button>
3839
);
3940

40-
return iconOnly ? <SimpleTooltip button={button} content={label} /> : button;
41+
return iconOnly ? (
42+
<SimpleTooltip
43+
asChild
44+
tabbable
45+
// Span wrapper: Button drops the pointer-event props Radix injects via asChild, so the
46+
// tooltip trigger has to be a plain element (same pattern as dashboardAgentLauncher).
47+
button={<span className="flex">{button}</span>}
48+
content={label}
49+
/>
50+
) : (
51+
button
52+
);
4153
}
Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,237 @@
1+
import { readdirSync, readFileSync } from "node:fs";
2+
import path from "node:path";
3+
import { fileURLToPath } from "node:url";
4+
// The repo's `typescript` is the 7.x native port, which has no createSourceFile in JS.
5+
import ts from "typescript-legacy-api";
6+
import type {
7+
Expression,
8+
JsxElement,
9+
JsxSelfClosingElement,
10+
Node as TsNode,
11+
} from "typescript-legacy-api";
12+
import { describe, expect, it } from "vitest";
13+
14+
/**
15+
* Radix tooltip content never becomes the accessible name of its trigger, and a `TooltipTrigger`
16+
* without `asChild` renders its own `<button>` (nesting any interactive child, and dropping it from
17+
* the tab order unless `tabbable`). This scans the JSX for both mistakes so the next one is loud.
18+
*/
19+
20+
const APP_DIR = path.resolve(fileURLToPath(new URL("../../", import.meta.url)));
21+
const WEBAPP_DIR = path.resolve(APP_DIR, "..");
22+
23+
const INTERACTIVE = new Set([
24+
"a",
25+
"button",
26+
"input",
27+
"Button",
28+
"DialogTrigger",
29+
"ExtLink",
30+
"Link",
31+
"LinkButton",
32+
"NavLinkButton",
33+
"PopoverMenuItem",
34+
"PopoverTrigger",
35+
"SelectTrigger",
36+
"SideMenuItemButton",
37+
"TextLink",
38+
]);
39+
40+
/**
41+
* Sites that already violated this before the scan existed. Entries are `<file>::<tag>`; they were
42+
* not reviewed or fixed here. Removing an entry as you fix it is the point — never add one.
43+
*/
44+
const UNNAMED_BASELINE = new Set([
45+
"app/components/metrics/QueryWidget.tsx::Button",
46+
"app/components/navigation/EnvironmentSelector.tsx::PopoverTrigger",
47+
"app/components/navigation/NotificationPanel.tsx::PopoverTrigger",
48+
"app/components/navigation/SideMenu.tsx::PopoverTrigger",
49+
"app/components/primitives/DateTimePicker.tsx::button",
50+
"app/components/primitives/SearchInput.tsx::button",
51+
"app/components/runs/v3/AIFilterInput.tsx::button",
52+
"app/components/runs/v3/TaskRunsTable.tsx::DialogTrigger",
53+
"app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.sessions.$sessionParam/route.tsx::TextLink",
54+
"app/routes/account.tokens/route.tsx::DialogTrigger",
55+
"app/routes/resources.incidents.tsx::PopoverTrigger",
56+
"app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx::TextLink",
57+
]);
58+
59+
const NO_AS_CHILD_BASELINE = new Set([
60+
"app/components/GitMetadata.tsx::LinkButton",
61+
"app/components/code/TSQLResultsTable.tsx::TextLink",
62+
"app/components/integrations/VercelLink.tsx::LinkButton",
63+
"app/components/primitives/CopyButton.tsx::Button",
64+
"app/components/primitives/LabelValueStack.tsx::a",
65+
"app/components/runs/v3/RunTag.tsx::Link",
66+
"app/components/runs/v3/TaskRunsTable.tsx::DialogTrigger",
67+
"app/routes/account.tokens/route.tsx::DialogTrigger",
68+
"app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx::TextLink",
69+
]);
70+
71+
type Violation = { key: string; where: string; problems: string[] };
72+
73+
function tsxFiles(dir: string): string[] {
74+
const out: string[] = [];
75+
for (const entry of readdirSync(dir, { withFileTypes: true })) {
76+
const full = path.join(dir, entry.name);
77+
if (entry.isDirectory()) out.push(...tsxFiles(full));
78+
else if (entry.name.endsWith(".tsx")) out.push(full);
79+
}
80+
return out;
81+
}
82+
83+
type JsxNode = JsxElement | JsxSelfClosingElement;
84+
85+
function tagOf(node: JsxNode): string {
86+
return (ts.isJsxElement(node) ? node.openingElement : node).tagName.getText();
87+
}
88+
89+
function attrOf(node: JsxNode, name: string) {
90+
const open = ts.isJsxElement(node) ? node.openingElement : node;
91+
return open.attributes.properties.find((p) => ts.isJsxAttribute(p) && p.name.getText() === name);
92+
}
93+
94+
/** Text anywhere under the element, ignoring an expression that can render nothing. */
95+
function hasText(node: TsNode): boolean {
96+
if (!ts.isJsxElement(node)) return false;
97+
return node.children.some((child) => {
98+
if (ts.isJsxElement(child) || ts.isJsxSelfClosingElement(child)) return hasText(child);
99+
if (ts.isJsxText(child)) return child.getText().trim().length > 0;
100+
if (ts.isJsxExpression(child)) {
101+
const expression = child.expression;
102+
if (!expression) return false;
103+
if (ts.isConditionalExpression(expression)) {
104+
const empty = (x: TsNode) =>
105+
x.kind === ts.SyntaxKind.NullKeyword ||
106+
(ts.isIdentifier(x) && x.text === "undefined") ||
107+
(ts.isStringLiteral(x) && x.text === "");
108+
return !empty(expression.whenTrue) && !empty(expression.whenFalse);
109+
}
110+
return true;
111+
}
112+
return false;
113+
});
114+
}
115+
116+
function scanFile(file: string, relative: string): Violation[] {
117+
const source = ts.createSourceFile(
118+
file,
119+
readFileSync(file, "utf8"),
120+
ts.ScriptTarget.Latest,
121+
true,
122+
ts.ScriptKind.TSX
123+
);
124+
125+
const locals = new Map<string, Expression>();
126+
const collectLocals = (node: TsNode) => {
127+
if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name) && node.initializer) {
128+
locals.set(node.name.text, node.initializer);
129+
}
130+
ts.forEachChild(node, collectLocals);
131+
};
132+
collectLocals(source);
133+
134+
const resolve = (expression: Expression | undefined, seen = new Set<string>()): Expression[] => {
135+
if (!expression) return [];
136+
if (ts.isParenthesizedExpression(expression)) return resolve(expression.expression, seen);
137+
if (ts.isConditionalExpression(expression)) {
138+
return [...resolve(expression.whenTrue, seen), ...resolve(expression.whenFalse, seen)];
139+
}
140+
if (ts.isIdentifier(expression) && !seen.has(expression.text)) {
141+
seen.add(expression.text);
142+
return resolve(locals.get(expression.text), seen);
143+
}
144+
return [expression];
145+
};
146+
147+
const triggersIn = (root: TsNode): JsxNode[] => {
148+
const found: JsxNode[] = [];
149+
const visited = new Set<TsNode>();
150+
const walk = (node: TsNode) => {
151+
if (visited.has(node)) return;
152+
visited.add(node);
153+
if (ts.isJsxElement(node) || ts.isJsxSelfClosingElement(node)) {
154+
const tag = tagOf(node);
155+
// Overlay panels render elsewhere in the DOM; their controls are not this trigger.
156+
if (tag.endsWith("Content")) return;
157+
if (INTERACTIVE.has(tag)) {
158+
found.push(node);
159+
return;
160+
}
161+
}
162+
// A wrapper's child may be a variable holding the real control.
163+
if (ts.isJsxExpression(node) && node.expression) {
164+
resolve(node.expression).forEach(walk);
165+
return;
166+
}
167+
ts.forEachChild(node, walk);
168+
};
169+
walk(root);
170+
return found;
171+
};
172+
173+
const violations: Violation[] = [];
174+
const visit = (node: TsNode) => {
175+
if (
176+
(ts.isJsxElement(node) || ts.isJsxSelfClosingElement(node)) &&
177+
tagOf(node) === "SimpleTooltip"
178+
) {
179+
const buttonAttr = attrOf(node, "button");
180+
const initializer =
181+
buttonAttr && ts.isJsxAttribute(buttonAttr) ? buttonAttr.initializer : undefined;
182+
if (initializer && ts.isJsxExpression(initializer)) {
183+
const asChild = !!attrOf(node, "asChild");
184+
for (const trigger of resolve(initializer.expression).flatMap(triggersIn)) {
185+
const named =
186+
!!attrOf(trigger, "aria-label") ||
187+
!!attrOf(trigger, "aria-labelledby") ||
188+
!!attrOf(trigger, "title") ||
189+
hasText(trigger);
190+
const problems = [!named && "unnamed", !asChild && "no-asChild"].filter(
191+
(p): p is string => typeof p === "string"
192+
);
193+
if (problems.length) {
194+
const line = source.getLineAndCharacterOfPosition(node.getStart()).line + 1;
195+
violations.push({
196+
key: `${relative}::${tagOf(trigger)}`,
197+
where: `${relative}:${line} <${tagOf(trigger)}>`,
198+
problems,
199+
});
200+
}
201+
}
202+
}
203+
}
204+
ts.forEachChild(node, visit);
205+
};
206+
visit(source);
207+
return violations;
208+
}
209+
210+
const violations = tsxFiles(APP_DIR).flatMap((file) =>
211+
scanFile(file, path.relative(WEBAPP_DIR, file).split(path.sep).join("/"))
212+
);
213+
214+
describe("SimpleTooltip triggers", () => {
215+
it("names every icon-only control it wraps", () => {
216+
const offenders = violations
217+
.filter((v) => v.problems.includes("unnamed") && !UNNAMED_BASELINE.has(v.key))
218+
.map((v) => v.where);
219+
220+
expect(offenders).toEqual([]);
221+
});
222+
223+
it("passes asChild so the trigger does not wrap the control in another button", () => {
224+
const offenders = violations
225+
.filter((v) => v.problems.includes("no-asChild") && !NO_AS_CHILD_BASELINE.has(v.key))
226+
.map((v) => v.where);
227+
228+
expect(offenders).toEqual([]);
229+
});
230+
231+
it("keeps the baselines honest — a listed site must still be found by the scan", () => {
232+
const keys = new Set(violations.map((v) => v.key));
233+
const stale = [...UNNAMED_BASELINE, ...NO_AS_CHILD_BASELINE].filter((key) => !keys.has(key));
234+
235+
expect(stale).toEqual([]);
236+
});
237+
});

apps/webapp/app/components/primitives/Buttons.tsx

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,14 @@ export const Button = forwardRef<HTMLButtonElement, ButtonPropsType>(
438438

439439
type LinkPropsType = Pick<
440440
LinkProps,
441-
"to" | "target" | "onClick" | "onMouseDown" | "onMouseEnter" | "onMouseLeave" | "download"
441+
| "to"
442+
| "target"
443+
| "onClick"
444+
| "onMouseDown"
445+
| "onMouseEnter"
446+
| "onMouseLeave"
447+
| "download"
448+
| "aria-label"
442449
> & { disabled?: boolean; replace?: boolean } & React.ComponentProps<typeof ButtonContent>;
443450
export const LinkButton = ({
444451
to,
@@ -449,6 +456,7 @@ export const LinkButton = ({
449456
download,
450457
disabled = false,
451458
replace,
459+
"aria-label": ariaLabel,
452460
...props
453461
}: LinkPropsType) => {
454462
const innerRef = useRef<HTMLAnchorElement>(null);
@@ -487,6 +495,7 @@ export const LinkButton = ({
487495
onMouseEnter={onMouseEnter}
488496
onMouseLeave={onMouseLeave}
489497
download={download}
498+
aria-label={ariaLabel}
490499
>
491500
<ButtonContent {...props} />
492501
</ExtLink>
@@ -503,6 +512,7 @@ export const LinkButton = ({
503512
onMouseEnter={onMouseEnter}
504513
onMouseLeave={onMouseLeave}
505514
download={download}
515+
aria-label={ariaLabel}
506516
>
507517
<ButtonContent {...props} />
508518
</Link>

0 commit comments

Comments
 (0)