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

EuiIcon a11y #2554

Merged
merged 20 commits into from
Dec 9, 2019
Merged

EuiIcon a11y #2554

merged 20 commits into from
Dec 9, 2019

Conversation

elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Nov 21, 2019

Summary

Closes #1917.

  • In this PR I’m changing the way SVGR compiles the icons by adding a title tag.

  • In the icon.tsx I’m passing a title prop to the SVG. If the prop is empty, by default the title is going to be the name of the icon.

  • When the icon is rendered as an inline SVG this title is passed to the title tag and it's added to the aria-label attribute. When the icon is rendered as an image tag the title is added to the alt attribute.

  • In cases, the aria-hidden is set to true the aria-labelledby or aria-label will be ignored.

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@snide snide requested a review from myasonik November 21, 2019 21:45
@snide
Copy link
Contributor

snide commented Nov 21, 2019

Added @myasonik as a required reviewer on this. He's likely the most expert on this subject. I know that using a camelCase icon name as a default is likely not optimal, but let's be considerate of the upgrade cost of a change like this and understand we likely need some baby steps to get out of it properly. We should consider putting a deprecation schedule tied to making title a required prop at a later date.

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

🎉 Thank you for tackling this!

I think if we can solve the naming thing, that it would actually be a pretty good default for a high amount of cases and we wouldn't have to force developers to provide a title or other label all the time. Though, solving the titling issue might be non-trivial as well...

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

Looks good to merge!

Two remaining issues can be tabled for later if you make issues for them:

  • translating the icon names
  • updating this to use aria-labelledby after SVGR is updated

@chandlerprall chandlerprall self-requested a review November 22, 2019 17:58
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

We also definitely need some sort of callout or section in the docs for providing the correct a11y prop to icons.


@chandlerprall Can you look this over for i18n correctness and the changes to the compile-icons script?

@elizabetdev elizabetdev changed the title Euiicon a11y EuiIcon a11y Nov 26, 2019
@snide snide mentioned this pull request Nov 26, 2019
19 tasks
@elizabetdev elizabetdev requested a review from cchaos November 27, 2019 11:39
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I have tested out all the permutations of adding title vs aria-label vs aria-labelledby vs aria-hidden and they all work correctly. I just have some improvement comments.

@cchaos
Copy link
Contributor

cchaos commented Nov 27, 2019

You will also want to update the summary of the PR since we're no longer using the <title> tag

@elizabetdev
Copy link
Contributor Author

elizabetdev commented Nov 29, 2019

You will also want to update the summary of the PR since we're no longer using the <title> tag

@cchaos we're actually using a title tag. When the SVGR compiles the icons it generates something like:

const EuiIconAlert = ({ title, ...props }) => (
  <svg
    width={16}
    height={16}
    viewBox="0 0 16 16"
    xmlns="http://www.w3.org/2000/svg"
    {...props}>
    <title>{title}</title>
    <path
      fillRule="evenodd"
      d="M7.59 10.059L7.35 5.18h1.3L8.4 10.06h-.81zm.394 1.901a.61.61 0 01-.448-.186.606.606 0 01-.186-.444c0-.174.062-.323.186-.446a.614.614 0 01.448-.184c.169 0 .315.06.44.182.124.122.186.27.186.448a.6.6 0 01-.189.446.607.607 0 01-.437.184zM2 14a1 1 0 01-.878-1.479l6-11a1 1 0 011.756 0l6 11A1 1 0 0114 14H2zm0-1h12L8 2 2 13z"
    />
  </svg>
);

The only issue we had with SVGR is that they don't allow to specify an id for the title tag. So something like aria-labelledby that is the recommended approach wouldn't' work.

The title enhances the SVGs a11y, but because the support isn't uniform we're using the aria-label as a fallback.

@elizabetdev elizabetdev requested a review from cchaos November 29, 2019 13:09
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Just notice that two comments were still not addressed

@elizabetdev elizabetdev requested a review from cchaos December 3, 2019 12:21
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for those last changes. LGTM 👍

@chandlerprall Can you do a final pass?

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM, pulled & tested locally

@elizabetdev elizabetdev merged commit 97119e5 into elastic:master Dec 9, 2019
thompsongl added a commit that referenced this pull request Dec 9, 2019
@elizabetdev elizabetdev mentioned this pull request Jan 23, 2020
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow title, alt on EuiIcon
5 participants