fix(site): add alt text to Avatar so workspaces images pass WCAG#26223
fix(site): add alt text to Avatar so workspaces images pass WCAG#26223tracyjohnsonux wants to merge 3 commits into
Conversation
Pass through an alt attribute on the underlying AvatarPrimitive.Image so every consumer (workspace rows, filter menus, empty state, batch dialogs) emits an alt-bearing <img>. Default is the empty string, treating the image as decorative because adjacent text always conveys the same information on the workspaces page. Also pass alt="" to the ExternalImage callsites for app icons and batch-delete resource icons. A Storybook play function on the AllStates story asserts every rendered <img> has an alt attribute.
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 2 | Last posted: Round 2, 5 findings (1 P2, 2 P3, 2 Nit), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel (12 reviewers). Netero: 1 P3. Panel: 1 P2, 1 P3, 2 Nit. 0 dropped. Reviewed against a26c46a..0b1d19d. Round 2Panel (4 reviewers). All 5 prior findings verified fixed. 0 new findings. Reviewed against a26c46a..29dafd1. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Clean, well-scoped WCAG fix. The Avatar component-level default is the right call: one prop addition covers every consumer site-wide, and the alt="" decorative pattern is correct given every workspaces-page avatar renders next to its label. The Storybook play function locks the behavior with a real assertion. PR description traces the root cause clearly.
Severity tally: 1 P2, 2 P3, 2 Nit.
The P2 is about applying the same structural fix to ExternalImage that this PR already applies to Avatar. 30 callsites across the codebase are left without alt, and a one-line component-level default would fix them all. The pattern is already established in this PR; it would be consistent to extend it.
Worth noting: Radix AvatarImage only renders the <img> element when the image actually loads (Chopper). In Storybook, mock URLs may not load, so querySelectorAll("img") primarily catches ExternalImage instances rather than Avatar images. This is acceptable because Avatar's fix is structural (default prop), but it means the test's regression coverage is narrower than it appears.
"The PR applies the right fix to Avatar: a component-level
alt=""default that heals every caller at once. Beautiful. Then it turns to ExternalImage and does the opposite: patches two callsites by hand, leaves 11 others untouched." (Hisoka, who undercounted; it's 30.)
🤖 This review was automatically generated with Coder Agents.
| }, | ||
| play: async ({ canvasElement }) => { | ||
| await within(canvasElement).findByText(allWorkspaces[0].name); | ||
| const images = canvasElement.querySelectorAll("img"); |
There was a problem hiding this comment.
P3 [CRF-1] querySelectorAll("img") returns an empty NodeList if no images render (e.g., all Radix Avatar fallbacks, no running workspaces with app icons), and the loop body never executes. The test passes with zero assertions.
"If a future change causes zero
<img>elements to render, the loop body never executes and the test passes with zero assertions." (Netero)
Add a guard before the loop:
expect(images.length).toBeGreaterThan(0);Currently non-vacuous in practice (allWorkspaces includes running workspaces whose MockWorkspaceApp produces an ExternalImage <img> via IconAppLink), but the risk is silent breakage from future mock or component changes.
🤖
There was a problem hiding this comment.
Verified fixed in 29dafd1. expect(images.length).toBeGreaterThan(0) added at line 199, before the loop.
🤖
| onClick={link.onClick} | ||
| > | ||
| <ExternalImage src={app.icon ?? "/icon/widgets.svg"} /> | ||
| <ExternalImage src={app.icon ?? "/icon/widgets.svg"} alt="" /> |
There was a problem hiding this comment.
P2 [CRF-2] ExternalImage should get the same structural alt="" default this PR applies to Avatar, rather than per-callsite patches.
The PR applies a component-level default to Avatar (one change, every caller fixed). For ExternalImage, it patches two callsites by hand, leaving 30 others across 15 files without alt. The component already carries biome-ignore lint/a11y/useAltText: alt should be passed in as a prop, which suppresses the lint that would catch missing callsites.
The fix mirrors what this PR already does for Avatar:
export const ExternalImage: React.FC<React.ComponentPropsWithRef<"img">> = ({
style,
alt = "",
...props
}) => {
// biome-ignore suppression can be removed
...
};One line, 30 callsites healed, biome-ignore removed, and the two per-callsite alt="" additions in this PR become harmless redundancy. (Zoro P2, Hisoka P3, Pariston P4)
🤖
There was a problem hiding this comment.
Verified fixed in 29dafd1. alt = "" destructured at the component level in ExternalImage.tsx:6, biome-ignore suppression removed, per-callsite patches dropped. All ~30 uncovered callsites now inherit the default.
🤖
| {Object.entries(resources).map(([type, summary]) => ( | ||
| <div key={type} className="flex items-center gap-2"> | ||
| <ExternalImage src={summary.icon} width={16} height={16} /> | ||
| <ExternalImage src={summary.icon} alt="" width={16} height={16} /> |
There was a problem hiding this comment.
P3 [CRF-3] This alt="" fix has no test coverage. The AllStates play function scans the workspace list table but never opens the batch-delete dialog, so querySelectorAll("img") never reaches this image (it's two clicks deep in the dialog: consequences -> workspaces -> resources).
"
BatchDeleteConfirmation.stories.tsxexists with mock workspaces andopen: true, but has no play function. If someone removes thisalt, nothing catches the WCAG regression." (Bisky)
If CRF-2 is adopted (component-level default on ExternalImage), this becomes moot since the component itself guarantees alt. Otherwise, consider a play function in the existing BatchDeleteConfirmation story that navigates to the resources stage and asserts alt presence.
🤖
There was a problem hiding this comment.
Verified fixed structurally via CRF-2. Component-level default eliminates per-callsite regression risk; explicit alt="" correctly removed from this callsite.
🤖
| VariantProps<typeof avatarVariants> & { | ||
| src?: string; | ||
| fallback?: string; | ||
| /** |
There was a problem hiding this comment.
Nit [CRF-4] The JSDoc is correct but restates information visible from the prop name (alt) and default (alt = ""). "Accessible text for the underlying <img>" paraphrases what alt means on any <img>. "Defaults to an empty string" restates the visible default.
Suggested trim:
/**
* Alt text for the inner `<img>`. Defaults to `""` (decorative, hidden
* from assistive tech). Pass a descriptive value when no adjacent text
* identifies the content.
*/(Gon)
🤖
There was a problem hiding this comment.
Verified fixed in 29dafd1. JSDoc trimmed to 3 lines.
🤖
| */ | ||
| imgFallbackText?: string; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Nit [CRF-5] Same JSDoc verbosity as CRF-4. "Accessible text for the avatar image rendered by the default avatar" rephrases the prop name.
Suggested trim:
/**
* Ignored when `avatar` is provided. Empty default marks the image as
* decorative because `title` already conveys the same content.
*/(Gon)
🤖
There was a problem hiding this comment.
Verified fixed in 29dafd1. JSDoc trimmed to 2 lines.
🤖
Apply the same component-level alt default to ExternalImage that this PR applies to Avatar, healing the remaining ~30 callsites with one change. Drop the redundant per-callsite alt="" patches in WorkspacesTable and BatchDeleteConfirmation, drop the biome useAltText ignore, trim Avatar and AvatarData JSDoc, and guard the WorkspacesPageView play function against an empty NodeList.
|
/coder-agents-review |
There was a problem hiding this comment.
All 5 round 1 findings addressed in 29dafd1. Four reviewers verified the fixes independently; no new findings.
CRF-2 (P2, the ExternalImage structural fix) was the most impactful change between rounds: the author moved alt="" to the component level, removed the biome-ignore lint/a11y/useAltText suppression, and dropped the now-redundant per-callsite patches. All ~30 previously-uncovered ExternalImage callsites inherit the default. The prop destructuring order is correct (alt extracted before ...props, so callers that pass their own alt get it, others get the decorative default).
CRF-1 (vacuous pass guard) confirmed: expect(images.length).toBeGreaterThan(0) prevents the loop from passing silently on an empty NodeList.
CRF-3 (BatchDeleteConfirmation test gap) closed structurally: the component-level default eliminates the per-callsite regression risk.
CRF-4/CRF-5 (JSDoc verbosity) trimmed to concise versions that communicate the WCAG reasoning without restating the code.
"I tried to build a case against this and couldn't. The problem is correctly understood, the solution is proportional, and the fix is at the right level." (Pariston)
🤖 This review was automatically generated with Coder Agents.
zenithwolf1000
left a comment
There was a problem hiding this comment.
Looks fine, minor nit. Also could add a story to specifically test an alt without default ""
| /** | ||
| * Ignored when `avatar` is provided. Empty default marks the image as | ||
| * decorative because `title` already conveys the same content. | ||
| */ |
There was a problem hiding this comment.
This comment doesn't really convey anything the code doesn't already do so, can drop.
Summary
Fixes the WCAG image-alt failures reported on https://dev.coder.com/workspaces. The audit flagged ~52
<img>elements without analtattribute, all matching the inner<img>rendered by RadixAvatarPrimitive.Imageinside ourAvatarcomponent (selectors like.size-full.object-contain,.size-[--avatar-lg].rounded-[6px],.size-[--avatar-sm]). TwoExternalImagecallsites on the same page were also missingalt.Changes
Avatar: add optionalalt?: stringand forward it toAvatarPrimitive.Image. Default is"", which marks the avatar as decorative and removes it from the accessibility tree. Every callsite on the workspaces page already renders the human-readable name (owner, template, organization, user) as adjacent text, so decorative-by-default is the WCAG-correct behavior. Callers that need a meaningful alt can override.AvatarData: thread an optionalaltthrough to the internal defaultAvatar.WorkspacesTableIconAppLinkExternalImage: passalt="". The wrappingBaseIconLinkalready exposes the app name through ansr-onlyspan on the link.BatchDeleteConfirmationresource iconsExternalImage: passalt="". The resource-type label sits next to each icon.WorkspacesPageView.stories.tsxAllStates: add a play function that scans the rendered canvas and asserts every<img>has analtattribute, to prevent regressions.Validation
pnpm check,pnpm lint,pnpm formatclean.pnpm test -- src/pages/WorkspacesPage/WorkspacesPage.test.tsxpasses (13/13).make pre-commit) passes locally.Implementation plan
Root cause
The
Avatarcomponent (site/src/components/Avatar/Avatar.tsx) renderedAvatarPrimitive.Imagewithout analtattribute. Every consumer (AvatarData,TopbarAvatar, workspace table rows, filter menus, empty state, batch dialogs, "New workspace" dropdown) inherited the missing-alt bug, which is why a single page produced ~52 violations.Fix
Avataraccept analtprop, default"", and forward it to the underlying<img>. Drop-in compatible with every existing call.AvatarDataso callers can label the implicit avatar without composing their own.ExternalImagecallsites as decorative because each is paired with adjacent text.Why
alt=""by defaultAll workspaces-page avatars are rendered next to the corresponding name. Per WCAG, repeating that name in the image's alt text would only add noise for screen-reader users. Empty alt removes the image from the accessibility tree, which is the correct decorative pattern.
PR opened by Coder Agents on behalf of @tracyjohnsonux.