Skip to content

fix(a11y): add accessible labels to icon-only controls#50

Merged
mattrothenberg merged 2 commits intocloudflare:mainfrom
msmps:fix/a11y-icon-control-labels
Apr 14, 2026
Merged

fix(a11y): add accessible labels to icon-only controls#50
mattrothenberg merged 2 commits intocloudflare:mainfrom
msmps:fix/a11y-icon-control-labels

Conversation

@msmps
Copy link
Copy Markdown
Contributor

@msmps msmps commented Feb 8, 2026

Description

  • Add aria-labels to icon-only controls in MenuBar and Combobox

Problem

Several icon-only controls were missing explicit accessible labels, so screen readers could announce them as unlabeled buttons.

Controls:

  • MenuBar option buttons (icon-only)
  • Combobox.TriggerInput clear button
  • Combobox.TriggerInput dropdown trigger button
  • Combobox.ChipRemove button in multi-select chips

Solution

Add aria-label to each icon-only control:

  • MenuBar: Uses the tooltip prop as the aria-label (consumer-provided)
  • Combobox.TriggerInput: New clearLabel and showOptionsLabel props with English defaults
  • Combobox.ChipRemove: New removeLabel prop with English default

All labels are customizable for i18n.


  • Reviews
    • bonk has reviewed the change
    • automated review not possible because: accessibility changes require manual screen reader testing
  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows: screen reader testing to verify labels are announced
    • Additional testing not necessary because:

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Feb 26, 2026

npm i https://pkg.pr.new/@cloudflare/kumo@50

commit: ea8bb17

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 26, 2026

Docs Preview

View docs preview

Commit: ea8bb17

Copy link
Copy Markdown
Collaborator

@mattrothenberg mattrothenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make these props with a default values of the english text so that we can ultimately pass a translated string down where we consume this?

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 26, 2026

Visual Regression Report — 6 changed, 7 unchanged

6 screenshot(s) with visual changes:

Combobox / Combobox

185 px (0.18%) changed

Before After Diff
Before After Diff

Combobox / Combobox Sizes

21 px (0.02%) changed

Before After Diff
Before After Diff

Combobox / Combobox Multiple

304 px (0.3%) changed

Before After Diff
Before After Diff

Combobox / Combobox Disabled

246 px (0.24%) changed

Before After Diff
Before After Diff

Combobox / Combobox Error

1,339 px (1.13%) changed

Before After Diff
Before After Diff

Combobox (Open)

0 px (0%) changed

Before After Diff
Before After Diff
7 screenshot(s) unchanged
  • Combobox / Combobox Sizes Searchable Inside
  • Combobox / Combobox Searchable Inside
  • Combobox / Combobox Grouped
  • Combobox / Combobox With Field
  • Menu Bar / Menu Bar Basic
  • Menu Bar / Menu Bar Text Formatting
  • Menu Bar / Menu Bar No Active

Generated by Kumo Visual Regression

Copy link
Copy Markdown
Collaborator

@geoquant geoquant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution — the a11y gaps you identified are real and the fix direction is correct. Merge conflict is now resolved. A few changes needed before this can land:

1. Make labels configurable props (per @mattrothenberg's feedback)

Hardcoded English strings prevent i18n. Each label should be a prop with an English default.

TriggerInput:

function TriggerInput({
  clearLabel = "Clear selection",
  showOptionsLabel = "Show options",
  ...props
}: ComboboxBase.Input.Props & {
  /** Accessible label for the clear button. @default "Clear selection" */
  clearLabel?: string;
  /** Accessible label for the dropdown trigger. @default "Show options" */
  showOptionsLabel?: string;
}) {
  // ...
  <ComboboxBase.Clear aria-label={clearLabel} ...>
  <ComboboxBase.Trigger aria-label={showOptionsLabel} ...>
}

Chip:

function Chip({
  removeLabel = "Remove",
  ...props
}: ComboboxBase.Chip.Props & {
  /** Accessible label for the chip remove button. @default "Remove" */
  removeLabel?: string;
}) {
  // ...
  <ComboboxBase.ChipRemove aria-label={removeLabel} ...>
}

2. Chip label wording

Change "Remove selection""Remove". In chip context, the chip is the selection — "Remove selection" is ambiguous when there are multiple chips.

3. MenuBar — looks good as-is

aria-label={tooltip} on the icon-only button is correct and necessary — the Tooltip component provides aria-describedby (supplementary), not aria-label (name). No change needed here.

@msmps msmps force-pushed the fix/a11y-icon-control-labels branch from 6be6e8b to d0456dd Compare April 10, 2026 11:18
@stritt
Copy link
Copy Markdown
Collaborator

stritt commented Apr 10, 2026

/bonk please review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The changes address all the accessibility concerns and incorporate the feedback from previous reviews.

Summary of changes:

  1. Combobox.TriggerInput - Added clearLabel and showOptionsLabel props with sensible English defaults ("Clear selection", "Show options") and JSDoc comments noting i18n support

  2. Combobox.Chip - Added removeLabel prop with default "Remove" (updated from "Remove selection" per feedback)

  3. MenuBar - Added aria-label={tooltip} to icon-only buttons, which correctly uses the tooltip text as the accessible name

The implementation follows Kumo conventions:

  • Props have descriptive JSDoc with @default values
  • Default strings are translatable via prop overrides
  • Uses semantic tokens and proper compound component patterns

All icon-only controls now have proper accessible labels for screen readers.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 10, 2026

I reviewed PR #50 by fetching the diff and reading the changed files (combobox.tsx, menubar.tsx, and changeset). After verifying the changes addressed all accessibility concerns and incorporated previous reviewer feedback (making labels configurable props with English defaults for i18n support), I submitted an approving review via the GitHub API.

github run

Copy link
Copy Markdown
Collaborator

@stritt stritt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks for making the labels configurable 👍

@mattrothenberg mattrothenberg force-pushed the fix/a11y-icon-control-labels branch from e20e2b5 to ea8bb17 Compare April 14, 2026 15:23
@mattrothenberg mattrothenberg merged commit 460a603 into cloudflare:main Apr 14, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants