fix: improve ARIA roles, semantic HTML, and touch targets#599
fix: improve ARIA roles, semantic HTML, and touch targets#599
Conversation
Add role="button" to sortable table headers and action badges. Add role="group" to related UI element clusters. Replace non-semantic elements with proper button/main landmarks. Refactor PopoverLink to separate internal/external link handling. Increase touch targets to 44px minimum for mobile accessibility. Extracted from #509. Co-Authored-By: Sebastian <115311276+Roaring30s@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Improves accessibility and mobile usability across the Explorer UI by adding semantic landmarks, refining ARIA usage, and increasing touch target sizes to better satisfy Lighthouse accessibility audits.
Changes:
- Adjusts home page action button sizing/spacing to meet 44×44px touch target guidance and improves icon accessibility.
- Introduces semantic
<main>landmark in the main layout container. - Updates multiple components (OrchestratorList, ExplorerChart, RoundStatus, Drawer, PopoverLink) with ARIA/interaction refinements and link-handling behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pages/index.tsx | Increases touch targets and adds spacing/gaps; hides decorative arrow icons from AT. |
| layouts/main.tsx | Replaces a generic wrapper with a semantic <main> landmark. |
| components/RoundStatus/index.tsx | Adds ARIA adjustments (including tooltip/grouping-related changes) and hides decorative status icons. |
| components/PopoverLink/index.tsx | Refactors internal vs external link rendering and hides decorative chevrons from AT. |
| components/OrchestratorList/index.tsx | Adds ARIA roles to headers/badges and refactors action trigger structure. |
| components/ExplorerChart/index.tsx | Adds role="group" wrappers around chart header clusters. |
| components/Drawer/index.tsx | Updates “Get LPT” trigger to use a button element instead of a text/link construct. |
Comments suppressed due to low confidence (5)
components/OrchestratorList/index.tsx:1063
DropdownMenuTriggerusesasChild, but the childBadgeis only givenrole="button". This won’t be keyboard-focusable unless it’s a real button (or you addtabIndex=0+ key handlers). PreferBadge as="button" type="button"(and style resets) so the trigger is natively interactive.
<DropdownMenuTrigger
onClick={(e) => {
e.stopPropagation();
}}
asChild
>
<Badge
role="button"
size="2"
css={{
cursor: "pointer",
color: "$white",
fontSize: "$2",
}}
components/OrchestratorList/index.tsx:1162
- Same keyboard accessibility issue:
PopoverTrigger asChildwraps aBadgewithrole="button", but it isn’t a native control. Use a real button element for the trigger (e.g.,as="button" type="button") or add the necessary focus/keyboard support.
<PopoverTrigger
onClick={(e) => {
e.stopPropagation();
}}
asChild
>
<Badge
role="button"
size="2"
css={{
cursor: "pointer",
color: "$white",
fontSize: "$2",
}}
>
components/OrchestratorList/index.tsx:1249
- Same pattern here:
DropdownMenuTrigger asChild+Badge role="button"leaves the trigger non-focusable for keyboard users. Prefer a native<button>(viaas="button" type="button") for the trigger element.
<DropdownMenuTrigger
onClick={(e) => {
e.stopPropagation();
}}
asChild
>
<Badge
role="button"
size="2"
css={{
cursor: "pointer",
color: "$white",
fontSize: "$2",
}}
components/OrchestratorList/index.tsx:1332
- Same issue again:
DropdownMenuTrigger asChildexpects an interactive child, butBadgeonly getsrole="button". Use a native button element (or add tabIndex + keyboard handlers) so this control is accessible.
<DropdownMenuTrigger
onClick={(e) => {
e.stopPropagation();
}}
asChild
>
<Badge
role="button"
size="2"
css={{
cursor: "pointer",
color: "$white",
fontSize: "$2",
}}
components/OrchestratorList/index.tsx:386
Badgeis being used as thePopoverTriggerchild withasChild, but onlyrole="button"is added. A non-interactive element withrole="button"is not focusable by default, so keyboard users may not be able to open the popover. Prefer rendering the trigger as a real<button type="button">(e.g.,as="button") and resetting button styles as needed, rather than relying onrolealone.
<Popover>
<PopoverTrigger disabled={isNewlyActive} asChild>
<Badge
role="button"
size="2"
css={{
cursor: !isNewlyActive ? "pointer" : "default",
color: "$white",
fontSize: "$2",
}}
>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| > | ||
| <Box>Orchestrator</Box> | ||
| <Box role="button">Orchestrator</Box> |
There was a problem hiding this comment.
Adding role="button" to the header label doesn’t make the column sortable header keyboard-accessible (the sort handlers are attached to the <Th> via getSortByToggleProps, and the <Box> isn’t focusable). Consider fixing this at the table header level by rendering an actual <button type="button"> inside the <Th>, moving the sort toggle props to that button, and setting appropriate aria-sort.
| <Box role="button">Orchestrator</Box> | |
| <Box>Orchestrator</Box> |
| </Text> | ||
| </Box> | ||
| <ExplorerTooltip | ||
| role="group" | ||
| multiline |
There was a problem hiding this comment.
role="group" is being passed to ExplorerTooltip, which forwards props onto Radix Tooltip.Content. This overrides the content’s default role="tooltip", breaking tooltip semantics for assistive tech. Remove the role prop from ExplorerTooltip and, if grouping is needed, apply role="group" (and an accessible name if appropriate) on a wrapper around the trigger content instead.
| </Flex> | ||
| </ExplorerTooltip> | ||
| <ExplorerTooltip | ||
| role="group" | ||
| multiline |
There was a problem hiding this comment.
Same issue here: role="group" on ExplorerTooltip will be applied to the tooltip content element and can override the required role="tooltip". Drop the role prop on ExplorerTooltip and put grouping semantics on a separate wrapper if needed.
| @@ -171,7 +172,7 @@ const Index = ({ | |||
| }} | |||
There was a problem hiding this comment.
This Box is now rendered as a <button>, but its styles don’t reset default button UI (background, border, padding), which can cause inconsistent appearance across browsers. In this codebase, other Box as="button" usages explicitly set background: "none" and border: "none" (e.g. components/Table/Pagination.tsx:35-45). Consider applying the same reset here.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| } | ||
| > | ||
| <Box>Orchestrator</Box> | ||
| <Box role="button">Orchestrator</Box> |
There was a problem hiding this comment.
Box elements inside ExplorerTooltip were given role="button" but lack keyboard handlers and onClick listeners, making the role semantically incorrect and potentially confusing to screen readers.
| }; | ||
|
|
||
| // For external links, use regular anchor tag to avoid Next.js Link issues | ||
| if (newWindow || href.startsWith("http")) { |
There was a problem hiding this comment.
PopoverLink component incorrectly uses regular anchor tag for internal links when newWindow=true, breaking Next.js routing
Summary
role="button"to sortable table headers and interactive badges in OrchestratorListrole="group"to related UI clusters in ExplorerChart and RoundStatus<Flex>content wrapper with semantic<main>landmark in layout<button>elementaria-hiddenon decorative chevronsExtracted from #509 by @Roaring30s — Lighthouse accessibility and touch target improvements.
Partially addresses #433.
Test plan
<main>landmark is detected by assistive technology🤖 Generated with Claude Code