[SvgIcon] Improve accessibility #12822
Conversation
oliviertassinari
added
accessibility
| @@ -52,6 +52,7 @@ function SvgIcon(props) { | |||
| color, | |||
| component: Component, | |||
| fontSize, | |||
| moreChildren, | |||
There was a problem hiding this comment.
@mbrookes Any idea for a better name?
There was a problem hiding this comment.
Not sure - it feels kinda funky having it at all. Does it have any use case besides <desc>? If not, perhaps we should make than an explicit (but optional) prop. Alternatively, could <desc> not be included in children along side the <path>?
There was a problem hiding this comment.
The use case if for the icons exposed in @material-ui/icons. There is a use case for title (if people want to add an id), desc, defs and probably more.
There was a problem hiding this comment.
Is this use case uncommon enough that we can justify letting users decide whether or not to break the pure() optimisation by passing the elements as children to the icon?
oliviertassinari
force-pushed
the
icon-accessibility
branch
from
a3e6d7e to
cb21d40
Compare
docs/src/pages/style/icons/icons.md
Outdated
|
|
||
| ### Semantic SVG Icons | ||
|
|
||
| If your icon has semantic meaning, all you need to do is throw a `titleAccess="meaning"` property. It should be enough but you can always use the `moreChildren` property to add a `desc` description element. |
docs/src/pages/style/icons/icons.md
Outdated
|
|
||
| If your icon has semantic meaning, all you need to do is throw a `titleAccess="meaning"` property. It should be enough but you can always use the `moreChildren` property to add a `desc` description element. | ||
|
|
||
| In the case of focusable interactive elements, like when used with a icon button, you can use the `aria-label` property: |
docs/src/pages/style/icons/icons.md
Outdated
|
|
||
| ### Semantic Font Icons | ||
|
|
||
| If your icons have semantic meaning, you need to provide a text alternative only visible assisitive technologies. |
| @@ -52,6 +52,7 @@ function SvgIcon(props) { | |||
| color, | |||
| component: Component, | |||
| fontSize, | |||
| moreChildren, | |||
There was a problem hiding this comment.
Not sure - it feels kinda funky having it at all. Does it have any use case besides <desc>? If not, perhaps we should make than an explicit (but optional) prop. Alternatively, could <desc> not be included in children along side the <path>?
| @@ -74,10 +75,12 @@ function SvgIcon(props) { | |||
| viewBox={viewBox} | |||
| color={nativeColor} | |||
| aria-hidden={titleAccess ? 'false' : 'true'} | |||
| role="img" | |||
There was a problem hiding this comment.
Shouldn't this be role={titleAccess ? 'img' : 'presentation'}
oliviertassinari
force-pushed
the
icon-accessibility
branch
2 times, most recently
from
cee7cb2 to
f664ca6
Compare
oliviertassinari
dismissed stale reviews from nathanmarks and mbrookes
changed
| // ... | ||
|
|
||
| <Icon>add_circle</Icon> | ||
| <span className="sr-only">Create a user</span> |
There was a problem hiding this comment.
I think that we should add a screanReaderOnly variant to the typography.
oliviertassinari
force-pushed
the
icon-accessibility
branch
from
f664ca6 to
e8401a7
Compare
oliviertassinari
force-pushed
the
icon-accessibility
branch
from
e8401a7 to
2d45474
Compare
* [SvgIcon] Improve accessibility * review
Help with #4489 too