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

Add accessible text to social icons #5000

Merged
merged 2 commits into from Oct 5, 2018

Conversation

Projects
None yet
3 participants
@hidde
Contributor

hidde commented Sep 24, 2018

the focusable=none attribute is used so that in IE11, the icon does not get focus

Signed-off-by: Hidde de Vries hidde@hiddedevries.nl

Add accessible text to social icons
Signed-off-by: Hidde de Vries <hidde@hiddedevries.nl>

@schalkneethling schalkneethling self-requested a review Oct 1, 2018

@schalkneethling schalkneethling self-assigned this Oct 1, 2018

@schalkneethling

Made a suggestion for your consideration.

@@ -1,4 +1,4 @@
<svg class="icon icon-facebook" xmlns="http://www.w3.org/2000/svg" width="16" height="28" viewBox="0 0 16 28">
<svg class="icon icon-facebook" xmlns="http://www.w3.org/2000/svg" width="16" height="28" viewBox="0 0 16 28" aria-label="Facebook" role="img" focusable="false">

This comment has been minimized.

@schalkneethling

schalkneethling Oct 2, 2018

Collaborator

Thank you for your contribution @hidde - I recently read this article to understand SVG and a11y better: https://developer.paciellogroup.com/blog/2013/12/using-aria-enhance-svg-accessibility/

Based on that, I wonder if it makes sense to instead do aria-labelledby="title"

For the Discourse SVG we would then need to add a title and then make the same change. Thoughts?

This comment has been minimized.

@hidde

hidde Oct 2, 2018

Contributor

I think in terms of the accessibility tree, an aria-label and aria-labelledby with an idref would yield the same result: they give the image an accessible name, which conveys alternative text to assistive technologies. The difference should be mostly personal preference.

If you prefer going the aria-labelledby route I am more than happy to update my PR accordingly.

This comment has been minimized.

@schalkneethling

schalkneethling Oct 3, 2018

Collaborator

True that. Seeing we already have the text there in the title tag, let's go with aria-labelledby. Thanks @hidde

This comment has been minimized.

@hidde

hidde Oct 3, 2018

Contributor

Aah, @schalkneethling, apologies, when I just started to fix it, I remembered that I did try and use ID's first, but found a problem with it: some icons are used twice. An example is the Twitter icon, which is used for MDN Twitter and Mozilla Twitter. That would mean the ID would occur twice on the page, and that's invalid HTML.

Would there be a way to make these IDs dynamic, so that we can add unique ones? One dirty yet easy way I could think of if have mdn-twitter and mozilla-twitter svg files, with different IDs in them. But that's extra requests for the users. What do you think?

This comment has been minimized.

@schalkneethling

schalkneethling Oct 3, 2018

Collaborator

If by ids you are referring to the title tag inside the SVG, these are/can be dynamic. There is a Jinja helper[https://github.com/mozilla/kuma/blob/master/kuma/wiki/templatetags/jinja_helpers.py#L252] that does this. So, if aria-labelledby points to the title tag, then there should be no problems. If a string needs to be translated, the above macro will also take care of that.

So, I would say adding aria-labelledby="title" to the root svg element, is what is needed here. Thanks!

This comment has been minimized.

@jwhitlock

jwhitlock Oct 3, 2018

Member

The include_svg Jinja2 macro is used to set localized titles for inlined SVG, such as in the footer:

<li class="footer-social">
<a href="https://twitter.com/mozilla">
{{ include_svg('includes/icons/social/twitter.svg', _('Twitter')) }}
</a>
</li>
<li class="footer-social">
<a href="https://www.facebook.com/mozilla">
{{ include_svg('includes/icons/social/facebook.svg', _('Facebook')) }}
</a>
</li>

It does this by parsing the SVG as XML and modifying the title:

@library.global_function
@lru_cache.lru_cache()
def include_svg(path, title=None):
"""Embded an SVG file by path, optionally changing the title."""
svg = loader.get_template(path).render()
if (title):
svg_parsed = pq(svg, namespaces={'svg': 'http://www.w3.org/2000/svg'})
svg_parsed('svg|title')[0].text = title
svg_out = svg_parsed.outerHtml()
else:
svg_out = svg
return jinja2.Markup(svg_out)

Similar magic could be used to modify the id value.

This comment has been minimized.

@schalkneethling

schalkneethling Oct 4, 2018

Collaborator

@hidde @jwhitlock Now this id bit is starting to bother me(I know, I suggested it ;)) a little. We will have to be very careful to ensure we do not get duplicate ids on a page.

If we for example have two or three instances of the same icon on the same page, it seems likely that we are going to have a problem where two have the same id.

Perhaps it will be better to specify the accessible title explicitly with aria-label, setting it to the same value as the title element. Seems a bit more sane, and less error prone. That way aria-label also inherits the benefit of being localised where appropriate.

So the tl;dr - I suggest we:

  1. Stick with using aria-label
  2. Add a title element to the Discourse SVG for consistency
  3. Post merge, I can update the helper to also update the aria-label

Thoughts?

This comment has been minimized.

@jwhitlock

jwhitlock Oct 4, 2018

Member

aria-label makes sense to me, even with duplication. Unique ids would be manageable when used for footer icons, but not for other uses, like the experimental and obsolete icons in sidebars.

This comment has been minimized.

@hidde

hidde Oct 4, 2018

Contributor

Sounds good to me, I will add the title when I'm at my computer.

This comment has been minimized.

@hidde

hidde Oct 5, 2018

Contributor

I've just added it.

@schalkneethling

Looks great, thx @hidde r+

@schalkneethling schalkneethling merged commit 947104d into mozilla:master Oct 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No new, high severity issues
Details

@hidde hidde deleted the hidde:a11y-text-social-icons branch Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment