Skip to content

Commit

Permalink
Make Text use Android's "auto" focus by default instead of setting fo…
Browse files Browse the repository at this point in the history
…cus to true

Summary:
[A recent fix](#33076) to Android to set focusable to true when accessible is true, and this caused several components not to work correctly.

This JS change essentially reverts the default back to Text not being focusable unless it is explicitly set. Android's "auto" behavior is better than setting `accessible=true`, and it's also the behavior React Native has had since accessibility on Android was implemented.

# Wall of Text Explanation

Explanation From Brett's comment [here](https://www.internalfb.com/diff/D35908559?dst_version_fbid=700876567897063&transaction_fbid=477905564133412)

blavalla

Generally speaking, "accessible" in react native maps to "focusable" in Android views, and the default value for "focusabe" for a TextView (and actually all views) is "auto" not "false".  The difference here is that "false" is telling the system to explicitly disallow focus on this element, where as "auto" is telling the system that it's up to whatever service is trying to focus to determine if it should or not.

In the case of text, Talkback generally does default to focusing on Text when it's set to "auto", though it also does try to combine this text together with other not-explicitly focusable siblings and roll the focus up to some common ancestor element.

In the case of TetraButton here, I would expect the default behavior would be that the text is "auto" focusable, so Talkback would combine the text here with the parent <TetraPressable> (which is explicitly focusable via accessible="true").

...

[This diff](#33076) was to fix the issue with "disabled" not properly announcing on text views, which was commonly occuring due to the description-combining feature described above. Basically, when Talkback decides to combine not-explicitly-focusable elements together, it ignores properties like "disabled", "selected", etc. so when combined only the text is transferred.

The "fix" here was to make sure that if disabled was set, that an element was always explicitly focusable so that it wouldn't be eligible to be combined with others. I think that as a general concept makes sense, but the fix actually surfaced an issue that is likely a much older bug.

This line in <Text>
```
accessible={accessible !== false}
```

Is basically always setting accessible="true" unless it's explicitly set to false, and has been in there for years. It was likely added to force text to be accessible by default for iOS.  But until [this diff](#33076) this line was basically a no-op for Android, since setting accessible="true" on text would do nothing at all.

[This diff](#33076)  changed this so that setting accessible="true" worked how you'd expect, by making the view explicitly focusable, which was necessary for the disabled behavior to work properly. But that means that now by default all text views are explicitly focusable on both iOS and Android, and this there is likely many components that were built that don't expect this to be the case.

It doesn't seem like the right fix here is to revert this behavior to its previous state, as it wasn't working how anyone would expect it to if they looked at the code, and it seems like we were relying on some fairly undocumented behavior of Talkback to get it to work how we wanted. If we truly only wanted accessible="true" to be set on all TextViews for iOS, we should be explicit about it and do a platform check before setting that property. If we didn't want this to be iOS-specific, then everything is now actually working as originally intended.

For reference, this is the diff that introduced the default-accessible text - https://www.internalfb.com/diff/D1561326, and the description makes it clear that this was only tested on iOS, and the behavior was explicitly trying to map to iOS norms such as not allowing nested accessible elements.

Changelog:
[Android][Fixed] Make Text not focusable by default

Reviewed By: ryancat

Differential Revision: D36991394

fbshipit-source-id: c45d2ada72bb2d6ffeee6947d676a07fb8899449
  • Loading branch information
kacieb authored and facebook-github-bot committed Jun 9, 2022
1 parent 340612a commit 8ced165
Showing 1 changed file with 7 additions and 1 deletion.
8 changes: 7 additions & 1 deletion Libraries/Text/Text.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* @format
*/

import Platform from '../Utilities/Platform';
import * as PressabilityDebug from '../Pressability/PressabilityDebug';
import usePressability from '../Pressability/usePressability';
import StyleSheet from '../StyleSheet/StyleSheet';
Expand Down Expand Up @@ -168,6 +169,11 @@ const Text: React.AbstractComponent<

const hasTextAncestor = useContext(TextAncestor);

const _accessible = Platform.select({
ios: accessible !== false,
default: accessible,
});

return hasTextAncestor ? (
<NativeVirtualText
{...restProps}
Expand All @@ -185,7 +191,7 @@ const Text: React.AbstractComponent<
{...restProps}
{...eventHandlersForText}
disabled={_disabled}
accessible={accessible !== false}
accessible={_accessible}
accessibilityState={_accessibilityState}
allowFontScaling={allowFontScaling !== false}
ellipsizeMode={ellipsizeMode ?? 'tail'}
Expand Down

0 comments on commit 8ced165

Please sign in to comment.