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

Toolbar icons need accessible names #1349

Closed
LJWatson opened this issue Apr 9, 2017 · 19 comments
Closed

Toolbar icons need accessible names #1349

LJWatson opened this issue Apr 9, 2017 · 19 comments
Labels
accessibility Screen readers and related bug Something isn't working expertise wanted Extra expertise is needed for implementation fit for beginners Low-hanging fruit ui Front-end, design

Comments

@LJWatson
Copy link

LJWatson commented Apr 9, 2017

The toolbar icons ("Getting started", "Local timeline", "Federated timeline", "Preferences", "Logout") do not have proper accessible names. For example:

<a title="Getting started" class="drawer__tab" href="/web/getting-started">
<i class="fa fa-fw fa-asterisk"></i>
</a>

This means that in some circumstances the purpose of the link is not being announced by screen readers. The problem is that the <i> element is the only content of the link, and it has no actual content for screen readers to use.

Note: When using the tab key to navigate the links, the title attribute does provide the necessary information. However, the title attribute should not be used in this context.

The solution is to remove the title attribute (to avoid the previous problem, and to avoid duplication with the correct solution), and use ARIA to enable screen readers to recognise the <i> element. For example:

<a class="drawer__tab" href="/web/getting-started">
<i role="img" aria-label="Getting started" class="fa fa-fw fa-asterisk"></i>
</a>
@wxcafe wxcafe added bug Something isn't working fit for beginners Low-hanging fruit expertise wanted Extra expertise is needed for implementation ui Front-end, design labels Apr 9, 2017
@wxcafe
Copy link
Contributor

wxcafe commented Apr 9, 2017

Can you submit a PR for this?

@LJWatson
Copy link
Author

LJWatson commented Apr 9, 2017

@wxcafe
I would, but I don't use React.js myself, sorry.

@AntonioVdlC
Copy link

I can work on this if nobody else is! 🙂

@wxcafe
Copy link
Contributor

wxcafe commented Apr 9, 2017 via email

@fvsch
Copy link

fvsch commented Apr 10, 2017

I would recommend keeping the title attribute as well, since it was probably intended to make the interface more usable (at least for some mouse/trackpad users).

<a title="Getting started" class="drawer__tab" href="/web/getting-started">
  <i role="img" aria-label="Getting started" class="fa fa-fw fa-asterisk"></i>
</a>

@IanPouncey
Copy link

The problem with using title to provide necessary information is that it only works for pointing device users. People using a keyboard, touch screen, voice input, etc. will still not be able to discover what the link is for.

When icons are used but their meaning is not clear they should be accompanied by text to provide a usable experience. Beyond the need for an accessible name, this is a design problem as much as a technical implementation problem.

Note that the small screen size icon links do not have a title attribute but also lack an accessible name.

@jpdevries
Copy link
Contributor

jpdevries commented Apr 10, 2017

I noticed this as well. Made a quick VoiceOver GIF showing the issue

GIF GIF showing inaccessible tooltips on VoiceOver

I'm with @fvsch on it is OK to keep title as long as there is an accessible attribute like aria-label as well. I have noticed that some assistive technology would complete ignore the <a><i></i></a> pattern because they see that element as semantically "empty" which is why a possibly safer pattern would be

<a title="Getting started" class="drawer__tab" href="/web/getting-started">
  <i role="img" class="fa fa-fw fa-asterisk"></i>
  <span class="visually-hidden">Getting started</span>
</a>

http://a11yproject.com/posts/how-to-hide-content/

UPDATE
Upon further review, as mentioned in the issue description, as long as aria-label is present AT will not ignore the <i>

@LJWatson
Copy link
Author

It really isn't a good idea to use the title attribute in this context. This article on using the HTML title attribute (written by one of the editors of the HTML5 specification) explains why.

The code suggested by @jpdevries has a few problems:

<a title="Getting started" class="drawer__tab" href="/web/getting-started">
  <i role="img" class="fa fa-fw fa-asterisk"></i>
  <span class="visually-hidden">Getting started</span>
</a>

If you use role="img" on the <i> element without giving it an accessible name, the browser will treat it like an <img> element without an alt attribute.

If you visually hide the text inside the <a> element, you stop it being usable by sighted people who need it to correctly interpret the purpose of the icon.

If you use the title attribute on the <a> element, you encounter all the problems described with the title attribute. Plus you are applying an accessible name to the wrong element (it needs to be on the <i> element, not the <a> element.

So taking @IanPouncey's comment into account, the best code to use would be:

<a class="drawer__tab" href="/web/getting-started">
  <i role="img" aria-label="" class="fa fa-fw fa-asterisk"></i>
Getting started
</a>

jpdevries added a commit to jpdevries/mastodon that referenced this issue Apr 10, 2017
Closes mastodon#1349

This is my first PR and I’m only checking in the source JSX file.
Please let me know if it should be checked in after being built also.
@jpdevries
Copy link
Contributor

jpdevries commented Apr 10, 2017

Thanks @LJWatson! That is all really good to know. I submitted a PR that adds aria-label and role to the <i>. While it is not the best code, it seemed the most likely to be merged. Design wise, the toolbar icons don't seem to have room to visually show text along with the icon for each action, and I'm assuming they'll want the inaccessible "title" mouse only tooltip to stay.

@jpdevries
Copy link
Contributor

jpdevries commented Apr 11, 2017

While there isn't enough room to display text next to the toolbar icons inline, what if we displayed the title of the active icon beneath the toolbar?

Perhaps aria-labelledby could be used to associate the icons and their associated description below? Either way it might help sighted users, particularly sighted keyboard users, understand where they are in the app more.

And maybe there could be a preference for whether or not to "Use Descriptive Buttons".

Screenshot showing Federated Icon selected. Below the horizontal row of the 5 icons centered text reads Federated Icons

@jessebeach
Copy link

jessebeach commented Apr 13, 2017

The solution is to remove the title attribute (to avoid the previous problem, and to avoid duplication with the correct solution), and use ARIA to enable screen readers to recognise the <i> element.

@LJWatson What's your opinion on putting that aria-label on the control element, in this case the <a href>, instead of the <i> element? I tend to prefer this over labelling icons, because in cases where the icon is presented in addition to text content, the icon will result in duplication in the label. Putting the aria-label on the control locates the label with the control element it will be associated with in the AX Tree ultimately.

@LJWatson
Copy link
Author

@jessebeach
If there is text content plus the <i> element inside the <a>, then putting aria-label on the <a> makes sense.

An additional thought is that if there is visible text content inside the <a> element, then the <i> might as well have aria-hidden on it instead?

<a class="drawer__tab" href="/web/getting-started" aria-label="Getting started">
<i aria-hidden="true" class="fa fa-fw fa-asterisk"></i>Getting started
</a>

@jessebeach
Copy link

jessebeach commented Apr 13, 2017

An additional thought is that if there is visible text content inside the <a> element, then the <i> might as well have aria-hidden on it instead?

As long as the <i> never has content, it shouldn't produce a node in the AX Tree. I'd stop just short of aria-hidden. Once the Pandora's box is opened, I've seen dev's throw their pattern-matching facilities into high gear and start applying it all over the place =D

@LJWatson
Copy link
Author

I've just realised that these toolbar links control what's displayed in one of the columns... and only because someone sighted pointed it out. Should there be aria-controls on each <a> pointing to the id of that column?

@jessebeach
Copy link

What behavior would that precipitate? Is it forward-looking AT support?

@LJWatson
Copy link
Author

It establishes a relationship between the controller (the link) and the thing being controlled (the column). Its useful when you can't appreciate that relationship visually.

Jaws is the only screen reader with support at present (though hopefully others will follow suit). When focus moves to the control, Jaws indicates that it controls some content and provides a shortcut for navigating straight to the controlled content if you wish.

@jessebeach
Copy link

When focus moves to the control, Jaws indicates that it controls some content and provides a shortcut for navigating straight to the controlled content if you wish

Nice, I did not know this. Thank you for the clarification.

@blackle blackle added the accessibility Screen readers and related label Apr 16, 2017
stephenburgess8 added a commit to stephenburgess8/mastodon that referenced this issue Apr 22, 2017
The drawer tabs which control primary navigation are only labelled by a title which is not available to many screenreaders. Add an aria-label attribute to each link to improve readability with screenreaders. Organize link attributes so link target is first followed by classname.
Issue mastodon#1349
stephenburgess8 added a commit to stephenburgess8/mastodon that referenced this issue Apr 22, 2017
Abstract aria roles such as section should not be used in content. Use non-abstract 'region' aria role instead. That role expects an aria-labelledby attribute with an id. Pass an ID to the column header. Remove the aria-label attribute on the ColumnHeader because the same value is output in plaintext as its child.
Issue mastodon#1349
stephenburgess8 added a commit to stephenburgess8/mastodon that referenced this issue Apr 22, 2017
Columns do not have wrappers, so these icons can't point to a column wrapper which it controls. Instead these icons function as triggers to show or hide individual columns.
mastodon#1349
@stephenburgess8
Copy link
Contributor

Let me know if there are adjustments I could make to this PR. Details in the description. Thanks. ✌️

Gargron pushed a commit that referenced this issue Apr 22, 2017
* feat(aria): Add aria-labels to underlabelled tab nav items

The drawer tabs which control primary navigation are only labelled by a title which is not available to many screenreaders. Add an aria-label attribute to each link to improve readability with screenreaders. Organize link attributes so link target is first followed by classname.
Issue #1349

* feat(aria): Replace abstract aria role of section with region

Abstract aria roles such as section should not be used in content. Use non-abstract 'region' aria role instead. That role expects an aria-labelledby attribute with an id. Pass an ID to the column header. Remove the aria-label attribute on the ColumnHeader because the same value is output in plaintext as its child.
Issue #1349

* fix(aria): Remove aria-controls attribute until solution is found

Columns do not have wrappers, so these icons can't point to a column wrapper which it controls. Instead these icons function as triggers to show or hide individual columns.
#1349

* fix(typo): Remove type of aria-labelledby instead of aria-label
@wxcafe
Copy link
Contributor

wxcafe commented Apr 22, 2017

@LJWatson @stephenburgess8 can we close this issue now that the PR has been merged? :)

Gargron pushed a commit that referenced this issue Apr 25, 2017
Closes #1349

This is my first PR and I’m only checking in the source JSX file.
Please let me know if it should be checked in after being built also.
seefood pushed a commit to Toootim/mastodon that referenced this issue Apr 26, 2017
Closes mastodon#1349

This is my first PR and I’m only checking in the source JSX file.
Please let me know if it should be checked in after being built also.
seefood pushed a commit to Toootim/mastodon that referenced this issue Apr 28, 2017
…2299)

* feat(aria): Add aria-labels to underlabelled tab nav items

The drawer tabs which control primary navigation are only labelled by a title which is not available to many screenreaders. Add an aria-label attribute to each link to improve readability with screenreaders. Organize link attributes so link target is first followed by classname.
Issue mastodon#1349

* feat(aria): Replace abstract aria role of section with region

Abstract aria roles such as section should not be used in content. Use non-abstract 'region' aria role instead. That role expects an aria-labelledby attribute with an id. Pass an ID to the column header. Remove the aria-label attribute on the ColumnHeader because the same value is output in plaintext as its child.
Issue mastodon#1349

* fix(aria): Remove aria-controls attribute until solution is found

Columns do not have wrappers, so these icons can't point to a column wrapper which it controls. Instead these icons function as triggers to show or hide individual columns.
mastodon#1349

* fix(typo): Remove type of aria-labelledby instead of aria-label
seefood pushed a commit to Toootim/mastodon that referenced this issue Apr 28, 2017
Closes mastodon#1349

This is my first PR and I’m only checking in the source JSX file.
Please let me know if it should be checked in after being built also.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Screen readers and related bug Something isn't working expertise wanted Extra expertise is needed for implementation fit for beginners Low-hanging fruit ui Front-end, design
Projects
None yet
Development

No branches or pull requests

9 participants