Adding pager, scrollview, viewgroup, webview, drawer roles #34477
Conversation
…mmit 2a19cb8) Add roles to strings_unlocalized.xml Call setRoleDescription for all roles not available in compositor.json https://github.com/google/talkback/blob/771de7cdbf55b6adb4ca4c64c27a52584f2337cc/compositor/src/main/res/raw/compositor.json#L1082-L1108 facebook#30839 facebook@da899c0 https://github.com/google/talkback/blob/771de7cdbf55b6adb4ca4c64c27a52584f2337cc/compositor/src/main/res/values/strings.xml#L223
facebook-github-bot
added
the
CLA Signed
Base commit: 12e5842 |
Base commit: 12e5842 |
fabOnReact
changed the title
|
DrawerLayoutAndroid not correctly announcing
TalkBack announces menu when opening the DrawerLayout. https://github.com/google/talkback/blob/771de7cdbf55b6adb4ca4c64c27a52584f2337cc/compositor/src/main/res/raw/compositor.json#L312-L318 No changes are required. The logic is triggered in the DrawerLayout class. The below behavior is NOT correct. The announcement is not complete. 2022-08-23.19-39-31.mp4 |
sliding drawer https://user-images.githubusercontent.com/24992535/186176021-ca3d8380-1ff9-449f-a738-f954edfe6328.mp4 drawerlayout (left) https://user-images.githubusercontent.com/24992535/186176241-d84b2458-6928-4a33-957e-86ec8b427835.mp4 drawerlayout (right) https://user-images.githubusercontent.com/24992535/186176280-4389b45c-1b0f-4d62-b6f8-29210f154558.mp4 iconmenu https://user-images.githubusercontent.com/24992535/186176715-57c6a752-dff2-4088-820c-886d10b70378.mp4
TalkBack announces the Toast text without accessibilityRole
The logic for the Toast TalkBack announcement is triggered here https://github.com/aosp-mirror/platform_frameworks_base/blob/e648d3cb91f9a0156c729ca18ec27e71f59f9ce2/core/java/android/widget/Toast.java#L617. The announcement is triggered manually without using AccessibilityDelegate. No changes or role are required to announce Toast. 2022-08-24.16-25-46.mp4 |
1) Remove toast AccessibilityRole facebook#34477 (comment) 2) Adding ScrollView AccessibilityRole
ScrollView announces in/out of ScrollView
2022-08-24.17-36-05.mp4 |
fabOnReact
changed the title
fabOnReact
changed the title
facebook-github-bot
added
the
Shared with Meta
| @@ -154,7 +165,21 @@ public static String getValue(AccessibilityRole role) { | |||
| case LIST: | |||
| return "android.widget.AbsListView"; | |||
| case GRID: | |||
| case SCROLLVIEW: | |||
| case HORIZONTALSCROLLVIEW: | |||
There was a problem hiding this comment.
I think SCROLLVIEW and HORIZONTALSCROLLVIEW should be returning their own classes.
See the talkback source here:
https://github.com/google/talkback/blob/f5d564fdc915a74d8cde4868608f307de9ccf957/utils/src/main/java/com/google/android/accessibility/utils/Role.java#L341-L347
It's only if they have collectionInfo that they fall back to the Grid or List role, and that can happen internally to Talkback, so we don't need to worry about that logic.
There was a problem hiding this comment.
@blavalla Thanks. I made the changes and also added an example of ScrollView to the Accessibility Example. I reviewed the react-native implementation of these accessibility functionalities, comparing them with the original Android implementation in their respective widgets. I tested the TalkBack functionalities and I did not detect any issue.
| <string | ||
| name="scrollview_description" | ||
| translatable="false" | ||
| >Scroll View</string> | ||
| <string | ||
| name="horizontalscrollview_description" | ||
| translatable="false" | ||
| >Horizontal Scroll View</string> | ||
| <string | ||
| name="slidingdrawer_description" | ||
| translatable="false" | ||
| >Sliding Drawer</string> | ||
| <string | ||
| name="iconmenu_description" | ||
| translatable="false" | ||
| >Icon Menu</string> |
There was a problem hiding this comment.
I don't think there's any value in adding strings for these roles unless Talkback does this itself. Some roles are purely functional and don't have different announcements upon focus. A user may get confused when they hear "Scroll View" since they aren't used to hearing these announcements in other native apps not built with React Native.
If Talkback does add these announcements though, feel free to keep these, but I couldn't find them referenced anywhere in Talkback's source.
And yeah, I realize this is a problem with existing roles added by others, but many of those are web-based, or ios-based roles that don't have any behavior defined by Talkback at all, so we aren't sure that it should do for those.
There was a problem hiding this comment.
@blavalla Thanks a lot. I reviewed the TalkBack implementation, and I made the changes requested.
roles scrollview and grid
2022-09-02.17-15-55.mp4DrawerLayoutAndroid with or without drawerlayout role
2022-09-02.17-41-40.mp4 |
Testing chime_up and chime_down sound feedback in Scrollable
2022-09-07.11-59-58.mp42022-09-07.12-03-10.mp4 |
fabOnReact
changed the title
fabOnReact
changed the title
fabOnReact
changed the title
|
@blavalla has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| | 'grid' | ||
| | 'pager' | ||
| | 'scrollview' | ||
| | 'horizontalscrollview' | ||
| | 'viewgroup' | ||
| | 'webview' | ||
| | 'drawerlayout' | ||
| | 'slidingdrawer' | ||
| | 'iconmenu'; |
There was a problem hiding this comment.
I thought the intent was for these to map roughly to ARIA roles? https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles
Are there existing cases where we have custom roles for things like this? Some of these seem like they don't quite fit with the theme of specifying a semantic role (e.g. I cannot imagine there ever being a "drawerlayout" or "webview" ARIA role).
cc @necolas
There was a problem hiding this comment.
This PR isn't related to aria, so there may be two different issues in this space. This was a PR that resolved the issue of not all native Android roles being supported in RN, which means components built in RN have different behavior than if those components were built natively.
Aria has no concepts that map to many of these (and "webview" specifically wouldn't even make sense for a web-spec like aria), but RN components need these roles if we want them to work like their native implementations.
Whether we want to support native-specific functionality like this comes down to a pretty core question about the RN framework itself, which is whether it's trying to be a framework that bridges the gap between iOS, Android, and other native platforms, or if it's trying to bring web-specific paradigms (such as aria) to those native platforms.
If it's meant to bring web paradigms to those platforms, we'll basically need to rethink the entire set of accessibility APIs and start from scratch, as they are currently almost entirely focused on the native-specific approaches that accessibility takes. The native approach to accessibility is often considerably different from the approach taken on the web, mainly due to the differences in the way accessibility services themselves are built on native platforms.
Things like custom actions, hints, magic tap, certain roles/traits, etc. that are native-specific would need to be left out. And things like tab-index, landmarks, and other web-specific features would have to be implemented from scratch.
The end result of this is that apps built with React Native would "feel" more like the web to users of assistive technology, and not feel like native apps. We've already heard this feedback about pretty basic roles like "radio group", which confused some users as they didn't expect it to be in a native application (@alextait1 has more details on that feedback).
Overall I think this is a decision the RN team needs to make, and provide clarity on, as right now the accessibility API is in a pretty weird state, with some web-specific features attempted to be included in (and not working the way they do on the web), some web-specific features left out (when they could work on native), some native-specific features left out (that have no web equivalents), and some native-specifics features includes with slightly different APIs, when there is a similar enough web equivalent to map to.
There was a problem hiding this comment.
The role prop is typed separately and equivalent values are then mapped to accessibilityRole values, so at a glance this looks ok as it's adding values to the latter.
There was a problem hiding this comment.
Hmm, if we have separated out role and accessibilityRole, then it seems like this could be a clean separation, where accessibilityRole can have roles that might be platform specific, divergent from ARIA. As long as any mapping still works correctly, I think this change should make sense then.
It does seem like we have a bit of a messy experience for developers trying to reason about what a role does, if it works, what platforms it works on, etc. If platforms do really have specific needs not represented in a common spec (and it seems like they still do), I am all in favor of letting RN expose them.
It might be worth leaving documentation as to what roles are platform specific. In code/on the website (@fabriziobertoglio1987 I'm guessing this change will be reflected in a documentation update?).
The TypeScript typings adjacent in ViewAccessibility.d.ts (which we should update, either as part of this change or a followup), have a split between platforms for individual properties, but not role. Maybe that can be split, so that users get some hint? Though platform-specific typechecking is a more general open problem (FYI @acoates-ms who has cared about that in the past).
There was a problem hiding this comment.
Also FYI to @lunaleaps that it looks like role and the ARIA types are missing from the TS declarations. We should update those over, if we are not immediately moving that file to TS generation.
There was a problem hiding this comment.
@NickGerleman this notifications did not go in my inbox for some reason. Sorry. The documentation is available at https://github.com/facebook/react-native-website/pull/3287/files
|
This pull request was successfully merged by @fabriziobertoglio1987 in 55c0df4. When will my fix make it into a release? | Upcoming Releases |
…34477) Summary: - adds missing roles - adds custom roles that don't exist in TalkBack (see the [compositor.json][10] and [string.xml][11] files). - fixes [issues with Drawer][12] - fixes issues with ScrollView missing roles - seek control already exist as adjustable facebook@d460d09 Relevant facebook#30839 (comment) fixes facebook#30839 ## Changelog [Android] [Fixed] - Adding pager, scrollview, viewgroup, webview, drawer roles Pull Request resolved: facebook#34477 Test Plan: Android - Drawer Layout and ScrollView (02/09/22) facebook#34477 (comment) - sliding drawer, drawer layout, icon menu facebook#34477 (comment) - Horizontal and Vertical ScrollView facebook#34477 (comment) - Toast facebook#34477 (comment) - CheckedTextView facebook#34477 (comment) - Spinner (dropdownlist) facebook#34477 (comment) - EditText facebook#34477 (comment) - WebView facebook#34477 (comment) - Testing chime_up and chime_down sound feedback in Scrollable facebook#34477 (comment) iOS facebook#34477 (comment) [10]: https://github.com/google/talkback/blob/771de7cdbf55b6adb4ca4c64c27a52584f2337cc/compositor/src/main/res/raw/compositor.json#L1082-L1108 [11]: https://github.com/google/talkback/blob/771de7cdbf55b6adb4ca4c64c27a52584f2337cc/compositor/src/main/res/values/strings.xml#L223 [12]: facebook#34477 (comment) Reviewed By: NickGerleman Differential Revision: D39894307 Pulled By: blavalla fbshipit-source-id: 4a8da78bae485ead0523689631d88d1031a07b74
Summary
Relevant #30839 (comment)
fixes #30839
Documentation facebook/react-native-website#3287
Changelog
[Android] [Fixed] - Adding pager, scrollview, viewgroup, webview, drawer roles
Test Plan
Android
iOS #34477 (comment)