-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(exoflex): support a11y for radio #455
Conversation
let handlePress = () => | ||
contextOnValueChange | ||
? contextOnValueChange(value) | ||
: onPress(!isChecked, value); // NOTE: `onPress` should returns nothing, so this behaviour should be changes in v4 | ||
|
||
return ( | ||
<TouchableOpacity | ||
{...otherProps} | ||
accessibilityLabel={accessibilityLabel || `Radio Item: ${label}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if the colon (:
) would be disruptive when the label is read or is it just not read at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, that's true! The VoiceOver might just read it literally as "colon".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's so, let's omit them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After I tried it on Android Emulator, the VoiceOver doesn't read the "colon" at all.
The VoiceOver said, "Radio Item: Male, Radio Button".
So the first "Radio Item: Male" is from the accessibilityLabel
and the last "Radio Button" is coming from the accessibilityRole
which we use radio
for mobile.
From that discovery, I think it would be better if we only use the label
prop for accessibilityLabel
. But if we do that, probably it will "Male Button" on the web as we use button
instead of radio
for the accessibilityRole
.
What do you think @danielsukmana?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are only two choices:
- We create separate a11y labels for both mobile and web.
- We won't provide default a11y labels and just let the user set it up on their own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, at this point, I think the second choice is more reasonable. We will just provide a default a11y role and state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Male Button" doesn't seem to be very clear on what it is for. I agree, let's go with option 2.
The reason I use |
Add a11y support for RichRadio and Radio component