-
Notifications
You must be signed in to change notification settings - Fork 24.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Making links independently focusable by Talkback #33215
Conversation
Summary: Pull Request resolved: facebook#31757 This follows up on D23553222 (facebook@b352e2d), which made links functional by using Talkback's Links menu. We don't often use this as the sole access point for links due to it being more difficult for users to navigate to, and easy for users to miss if they don't listen to the entire description, including the hint text that announces that links are available. Instead, we generally allow links to be focusable after the main text that contains them is focused. This diff adds this functionality for both Paper and Fabric, and also retains the existing Links menu functionality as well, for users who prefer to use it. Reviewed By: yungsters Differential Revision: D28691177 fbshipit-source-id: 55e675cf84df5dda3b15d81a7f9bf1f071006bd2
More info at fabOnReact/react-native-notes#8 facebook#31128 (comment) >If you want to see how these files are actually stored: >Remove .gitattributes (so that no conversion happens) I solved the problem with the following steps: 1) checkout branch I want to rebase 2) the branch was already rebased and included commit facebook@7384471 which brought `.gitattributes` file in the branch The file below triggers the error `error: add_cacheinfo failed to refresh for path 'packages/react-native-gradle-plugin/gradlew.bat'; merge aborting.` when running `git merge master`. ``` -# Windows files should use crlf line endings -# https://help.github.com/articles/dealing-with-line-endings/ -*.bat text eol=crlf ``` 3) I remove the `.gitattributes` files from the branch. 4) `git` stops converting the line ending 5) I merge main branch. `.gitattributes` files was already removed from main branch with https://github.com/facebook/react-native/pull/31398/files. Git runs the merge without problems. Also no diff because the file already deleted from main.
Base commit: 97f3eb2 |
Base commit: 97f3eb2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment to verify, but overall it looks good to me.
// ID is the reverse of what is expected, since the ClickableSpans are returned in reverse | ||
// order due to being added in reverse order. If we don't do this, focus will move to the | ||
// last link first and move backwards. | ||
// | ||
// If this approach becomes unreliable, we should instead look at their start position and | ||
// order them manually. | ||
link.id = spans.length - 1 - i; | ||
links.add(link); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you verify that this is still necessary? At the time this was written, if this didn't happen focus would move to the last link in the paragraph on the first swipe, and then the next swipe would start navigating through the links from the bottom to the top. I'm not sure if this is still necessary or not, as it seemed like it may have been an unintentional side effect of the way we create these ClickableSpans, and that code may have changed since then.
A video showing a focus on a paragraph and then a swipe through the links both with this code and without it should be enough.
This may also be impacting the order that the links appear in the "Links" menu, although their order there isn't as important, as it's out of the context of the text itself. Ideally these would still match though and the first link in the text would be first in the list as well as the first swipe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you verify that this is still necessary? At the time this was written, if this didn't happen focus would move to the last link in the paragraph on the first swipe, and then the next swipe would start navigating through the links from the bottom to the top. I'm not sure if this is still necessary or not, as it seemed like it may have been an unintentional side effect of the way we create these ClickableSpans, and that code may have changed since then.
Thanks @blavalla Yes. It is still necessary.
This may also be impacting the order that the links appear in the "Links" menu, although their order there isn't as important, as it's out of the context of the text itself. Ideally these would still match though and the first link in the text would be first in the list as well as the first swipe.
I remain available for improvements. Thanks
A video showing a focus on a paragraph and then a swipe through the links both with this code and without it should be enough.
I included below the test cases and updated the summary of the Pull Request.
TalkBack focus moves through links IN THE CORRECT ORDER from top to bottom (PR Branch with link.id)
Testing with the link.id in AccessibilityLink (discussion)
// ID is the reverse of what is expected, since the ClickableSpans are returned in reverse
// order due to being added in reverse order. If we don't do this, focus will move to the
// last link first and move backwards.
//
// If this approach becomes unreliable, we should instead look at their start position and
// order them manually.
link.id = spans.length - 1 - i;
links.add(link);
Expected Result:
Swiping move TalkBack focus in this order:
- Parent Text
This is a test of inline links in React Native. Here's another link. Here is a link that spans multiple lines because the text is so long. This sentence has no links in it. links available
- Nested Texts in order from top to bottom (
test
,inline links
,another link
,link that spans multiple lines...
)
Links are displayed in the above order in the TalkBack menu
Actual Results:
RESULT 1 (SUCCESS) - Swiping moves TalkBack focus in the correct order
- Parent Text
This is a test of inline links in React Native. Here's another link. Here is a link that spans multiple lines because the text is so long. This sentence has no links in it. links available
- Nested Texts in order from top to bottom
2022-03-07.09-11-19.mp4
RESULT 2 (FAIL) - Links are NOT displayed in the correct order in the TalkBack menu
2022-03-07.09-13-23.mp4
TalkBack focus does NOT move through links in the correct order from top to bottom (PR Branch without link.id)
Testing without the link.id in AccessibilityLink (discussion)
// ID is the reverse of what is expected, since the ClickableSpans are returned in reverse
// order due to being added in reverse order. If we don't do this, focus will move to the
// last link first and move backwards.
//
// If this approach becomes unreliable, we should instead look at their start position and
// order them manually.
// link.id = spans.length - 1 - i;
links.add(link);
Expected Result:
Swiping move TalkBack focus in this order:
- Parent Text
This is a test of inline links in React Native. Here's another link. Here is a link that spans multiple lines because the text is so long. This sentence has no links in it. links available
- Nested Texts in order from top to bottom (
test
,inline links
,another link
,link that spans multiple lines...
)
Links are displayed in the above order in the TalkBack menu
Actual Results:
RESULT 1 (FAIL) - Swiping moves TalkBack focus in this wrong order
- Parent Text
This is a test of inline links in React Native. Here's another link. Here is a link that spans multiple lines because the text is so long. This sentence has no links in it. links available
- Swipe right or down moves the focus to the last link
link that spans multiple lines because the text is too long
.
2022-03-07.08-07-55.mp4
RESULT 2 (FAIL) - Links are NOT displayed in the correct order in the TalkBack menu
2022-03-07.08-17-20.mp4
@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @fabriziobertoglio1987 in 7b5b114. When will my fix make it into a release? | Upcoming Releases |
Summary: This issue fixes [32004][23]. The Pull Request was previously published by [blavalla][10] with [31757][24]. >This is a follow-up on [D23553222 (https://github.com/facebook/react-native/commit/b352e2da8137452f66717cf1cecb2e72abd727d7)][18], which made links functional by using [Talkback's Links menu][1]. We don't often use this as the sole access point for links due to it being more difficult for users to navigate to and easy for users to miss if they don't listen to the full description, including the hint text that announces that links are available. The Implementation of the functionality consists of: Retrieving the accessibility links and triggering the TalkBack Focus over the Text 1. nested Text components with accessibilityRole link are saved as [ReactClickableSpan][17] instances in Android native [TextView][20] ([more info][19]) 1. If the TextView contains any [ClickableSpans][15] (which are [nested Text][14] components with role link), set a view tag and reset the accessibility delegate. 3. Obtain each link description, start, end, and position relative to the parent Text (id) from the Span as an [AccessibilityLink][16] 4. Use the [AccessibilityLink][16] to display TalkBack focus over the link with the `getVirtualViewAt` method (more [info][13]) Implementing ExploreByTouchHelper to detect touches over links and to display TalkBack rectangle around them. 1. ReactAccessibilityDelegate inherits from [ExploreByTouchHelper][12] 2. If the [ReactTextView][21] has an accessibility delegate, trigger ExploreByTouchHelper method [dispatchHoverEvent][22] 3. Implements the methods `getVirtualViewAt` and `onPopulateBoundsForVirtualView`. The two methods implements the following functionalities (more [info][13]): * detecting the TalkBack onPress/focus on nested Text with accessibilityRole="link" * displaying TalkBack rectangle around nested Text with accessibilityRole="link" ## Changelog [Android] [Added] - Make links independently focusable by Talkback Pull Request resolved: facebook#33215 Test Plan: [1]. User Interacts with links through TalkBack default accessibility menu ([link][1]) [2]. The nested link becomes the next focusable element after the parent element that contains it. ([link][2]) [3]. Testing accessibility examples in pr branch ([link][3]) [4]. Testing accessibility android examples in pr branch ([link][4]) [7]. TalkBack focus moves through links in the correct order from top to bottom (PR Branch with [link.id][25]) ([link to video test][7]) ([discussion][26]) [8]. TalkBack focus does not move through links in the correct order from top to bottom (PR Branch without [link.id][25]) ([link to video test][8]) ([discussion][26]) Test on main branch [5]. Testing accessibility examples in main branch ([link][5]) [6]. Testing accessibility android examples in main branch ([link][6]) [1]: fabOnReact/react-native-notes#9 (comment) [2]: fabOnReact/react-native-notes#9 (comment) [3]: fabOnReact/react-native-notes#9 (comment) [4]: fabOnReact/react-native-notes#9 (comment) [5]: fabOnReact/react-native-notes#9 (comment) [6]: fabOnReact/react-native-notes#9 (comment) [7]: fabOnReact/react-native-notes#9 (comment) [8]: fabOnReact/react-native-notes#9 (comment) [10]: https://github.com/blavalla "blavalla github profile" [12]: https://github.com/aosp-mirror/platform_frameworks_base/blob/1ac46f932ef88a8f96d652580d8105e361ffc842/core/java/com/android/internal/widget/ExploreByTouchHelper.java#L48 "com/android/internal/widget/ExploreByTouchHelper.java#L48" [13]: fabOnReact/react-native-notes#9 (comment) "explanation of getVirtualViewAt and onPopulateBoundsForVirtualView" [14]: https://github.com/aosp-mirror/platform_frameworks_base/blob/1ac46f932ef88a8f96d652580d8105e361ffc842/core/java/android/text/Spannable.java#L3 "core/java/android/text/Spannable.java#L3" [15]: https://github.com/fabriziobertoglio1987/react-native/blob/561266fc180b96d6337d6c6c5c3323522d66cc44/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java#L70-L71 "react/views/text/ReactTextViewManager.java#L70-L71" [16]: https://github.com/fabriziobertoglio1987/react-native/blob/561266fc180b96d6337d6c6c5c3323522d66cc44/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactAccessibilityDelegate.java#L680-L685 "react/uimanager/ReactAccessibilityDelegate.java#L680-L685" [17]: https://github.com/facebook/react-native/blob/561266fc180b96d6337d6c6c5c3323522d66cc44/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java#L126-L129 "react/views/text/TextLayoutManager.java#L126-L129" [18]: facebook@b352e2d [19]: facebook#30375 (comment) "explanation on how nested Text are converted to Android Spans" [20]: https://github.com/aosp-mirror/platform_frameworks_base/blob/1ac46f932ef88a8f96d652580d8105e361ffc842/core/java/android/widget/TextView.java#L214-L220 "core/java/android/widget/TextView.java#L214-L220" [21]: https://github.com/facebook/react-native/blob/485cf6118b0ab0b59e078b96701b69ae64c4dfb7/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextView.java#L577 "dispatchHoverEvent in ReactTextView" [22]: https://github.com/aosp-mirror/platform_frameworks_base/blob/1ac46f932ef88a8f96d652580d8105e361ffc842/core/java/com/android/internal/widget/ExploreByTouchHelper.java#L120-L138 "dispatchHoverEvent in ExploreByTouchHelper" [23]: facebook#32004 [24]: facebook#31757 [25]: https://github.com/fabriziobertoglio1987/react-native/blob/485cf6118b0ab0b59e078b96701b69ae64c4dfb7/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactAccessibilityDelegate.java#L648 "setting link.id in the AccessibilityLink constructor" [26]: https://github.com/facebook/react-native/pull/33215/files/485cf6118b0ab0b59e078b96701b69ae64c4dfb7#r820014411 "comment on role of link.id" Reviewed By: blavalla Differential Revision: D34687371 Pulled By: philIip fbshipit-source-id: 8e63c70e9318ad8d27317bd68497705e595dea0f
Summary
This issue fixes 32004. The Pull Request was previously published by blavalla with 31757.
Retrieving the accessibility links and triggering the TalkBack Focus over the Text
getVirtualViewAt
method (more info)Implementing ExploreByTouchHelper to detect touches over links and to display TalkBack rectangle around them.
getVirtualViewAt
andonPopulateBoundsForVirtualView
.The two methods implements the following functionalities (more info):
Changelog
[Android] [Added] - Make links independently focusable by Talkback
Test Plan
1. User Interacts with links through TalkBack default accessibility menu (link)
2. The nested link becomes the next focusable element after the parent element that contains it. (link)
3. Testing accessibility examples in pr branch (link)
4. Testing accessibility android examples in pr branch (link)
7. TalkBack focus moves through links in the correct order from top to bottom (PR Branch with link.id) (link to video test) (discussion)
8. TalkBack focus does not move through links in the correct order from top to bottom (PR Branch without link.id) (link to video test) (discussion)
Test on main branch
5. Testing accessibility examples in main branch (link)
6. Testing accessibility android examples in main branch (link)