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

fix(icon): remove automatic aria labelling and add a11y guidance #4665

Merged
merged 1 commit into from
Jun 8, 2017

Conversation

jelbourn
Copy link
Member

The automatic application of aria-label for md-icon, in hindsight, was
not a great idea. Neither the ligature string nor the SVG filename are
likely to be meaningful descriptions. Even in cases where they are,
the icon may be used in such a way that the application of a label is
still the wrong thing. On top of that, adding role="img" is usually
not the right approach for an icon, as that role typically refers to
content images, rather than icons that are part of the UI.

Ultimately, it is up to the application developer to add the appropriate
meaning for icons based on how they're used. To this end, we add
guidance to the documentation for what to do in different situations.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 19, 2017
@jelbourn jelbourn added Accessibility This issue is related to accessibility (a11y) pr: needs review and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels May 19, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 19, 2017
the icon is used.

In thinking about accessibility, it is useful to place icon use into one of three categories:
1. **Decorative**: the icon conveys no real semantic meaning and is purely cosmetic.
Copy link
Contributor

Choose a reason for hiding this comment

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

since all 3 of these cases ask the user to add aria-hidden="true", can we just do that for them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hesitant to always add aria-hidden, because the failure mode is that something that should be visible to screen reader users isn't

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can check for a user-added aria-label, aria-labeledby, or role attribute and in those cases we wouldn't set aria-hidden, but if they don't provide any useful description anyways I think its safe to leave it hidden

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to mark as aria-hidden by default based on feedback from Rob Dodson

* The parent `<button>` or `<a>` should either have a meaningful label provided either through
direct text content, `aria-label`, or `aria-labelledby`.

#### Indicator icons
Copy link
Contributor

Choose a reason for hiding this comment

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

to make this easier we could add a simple component like this (it could also automatically add a tooltip):

<md-indicator [message]="msg">
  <md-icon>error</md-icon>
</md-indicator>

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed that this would be useful. Out of scope for this PR, but maybe add eventually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, created an issue to track

Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewseguin andrewseguin added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jun 6, 2017
@andrewseguin
Copy link
Contributor

Needs rebase

@andrewseguin andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Jun 7, 2017
The automatic application of aria-label for md-icon, in hindsight, was
not a great idea. Neither the ligature string nor the SVG filename are
likely to be meaningful descriptions. Even in cases where they *are*,
the icon may be used in such a way that the application of a label is
still the wrong thing. On top of that, adding `role="img"` is usually
not the right approach for an icon, as that role typically refers to
*content* images, rather than icons that are part of the UI.

Ultimately, it is up to the application developer to add the appropriate
meaning for icons based on how they're used. To this end, we add
guidance to the documentation for what to do in different situations.
@andrewseguin andrewseguin merged commit 657f0e8 into angular:master Jun 8, 2017
@jelbourn jelbourn deleted the icon-a11y branch September 13, 2017 04:36
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants