Skip to content

fix(IconButton): respect consumer-supplied aria-label#1140

Open
munzzyy wants to merge 2 commits into
ClickHouse:mainfrom
munzzyy:fix/iconbutton-aria-label-override
Open

fix(IconButton): respect consumer-supplied aria-label#1140
munzzyy wants to merge 2 commits into
ClickHouse:mainfrom
munzzyy:fix/iconbutton-aria-label-override

Conversation

@munzzyy

@munzzyy munzzyy commented Jul 5, 2026

Copy link
Copy Markdown

Why?

IconButton sets aria-label to the raw icon token ("trash", "sparkle", etc.) and there's no way to override it. The line is placed after {...props} in the render, so if you pass your own aria-label, it gets silently thrown away and screen readers just announce the icon's internal name instead of what the button actually does. Closes #1056.

How?

  • Changed aria-label={iconName} to aria-label={props['aria-label'] ?? iconName} so a consumer's label wins and the icon name is only the fallback
  • Added two tests: one confirming the icon-name fallback still works with no label, one confirming a supplied aria-label now actually sticks
  • Added a changeset (patch bump)

Tickets?

Contribution checklist?

  • You've done enough research before writing
  • You have reviewed the PR
  • The commit messages are detailed
  • The build command runs locally
  • Assets or static content are linked and stored in the project
  • For documentation, guides or references, you've tested the commands

Security checklist?

  • All user inputs are validated and sanitized
  • No usage of dangerouslySetInnerHTML
  • Sensitive data has been identified and is being protected properly
  • Build output contains no secrets or API keys

Preview?

N/A — one-line prop precedence fix, no visual change.

aria-label was set after the {...props} spread, so it always
overrode a consumer-supplied aria-label with the raw icon token
name (e.g. 'trash', 'sparkle'). Screen readers announced the
icon's internal name instead of the button's actual purpose.

Now a consumer aria-label wins, falling back to the icon name
only when none is given. Added tests covering both paths.
@changeset-bot

changeset-bot Bot commented Jul 5, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 26b903f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@CLAassistant

CLAassistant commented Jul 5, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit b84db50. Configure here.

Comment thread src/components/IconButton/IconButton.tsx Outdated
Comment thread src/components/IconButton/IconButton.tsx Outdated
… fallback

The Icon rendered inside IconButton still exposed its own role="img"
aria-label, so assistive tech announced two names for one control. Mark
it aria-hidden, matching how Button and ButtonGroup already hide their
icons.

Also switch the aria-label fallback from ?? to ||, so an explicit empty
aria-label="" falls back to the icon name instead of leaving the button
nameless.
@munzzyy

munzzyy commented Jul 5, 2026

Copy link
Copy Markdown
Author

Pushed a follow-up commit for both review points.

  • The inner Icon is now aria-hidden, matching how Button/ButtonGroup already hide their icons. Before this, the icon's own role="img" aria-label was still exposed alongside the button's aria-label, so assistive tech announced two names for one control.
  • Switched the fallback from ?? to ||, so aria-label="" now falls back to the icon name instead of leaving the button nameless.

Added tests for both. yarn test on IconButton is 11/11 passing, and yarn typecheck / eslint are clean on the changed files.

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.

IconButton hard-codes aria-label to the icon name and ignores a consumer-provided aria-label

2 participants