Add missing accessibility labels #5390
Conversation
|
Thanks for the quick approval, but no rush. I left it intentionally as a draft as I want to keep working on it. I think there are a few more buttons that need labels. If you have any tips, my local build still fails with the following:
|
|
On M1 you need to run xcode with roseta enabled |
|
Thanks for the tip, that did not work. I tried a bunch of different things, searched online for other fixes, but no luck. |
|
LGTM. Thank you! |
|
i personally develop on android emu. @marcosrdz can you help @GBKS with running on apple sim? |
@GBKS any chance you can share a screenshot of the build error (after quitting xcode, enabling rosetta on xcode, then reopening and rebuilding)? It's possible Rosetta was enabled while xcode was still open. |
|
Thanks for the tip. I even restarted my computer to ensure that the change is properly applied. Command line gives me this one: Xcode gives me this one: Thanks for your help. |
I used the "eslint-plugin-react-native-a11y" plug-in to identify more problems and went through all the commendations for buttons. I ignored input fields (about 25 issues found), since I am not 100% sure about the best way to fix them and it might be better to tackle that in a separate PR and keep this one focused on buttons.
|
I pushed a bunch more fixes. This time I used the "eslint-plugin-react-native-a11y" (via this recommendation) plug-in to identify more problems and went through all the recommendations for buttons. Sometimes the linter also recommends to add an accessibility hint, but I am not convinced it is always needed (the hint is in addition to the label, to describe what happens when interacting). This might have to be revisited on a case-by-base basis. I ignored input fields (linter found about 25 issues), since I am not 100% sure about the best way to fix them (do we need label properties, or link to respective text labels on the page, or is the placeholder enough, or based on context, etc) and it might be better to tackle that in a separate PR and keep this one focused on buttons. Small note about line 398 in ScanQRCode.js, there's a note that reads I still can't build the app, same issues as before. But I'll remove draft status now, as the PR includes a lot of changes now and should cover most simple button fixes. I'd appreciate your reviews, and ideally someone can also build the app and verify hands-on that elements can be properly identified now with the accessibility labels. We have several videos linked to in the Bitcoin Design Guide that show how to set up your phone for testing and perform quick tests. It really doesn't take much time at all. Let me know if I added the localization labels in all the right places. There are also some spots where I used labels from specific categories that may not have been appropriate. I'm happy to get your feedback and implement it. |
As complained about CircleCi.
|
full test suite & builds run here: #5411 |
|
Just merged master to resolve the conflict in |
For the "Add" button on the wallet list screen For more options in send details I found these through testing. The A11y linter did not catch them.
|
runnint builds & tests here: #5424 |
|
looks like all tests in #5424 passed. |
Yup |
|
Merging sounds good to me if you are confident in my code changes 😀 |
Various buttons around the application are missing accessibility labels. A user who can only interact with the app using a screen reader will not know what the buttons do.
I cannot get the application to build locally, so I cannot test my changes (will try again, but already wasted 2 hours). But I still want to see progress on this task, so I am starting anyways. The changes are really basic, just adding single attributes to elements in various components and screens, so it should not be a huge big issue. I'd appreciate it if someone else could help test.
To find issues, I am testing the app using a screen reader (here's how I do that), browsing the old branch by @limpbrains, and searching the code.
I am not aiming for 100% with this PR, but I will consider it a success and will open it for review when a good majority of accessibility labels have been added.
Here are some screenshots with examples, with the missing button labels outlined in red (remember that a user will not be able to see the screen at all, they will only know what's in the grey tool tips).