Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] feat(Icon): add aria-hidden for unlabeled icons #4835

Merged
merged 14 commits into from
Aug 26, 2021

Conversation

evansjohnson
Copy link
Contributor

Fixes #4793

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

set aria-hidden="true" on Icons that don't have a title defined.

this is a change in documented behavior though I think it may be reasonable to view the previous behavior as undesired for accessibility and so okay to change within a major version. previously desc was set to the icon's name which wasn't formatted. perhaps screen readers handle this but I wonder if less-than-or-equal-to is read as lessdashthandashordashequaldashto (any way to check this?). regardless of whether that is handled, I think it's a better default to not announce every icon.

it could be considered to release this as part of v4 to include guidance in the upgrade notes that standalone icons without other text should now provide the title prop to be labeled appropriately

Reviewers should focus on:

opinion on if this is an appropriate thing to do. I started off setting aria-hidden for decorative icons but realized it was almost all of them

Screenshot

Screen Shot 2021-08-04 at 1 56 41 PM

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @evansjohnson! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

this seems like the right direction w/r/t icon aria attributes. there are some more cases we need to consider:

  • icon-only buttons without text/children (need a way to send title to the rendered <Icon>)
  • "double-caret-vertical" icon inside <HTMLSelect> needs a title
  • <KeyCombo> icons probably need titles
  • <TimePicker> arrow icons probably need titles

packages/core/src/components/menu/menuItem.tsx Outdated Show resolved Hide resolved
packages/core/src/components/icon/icon.tsx Outdated Show resolved Hide resolved
packages/core/src/components/breadcrumbs/breadcrumb.tsx Outdated Show resolved Hide resolved
packages/core/src/components/tree/treeNode.tsx Outdated Show resolved Hide resolved
packages/table/src/headers/header.tsx Outdated Show resolved Hide resolved
@evansjohnson
Copy link
Contributor Author

did a pass at the extra cases you mentioned, I did search through every <Icon but went quick so missed at least those and there could be more. would hope to be able to merge with this incremental progress knowing there may be some caught over time.

only one I didn't add for now:

  • "double-caret-vertical" icon inside - I imagine a <select> component is already semantic usage that we can expect a user to understand and the icon we add is decorative but happy to add "Open options" or another title if you could make a decision there?

packages/core/src/components/button/abstractButton.tsx Outdated Show resolved Hide resolved
packages/core/test/icon/iconTests.tsx Outdated Show resolved Hide resolved
@adidahiya adidahiya changed the title aria-hidden update to Icon component [core] feat(Icon): add aria-hidden for unlabeled icons Aug 17, 2021
@adidahiya
Copy link
Contributor

Regarding <HTMLSelect>... I guess I could go either way but slightly lean towards adding the title for now (something like "Open dropdown")

@adidahiya adidahiya merged commit 67c9e2e into palantir:develop Aug 26, 2021
@adidahiya
Copy link
Contributor

adidahiya commented Aug 30, 2021

I am reconsidering one of the suggestions I made earlier:

icon-only buttons without text/children (need a way to send title to the rendered <Icon>)

I believe this turns out to be superfluous. icon-only buttons without text/children should have their a11y labels set with aria-label and/or title, not by labeling the icon. I will make a PR to remove the iconTitle prop.

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.

Callout icons should have aria-hidden="true" to indicate that the SVG is decorative in nature
3 participants