Fix duplicate accessibilityLabels & app crash on iOS

[nfriedly]

Summary

I came across this because one of my customer's apps was freezing and crashing when my code tries to read the accessibilityLabel of a view containing a chat component. Because the labels are recursively generated from the text content, any sufficiently long chat ends up with all of the parent views having a ridiculously long accessibilityLabel.

After 10 seconds of non-responsiveness from trying to generate all of this on the UI thread (when backgrounded), iOS decides the app has crashed and force-closes it.

Before this change, every single view in a RN app returned true for the conditional.

I believe this change will also improve both the accessibility and the testability of RN apps, in addition to fixing that crash.

This change also allows for correct behavior with this code:

if ([view respondsToSelector:@selector(accessibilityLabel)])
    return view.accessibilityLabel;

Before this change, any RCTView could potentially scan its entire tree and return nil.
After, only RCTViews that actually have an accessibilityLabel set will return one, and ones that don't will be skipped by the above code.

This PR supersedes or fixes #21830, #25963, #24113, #24118, appium/appium#10654, possibly #25220, and probably a few other tickets I haven't identified.

The changes in this PR are for RCTView. It's worth mentioning that there is very similar code in RCTViewComponentView - I haven't dug into that as much, but I suspect it should probably get the same treatment. If desired, I can include it in this PR.

I also haven't yet reviewed the Android side of things to see what happens there, but whatever the case, the same component structure doesn't seem to be leading to crashes in Android.

I might need to change the documentation too.

Changelog

[iOS] [Removed] - Deleted RCTView's accessibilityLabel wrapper and RCTRecursiveAccessibilityLabel, allowing for default native behavior

Test Plan

This is where I need a little help. What's a good way to test this?

I spent a while digging through the RN iOS tests, I can run the tests, and even step through the harness, but I still don't feel like I fully understand what's happening. I don't have a ton of iOS or RN experience, so it may be something obvious, but I'm just not sure what I'm missing.

[facebook-github-bot]

Hi @nfriedly!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,004,694	-203,963
android	hermes	armeabi-v7a	6,668,629	-189,200
android	hermes	x86	7,424,999	-218,290
android	hermes	x86_64	7,315,871	-218,366
android	jsc	arm64-v8a	9,164,411	-203,071
android	jsc	armeabi-v7a	8,820,493	-188,293
android	jsc	x86	9,012,809	-217,405
android	jsc	x86_64	9,589,876	-217,482

Base commit: 54e19a6

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 54e19a6

[shergin]

I think @rigdern has a context why those functions are needed.

[rigdern]

@shergin Unfortunately, I don't have context on those functions. They've been in RCTView since the first open source commit of React Native. If we can find somebody that is intimately familiar with React Native's accessibility implementation to help us, that would be great. If not, since these functions have been around for so long, here are some things I would check before making the change to ensure it doesn't cause a regression:

• Does RCTView exhibit the same issues? Or did RCTViewComponentView introduce them or exacerbate them? For example, perhaps in RCTViewComponentView hierarchies some views are marked as accessible that aren't in RCTView hierarchies that cause heavier use of the accessibiiltyLabel override.
• Check with some app developers that have been using React Native's accessibility framework for a long time. Have they encountered any problems with React Native's accessibilityLabel implementation? Is it doing what they want? Would they prefer the new implementation?
• Does Android exhibit a similar behavior and do we want to change it? When I was actively contributing to React Native, I was advocating for consistency between iOS and Android for the accessibility APIs. That way, a developer can write some accessibility code and expect it'll do the right thing on iOS and Android. However, that was a couple of years ago so I'm not familiar with how the accessibility APIs have evolved or the current philosophy behind them.

[nfriedly]

I spent some time testing this in the rn-tester app on a real iPhone (it took me a while to realize that VoiceOver just doesn't work in the simulator), and I have to admit that it seems to make a somewhat worse experience there. A lot of "Possible text" statements prefixing things where it would previously just read the text. And occasionally some text is omitted entirely. So this isn't the accessibility improvement I had hoped it would be.

My main goal here is to prevent the crash, which obviously impairs accessibility. I think that can be done without a regression in functionality. Here's a couple of ideas I thought of:

• Only generate accessibilityLabels on selectable elements, otherwise return nil.
• Put a limit on the string-length, or depth, or absolute number of child nodes that RCTRecursiveAccessibilityLabel looks at
• Cache the results of RCTRecursiveAccessibilityLabel - this would prevent the exponential as RCTRecursiveAccessibilityLabel when my code scans every view and RCTRecursiveAccessibilityLabel in turn scans every sub-view. As usual, cache invalidation is the hard part there.

What do you guys think?

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[nfriedly]

A couple of quick updates:

1. We ended up working around this issue by special-casing React Native views in our code - for RCTViews and RCTViewComponentViews and their subclasses, we now use the native accessibilityLabel implementation from the UIView class.

2. I have an idea for how to make this better for everyone else without negatively impacting actual accessibility: I believe that the accessibilityLabels are only used by VoiceOver when accessibilityElement is true - so when RCTView.accessibiltyLabel() is called, I'd like to make it check that and only do the recursive string building in that case.

I feel like there might be a better way to build the string also - maybe pass in the NSMutableString instead of creating a new one for each view.

I'll experiment with the above when I have time, but I'm not sure when that will be now that the immediate pressure is off.

[nfriedly]

I'm going to close this PR, as there are other PRs up now that address the problem better than this one (e.g. #31222 improves performance without changing behavior, #31924 improves the performance and the behavior.)