-
Notifications
You must be signed in to change notification settings - Fork 224
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
Improvements to Android accessibility #168
Conversation
@sfuqua thanks for this PR! I'm sorry I haven't reviewed it yet. Is there any customization that is needed to enable the equivalent for iOS (I know this implementation is Android only, but curious what the API is on iOS). Ideally, we'd match that as much as possible, my only thought is to make this configurable perhaps initially. |
@bradbumbalough It should work out of the box for iOS if I force iOS down the custom code path, but let me test that and get back to you. |
@bradbumbalough I was unable to test functionality on iOS due to the lack of TouchableNativeFeedback support, but... The Android In my latest push I added a disabled-by-default FWIW if #169 is close to ready, I'd recommend pausing getting this in until after that change - would definitely want to test my changes again for compatibility with rendering in a modal. |
@sfuqua I'm sorry this has taken so long -- I was finally able to wrap up review of the modal PR, which introduced a conflict into this one. I can take a shot at the conflict as it looks pretty straight forward, but if you to pull down and update it I'll move to getting this in the next release. |
@bradbumbalough no worries! I'll see if I can resolve conflicts today and re-test the changes in modal mode to make sure I didn't miss anything in that scenario. |
@bradbumbalough merged and fixed a bug. Current status:
If you like I can update the README to indicate |
Hi @bradbumbalough :) Do you have any concerns with this change that I can help with to get this merged? |
This is really good. Great work @sfuqua, thank you for the contribution! |
🎉 This PR is included in version 3.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hi there - love this library but have run into some trouble with Android screen reader support:
I didn't see existing issues for these (especially the second one), and can open if desired!
This changes adds the Android-focused prop
importantForAccessibility="yes"
to the sheet while it's being rendered, as well as"no-hide-descendants"
to the wrapped app content while the sheet is open. This ensures that only the action sheet content is accessible until it gets closed. This requires the app content to be wrapped in a new<View>
, because there's no guarantee that the user is passing in something that takes accessibility props), but this seems to work out fine withflex: 1
(and react-native-action-sheet is adding its own wrapper already anyway).I also added the "button" role to the action sheet options, and added a ref callback that will cause the screen reader to focus the first option/button in the list when the sheet opens (iOS has this behavior).
I tested this in my application and am happy with the results - our use case is rendering a React fragment inside of the ActionSheetProvider, I also tested it with a View being rendered as the child instead.
Also tested running the repo's example app on both my Android device (worked for all the examples) and the web variant. Verified that Narrator, using Edge on Windows, is reading the options as buttons now and no longer focuses the example app content while the sheet is open.
Happy to do additional testing if there scenarios that need to be covered.
New Android screen reader behavior (tested on a Galaxy S8, Android 9)
RN Documentation References: