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: bottom sheet not appearing for users that have reduced motion turned on #1743

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

fobos531
Copy link
Contributor

Hello @gorhom

Motivation

Thank you for this great library! Here's a super simple PR that fixes #1560 and #1674. In recent versions of Android and in iOS, there's a "Reduce motion" feature which decreases the overall number of animations alongside the OS and slightly alters them as well. With Reanimated's default settings, this now means that some animations will fail to run, including the very critical one which this library uses to present the sheet. Thankfully, Reanimated exposes an ability to override this config via a reduceMotion option on the animation configs. This PR hardcodes the ReduceMotion.Never option to the animate function. I don't think it would ever be in the interest of the user to override this, otherwise the sheet could fail to be presented, so that's the reason why I hardcoded it. I also pointed this to the v5 branch since v5 has reanimated 3.x and it seems this API is available only since 3.x, and master is still on 2.x. I've verified that this works on the example app and that sheets are presented with Reduce Motion turned on a physical iPhone 14 Pro running 17.3, whereas it was broken when ReduceMotion.Never was NOT used.

@Vinaum8
Copy link

Vinaum8 commented Feb 19, 2024

When will this fix be released? Is there a projection?

@gorhom gorhom self-assigned this Feb 25, 2024
@gorhom
Copy link
Owner

gorhom commented Feb 25, 2024

hi @fobos531, thanks for submitting this PR but i have some concerns about it

  1. I have tried to repo the issue but i couldn't, tested with iOS 17.2
  2. why setting the value from the library code base when user can do so by providing the property as they pleased with useBottomSheetTimingConfigs or useBottomSheetSpringConfigs ?

@fobos531
Copy link
Contributor Author

fobos531 commented Feb 26, 2024

hi @fobos531, thanks for submitting this PR but i have some concerns about it

  1. I have tried to repo the issue but i couldn't, tested with iOS 17.2
  2. why setting the value from the library code base when user can do so by providing the property as they pleased with useBottomSheetTimingConfigs or useBottomSheetSpringConfigs ?

Hello @gorhom, thank you for the reply, below find my response:

  1. Check the video on this link for a clear demo of the issue on iPhone 15 Pro simulator (iOS 17.2): https://dropover.cloud/d22882 (it is over 10 MB so I can't attach to github)
  2. It is true that we can easily do this in the animation configs that are exposed by the bottom-sheet, and that's how I fix it in my app that is running bottom-sheet v4.6.1, but my argument here (as outlined in the original post) is that running the animation is a core and essential part of the functionality of the bottom-sheet. I see virtually no situation where the user would want to have a value different than ReduceMotion.Never, since in any other case the sheet would fail to be presented at all, which breaks the core functionality of the bottom-sheet. An alternative would be to find a way to present the sheet "without animations", but I don't think its worth exploring that option here. That's why I opted to hardcode this in the animation config on the library level, because I think in this specific case the value is completely justified.

@obasille
Copy link

FYI a simple workaround was posted in a related GitHub issue:
#1674 (comment)

@gorhom Thank you so much for your work on this very useful library! I hope a solution can be soon integrated in the library as virtually any app using react-native-bottom-sheet will have this problem (that is the bottom sheets are not showing if reduce motion is enabled). I imagine most developers are unaware that they need to implement a manual fix.

@asozcan
Copy link

asozcan commented Feb 26, 2024

Reduced motion is not activated only manually from accessibility settings. It is also automatically activated by iOS in low battery mode. So, there are more users facing with this issue than we guess. I had hundreds of user reports/complaints only in a couple weeks after I upgrade reanimated to v3.5. Most of the screenshots from users were pointing battery < 15%. And all complaints stop after setting ReduceMotion.Never for BottomSheet.

@gorhom
Copy link
Owner

gorhom commented Feb 26, 2024

@fobos531

Check the video on this link for a clear demo of the issue on iPhone 15 Pro simulator (iOS 17.2): https://dropover.cloud/d22882 (it is over 10 MB so I can't attach to github)

I have managed to repo the issue, thanks

It is true that we can easily do this in the animation configs that are exposed by the bottom-sheet, and that's how I fix it in my app that is running bottom-sheet v4.6.1, but my argument here (as outlined in the original post) is that running the animation is a core and essential part of the functionality of the bottom-sheet. I see virtually no situation where the user would want to have a value different than ReduceMotion.Never, since in any other case the sheet would fail to be presented at all, which breaks the core functionality of the bottom-sheet. An alternative would be to find a way to present the sheet "without animations", but I don't think its worth exploring that option here. That's why I opted to hardcode this in the animation config on the library level, because I think in this specific case the value is completely justified.

Although I agree with your reasoning here, i still believe we should respect the customer settings. I have kicked off an investigation and found multiple issues that i have already started (v5):

  • Sequencing Issue between ( on mount, snap point changes ).
  • Better handling for no animation snapping.
  • Add a new prop to opt-out
  • Document

Maybe for v4: i'll just merge your CR but i'm still hesitant, but i'll let you know later today.

@gorhom gorhom merged commit 9b4ef4d into gorhom:v5 Feb 26, 2024
1 check failed
@gorhom
Copy link
Owner

gorhom commented Feb 26, 2024

@fobos531 decided to merge this PR and introduce a long term fix in a follow up PR in the future. thanks again :)

@fobos531 fobos531 deleted the v5 branch February 27, 2024 13:20
@fobos531
Copy link
Contributor Author

Thank you for the careful consideration and merging of the PR, much appreciated!

yayvery pushed a commit to discord/react-native-bottom-sheet that referenced this pull request Mar 18, 2024
gorhom#1674)

fix: bottom sheet not appearing for users that have reduced motion turned on (gorhom#1743)(by @fobos531)
yayvery pushed a commit to discord/react-native-bottom-sheet that referenced this pull request Mar 18, 2024
gorhom#1674)

fix: bottom sheet not appearing for users that have reduced motion turned on (gorhom#1743)(by @fobos531)
@MikeWoots
Copy link

I'm still experiencing this issue on version 4.6.1 when using the BottomSheetModal component

@christophby
Copy link

Same here. Issue still exists in 4.6.1 with BottomSheetModal + BottomSheetScrollView

@rkstar
Copy link

rkstar commented May 2, 2024

Same here. Issue still exists in 4.6.1 with BottomSheetModal + BottomSheetScrollView

@christophby @MikeWoots i was able to solve this in 4.6.1 (w/reanimated 3) by applying the animationConfigs to everywhere i implemented the bottom sheet. useBottomSheetTimingConfigs did not work, i had to useBottomSheetSpringConfigs for it to override the reduce motion ios setting. you can play with the settings to customize the animation.

i made a hook to make it easier on myself:

import { useBottomSheetSpringConfigs } from '@gorhom/bottom-sheet';
import { ReduceMotion } from 'react-native-reanimated';

export function useBottomSheetIOSReduceMotionOverride() {
  // NOTE: @rkstar -- this is REQUIRED for bottom-sheet to work in
  // ios if the "reduce motion" setting is enabled on the device.
  // this is turned on by default on ios devices > iphone 14 when
  // the "low battery" warning happens.
  const animationConfigs = useBottomSheetSpringConfigs({
    reduceMotion: ReduceMotion.Never,
    stiffness: 200,
    overshootClamping: true,
  });

  return { animationConfigs };
}

and then in my bottom sheet usages....

const iosReduceMotionOveride = useBottomSheetIOSReduceMotionOverride();

<BottomSheet
  ref={ref}
  snapPoints={['50%']}
  // etc... 
  {...iosReduceMotionOverride}
/>

@arnoldc
Copy link

arnoldc commented Jun 19, 2024

if someone is still caught up and not knowing where this is fix

this is fix at release 4.6.3
im running react-native 0.73.7 and I couldnt detect any issue anymore

my test

  • tried running iOS 15 to iOS 17 seems working fine (some device i use is simulator)
  • when reducing/removing animation in android its also seems fine tested Android 11 to Android 14 (some device i use is emulator)

Thanks again for this fix

@lutfi-haslab
Copy link

  const animationConfigs = useBottomSheetSpringConfigs({
    reduceMotion: ReduceMotion.Never,
    stiffness: 200,
    overshootClamping: true,
  });

Not work in Android for 4.6.3, i am still facing WARN [Reanimated] Tried to modify key `reduceMotion` of an object which has been already passed to a worklet.

This was referenced Sep 8, 2024
This was referenced Sep 19, 2024
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.

10 participants