SVG primitive: Support custom aria-hidden value #21173
Conversation
|
Size Change: +15 B (0%) Total Size: 857 kB
ℹ️ View Unchanged
|
WunderBart
force-pushed
the
update/svg-a11y
branch
from
9d66623 to
a3e7d72
Compare
WunderBart
changed the title
WunderBart
force-pushed
the
update/svg-a11y
branch
from
a3e7d72 to
89b7a6c
Compare
WunderBart
added
[Package] Components
|
I noticed that the React Native implementation does not force the same attributes ( |
| @@ -26,7 +26,9 @@ export const SVG = ( { className, isPressed, ...props } ) => { | |||
| className: | |||
| classnames( className, { 'is-pressed': isPressed } ) || undefined, | |||
| role: 'img', | |||
| 'aria-hidden': 'true', | |||
| 'aria-hidden': props.hasOwnProperty( 'aria-hidden' ) | |||
There was a problem hiding this comment.
Did you consider using a new prop like isFocusable that defaults to false to make it more explicit? ARIA is web-specific, it doesn't exist in React Native.
There was a problem hiding this comment.
My focus for this PR was only the aria-hidden attribute. Nevertheless, if we'd like to make focusable attribute optional with default value set, wouldn't it be better if we move it above the props spread instead of creating custom isFocusable prop? E.g.
export const SVG = ( { className, isPressed, ...props } ) => (
<svg
className={
classnames( className, { 'is-pressed': isPressed } ) || undefined
}
role="img"
aria-hidden="true"
focusable="false"
{ ...props }
/>
);There was a problem hiding this comment.
Yes, it would help in this case. I'm not against this change, I just wanted to raise the awareness of the React Native and all the challenges it creates.
|
Can we just change the order of the object spread so that I believe the idea with those hard-coded values is to serve as smart defaults. But they should be just that: defaults. I don't know that we need be so opinionated that they can't be overridden. |
I think that's a great idea 🙌 I'm not sure how best to handle the Currently: Proposed change to move |
Yes, I believe your assessment here is correct. It should be last, in order to both retain the passed prop value and include additional class names specific to the component. |
|
Sorry, my last comment is wrong. It doesn't need to be last. Since |
|
Related to the ongoing Slack conversation linked in the original comment, it would be good to have demonstrated use-cases for what this is intended to be used for, not simply the fact that it would be needed if there was a hypothetical need to use the Also note that aside from smart defaults, there is a secondary purpose of this component, which is mobile compatibility. My understanding is that usage of the default |
All HTML tags and their attributes aren’t supported by React Native. See my comment: #21173 (comment). We should try to avoid platform specific language in the package containing primitives. |
Ah, right, I think I was making bad interpretations of the intent behind previous efforts of #9685 and #9294. |
|
Thanks for reviewing, @gziolo @aduth @jeryj 🙌
By the time I created this PR we had a use-case for the
instead of just
To achieve that we chose to include the The alternative approach in our case was adding the visually hidden text outside the SVG, e.g.: <span class="visually-hidden">Playing: </span><svg aria-hidden="true" /> [track name]...which we eventually decided to continue with (p1585251445460300-slack-ajax). TL;DR, since we're not going to continue with the |
I'd second this.
The alternative approach with visually hidden text seems better to me, as icons are not appropriate to communicate a state in the first place. I'd agree with @WunderBart's last comment and suggest close. Aside: re: the |
Description
This PR introduces support for providing a custom
aria-hiddenattribute for theSVGprimitive.Currently, the SVG component has the
aria-hiddenvalue fixed totrue. This is fine for decorative icons that are used within a button or a link to other interactive controls. Unfortunately, this prevents us from using ARIA when we need to enhance SVG accessibility. E.g., if we wanted to add the<title>element inside SVG as suggested here, it wouldn't take any effect because of the hardcodedaria-hidden="true"presence.Related discussion: https://wordpress.slack.com/archives/C02RP4X03/p1585161710006700
How has this been tested?
If explicitly specified, the
aria-hiddenprop value should be passed as is to thearia-hiddenSVG attribute. This means that theundefinedwill also be accepted, resulting in the removal of that attribute.undefinedshould be the preferred way over"false"as it's reported to behave inconsistently across browsers.Types of changes
New feature (non-breaking change which adds functionality)
Checklist: