jspizziri commented 1 year ago

Bug

When a resize event occurs such as a screen rotation (in native) or a window resize (in web) the bottom sheet index can change suddenly. This seems to be due to animatedContainerHeight being used in the calculation of animatedIndex.

Environment info

Library	Version
@gorhom/bottom-sheet	4.4.7
react-native	0.71.6
react-native-reanimated	^2.6.0
react-native-gesture-handler	^2.4.2

Steps To Reproduce

Web

Open the bottom sheet.
Resize the window.

Mobile I haven't tested this on mobile but theoretically it should happen there too.

Open the bottom sheet.
Rotate the device (triggering a change in the screen dimensions)

Describe what you expected to happen:

A window/screen resize should not result in a change in the animatedIndex

Reproducible sample code

N/A

Notes

I've done a little exploration on fixing this but I'm not super happy with any of my approaches or results. I've effectively tried to set a property in the component when a resize reaction takes place and then adding a conditional to check if a resize event is the responsible for triggering a recalc on the animatedIndex property.

If I could get some direction from someone who knows more about the library and the best path forward I'd happily do the implementation and submit a PR!

gorhom commented 1 year ago

i will look into this shortly

gorhom commented 1 year ago

I tried with rotating the device, and it seems working as expected

https://github.com/gorhom/react-native-bottom-sheet/assets/4061838/d74c6efd-62f1-4108-859d-78b608a1cb2e

gorhom commented 1 year ago

tested on v5 web, and it works as expected too.

https://github.com/gorhom/react-native-bottom-sheet/assets/4061838/47b80d29-28e7-4054-a189-6ae72ec82ed9

jspizziri commented 1 year ago

@gorhom thanks for your response! I'm going to upgrade to v5 and double check. If I'm still seeing an issue there I'll reopen.

jspizziri commented 1 year ago

@gorhom sadly this issue does still happen in v5. It's much better in v5 than in v4 (v4 even the slightest resize caused it).

I've created a repro example here and I also have a video of what I'm talking about here:

https://github.com/gorhom/react-native-bottom-sheet/assets/1452066/ef069259-a855-4341-a0da-bee5372dc500

You really have to quickly resize the window in order to see it now, however I think such resizes are fairly practical when considering things like entering and exiting fullscreen. Like I said before, it's definitely way better than before as before even a 1px nudge would cause a close.

I think the two conditions for a index changing are:

a fast resize.
two snap-points that are sufficiently close together.
- in my case a closed snap point and a "near the bottom" snap point
- I use bottom-sheet to provide a fullscreen and "mini" media player which is why it's so close to the bottom in my case.

EDIT: I can confirm that the most practical use-case for this is if a user has a window of a smaller size and has the bottom sheet open close to the bottom (like in the video) and then they enter and then exit fullscreen. Upon exit, the bottom sheet closes.

The second half of my video demonstrates that fast resizes can cause a closed bottom sheet to temporarily reveal itself. This definitely is a bit of a "nit", but it can be undesirable if the state of a closed player is "ugly" or "confusing" because it has no content loaded. This point I bring up as more food for thought. The thing I'm most concerned with is the closing behavior.

Let me know your thoughts! I'm happy to pitch in and write some code.

jspizziri commented 1 year ago

Another way to reproduce this on native Android is by opening the keyboard when the bottom-sheet is open (and sufficiently close to the bottom of the screen). The default configuration for android is android:windowSoftInputMode="adjustResize" which resizes the entire screen when the keyboard opens. As a result of the rapid decrease in size of the screen the bottom-sheet collapses. Despite the fixes that were put in place to handle android keyboard interactions this issue is present in v5.0.0-alpha.3.

gorhom commented 1 year ago

@jspizziri thanks for providing a repo with setup to reproduce the issue. I am just wondering if this issue also occurs on apps too ? cause resizing the container with the speed that you show is not realistic to me, and if that is the case then we need to have a solution only for web and not effect the apps.

jspizziri commented 1 year ago

@gorhom Yes it does happen in apps too. One way I've gotten it to reproduce is by opening an android keyboard when the sheet is open and docked close to the bottom. I think I document that somewhere, it might be on the PR I sent.

gorhom commented 1 year ago

@jspizziri could i use https://github.com/jspizziri/rna-test/blob/main/src/pages/index.tsx to repo the issue on Android ?

jspizziri commented 1 year ago

That's a web build on nextjs, so it would need to be modified to view on native. If you add a text input on the underlying view that pops a keyboard in a native project that should do it. I can try to send you an example app but it might take a bit.

jspizziri commented 1 year ago

@gorhom I realize now I might've misunderstood your question. You could basically copy the contents of that index file and strip out all the NextJS specific stuff and you should be able to reproduce it on Android yes. Make sure that you have android:windowSoftInputMode="adjustResize" set in your AndroidManifest.xml file as I mentioned here. You'll also need to add a text input on the underlying screen that would cause the keyboard to pop.

jspizziri commented 1 year ago

@gorhom if it's helpful I just created a repo in the example expo app. Here's the diff:

diff --git a/example/app/src/screens/basic/BasicExamples.tsx b/example/app/src/screens/basic/BasicExamples.tsx
index 8988e3c..7ac3150 100644
--- a/example/app/src/screens/basic/BasicExamples.tsx
+++ b/example/app/src/screens/basic/BasicExamples.tsx
@@ -24,7 +24,7 @@ const createExampleScreen = ({ type, count = 25 }: ExampleScreenProps) =>
     //#endregion

     //#region variables
-    const snapPoints = useMemo(() => ['25%', '50%', '90%'], []);
+    const snapPoints = useMemo(() => [60, '100%'], []);
     const enableContentPanningGestureButtonText = useMemo(
       () =>
         enableContentPanningGesture

Here's a video of it happening in the example app with the above changes:

https://github.com/gorhom/react-native-bottom-sheet/assets/1452066/75ddd0b9-c2ee-408d-ac9c-8f18f9d13c17

As I mentioned elsewhere it frequently happens as the result of a screen maximize/minimize. My PR does in fact seem to fix it, I just don't love the way it works.

fluorescent23 commented 1 year ago

I think this also seems to be happening on Android. @gorhom @jspizziri

Steps to reproduce -

Have multiple bottom sheets on a single screen with initialIndex set to -1 and snap points set to be calculated dynamically. (Works fine if there's only a single bottom sheet)
Use the BottomSheetBackdrop as the backdropComponent.
Have a Textinput NOT nested inside any of the bottom sheets.
Focus the TextInput and let the keyboard open.
Blur the TextInput.

When the TextInput is blurred, that's when the flickering happens. The animatedIndex.value becomes 0 for a split second, which probably happens because the animatedPosition.value is reduced by the size of the keyboard, this makes the bottom sheet visible for a split second.

gorhom commented 1 year ago

@fluorescent23 could you provide a sample repo code ?

jspizziri commented 1 year ago

@gorhom any thoughts on my PR for this issue? https://github.com/gorhom/react-native-bottom-sheet/pull/1435