Update SvgIcon for the latest axe-core #540
Conversation
e9444b1 to
f226bd5
Compare
Codecov Report
@@ Coverage Diff @@
## main #540 +/- ##
==========================================
- Coverage 91.10% 91.07% -0.04%
==========================================
Files 21 21
Lines 281 280 -1
Branches 81 81
==========================================
- Hits 256 255 -1
Misses 14 14
Partials 11 11
Continue to review full report at Codecov.
|
|
Don't know why codecov thinks there's -0.04% less test coverage. |
There was a problem hiding this comment.
size-limit report
| Path | Size |
|---|---|
| components | 12.71 KB (+1.43% 🔺) |
| styles | 3.81 KB (0%) |
| children: ( | ||
| <> | ||
| Link with icon <CheckCircle type="decorative" /> | ||
| </> | ||
| ), |
There was a problem hiding this comment.
Thanks for updating this! That heart icon looked seriously fugly 😅
| <svg {...svgCommonProps} role="img"> | ||
| <title>{props.title}</title> |
There was a problem hiding this comment.
I kinda wonder if we should use an aria-label and drop the role + title for informative icons for consistency. It feels a little weird to me that one version of the svgs has a role and the other doesn't. 🤷
There was a problem hiding this comment.
One benefit of <title> (which our auditors request sometimes) is it'll appear when hovering on the icon, in case it's not clear what an icon means. I do generally like aria-label better, but that could be a reason to keep this
There was a problem hiding this comment.
Weird that it doesn't allow role="presentation" on svgs, but I agree that aria-hidden makes more sense anyway.
Re: the API change, I think it would be a little misleading to have consumers pass in a role that won't actually result in a role attribute on the element, but I also feel like type is really vague. Maybe something like "purpose"? I also like the idea of switching from "img" to "informative" and from "presentation" to "decorative" because I think that will make it easier for people to reason about which category any particular icon falls into.
|
Good point that I'll change it to |
Instead of the presentation role. After upgrading axe-core, we're now getting failures for presentation not being a valid role for <svg> elements. I'm not 100% sure that's the case. But we can see that axe-core has an allowlist for what elements can have that role [1], and svg is not on it. There doesn't seem to be documentation anywhere that it's explicitly not allowed. But this change has the same effect, and plays nicely with axe. Also, MDN lists some interesting differences between role="presentation" and aria-hidden [2]. aria-hidden is for literally hiding something from assistive technology, while role="presentation" only strips an element of any semantics. Hiding it in this case seems reasonable and what we actually intend. 1. https://github.com/dequelabs/axe-core/blob/2777dbc20c028d88a906e49a3c d7032b482f0988/lib/commons/aria/index.js#L1462-L1488 2. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_ Techniques/Using_the_aria-hidden_attribute
Instead of "img" and "presentation". Now that we're using aria-hidden instead of role="presentation", our "role" prop is decoupled from the actual HTML attributes anyway. I figured we may as well complete the decoupling and not even pretend anymore like we're using an HTML/ARIA role. The names "informative" and "decorative" also may better indicate their use cases.
f226bd5 to
2d165c0
Compare
There was a problem hiding this comment.
size-limit report
| Path | Size |
|---|---|
| components | 14.47 KB (+1.37% 🔺) |
| styles | 3.81 KB (0%) |
"kind" is kind of generic, and "purpose" better describes the... purpose... of the prop.
2d165c0 to
aadd789
Compare
|
Thanks! Merging (not sure what's up with codecov) |
|
FYI, since dequelabs/axe-core#3124 axe will now allow svg elements to have |
Summary:
This PR
aria-hiddeninstead ofpresentation="role"for presentational/decorative icons (will explain more below)roleprop withtypepurpose(will explain more below)Wat, why?
Grab a cup of coffee (or your flavored beverage of choice) and let me tell you the story of why I made all these changes 📖
Chapter 1 - More upgrades, more problems
All I wanted to do was update axe-core (213e146), but that caused some axe testing failures. Looks like axe-core's allowlist of elements that can have
role="presentation"does not include<svg>.I can't find any documentation anywhere that svg elements shouldn't have that role. So I don't really agree that it should be forbidden.
Chapter 2 - Embrace the tools
But mdn has some interesting docs on when to use aria-hidden vs presentation="role", and our use case for icons seems closer to aria-hidden to me (completely remove from the accessibility tree).
So I changed SvgIcon to use aria-hidden when they're presentational/decorative (2e7650f).
Chapter 3 - Words have meaning, I think
An interesting ramification of that is now the
roleprop is decoupled from what actually happens on the HTML svg element. Meaning,role="presentation"would no longer result in the svg element literally having that role.So I got to thinking (always dangerous) why not embrace that decoupling? And I renamed the prop from
roletopurpose(thank you Diedra for the prop name idea), and changed its possible values to "informative" and "decorative" (f226bd5).Those words may still be kind of fuzzy, but they could better indicate the purpose of them. An "informative" icon has information to communicate. A decorative one is for decoration - it's extraneous.
If you've made it this far, you can tell I'm way over-analyzing this. But over-analyzing is half the fun.
Test Plan: