-
Notifications
You must be signed in to change notification settings - Fork 4
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
cu: 5779 Audit VABulletList for a11yLabel usage #9736
Conversation
d29b1cb
to
0a491ef
Compare
<TextView {...textViewProps} accessibilityLabel={a11yLabel || text}> | ||
<TextView | ||
{...textViewProps} | ||
accessibilityLabel={getBulletListTextAccessibilityLabel({ |
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.
Instead of generating an a11yLabel by concatenating the various props, would it work to only use the a11yLabel when it's provided, and otherwise not use an a11yLabel at all? If VoiceOver/TalkBack can read the text correctly without an a11yLabel, I think that approach would be simplest. So we'd just have:
<TextView {...textViewProps} accessibilityLabel={a11yLabel}>
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.
Thanks @alexandec in short yes and it was the way I initially approached the change. But changed it after issues with tests.
It seems that while running snapshot tests the accessibilityLabel
does not get auto aggregated from the children of the text and causes some tests to fail. so In order for the test to succeed I had 2 options.
- Manually generate the label from the various props so that all tests sucesfully assert the values.
- Modify the Tests that fail to not check using
getByLabelText
as it is no longer fallbacks totext
We can use the simple <TextView {...textViewProps} accessibilityLabel={a11yLabel}>
if we modify some tests to check for getByText
instead of getByLabelText
0a491ef
to
e472cdb
Compare
@juancstlm-a6 cool, looks like you made the change. I don't mind changing the tests slightly, because we get the benefit of a simpler component. Looks clean 👍 One other thing that would be useful to include in this PR is a video with the screen reader so we can hear that the announcements are still correct after the change. The video doesn't have to include all the modified components, just one is enough to get a sense. |
49f6b47
to
2b16f8a
Compare
Added video of the accessibility helper reading out the text |
Seems there is document built failiure due to the |
@juancstlm-a6 looks good! I approved and moved this to With QA |
Nope, we don't directly have any icons in the design system section of the doc site. Just to document: seems it was related to Dylan's icon work transitioning over to design system icons and removing the icons without cleaning up the section referencing them in the doc site. |
2b16f8a
to
a54758c
Compare
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.
The only failing detox test (Android, messages) is the URL one that's also failing in develop. Approved to merge.
06b8856
to
72b062d
Compare
Description of Change
accessibilityLabel
is only set whena11yLabel
is specified otherwise it is automatically created from traversing the children elements. (although not the case in tests)a11yLabel
a11yLabel
whena11yLabelVA()
is used to maintain accesibilityaccessibilityLabel
not being generated and changing some usages ofgetByLabelText
togetByText
Screenshots/Video
Kapture.2024-10-01.at.10.12.33.mp4
Testing
Tested a single instance of the change
LoaGate
. All affected pages need to be checked.Remaining
Dev Test Plan
Reviewer Validations
PR Checklist
Reviewer: Confirm the items below as you review
For QA
Run a build for this branch