Porting BottomSheet accessibility from v2 to v4 #1288

Mahmoud-SK · 2023-02-08T11:39:32Z

Motivation

I'm picking up this task of porting accessibility features from V2 to V4 as per #548

jenskuhrjorgensen · 2023-02-09T20:11:51Z

Awesome work <3

This missing functionality is what is keeping us on v2 of this library!

BethThomas141 · 2023-03-06T11:09:56Z

Would love to see this merged in 👍

github-actions · 2023-04-06T09:40:06Z

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.

BethThomas141 · 2023-04-06T09:41:48Z

Commenting to keep this open - would be a real help if this gets merged in.

BethThomas141 · 2023-04-26T15:59:00Z

@gorhom any chance of this going in soon? 🙏

Mahmoud-SK · 2023-04-26T19:55:39Z

We would very much also like to get this merged in. @gorhom

manipatel30 · 2023-05-04T06:40:01Z

@gorhom Is this change available in 4.4.6 ?
Ohh sorry PR is still OPEN

github-actions · 2023-06-03T09:40:02Z

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.

jenskuhrjorgensen · 2023-06-03T15:14:04Z

Please keep this open - we are still longing for it!

jenskuhrjorgensen · 2023-06-22T13:10:43Z

Hi @gorhom

I'm sure you are super busy maintaining this awesome library, but can we at least get a comment on why this PR is not getting any attention? To me it is a fairly small PR that only adds some accessibility capabilities and doesn't touch any core functionality that risks breaking the lib.

Please just give us a small sign of life from you and let us know if there is anything we can do in order to get this merged <3

Mahmoud-SK · 2023-08-11T09:47:30Z

Hi @gorhom I see you have assigned yourself to this PR some time ago now, do you have any ETA on when we can get this reviewed and approved?

We are still very much waiting on this. ❤️

BethThomas141 · 2023-09-29T09:44:29Z

@gorhom Please can you take a quick look at this and merge it in? My app cares about users with accessibility requirements so we really need this

C-Dykes · 2023-10-31T19:42:54Z

Any news on this? its still an issue

jenskuhrjorgensen · 2023-11-10T13:47:43Z

Hi again @gorhom

We are currently running on an old forked version of your awesome lib, but now we have to bump in order to get to use reanimated v3. However, since v4 of this lib still doesn't cater for accessibility users, we will unfortunately have to create a new fork of react-native-bottom-sheet, since accessibility support is being enforced in more and more countries. For instance, the European Accessibility Act (EEA) has already been implemented in several European countries including some of the countries that we operate in:

The European Accessibility Act applies to most private companies. However, it is not yet fully enacted: The legislation will be enforced by July 28, 2025, and member states have until June 28, 2022 to adopt the EAA into their national laws.
Source: https://www.boia.org/blog/european-union-accessibility-laws-an-overview

I believe this PR is fairly simple as it doesn't touch any core or complex functionality but instead just adds some additional accessibility props but if there is something that you would like to have done in a different way, please at least provide us with a review and some feedback on whatever is holding you back from reviewing/approving/merging this PR. If you have been inactive on this PR because you think "accessibility" is just dull and doesn't matter (I don't blame you if you do - I did so too) I think the EEA and below quote shows that accessibility is actually quite important and is more and more becoming a standard requirement/expected feature both by end users as well as developers:

The percentage of Americans with disabilities is a whopping 26%, which equals roughly one in four adults with disabilities on a population level. To be able to reach as many people as possible, you need to make your website accessible to people with disabilities.
Source: https://monsido.com/blog/accessibility-statistics

Looking forward to hear an answer from you that can hopefully stop our process of forking yet another version of react-native-bottom-sheet.

Best regards
Jens

jenskuhrjorgensen

Hi @Mahmoud-SK

I've added some review comments to your PR in order to maybe make the change a little smaller in an attempt to maybe get this merged/prioritized.

Let me know what you think.

Best regards
Jens

jenskuhrjorgensen · 2023-12-05T09:43:17Z

src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx

+        accessibilityRole={_providedAccessibilityRole ?? undefined}
+        accessibilityLabel={_providedAccessibilityLabel ?? undefined}
+        accessibilityHint={_providedAccessiblityHint ?? undefined}
+        {...rest}


@Mahmoud-SK why is the spread of the rest props needed?

It's needed to pass the rest of the properties to the Animated.View - in this case it's only animatedPosition-prop.

jenskuhrjorgensen · 2023-12-05T09:45:03Z

src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx

+        accessible={_providedAccessible ?? undefined}
+        accessibilityRole={_providedAccessibilityRole ?? undefined}
+        accessibilityLabel={_providedAccessibilityLabel ?? undefined}
+        accessibilityHint={_providedAccessiblityHint ?? undefined}


@Mahmoud-SK can we maybe do this in order to not break the existing behaviour?

accessibilityHint ={_providedAccessiblityHint ? _providedAccessibilityHint : `Tap to ${ typeof pressBehavior === 'string' ? pressBehavior : 'move' } the Bottom Sheet`}

jenskuhrjorgensen · 2023-12-05T09:48:44Z

src/components/bottomSheet/BottomSheet.tsx

@@ -323,7 +341,7 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
      const extendedPositionWithKeyboard = Math.max(
        0,
        animatedContainerHeight.value -
-          (animatedSheetHeight.value + keyboardHeightInContainer)
+        (animatedSheetHeight.value + keyboardHeightInContainer)


I think this was correctly indented before - can you revert this change?

It looks correctly indented locally, so I'm not completely sure why it shows it like this.

jenskuhrjorgensen · 2023-12-05T09:49:12Z

src/components/bottomSheet/BottomSheet.tsx

+          adjustedSnapPoints,
+          adjustedSnapPointsIndexes,
+          Extrapolate.CLAMP
+        )


I think this was correctly indented before - can you revert this change?

Same as the previous comment, locally it's indented.

jenskuhrjorgensen · 2023-12-05T09:49:43Z

src/components/bottomSheet/BottomSheet.tsx

-            )
+            _keyboardHeight -
+            Math.abs(bottomInset - animatedContainerOffset.value.bottom)
+          )


Wasn't this also correctly indented before?

Same as the previous comment, locally it's indented.

src/components/bottomSheetBackdrop/constants.ts

src/components/bottomSheet/constants.ts

src/components/bottomSheetHandle/BottomSheetHandle.tsx

src/components/bottomSheetHandle/constants.ts

src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx

jenskuhrjorgensen · 2023-12-06T12:21:23Z

src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx

+        accessibilityRole={_providedAccessibilityRole ?? undefined}
+        accessibilityLabel={_providedAccessibilityLabel ?? undefined}
+        accessibilityHint={_providedAccessiblityHint ?? undefined}
+        {...rest}
      >
        {children}
      </Animated.View>


I think you also need to add the accessibility props to the <Animated.View> below:

<Animated.View style={containerStyle} pointerEvents={pointerEvents} accessible={_providedAccessible ?? undefined} accessibilityRole={_providedAccessibilityRole ?? undefined} accessibilityLabel={_providedAccessibilityLabel ?? undefined} accessibilityHint={_providedAccessibilityHint ?? undefined} {...rest} > {children} </Animated.View>

Maybe add this to a local const and use that because it is the exact same component as is used inside <TapGestureHandler/> when pressBehavior !== 'none'.

Good idea, done.

liviu-padurariu · 2023-12-21T06:38:04Z

Hi all, is there anything stopping for this branch to be merged?

gorhom

thanks @Mahmoud-SK, for submitting this pr, i have left some comments. and most importantly i think we should not introduce the change announcement, since i believe it is on the user to set it up not the library.

src/components/bottomSheet/types.d.ts

src/components/bottomSheet/BottomSheet.tsx

src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx

src/components/bottomSheetHandle/BottomSheetHandle.tsx

Mahmoud-SK · 2024-01-02T15:52:47Z

Hi @gorhom I've went ahead and done the changes that you requested. 😄 Could you please have a look again?

Thanks in advance!

removed the change announcement part of the code, and the ..rest from the two mentioned files.

gorhom · 2024-01-03T19:24:03Z

rebased the pr branch.. going to test the changes on the backdrop then i'll merge it

gorhom · 2024-01-03T19:29:12Z

thanks @Mahmoud-SK for submitting this PR, i appreciate your amazing work 🙌

this should be release on the next few days

jenskuhrjorgensen · 2024-01-03T19:46:47Z

Awesome news, @gorhom!

The accessibility features were once added to V2 but were not added to newer versions of the lib. Will the changes in this PR be added to V5 or should we create a separate PR for that towards the V5 branch? Just to avoid having to request this feature again for the next major version.

* Added accessibility to the bottomSheet component * Accessibility changes - BottomSheetBackdrop * Accessibility changes - BottomSheetHandle * Minor adjustments after review * removed unused spread operator * fix: removed change announcement removed the change announcement part of the code, and the ..rest from the two mentioned files.

gorhom · 2024-01-03T21:07:30Z

Awesome news, @gorhom!

The accessibility features were once added to V2 but were not added to newer versions of the lib. Will the changes in this PR be added to V5 or should we create a separate PR for that towards the V5 branch? Just to avoid having to request this feature again for the next major version.

I have already cherrypicked this PR into v5 latest release 👏

TrustDec · 2024-01-05T07:11:21Z

Awesome news, @gorhom!
The accessibility features were once added to V2 but were not added to newer versions of the lib. Will the changes in this PR be added to V5 or should we create a separate PR for that towards the V5 branch? Just to avoid having to request this feature again for the next major version.

I have already cherrypicked this PR into v5 latest release 👏

Hi, can you take a look at this question? #1560 (comment)

aweary

@gorhom we recently updated our fork of this library and this commit broke VoiceOver for all of our sheets. I don't think this change makes sense for any sheet that would have interactive content in it (which I imagine is most?).

I would recommend reverting, this doesn't improve accessibility at all.

aweary · 2024-03-04T17:17:57Z

src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx

-        accessibilityHint={`Tap to ${
-          typeof pressBehavior === 'string' ? pressBehavior : 'move'
-        } the Bottom Sheet`}
+        accessible={_providedAccessible ?? undefined}


Why would you mark the root View as accessible for the bottom sheet? That makes it so you cannot focus any of the descendant views (which is all of the sheet content) with VoiceOver on iOS.

Yep, just tested this and this is the problem I'm seeing as well. @flo-sch I see you just opened an accessibility PR, did you want to take a look at this as well there?

…-changes revert gorhom#1288

…Mahmoud-SK

…Mahmoud-SK)" This reverts commit 2dea730

haileyok · 2024-04-29T23:39:27Z

@gorhom We also had to revert this for the same reasons aweary mentioned. I think it would probably be a good idea revert as well, especially if this has made it's way into v5.

Mahmoud-SK changed the title ~~[WIP] Porting BottomSheet accessibility from v2 to v4~~ Porting BottomSheet accessibility from v2 to v4 Feb 8, 2023

github-actions bot added the no-pr-activity label Apr 6, 2023

github-actions bot removed the no-pr-activity label Apr 7, 2023

github-actions bot added the no-pr-activity label Jun 3, 2023

github-actions bot removed the no-pr-activity label Jun 4, 2023

gorhom self-assigned this Jun 25, 2023

gorhom added v4 Written in Reanimated v2 v5 labels Jun 25, 2023

jenskuhrjorgensen reviewed Dec 5, 2023

View reviewed changes

jenskuhrjorgensen reviewed Dec 6, 2023

View reviewed changes

src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx Outdated Show resolved Hide resolved

jenskuhrjorgensen reviewed Dec 6, 2023

View reviewed changes

gorhom requested changes Jan 1, 2024

View reviewed changes

Mahmoud-SK added 2 commits January 3, 2024 20:19

Added accessibility to the bottomSheet component

f6f52aa

Accessibility changes - BottomSheetBackdrop

4144828

fix: removed change announcement

01d49e2

removed the change announcement part of the code, and the ..rest from the two mentioned files.

gorhom force-pushed the feature/accessibility-to-v4 branch from a76555c to 01d49e2 Compare January 3, 2024 19:23

gorhom approved these changes Jan 3, 2024

View reviewed changes

gorhom merged commit 275bd64 into gorhom:master Jan 3, 2024

fwielstra mentioned this pull request Jan 8, 2024

[v4] How to use the 'new' accessibility overrides? #1687

Closed

aweary reviewed Mar 4, 2024

View reviewed changes

Eli-Nathan mentioned this pull request Mar 12, 2024

Fix: allow screen readers to detect modal content #1764

Closed

haileyok added a commit to bluesky-social/react-native-bottom-sheet that referenced this pull request Apr 29, 2024

revert gorhom#1288

4af77ba

This was referenced Apr 29, 2024

revert #1288 discord/react-native-bottom-sheet#19

Closed

revert #1288 bluesky-social/react-native-bottom-sheet#1

Merged

haileyok added a commit to bluesky-social/react-native-bottom-sheet that referenced this pull request Apr 29, 2024

Merge pull request #1 from bluesky-social/hailey/revert-accessibility…

855024f

…-changes revert gorhom#1288

haileyok added a commit to bluesky-social/react-native-bottom-sheet that referenced this pull request Apr 29, 2024

Revert "feat: added accessibility overrides support (gorhom#1288)(by @…

2b3f77e

…Mahmoud-SK)" This reverts commit 2dea730

haileyok mentioned this pull request Apr 29, 2024

switch branch for installing react-native-bottom-sheet bluesky-social/social-app#3760

Merged

GleidsonDaniel mentioned this pull request Jul 4, 2024

feat: migrate to bluesky-social/react-native-bottom-sheet RocketChat/Rocket.Chat.ReactNative#5769

Merged

10 tasks

Porting BottomSheet accessibility from v2 to v4 #1288

Porting BottomSheet accessibility from v2 to v4 #1288

Conversation

Mahmoud-SK commented Feb 8, 2023 • edited Loading

Motivation

jenskuhrjorgensen commented Feb 9, 2023

BethThomas141 commented Mar 6, 2023

github-actions bot commented Apr 6, 2023

BethThomas141 commented Apr 6, 2023

BethThomas141 commented Apr 26, 2023

Mahmoud-SK commented Apr 26, 2023

manipatel30 commented May 4, 2023 • edited Loading

github-actions bot commented Jun 3, 2023

jenskuhrjorgensen commented Jun 3, 2023

jenskuhrjorgensen commented Jun 22, 2023

Mahmoud-SK commented Aug 11, 2023 • edited Loading

BethThomas141 commented Sep 29, 2023

C-Dykes commented Oct 31, 2023

jenskuhrjorgensen commented Nov 10, 2023

jenskuhrjorgensen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenskuhrjorgensen Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liviu-padurariu commented Dec 21, 2023

gorhom left a comment

Choose a reason for hiding this comment

Mahmoud-SK commented Jan 2, 2024

gorhom commented Jan 3, 2024 • edited Loading

gorhom commented Jan 3, 2024

jenskuhrjorgensen commented Jan 3, 2024

gorhom commented Jan 3, 2024

TrustDec commented Jan 5, 2024

aweary left a comment • edited Loading

Choose a reason for hiding this comment

aweary Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haileyok commented Apr 29, 2024

Mahmoud-SK commented Feb 8, 2023 •

edited

Loading

manipatel30 commented May 4, 2023 •

edited

Loading

Mahmoud-SK commented Aug 11, 2023 •

edited

Loading

jenskuhrjorgensen Dec 5, 2023 •

edited

Loading

gorhom commented Jan 3, 2024 •

edited

Loading

aweary left a comment •

edited

Loading

aweary Mar 4, 2024 •

edited

Loading