accessibility: Added captions for visually impaired users #4355

Gautam-Arora24 · 2020-12-22T20:14:13Z

Why and how the change was made
This PR proposed to add captions for visually impaired users. I added captions for buttons like back button and send button. I used React Native's accessibilityLabel prop to add captions.

Tests done
I tested this app on both simulator and on the actual device, and it worked well for both the scenarios.

Fixes #4232

gnprice

Thanks @Gautam-Arora24 for these changes!

The most important piece of feedback I have is that these changes need to be manually tested: try navigating the app by audio, and find out what effect your changes have. Here's the upstream guides for doing so with either Android or iOS:
https://developer.android.com/guide/topics/ui/accessibility/testing
https://developer.apple.com/documentation/uikit/accessibility_for_ios_and_tvos/supporting_voiceover_in_your_app
Either one would be enough for a PR. (Later, before we really consider the accessibility support complete, we'll want to try it out on both because there may be relevant differences.)

gnprice · 2020-12-23T07:45:39Z

src/nav/NavButtonGeneral.js

      <Touchable onPress={onPress}>
-        <View style={this.styles.navButtonFrame}>{children}</View>
+        <View
+          accessible={true}
+          accessibilityLabel="Go Back!"
+          accessibilityHint="Navigates to the previous screen"
+          style={this.styles.navButtonFrame}
+        >
+          {children}
+        </View>
      </Touchable>


Take a look at the RN doc on accessibility:
https://reactnative.dev/docs/accessibility
The examples show touchable components with View children, and the accessibility props are all set on the touchable.

That makes sense to me because that's the component the user actually interacts with. Let's follow that.

I suspect that in fact if you try this version, with TalkBack or VoiceOver, what you find may be that it actually work properly. See in particular from that doc:

When a view is an accessibility element, it groups its children into a single selectable component. By default, all touchable elements are accessible.

So the Touchable is already an accessibility element; and it sounds like that may mean that its children, like this View, won't be focusable at all so these props there won't be helpful.

gnprice · 2020-12-23T07:46:54Z

src/compose/ComposeBox.js

+          <View
+            accessible={true}
+            accessibilityLabel="Send Message"
+            accessibilityHint="Sends a message !"
+          >
+            <FloatingActionButton
+              style={this.styles.composeSendButton}
+              Icon={editMessage === null ? IconSend : IconDone}
+              size={32}
+              disabled={message.trim().length === 0}
+              onPress={editMessage === null ? this.handleSend : this.handleEdit}
+            />
+          </View>


I think we can make this cleaner in the code by making this a feature of FloatingActionButton. See these commits in the previous PR for an example of how to do this:
0082727
9cf4267

gnprice · 2020-12-23T08:29:03Z

src/nav/NavButtonGeneral.js

-        <View style={this.styles.navButtonFrame}>{children}</View>
+        <View
+          accessible={true}
+          accessibilityLabel="Go Back!"


This is much too broad, because this component is used in a number of other places in the app.

For a clean approach to apply this change in only the relevant spots, see these commits in the earlier PR:
6935e5b
d26962d
a0d74d9

gnprice · 2020-12-23T08:30:22Z

src/compose/ComposeBox.js

+            accessibilityLabel="Send Message"
+            accessibilityHint="Sends a message !"


No need for a hint when the label already conveys all the same information.

gnprice · 2020-12-23T08:33:52Z

Also be sure to take a look at the Zulip project's guidelines for commit messages:
https://zulip.readthedocs.io/en/latest/contributing/version-control.html#commit-messages
as well as the other information in the mobile-specific style guide:
https://github.com/zulip/zulip-mobile/blob/master/docs/style.md

gnprice · 2021-01-06T06:21:59Z

Thanks @Gautam-Arora24 ! This is looking like a promising structure.

Let's split this up into several commits. That will help make it easier to analyze the different changes that are happening. You can read more about our approach to small, coherent commits here:
https://zulip.readthedocs.io/en/latest/contributing/version-control.html#commit-discipline

Specifically, for this PR, let's do:

One commit for FloatingActionButton, NavButtonGeneral, and NavButton, which just add the accessibilityLabel prop to be passed through. (This could even be two commits, one for FloatingActionButton and one for the others.)
One commit for ComposeBox, adding the label on the send-message button.
One commit for ChatNavBar, ModalNavBar, and ModalSearchNavBar, adding the label on the "navigate back/up" button.

Then for the commits that actually add specific labels:

Please mention in the commit message what testing you did to find the most natural wording to use for the given action.
- I've never been a regular user of this kind of UI (and I believe you haven't either), so I don't feel like I automatically know what fits in naturally with the UX that someone will get when they're relying on it. It's best to choose something that'll be similar to what the user gets from other apps.
- For the back/up button, a good model would be the system settings app, or other Google-written apps that have plenty of that kind of navigation. From when I tried this out when first reviewing this PR, I think the wording there was like "Navigate up" -- note "up", not "back". For background on what that's about, see this article: https://developer.android.com/guide/navigation/navigation-principles A user won't have read that article, but they will know that the same wording they hear in other apps means the thing they're looking for.
  - (This should probably be different between iOS and Android. I'm OK with merging a PR like this with just the Android wording for now, and we can handle the iOS wording in a followup.)
- For the "send message" button, a good model would be the system Messages (i.e. SMS and the like) app, and also FB Messenger, Slack, Discord, WhatsApp, Signal, and so on. No need to try every one of them if you don't have some of them conveniently lying around, but at least two or three would be good.

I'm not sure how much experience you have already with Git, but this is one of those things that might look like a pain but becomes very quick to do once you know the right commands. Here, I'd do it with commands like these:

$ git reset HEAD^
$ git status # look at what changes you have
$ git commit file1 file2
$ git status # look again
$ git commit file3
$ # … repeat until all changes are committed

There's a bunch of related useful information in Zulip's Git guide, and you can also always ask questions on chat.zulip.org in #development help, #general, or #mobile.

gnprice · 2021-01-06T06:36:32Z

Oh, one other thought: ideally we should really have these strings translated. 🙂

I'll be fine with merging this PR with them just always in English, though -- and then we can work through wiring them up for translation as a followup.

Gautam-Arora24 · 2021-01-06T20:52:29Z

This PR is now splitted into several commits as per the requirements!

gnprice · 2021-01-07T03:16:34Z

Thanks @Gautam-Arora24 ! Please also do this part:

Then for the commits that actually add specific labels:

Please mention in the commit message what testing you did to find the most natural wording to use for the given action.

I've never been a regular user of this kind of UI (and I believe you haven't either), so I don't feel like I automatically know what fits in naturally with the UX that someone will get when they're relying on it. It's best to choose something that'll be similar to what the user gets from other apps.

For the back/up button, a good model would be the system settings app, or other Google-written apps that have plenty of that kind of navigation. From when I tried this out when first reviewing this PR, I think the wording there was like "Navigate up" -- note "up", not "back". For background on what that's about, see this article: https://developer.android.com/guide/navigation/navigation-principles A user won't have read that article, but they will know that the same wording they hear in other apps means the thing they're looking for.

(This should probably be different between iOS and Android. I'm OK with merging a PR like this with just the Android wording for now, and we can handle the iOS wording in a followup.)

For the "send message" button, a good model would be the system Messages (i.e. SMS and the like) app, and also FB Messenger, Slack, Discord, WhatsApp, Signal, and so on. No need to try every one of them if you don't have some of them conveniently lying around, but at least two or three would be good.

Separately, the last commit on top looks like a fix to a previous commit, which should be squashed into that one. See the discussion of coherent commits in the Zulip project's Git style guide:
https://zulip.readthedocs.io/en/latest/contributing/version-control.html

You can make both of these changes conveniently -- squashing that commit into a previous one, and editing the commit messages of other commits -- with git rebase -i. See the quick guide to fixing commits here:
https://zulip.readthedocs.io/en/latest/git/fixing-commits.html
and Git questions are also always welcome on chat.zulip.org.

Through various tests on different messaging apps like WhatsApp, Messenger and Discord etc., I found that the label was either "Send message" or "Send button". So I used "Send message".

Through tests on system settings app and system messages app on Android, a user hears Navigate up when they click on back button. So I used this as a label on back button. For iOS the label should probably be something else, but this is better than not having it.

This already would have been good, but after adding the accessibilityLabel, this bit of code is definitely complex enough we want to avoid duplicating it.

gnprice · 2021-01-11T20:30:13Z

Thanks @Gautam-Arora24 for the revision! Looks good -- merged.

I made some small style edits to the commit messages before merge. Take a look:

commit 4775de5

a11y: Add accessibilityLabel prop on FloatingActionButton.

commit 311ffaf

a11y: Add accessibilityLabel prop on NavButton and NavButtonGeneral.

commit 0c66376

a11y: Add label on "send message" button.

Through various tests on different messaging apps like WhatsApp,
Messenger and Discord etc., I found that the label was either
"Send message" or "Send button". So I used "Send message".

commit 7a17dd6

a11y: Add "Navigate up" label on up/back button.

Through tests on system settings app and system messages app on
Android, a user hears Navigate up when they click on back button.
So I used this as a label on back button.

For iOS the label should probably be something else, but this is
better than not having it.

The a11y: prefix refers to a common abbreviation for "accessibility". We prefix our commit summary lines with the general area that a commit is about, and don't use the "feat:" system that some other projects do. I also wrapped the text to a width of 68 columns.

I highly recommend browsing through our Git history -- it's good for seeing examples of our commit style, among many other things. See our tips for reading Git history here: https://github.com/zulip/zulip-mobile/blob/master/docs/howto/git.md

I also added a commit on top, just making a small refactor that this branch caused me to notice we should do.

gnprice reviewed Dec 23, 2020

View reviewed changes

Gautam-Arora24 changed the title ~~Accessibility: Added captions for visually impaired users~~ accessibility: Added captions for visually impaired users Dec 29, 2020

Gautam-Arora24 force-pushed the accessiblity branch from 3b8966a to 27ed828 Compare December 29, 2020 21:36

Gautam-Arora24 requested a review from gnprice December 31, 2020 07:47

Gautam-Arora24 force-pushed the accessiblity branch 4 times, most recently from 25a1816 to 5e7ae75 Compare January 6, 2021 20:41

Gautam-Arora24 force-pushed the accessiblity branch from 5e7ae75 to 0efc001 Compare January 8, 2021 09:22

Gautam-Arora24 closed this Jan 8, 2021

Gautam-Arora24 force-pushed the accessiblity branch from 0efc001 to 7dfa1c2 Compare January 8, 2021 09:26

Gautam-Arora24 reopened this Jan 8, 2021

Gautam-Arora24 and others added 5 commits January 11, 2021 11:53

a11y: Add accessibilityLabel prop on FloatingActionButton.

4775de5

a11y: Add accessibilityLabel prop on NavButton and NavButtonGeneral.

311ffaf

a11y: Add label on "send message" button.

0c66376

Through various tests on different messaging apps like WhatsApp, Messenger and Discord etc., I found that the label was either "Send message" or "Send button". So I used "Send message".

nav [nfc]: Factor out NavBarBackButton into one place.

4e524e8

This already would have been good, but after adding the accessibilityLabel, this bit of code is definitely complex enough we want to avoid duplicating it.

gnprice force-pushed the accessiblity branch from 6161b9a to 4e524e8 Compare January 11, 2021 20:26

gnprice merged commit 4e524e8 into zulip:master Jan 11, 2021

gnprice mentioned this pull request Jan 11, 2021

Make the app more accessible with a screen-reader #4394

Open

26 tasks

Gautam-Arora24 deleted the accessiblity branch January 21, 2021 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

accessibility: Added captions for visually impaired users #4355

accessibility: Added captions for visually impaired users #4355

Uh oh!

Gautam-Arora24 commented Dec 22, 2020 •

edited

Loading

Uh oh!

gnprice left a comment

Uh oh!

gnprice Dec 23, 2020

Uh oh!

gnprice Dec 23, 2020

Uh oh!

gnprice Dec 23, 2020

Uh oh!

gnprice Dec 23, 2020

Uh oh!

gnprice commented Dec 23, 2020

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

Gautam-Arora24 commented Jan 6, 2021

Uh oh!

gnprice commented Jan 7, 2021

Uh oh!

gnprice commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		accessibilityLabel="Send Message"
		accessibilityHint="Sends a message !"

Uh oh!

accessibility: Added captions for visually impaired users #4355

accessibility: Added captions for visually impaired users #4355

Uh oh!

Conversation

Gautam-Arora24 commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 23, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 23, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 23, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Dec 23, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice commented Dec 23, 2020

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

Gautam-Arora24 commented Jan 6, 2021

Uh oh!

gnprice commented Jan 7, 2021

Uh oh!

gnprice commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Gautam-Arora24 commented Dec 22, 2020 •

edited

Loading