Skip to content
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

Fix: allow screen readers to detect modal content #1764

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

flo-sch
Copy link

@flo-sch flo-sch commented Mar 4, 2024

Hello @gorhom, thank you for this nice package :)

Here is a small contribution based on accessibility findings using TalkBack on Android and Voice Over on iOS.
If you are interested I could also port it to version 5

Motivation

In the project I work on, accessibility support is a requirement. I noticed a few issues when integrating <BottomSheetModal /> (we currently do not use <BottomSheet />):

  1. Children cannot be read by Voice Over (iOS), only the modal itself is interpreted as a single "adjustable" view (my guess here is that elements with "accessibleRole: 'adjustable'" are not meant to have children? I could be wrong, this is just an assumption)
  2. Text elements cannot be interpreted by Talk Back (Android) when using the default background, due to its accessibility props. This could easily be workaround by providing a custom background but I figured I could also propose this fix here.
  3. I also face the issues described in [v4] When Talkback is enabled and a modal is displayed, the items below the modal sheet are still accessible by tap and the sheet is not focused #1641, but I did not manage to understand how to fix those myself unfortunately. I tried implementing some state in BottomSheetModalProvider (as initially proposed in [BottomSheetModal], a prop should allow elements below the sheet to not be accessible and the sheet should receive focus when presented #687), but since modals can be closed/dismissed in different ways, I could not implement a reliable state determining whether or not a modal is currently being presented (and adjust importantForAccessibility props based on that).

I recorded videos to illustrate what this PR fixes:

  1. TalkBack

https://github.com/gorhom/react-native-bottom-sheet/assets/1577155/a7fa04c4-2e02-429a-a0f6-6e51d5644cd2
https://github.com/gorhom/react-native-bottom-sheet/assets/1577155/106c7c0c-c625-450a-ba29-3c34a3eb2927

  1. Voice Over (unavailable in iOS simulator, but I used Accessibility Inspector instead)

https://github.com/gorhom/react-native-bottom-sheet/assets/1577155/dd0f8f89-71ec-49be-8d0a-09dd84943d6e
https://github.com/gorhom/react-native-bottom-sheet/assets/1577155/2d5ca920-d1e9-4afc-9b67-38122e016844

@Eli-Nathan
Copy link

Eli-Nathan commented Mar 12, 2024

I think this can currently be overridden by passing accessible={false} to the BottomSheet.
Accessibility was only just recently added back in to this repo so you may need to update to at least 4.6.0

#1288
Release notes

@flo-sch
Copy link
Author

flo-sch commented Mar 25, 2024

I think this can currently be overridden by passing accessible={false} to the BottomSheet.
Accessibility was only just recently added back in to this repo so you may need to update to at least 4.6.0

Hi @Eli-Nathan, setting accessible to false fixes it for VoiceOver only (iOS). I included that in this PR (or well, setting it to null which works as well)

I already tried 4.6.0 and 4.6.1

Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@flo-sch
Copy link
Author

flo-sch commented Apr 30, 2024

Un-stale

@thienautran
Copy link

I've been able to get focus to child elements by using accessible=false, however still haven't been able to figure out how to make elements behind the bottom sheet inert, either with BottomSheetModal or regular BottomSheet. Anyone have any ideas on how to fix this?

@flo-sch
Copy link
Author

flo-sch commented May 6, 2024

I believe it would require some implementation from this library first, as suggested there. At least I have not found any way to achieve that yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants