-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Add TYPE_VIEW_HOVER_ENTER to AccessibilityNodeInfo sendAccessibilityEvent #34969
Conversation
"mHostView.addView adds the react views to the modal. The modal is a wrapper around react views. They are added to the modal. mDialog.setTitle does not display the title. The react-native implementation must have changed the functionality. Commenting addTitle creates issues. Try to call setTitle on the Activity (the Dialog is included in a Custom Activity) Investigate PhoneWindow.setTitle in particular onWindowTitleChanged and dispatchWindowAttributesChanged UPDATE: The method is WindowManager.setTitle" "https://github.com/fabriziobertoglio1987/react-native/blob/62f83a9fad027ef0ed808f7e34973bb01cdf10e9/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.java#L114 https://github.com/aosp-mirror/platform_frameworks_base/blob/e648d3cb91f9a0156c729ca18ec27e71f59f9ce2/core/java/android/view/WindowManager.java#L3794" "Read TalkBack logic around dialog and title. Research for string associated with the Dialog role Research for logic related to the title attribute" Read logic mDialog https://github.com/fabriziobertoglio1987/react-native/blob/62f83a9fad027ef0ed808f7e34973bb01cdf10e9/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.java#L285 Read logic from AOSP Dialog, Window and WindowManager https://github.com/aosp-mirror/platform_frameworks_base/blob/e648d3cb91f9a0156c729ca18ec27e71f59f9ce2/core/java/android/app/Dialog.java#L641 https://github.com/aosp-mirror/platform_frameworks_base/blob/e648d3cb91f9a0156c729ca18ec27e71f59f9ce2/core/java/android/view/Window.java#L1444 https://github.com/aosp-mirror/platform_frameworks_base/blob/e648d3cb91f9a0156c729ca18ec27e71f59f9ce2/core/java/android/view/WindowManager.java#L3801 "Quickly test the following solution: Implement prop accessibilityTitle Implement method DialogManager accessibilityTitle Call mDialog.setTitle(“my title”) The setter method setAccessibilityTitle is not called" "The setter method setAccessibilityTitle is not called, while the other props are called: Read other files in the same folder Read cpp fabric files Add fabric configurations Read reactnative.dev codegen instructions" "Troubleshoot codegen generation of the accessibilityTitle prop. The prop is not added to jave codegen files generated in ReactAndroid/generated/codegen/Modal. Make sure the prop is added and test the outcome: Read instructions on reactnative.dev on codegen types to use for this prop (optional) Codegen does not include accessibilityTitle prop. Make sure codegen adds the title. Try testing on Paper. If the issue does not reproduce on Paper, it is caused by a missing CPP prop configuration" https://reactnative.dev/docs/the-new-architecture/pillars-codegen
Base commit: 753bccc |
TODOs
|
- use Stringish instead of string (internal type used in react-native) - add Nullable to Java setAccessibilityTitle, we use setTitle(null) to clear and previous value
Base commit: 8b00b4f |
I'm curious what impact (if any) this property should have on iOS. As far as I know, iOS doesn't have the concept of accessibility-specific titles in UIAlertController. They only have the option for a visible text title (which is announced on display). We could attempt to polyfill this behavior into iOS by making an announcement upon display of the modal, but this will be tricky as the modal will likely already be announcing something else (whatever gets focused), and I'm not sure it's worth the complexity. Maybe we should simply have this prop be named "title" and set a visible title for both iOS and Android, rather than making this title for accessibility only to simplify things here. As long as there is some way to give these dialogs a title for both platforms that is announced on display, I don't think it matters much if that title is only for accessibility services. It likely should be visible for all users anyways. |
This is the result of my research. The modal is implemented in Fabric with the RCTModalHostViewComponentView (UIView) and RCTFabricModalHostViewController (UIViewController) as explained in this conversation, it is possible to set a title in the navigation bar, but react-native does not handle navigation which is managed with other libraries (an ex. in react-native-navigation or react navigation), so I would not follow those solutions. Another solution I tested, was triggering the announcement after delay of X seconds:
Video of the above delayed announcement https://drive.google.com/file/d/1MMXHFraj6q2LZTNqoORB69C8HZ2HtUzg/view?usp=sharing announceForAccessibilityWithOptions queue would not work in this case as explained (StackOverflow explanation, detailed breakdown) https://reactnative.dev/docs/accessibilityinfo#announceforaccessibilitywithoptions The same issue affected PR #33468. The solution implemented in #33468 consisted of: 1) Trigger manually the announcement of the VoiceOver error message with method setAccessibilityErrorMessage
2) Trigger manually the VoiceOver text announcement after text changes (setAttributedText)
A queue mechanism could be built in the future to fix this issue (example includere here).
Tomorrow I will try an alternative JavaScript approach in the Modal Component to implement the above requirement. Thanks a lot for the feedback @blavalla. |
changes discussed in facebook#34969 (comment) which consist in: - Modal accepts prop TitleComponent which is a React.Element or Component and represents the title of the modal. The solution is taken from ListHeaderComponent in VirtList - AccessibilityInfo.sendAccessibilityEvent is triggered to focus on the Title when the modal opens - sendAccessibilityEvent needs to set a timeout of 1s (see facebook#30097 and https://stackoverflow.com/questions/28472985/android-set-talkback-accessibility-focus-to-a-specific-view) - TitleComponent Styling and positioning is defined outside of reactnative This has to be reviewed as ListHeaderComponent uses ListHeaderComponentStyle Notes: "Improvements: Trigger focus on components using Fabric API (findNodeHandle is deprecated, setAccessibilityFocus). setAccessibilityFocus has several issues (stackoverflow), and I can not set a ref on a modal. componentDidMount seems to trigger before rendering the Modal children, the lifecycle does not work correctly with a modal on Fabric. You need to use forwardRef to call sendAccessibilityEvent Need to change the way you create the title component so that you create a forwardRef and then use this to send the accessibility event" "https://reactnative.dev/docs/new-architecture-library-intro#preparing-your-javascript-codebase-for-the-new-react-native-renderer-fabric https://reactnative.dev/docs/accessibilityinfo#setaccessibilityfocus https://reactnative.dev/docs/new-architecture-library-intro#preparing-your-javascript-codebase-for-the-new-react-native-renderer-fabric" "Test Text component solution without using getNativeRef, as the problem could be caused by timeout and not ref. Check the keys and try to use the _nativeRef Implement getNativeRef for the Text component and use it in Modal to call setAccessibilityFocus" https://github.com/fabriziobertoglio1987/react-native/blob/accbbfc01af57eda34374ce3b13e36b6e7c12a93/Libraries/Components/TextInput/TextInput.js#L1213 "Adding setTimeout with a wait of 1-second fixes issues with setAccessibilityFocus does not onLoad The reason is triggered focus on other elements, so we need to use an API to wait for the focus to display on that element Try to use runAfterInteraction" "facebook#30097 https://reactnative.dev/docs/next/interactionmanager#runafterinteractions" "Review meeting notes and Brett suggestion Consider removing the sendAccessibilityEvent as violates WCAG 2.0 regulations, including 3.2.1 and 3.2.3" Test Android "- The title component displays under the Modal on Android. The title receives focus with the ModalPresentation example. The title does not receive focus in the ModalOnShow example. => In both examples, the titles displays under the modal => In ModalOnShow, the title does not receive focus Apply the same prop as in the ModalPresentation and see if the title receives focus Remove style Put the title directly in the ModalOnShow and trigger the focus Trigger the focus manually with a button instead of using useEffect( _ref ) Trigger focus manually with a button in the Modal.js" "setAccessibilityFocus correctly moves the focus when triggered with Button. The issue could be caused: ref is undefined read conversation facebook#30097 alternative callback to _showModal (onLoad) TalkBack moves focus after calling sendAccessibilityEvent => increase timeout" facebook#30097 "Test functionality on iOS: test functionality on iOS with Xcode build" "Improve solution and finalize diff before final commit: Fix issue with sendAccessibilityEvent based on stackoverflow solution or runAfterIteraction (setAccessibilityFocus does not onLoad) TitleComponent should move to rn-tester ModalPresentation and ModalOnShow forwardRef may not work" "https://github.com/fabriziobertoglio1987/react-native/blob/accbbfc01af57eda34374ce3b13e36b6e7c12a93/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java#L968 https://stackoverflow.com/questions/28472985/android-set-talkback-accessibility-focus-to-a-specific-view facebook#30097"
PR build artifact for 5e1e72f is ready. |
PR build artifact for 5e1e72f is ready. |
|
PR build artifact for 4213491 is ready. |
PR build artifact for 4213491 is ready. |
PR build artifact for bf37a34 is ready. |
PR build artifact for bf37a34 is ready. |
PR build artifact for dc4c54e is ready. |
PR build artifact for dc4c54e is ready. |
iOS - testing viewHoverEvent on iOS
view_hover_event_ios.mp4 |
@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@lunaleaps merged this pull request in a0adf57. |
…vent (facebook#34969) Summary: - Adds `AccessibilityEvent.TYPE_VIEW_HOVER_ENTER` to AccessibilityNodeInfo sendAccessibilityEvent - Adds an example implementation. fixes facebook#30860 fixes facebook#30097 Related Documentation facebook/react-native-website#3438 ## Changelog [Android] [Added] - Add TYPE_VIEW_HOVER_ENTER to AccessibilityNodeInfo sendAccessibilityEvent Pull Request resolved: facebook#34969 Test Plan: Android: facebook#34969 (comment) facebook#34969 (comment) iOS: facebook#34969 (comment) facebook#34969 (comment) Reviewed By: christophpurrer Differential Revision: D42613990 Pulled By: lunaleaps fbshipit-source-id: 8c8950610799dcc74067d2b47b44d4ff030f66e5
Summary
AccessibilityEvent.TYPE_VIEW_HOVER_ENTER
to AccessibilityNodeInfo sendAccessibilityEventfixes #30860 fixes #30097
Related Links:
findNodeHandle
andmeasure*()
New Architecture Migration Steps react-native-website#3488 455ca8csetNativeProps
off of refs. react-native-community/discussions-and-proposals#72 RN: ImplementsendAccessibilityEvent
in RN Renderer that proxies between Fabric/non-Fabric react#20554 https://github.com/facebook/react/compare/main...fabriziobertoglio1987:react:adding-send-accessibility-event-to-ref?expand=1Changelog
[Android] [Added] - Add TYPE_VIEW_HOVER_ENTER to AccessibilityNodeInfo sendAccessibilityEvent
Test Plan
Android: #34969 (comment) #34969 (comment)
iOS: #34969 (comment) #34969 (comment)