fix(a11y): avoid plain divs as button or tabs#6084
Conversation
…te...) Signed-off-by: Cynthia Rey <cynthia@cynthia.dev>
Signed-off-by: Cynthia Rey <cynthia@cynthia.dev>
haileyok
left a comment
haileyok
left a comment
There was a problem hiding this comment.
Thank you! Made a small tweak, will merge after I test it works...
There was a problem hiding this comment.
Might not be something we address in this PR but shouldn't the web implementation use a literal button element rather than relying on an aria role?
There was a problem hiding this comment.
React Native is quite smart here and makes it use a literal button on web (this also applies to the other places where a role has been added).
There was a problem hiding this comment.
Ah, I see. That's great to know 😄
| hoverStyle={pal.viewLight} | ||
| onPress={() => onPressItem(i)}> | ||
| onPress={() => onPressItem(i)} | ||
| accessibilityRole="tab"> |
There was a problem hiding this comment.
Are there any tab panels we can associate with these tabs?
There was a problem hiding this comment.
Ideally the panel views would be associated with the tabs (as tabpanels with aria-labelledby), but I didn't feel confident adding it (and afraid it'd require adapting multiple places where the component is used); so I kept it simple.
FWIW, not associating a tabpanel is not a violation of the WAI-ARIA specification -- it's a should -- and should not be an issue so long the semantics of the page still allow assistive technologies to let the user navigate. It's best practice to associate them, but getting rid of clickable plain divs is already a huge step up. 😅
I think (am pretty sure) there are many accessibility nits and issues throughout the app, so properly doing the association could be handled in future improvements
There was a problem hiding this comment.
Agreed, it's definitely nice to get some semantics in there without having to do a huge refactor 👍 would be nice to eventually have it all hooked up.
There was a problem hiding this comment.
Yea there's definitely a ton of problems that we are slowly fixing up. Lots of debt that has to get cleaned up along the way...these PRs are super helpful in getting that debt cleaned up (I honestly didn't even realize this particular problem!)
|
Verified the tweak I made works, really appreciate you cleaning these problems up 🙏 |
SleeplessByte
left a comment
SleeplessByte
left a comment
There was a problem hiding this comment.
I know this was merged, and definitely an improvement. I did leave a comment about something I ran into just now.
| </NewText> | ||
|
|
||
| <NewText | ||
| <InlineLinkText |
There was a problem hiding this comment.
This should not be a link but a button.
The label is not necessary and adding it will actually break translation (e.g Google Translate) as it will not translate the aria-label.
4 participants
This PR improves accessibility by turning skeet controls (Translate, Reply, Reskeet, Like, Share, More) actual buttons instead of
divs with an event handler.Same with tabs such as on profiles, they are now properly marked as tabs for assistive technologies to pick up.
Closes #4409