Uh oh!
There was an error while loading. Please reload this page.
Text Component does not announce disabled and disables click functionality when disabled - #33076
Closed
fabOnReact wants to merge 4 commits into
Closed
Text Component does not announce disabled and disables click functionality when disabled#33076fabOnReact wants to merge 4 commits into
fabOnReact wants to merge 4 commits into
Conversation
Adding the prop `accessible` to `ReactTextAnchorViewManager` fixes the problem for this component. The same solution from my previous pr react#30935 (comment). See test case at fabOnReact/react-native-notes#1 (comment)
Adding the prop `accessible` to `ReactTextAnchorViewManager` fixes the problem for this component. The same solution from my previous pr react#30935 (comment). See test case at fabOnReact/react-native-notes#1 (comment)666647e
analysis-bot
commented
Feb 9, 2022
Base commit: 97064ae |
analysis-bot
commented
Feb 9, 2022
Base commit: 97064ae |
This was referenced Feb 9, 2022
fabOnReact
marked this pull request as ready for review
February 11, 2022 06:30
blavalla
approved these changes
Feb 11, 2022
blavalla
left a comment
Contributor
There was a problem hiding this comment.
Code and test cases look good to me.
I'm curious why the change in ReactTextAnchorViewManager was needed. What would happen without this change? It doesn't seem like it should've made a difference, since the "disabled" attribute in Android shouldn't affect focusability of an element, although it does seem weird that it was missing, so maybe it was just fixing another bug.
ContributorAuthor
Thanks @blavalla
The DOES NOT ANNOUNCE DISABLEDnotAnnounceDisabled.mp4ANNOUNCES DISABLEDannouncesDisabled.mp4The change was already introduced in the Image Component with my PR #31252. I added two additional tests cases to the Pull Request Summary (test case 6 and test case 7.7). |
facebook-github-bot
commented
Feb 14, 2022
Contributor
@ShikaSD has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
react-native-bot
commented
Feb 15, 2022
Collaborator
This pull request was successfully merged by @fabriziobertoglio1987 in 7b2d817. When will my fix make it into a release? | Upcoming Releases |
facebook-github-bot pushed a commit
that referenced
this pull request
Jun 9, 2022
…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
Saadnajmi pushed a commit
to Saadnajmi/react-native-macos
that referenced
this pull request
Jan 15, 2023
…ality when disabled (react#33076) Summary: This issue fixesreact#30937fixesreact#30947fixesreact#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) . The issue is caused by: 1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]). 2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR react#31252) Related PR react#33070 PR callstack/react-native-slider#354 [20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289 [Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled Pull Request resolved: react#33076 Test Plan: [1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1]) [2]. Text has `disabled` ([link][2]) [3]. Text has `accessibilityState={{disabled: true}}` ([link][3]) [4]. Text has `accessibilityState={{disabled:false}}` ([link][4]) [5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5]) [6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6]) 7. Test Cases on the main branch [7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1]) [7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3]) [7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5]) [7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6]) [7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7]) [1]: fabOnReact/react-native-notes#1 (comment) [2]: fabOnReact/react-native-notes#1 (comment) [3]: fabOnReact/react-native-notes#1 (comment) [4]: fabOnReact/react-native-notes#1 (comment) [5]: fabOnReact/react-native-notes#1 (comment) [6]: fabOnReact/react-native-notes#1 (comment) [7.1]: fabOnReact/react-native-notes#1 (comment) [7.3]: fabOnReact/react-native-notes#1 (comment) [7.5]: fabOnReact/react-native-notes#1 (comment) [7.6]: fabOnReact/react-native-notes#1 (comment) [7.7]: fabOnReact/react-native-notes#1 (comment) [10]: react@17095c6 [11]: react@6ab7ab3 Reviewed By: blavalla Differential Revision: D34211793 Pulled By: ShikaSD fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This issue fixes#30937fixes#30947fixes#30840 (Test Case 7.1, Test Case 7.3, Test Case 7.5) .
The issue is caused by:
accessibilityStatein the Text component fabOnReact@6ab7ab3 (as previously implemented in Button).accessibleinReactTextAnchorViewManagerfabOnReact@17095c6 (More information in previous PR [Accessibility] Fix Image does not announce "disabled" #31252)Related PR #33070 PR callstack/react-native-slider#354
Changelog
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Test Plan
1. Text has
disabledandaccessibilityState={{disabled: false}}(link)2. Text has
disabled(link)3. Text has
accessibilityState={{disabled: true}}(link)4. Text has
accessibilityState={{disabled:false}}(link)5. Text has
disabled={false}andaccessibilityState={{disabled:true}}(link)6. Text has
accessibilityState={{disabled:true}}and methodsetAccessibleinReactTextAnchorViewManager(tested on commit b4cd8) (link)7. Test Cases on the main branch
7.1. Text has
disabledandaccessibilityState={{disabled: false}}(link)7.3 Text has
accessibilityState={{disabled: true}}(link)7.5 Text has
disabled={false}andaccessibilityState={{disabled:true}}(link)7.6 Text has
onPress callbackandaccessibilityState={{disabled: true}}(link)7.7 Text has
accessibilityState={{disabled:true}}and no methodsetAccessibleinReactTextAnchorViewManager(tested on commit c4f98dd) (link)