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

SVG icons accessibility #528

Closed
afercia opened this issue Apr 27, 2017 · 7 comments · Fixed by #1012
Closed

SVG icons accessibility #528

afercia opened this issue Apr 27, 2017 · 7 comments · Fixed by #1012
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).

Comments

@afercia
Copy link
Contributor

afercia commented Apr 27, 2017

SVG icons are tricky for accessibility. Some browsers/screen readers combinations announce the SVG <title> (if any) no matter what you try to do to avoid it. Even adding aria-hidden="true" to the icons doesn't work for some combos.

IE 11 + NVDA:

ie11 nvda svg title

IE 11 and AViewer (see the SVG title is part of the calculated accessible name):

ie11 svg title

Are the <title> really needed? Is there any case where we'd want the SVG icons to be announced? I guess the titles could be entirely removed, and when UI controls that use the icons need to be labeled, that could be done on the controls, for example:
<button aria-label="Some label here"><SVG here></button>

@davidakennedy and @samikeijonen could chime in here, they have a lot of experience with SVG icons for all the job done for Twenty Seventeen 🙂

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Apr 27, 2017
@samikeijonen
Copy link
Contributor

At first look buttons (SVGs inside them) definitely need to be announced. At this point aria-label seems the most bulletproof way. This is the approach we used in Twenty17 Social link menu for example.

Okay in there we used screen-reader-class but same end result.

We can look this example more <button aria-label="Some label here"><SVG here aria-hidden="true"></button>. Sorry that I really haven't have time to test at all the new editor for accessibility.

@jasmussen
Copy link
Contributor

I think we might want to move this issue to the dashicons repo instead: https://github.com/WordPress/dashicons

@jasmussen
Copy link
Contributor

This is admittedly an are where I don't have a ton of expertise. But the titles were added initially due to reading this: https://css-tricks.com/accessible-svgs/

I don't proclaim to note that article is wither right nor wrong. But I do know that it would be easy for us to remove the title from the grunt script that builds the dashicons component. But this has to happen upstream in https://github.com/WordPress/dashicons, so I'm closing this one. Feel free to open a ticket there, and I volunteer to shepard it through.

@afercia
Copy link
Contributor Author

afercia commented May 31, 2017

Thanks @jasmussen I've just opened WordPress/dashicons#188
The CSS trick post is basically right, I'd recommend having a look at this post from Léonie Watson to get an idea of all the subtleties needed to make SVGs really accessible: https://www.paciellogroup.com/blog/2013/12/using-aria-enhance-svg-accessibility/

However, as I've tried to explain on WordPress/dashicons#188, that's not what we need in the vast majority of cases.

Reopening this issue because when the title element will be removed upstream, there will still be the need to wrap the SVG icons in some element of your choice (e.g. a span).

Worth considering UI controls made of only icons should be avoided for the reasons mentioned in WordPress/dashicons#188
However, if really, really, needed they should at least have a visible "tooltip" and an aria-label with some meaningful text. So, to recap:

Purely decorative SVGs need to be hidden from assistive technologies:
<span aria-hidden="true">{svg with no title element here}</span>

SVGs that need to be communicated (to avoid):
<span aria-label="some title here" role="img">{svg with no title element here}</span>
(would need also a "tooltip")

@afercia afercia reopened this May 31, 2017
jasmussen added a commit that referenced this issue Jun 3, 2017
This removes the title element from the sprite. This should finally fix #528.
@aduth
Copy link
Member

aduth commented Jun 6, 2017

Reopening per remaining task noted at #1012 (comment)

@aduth aduth reopened this Jun 6, 2017
@mtias
Copy link
Member

mtias commented Aug 18, 2017

@aduth is this concluded?

@aduth
Copy link
Member

aduth commented Aug 18, 2017

Dashicon now applies aria-hidden and no title attribute.

Icons otherwise should be implemented "correctly" by the consuming component. We have some helpers here in case of IconButton which supports adjacent text, or if adjacent text is omitted, defaulting to tooltip as provided by incoming label prop.

In the case of InserterMenu, we're not using IconButton, but provide the full title of the block adjacent the icon, and the icon is otherwise hidden from assistive tools (via aria-hidden on icon).

Or so goes my understanding. All the same, if there's remaining problems, they should be surfaced as individual actionable issues.

@aduth aduth closed this as completed Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants