Make the app more accessible to blind people #2753

borisyankov · 2018-07-02T21:20:37Z

We already follow standard usability best-practices like having big enough touch targets.

This PR improves the usability for blind people.
It was tested with TalkBack on Android and VoiceOver on iOS.

It consists of separate changes that ensure UI elements are identified correctly and their purpose can be determined (usually by adding a descriptive text)

gnprice

Awesome -- thanks @borisyankov ! Very glad to see all this. I think accessibility is an important thing for us to stay on top of.

I like the basic direction of all of this. Comments below on specific areas of code style and commit style. I'll go ahead and merge now the bits that I can easily merge as they are; and I think the rest will be easy for you to adjust.

There's one other piece I think would be very helpful to have as part of this effort: a short doc, maybe in docs/architecture/, about how accessibility in the app works. This really has two audiences:

Everyone contributing regularly to the app, to be up to speed on how to keep it accessible when making changes and how to fix accessibility issues.
People interested in using Zulip, for whom accessibility is critical (perhaps because they rely on accessibility features themselves) and who are willing to contribute code, bug reports / user feedback, or other help.

The most day-to-day practical info there might just be a link to the upstream guide:
https://facebook.github.io/react-native/docs/accessibility . No need to duplicate that, though there might be key bits worth highlighting. Other key technical info includes:

a mention of the webview; this deserves a docs/architecture/ doc of its own, which I've been meaning to try a draft of, but a brief mention here would be helpful.
maybe a few words about our Touchable#accessibilityLabel prop?

Then there's the most important bit of info of all, which is not technical: just saying (to both of those audiences) that this is something we care about, put effort into, and welcome bug reports on. Mostly it's the existence of the doc itself (plus we should link to it appropriately) that communicates that, along with our ongoing response to bug reports; but a sentence or two saying so, plus repeating specifically where to report bugs, will help too.

I'm happy to write up that doc myself if you'd like, or I'd be glad to see a draft in a PR.

gnprice · 2018-07-06T00:32:38Z

src/webview/css/css.js

@@ -414,6 +414,13 @@ blockquote {
  border-radius: 50%;
  background: rgba(82, 194, 175, 0.5);
 }
+#scroll-bottom .text {
+  clip: rect(0 0 0 0);


The web tells me this attribute is deprecated:
https://developer.mozilla.org/en-US/docs/Web/CSS/clip

Hrm, but the suggested replacement isn't yet widely supported:
https://developer.mozilla.org/en-US/docs/Web/CSS/clip-path

Shrug.

gnprice · 2018-07-06T01:04:18Z

src/common/Touchable.js

+      );
+    }
+
+    if (Platform.OS === 'ios') {


👍 to merging these two files -- I think it's actually a win even before the additional logic added in the subsequent commit.

One reason is this deep dark secret 😉 of React Native and Flow, which I noticed recently in our .flowconfig:

[ignore] ; We fork some components by platform .*/*[.]android.js

(That's verbatim from the template file in RN upstream -- it's not our doing.) In other words, this .android.js / .ios.js convention is totally opaque to Flow, and as a result the best we know how to do is to completely leave .android.js files out from type-checking.

Similarly, the spiffy navigation features in VS Code, to jump to definition or find references, find this totally opaque.

So there's a pretty big cost to using this .android.js/.ios.js feature, as opposed to plain old conditionals.

I think this actually illustrates a kind of classic pattern: once any whiff of the logic of a program starts being expressed in ad-hoc fashion in some kind of configuration outside of the Real Programming Language(s) the rest of the program is written in, it generally isn't long before it begins to hurt to be doing that (a) without the nice, full-featured, composable, semantics of a Real Programming Language and (b) without the benefit of all the tooling that you and your language community have built up around that language (or languages). A very common form of this pattern is templating languages, which either are painfully restrictive or rapidly grow to encompass the main language or both; config-file formats tend to go this way as well.

For us, the Real Programming Language we use for the bulk of the app (including all this code) is JavaScript, and I think plain JS conditionals should express the same thing as the .android.js / .ios.js feature quite nicely. 😁

(I'll go file an issue for that refactor in general, and probably also discuss that general software-systems pattern in chat.)

(I'll go file an issue for that refactor in general, and probably also discuss that general software-systems pattern in chat.)

(It's late and I think I'm done with focused work for the day, but I've made a note to do this tomorrow.)

Just ran across this again; filed #3018.

gnprice · 2018-07-06T01:09:49Z

src/common/Touchable.js

+const androidBackground =
+  Platform.Version >= 21
+    ? TouchableNativeFeedback.Ripple(HIGHLIGHT_COLOR)
+    : TouchableNativeFeedback.SelectableBackground();


Are these lines definitely OK to run on iOS? They're at top level.

Maybe lines like this are (as a matter of RN API policy) definitely always OK to run on the platform they don't apply to; I don't really know.

I guess you say in the PR message this is tested on iOS, so that's an answer. There's nothing I see in the upstream doc to say what should happen.

gnprice · 2018-07-06T01:20:42Z

src/common/Touchable.js

+    if (!onPress && !onLongPress) {
+      return (
+        <View
+          accessible={!!accessibilityLabel}


The commit message says

By default all 'touchable' components are accessible. But if we provide an `accessibilityLabel` and render a `View` we set `accessible` to `true`.

which had me kind of confused for a bit -- by default they're accessible, but we can make them not be? Why would we ever actively make something not accessible? Also, how can something that generic be accessible by default in the first place -- surely that requires more help from the app?

Then the name of this property (on upstream's View) gave me a hint what you meant, and a bit in this upstream guide:
https://facebook.github.io/react-native/docs/accessibility
explains it: you're referring to this boolean property, which means this (and maybe more things): "When a view is an accessibility element, it groups its children into a single selectable component."

I think the commit message would become almost clear if it just said:

By default all "touchable" components are `accessible`.

so that accessible is an identifier, not the ordinary word, and "touchable" is in quotes because it's an undefined bit of jargon. Then the message becomes actually clear by including a link to that upstream guide. :-)

This is also an excellent case for a JSDoc comment, explaining this accessibilityLabel prop in particular.

gnprice · 2018-07-06T01:24:18Z

src/common/Touchable.js

+    if (!onPress && !onLongPress) {
+      return (
+        <View
+          accessible={!!accessibilityLabel}


Should we perhaps always set this to true here? After all, if onPress or onLongPress were set, it'd end up effectively true even without accessibilityLabel. Also, if the caller thinks of this component as being something touchable, that suggests that it ought to be presented as one thing to select.

Of course ideally we'll make this moot by always providing the label 😄 , but even once we finish doing that there will probably be places we temporarily miss, in the present or the future.

Hmm, now I see your later commit removing uses of nullFunction.

I don't 100% follow what that commit message is saying, though. Would you write in the identifiers for the components you're mentioning? I'm not quite sure which ones they are.

gnprice · 2018-07-06T01:26:58Z

src/utils/accessibility.js

+    return text;
+  }
+
+  return `${text}, ${unreadCount} unread message${unreadCount > 1 ? 's' : ''}`;


This won't translate well.

I think https://github.com/yahoo/react-intl/wiki/Components#formattedplural looks like the API we want to use here.

gnprice · 2018-07-06T01:34:02Z

src/nav/NavButton.js

@@ -7,6 +7,7 @@ import { ComponentWithOverlay, UnreadCount } from '../common';
 import Icon from '../common/Icons';

 type Props = {
+  accessibilityLabel: string,


Making this mandatory seems exactly right to me, for the reason you say in the commit message.

I don't like doing so in a commit where the callers don't already comply with that, though -- that's a case of leaving things in a broken state mid-branch.

One very clean way to do this would be to have the first commit add this prop but leave it optional; then add it at all the call sites in subsequent commits, as you do; then have a followup commit just flip it to mandatory.

In general the other clean way to introduce a non-optional prop is to add it at all the call sites in the same commit that introduces it; but in this case there's enough going on that I think it's definitely better to have several separate commits for the call sites, just like you do.

gnprice · 2018-07-06T01:54:24Z

src/nav/MainNavBar.js

  backgroundColor: string,
-  canGoBack: boolean,


When removing a connect-supplied prop like this one, please also remove it from the connect call.

Not only does that leave the code in a tidier end state, it actually makes the diff itself easier to read -- because it causes whatever had been the argument there (in this case, getCanGoBack(state)) to show up in the diff, making it easier to compare before vs. after. 😃

gnprice · 2018-07-06T03:21:46Z

src/compose/ComposeBox.android.js

@@ -357,6 +357,7 @@ class ComposeBox extends PureComponent<Props, State> {
          </View>
          <View style={styles.alignBottom}>
            <FloatingActionButton
+              accessibilityLabel="Send"


These should probably follow the Icon prop's logic below, right? So when editMessage is false, it should be "Done" or "Update message" or something.

Also, the new required prop on FloatingActionButton is a similar story to NavButton. In this case I think it'd be fine to have a single commit add the prop and adjust all the callers, because that's just a line each in three places; but the cleanest thing would be just like you have, except the first commit leaves it optional and there's an extra commit at the end to make it mandatory.

While splitting code in two files depending on the platfrom is generally a good idea, in the case of Touchable there is too little unique code and too much duplication. [greg: I think this is actually an improvement already, even before the additional code added later! Discussion in #2753.] In anticipation of adding some more code that is exactly the same it is apparent merging them and `if`-ing the differences is better. While some improvements can be made, this refactor keeps the behavior strictly the same but simplifies the logic and makes it more obvious. Instead of WrapperComponent we just check if the component has any onPress or onLongPress event attached, if not returns a `View`

zulipbot · 2018-07-06T03:28:41Z

Heads up @borisyankov, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

gnprice · 2018-07-06T03:29:37Z

Merged the core Touchable changes and several others.

Most of the other commits are also ready to merge, once their dependencies (e.g. textWithUnreadCount) are ready.

While splitting code in two files depending on the platfrom is generally a good idea, in the case of Touchable there is too little unique code and too much duplication. [greg: I think this is actually an improvement already, even before the additional code added later! Discussion in zulip#2753.] In anticipation of adding some more code that is exactly the same it is apparent merging them and `if`-ing the differences is better. While some improvements can be made, this refactor keeps the behavior strictly the same but simplifies the logic and makes it more obvious. Instead of WrapperComponent we just check if the component has any onPress or onLongPress event attached, if not returns a `View`

Several components use unread count displays in addition to text describing the item (stream, topic, user etc.) To make adding accessibility labels easier and consistent we create a function (and tests) to create the label.

Including this makes sure the voice assistant does not read the initials in the TextAvatar and also provides the unread count in a human understandable way. Previously it would say: 'JDJD John Doe, Jane Doe, 3' Now: 'John Doe, Jane Doe, 3 unread messages'

Include understandable unread counts

Include unread counts after the topic name.

Use name and unread count

We add `accessibilityLabel` to NavButton, but in contrast with the previously added optional props, this one is mandatory. A NavButton consists only of an icon thus without the label they are completely not-accessible.

This is the same code we use in MainNavBar and ModalNavBar, extracted as a stand-alone and reusable component and also with `accessibilityLabel` set depending on the Platform. Consistent with all other apps: * on iOS the accessibility label is 'Back button' * on Android the label is 'Navigate up button'

Replace the customized NavButton in MainNavBar with the better BackButton for reusability and added accessibility.

Replace customized NavButton with the accessible BackButton.

Replace the slightly outdated NavBar plus logic code with the accessible BackButton component.

Now that NavBar requires an `accessibilityLabel` property, it is obvious what components need to have this added. Previously these components were invisible to the voice-over functionality, now they have a meaningful text associated with them.

Any `Touchable` is accessible by default. Any `Touchable` that does not have an `onPress` though is rendered as `View` which can be accessible but is not so automatically. We do not want all avatars to be accessible. To the average user an avatar with onPress of `nullFunction` is exactly the same as one with none. To a person using voice-over there is a difference. By removing the default onPress value entirely we make sure these components are not accessible by default. Thus many elements that were misleading or 'fake accessible' are now ignored (meaning that the voice-over function will try to read them but we don't want to) Such components are: * PM titlebar's avatar * Group titlebar avatars * any other unanticipaded component

The button consist of a single icon, thus was not accessible. We add `accessibilityLabel` prop that is passed to the touchable component inside.

This is the most important UI element remaining inaccessible. The buttons are currently icon-only and need accessibilityLabel. In addition to that, the IconUnreadConversations is enhanced with a readable (hearable) unread counts.

jesse-troy · 2020-10-01T15:17:39Z

@gnprice would it be possible to get some priority on this PR? Our organization currently has a visually impaired user who is unable to use our main communication application on his iphone.

If passes are checking, could we please merge this so that the application is more accessible?

Also here is the issue I created a while back about this problem #4232

gnprice · 2020-10-03T04:42:55Z

@jesse-troy Thanks for the ping. Let's discuss on the issue thread (or chat.zulip.org) -- I've just added a comment there.

chrisbobbe · 2021-04-09T20:58:06Z

#4419 is open, which may mean it'll soon be time to close this PR. We'll want to do at least a quick check that there isn't anything else here in Boris's PR that we want before closing it.

Greg said, at #4394 (comment):

Another note: the old PR #2753 contains fixes for some of the items above. In particular:

On the PM-conversations screen, if there's a group-PM thread, the initials that we use for making up an avatar get read out. They shouldn't be.

when you select a stream it says something like "Test here. Five thousand seven hundred and five." (for #test-here with 5705 unreads.) The number should probably say "unreads" or something. Needs playing with other apps to see what conventions are.

most, possibly all, other nav buttons on main screen and in nav bars

On the PM-conversations screen, the avatars are selectable. They shouldn't be

different message for NavBarBackButton between iOS vs. Android

A good place to start would be to revive the changes from that PR.

borisyankov force-pushed the a11y branch from 8ceb8d3 to b8d28e8 Compare July 2, 2018 21:26

gnprice reviewed Jul 6, 2018

View reviewed changes

gnprice added the a-a11y Accessibility label Jul 6, 2018

borisyankov added this to the Next milestone Oct 1, 2018

gnprice mentioned this pull request Oct 2, 2018

Combine separate *.{android,ios}.js files #3018

Closed

borisyankov added 15 commits October 4, 2018 23:07

a11y: Add textWithUnreadCount util function

3584caa

Several components use unread count displays in addition to text describing the item (stream, topic, user etc.) To make adding accessibility labels easier and consistent we create a function (and tests) to create the label.

a11y: Add accessibility label to StreamItem

79faf90

Include understandable unread counts

a11y: Add accessibility label to TopicItem

fa31857

Include unread counts after the topic name.

a11y: Add accessibility label to UserItem

8e76520

Use name and unread count

a11y: Add a mandatory accessibility label to NavButton

6935e5b

We add `accessibilityLabel` to NavButton, but in contrast with the previously added optional props, this one is mandatory. A NavButton consists only of an icon thus without the label they are completely not-accessible.

a11y: Use BackButton component in MainNavBar

a0d74d9

Replace the customized NavButton in MainNavBar with the better BackButton for reusability and added accessibility.

a11y: Use BackButton component in ModalNavBar

85051f8

Replace customized NavButton with the accessible BackButton.

a11y: Use BackButton component in ModalSearchNavBar

a195298

Replace the slightly outdated NavBar plus logic code with the accessible BackButton component.

a11y: Add accessibility to FloatingActionButton

0082727

The button consist of a single icon, thus was not accessible. We add `accessibilityLabel` prop that is passed to the touchable component inside.

a11y: Add accessibility labels to floating action buttons

9cf4267

a11y: Make main tabs buttons accessible

0e77005

This is the most important UI element remaining inaccessible. The buttons are currently icon-only and need accessibilityLabel. In addition to that, the IconUnreadConversations is enhanced with a readable (hearable) unread counts.

borisyankov force-pushed the a11y branch from c9f6ba0 to 0e77005 Compare October 4, 2018 22:24

gnprice force-pushed the master branch from 5834b53 to e91273d Compare September 4, 2019 22:15

gnprice mentioned this pull request Oct 3, 2020

Button Captions for Screen Readers (Accessibility) #4232

Closed

gnprice mentioned this pull request Jan 11, 2021

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

Open

26 tasks

Uh oh!

Make the app more accessible to blind people #2753

Are you sure you want to change the base?

Make the app more accessible to blind people #2753

Uh oh!

Conversation

borisyankov commented Jul 2, 2018

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zulipbot commented Jul 6, 2018

Uh oh!

gnprice commented Jul 6, 2018

Uh oh!

jesse-troy commented Oct 1, 2020

Uh oh!

gnprice commented Oct 3, 2020

Uh oh!

chrisbobbe commented Apr 9, 2021

Uh oh!

Uh oh!