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

Accessibility updates to arrow SVGs #2231

Merged
merged 6 commits into from
Jan 24, 2017
Merged

Accessibility updates to arrow SVGs #2231

merged 6 commits into from
Jan 24, 2017

Conversation

gemfarmer
Copy link
Contributor

Fixes issue(s) #2221

CircleCI

😎 PREVIEW

Changes proposed in this pull request:

  • Updates to our arrow-right and arrow-left icons. They now have role="img" which makes them presentational and not apparent while tabbing on a screenreader. Checked manually with ChromeVoxLite
  • Added sr-only help text near arrows in place of arrow image descriptions.
  • restructured the cards on the homepage. The outer element is no longer an <a>, but has a click handler that follows the link provided. I then added anchors to the title and Read more text
  • Pulls in a few unrelated changes to what is excluded by the ./serve build script. We don't need to include markdown and helper files in our development build!

/cc @maya @nickbristow @coreycaitlin

</g>
<g>
</g>
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" x="0px" y="0px" width="512px" height="512px" viewBox="0 0 444.531 444.531" style="enable-background:new 0 0 444.531 444.531;" xml:space="preserve" role="img">

Choose a reason for hiding this comment

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

For this svg add the attribute aria-hidden="true" so the screen reader won't read the "left arrow" part

<svg ... aria-hidden="true"> 

Copy link
Contributor Author

@gemfarmer gemfarmer Jan 23, 2017

Choose a reason for hiding this comment

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

I tried it out using the ChromeVoxLite just adding role="img" and it seems to avoid the <title> and <desc> text. Are there other screen readers that need aria-hidden?

Choose a reason for hiding this comment

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

Yeah when i used Voiceover it was still reading the arrow.

@@ -35,6 +35,7 @@ h4 {

a {
color: $color-medium;
cursor: pointer;

Choose a reason for hiding this comment

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

The whole card is clickable, I noticed a couple of areas where there wasn't a pointer. The top image and some white space between text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@gemfarmer
Copy link
Contributor Author

@nickbristow I made both of your suggested changes. Let me know if you think this is 👌 to merge!

</g>
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" x="0px" y="0px" width="512px" height="512px" viewBox="0 0 444.531 444.531" style="enable-background:new 0 0 444.531 444.531;" xml:space="preserve" role="img" aria-hidden="true">
<title>Arrow left</title>
<desc>Icon image of an arrow pointing to the left</desc>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to include title and desc tags in the SVG? /cc @nickbristow

Choose a reason for hiding this comment

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

yeah, screen readers can read that. Although putting aria-hidden='true' on them will hide them anyway.

Its kind of a wash. At the very least its there if someone forces a screen reader to read them anyway.

Read more
<span class="usa-sr-only">about {{ include.tagline }}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this in VoiceOver and it's reading it backwards ("about..." is first, then "Read more"):
screen shot 2017-01-23 at 3 19 20 pm

Does "Read more" have to be in a span to be read first, since the other text is in a span? /cc @nickbristow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that's interesting. I'll try doing that and check with VoiceOver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maya mine reads more about ... either way 😕

screen shot 2017-01-23 at 5 54 06 pm

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's interesting! I have no idea why it's doing that then on my machine. 🤷‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could just be on my machine. If you feel motivated to check it with/without the <span> we could determine if it is how one of our machines renders it

<article class="card {{ wds_grid_class }}">
<div class="card-link" onclick='window.location = "{{ include.link }}";' tabindex="-1">
<div role="img"
title="{{ include.image_alt }}"
Copy link
Contributor

@maya maya Jan 24, 2017

Choose a reason for hiding this comment

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

So it seems like you set your alt image tags manually in the post pages, but I just want to mention that you shouldn't be using "Image of..." bc that's redundant as it being an image is already announced to users with the role="img" property. More info here: http://webaim.org/techniques/alttext/

Choose a reason for hiding this comment

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

I believe I've addressed this in 9c36a35, but we should make sure this is noted elsewhere as an accessibility training resource, since I know people are often writing alt-text on the fly (not just for this site, but across 18F).

Choose a reason for hiding this comment

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

j/k, this is covered in the accessibility guide!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 👍 to both of these. @coreycaitlin maybe in our example post we could make sure to phrase the alt text so that we don't get duplication like Maya is talking about??

@coreycaitlin coreycaitlin merged commit 9ce90c1 into dev Jan 24, 2017
@gboone gboone deleted the carat-trouble branch February 15, 2017 20:39
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.

4 participants