fix: prevent start animation when sheet is closing

[gorhom]

closes #204

Motivation

this should solve the issue caused when sheet is closing and something else trigger snapping.

Installation

yarn add ssh://git@github.com:gorhom/react-native-bottom-sheet#fix/prevent-snapping-when-closing

[gorhom]

@heejongahn could you test this pr 👍

[heejongahn]

@gorhom Thanks for the quick response! I really appreciate your time.

I've tested your branch with both example app and my app. I'm afraid the problem still persists.

I'm not really familiar with the internals so maybe this might be totally irrelevant but: by console.log-ing here and there, I found out snapPoints changes when the content changes, so this effect gets called while closing, thus re-opening the sheet. When I added !isClosing.current check to this if statement, the problem seems to be gone, but I'm not sure if this would be the right solution or not.

[gorhom]

@heejongahn thanks for testing it ,

this is weird , it seems that your snapPoints change when you update the state that's why cause the internal effect to reflect the change and animate the sheet 🧐

anyway will also add the if-condition to the effect to prevent that too

[gorhom]

@heejongahn I have updated the pr, please test it again and let me know :)

[heejongahn]

Thanks for the update!

it seems that your snapPoints change when you update the state

I think this happens when

1. while updating the state, the bottom sheet content chagnes
2. and snap points depend on the sheet content height (like DynamicSnapPoint example)

Anyway, the update seems to work fine with BottomSheetModal, but now BottomSheet shows some weird behavior :(

bottomsheet.mov

Code sample based on example app "ADVANCED > Dynamic Snap Point" screen:

import React, { useCallback, useMemo, useRef, useState } from 'react';
import { View, StyleSheet, Text } from 'react-native';
import BottomSheet, { BottomSheetView } from '@gorhom/bottom-sheet';
import { useSafeAreaInsets } from 'react-native-safe-area-context';
import { Easing } from 'react-native-reanimated';
import Button from '../../components/button';

const DynamicSnapPointExample = () => {
  // state
  const [count, setCount] = useState(0);
  const [contentHeight, setContentHeight] = useState(0);

  // hooks
  const bottomSheetRef = useRef<BottomSheet>(null);
  const { bottom: safeBottomArea } = useSafeAreaInsets();

  // variables
  const snapPoints = useMemo(() => [0, contentHeight], [contentHeight]);

  // callbacks
  const handleIncreaseContentPress = useCallback(() => {
    setCount(state => state + 1);
  }, []);
  const handleDecreaseContentPress = useCallback(() => {
    setCount(state => Math.max(state - 1, 0));
  }, []);
  const handleExpandPress = useCallback(() => {
    bottomSheetRef.current?.expand();
  }, []);
  const handleClosePress = useCallback(() => {
    setCount(state => state + 1);
    bottomSheetRef.current?.close();
  }, []);
  const handleOnLayout = useCallback(
    ({
      nativeEvent: {
        layout: { height },
      },
    }) => {
      setContentHeight(height);
    },
    []
  );

  // styles
  const contentContainerStyle = useMemo(
    () => ({
      ...styles.contentContainerStyle,
      paddingBottom: safeBottomArea,
    }),
    [safeBottomArea]
  );
  const emojiContainerStyle = useMemo(
    () => ({
      ...styles.emojiContainer,
      height: 50 * count,
    }),
    [count]
  );

  // renders
  const renderBackground = useCallback(
    () => <View style={styles.background} />,
    []
  );

  return (
    <View style={styles.container}>
      <Button
        label="Expand"
        style={styles.buttonContainer}
        onPress={handleExpandPress}
      />
      <Button
        label="Close"
        style={styles.buttonContainer}
        onPress={handleClosePress}
      />
      <BottomSheet
        ref={bottomSheetRef}
        index={1}
        snapPoints={snapPoints}
        animateOnMount={true}
        animationEasing={Easing.out(Easing.quad)}
        animationDuration={250}
        backgroundComponent={renderBackground}
      >
        <BottomSheetView
          style={contentContainerStyle}
          onLayout={handleOnLayout}
        >
          <Text style={styles.message}>
            Could this sheet resize to its content height ?
          </Text>
          <View style={emojiContainerStyle}>
            <Text style={styles.emoji}>😍</Text>
          </View>
          <Button
            label="Yes"
            style={styles.buttonContainer}
            onPress={handleIncreaseContentPress}
          />
          <Button
            label="Maybe"
            style={styles.buttonContainer}
            onPress={handleDecreaseContentPress}
          />
        </BottomSheetView>
      </BottomSheet>
    </View>
  );
};

const styles = StyleSheet.create({
  container: {
    flex: 1,
    padding: 24,
  },
  background: {
    ...StyleSheet.absoluteFillObject,
    backgroundColor: 'white',
  },
  buttonContainer: {
    marginBottom: 6,
  },
  contentContainerStyle: {
    paddingTop: 12,
    paddingHorizontal: 24,
    backgroundColor: 'white',
  },
  message: {
    fontSize: 24,
    fontWeight: '600',
    marginBottom: 12,
  },
  emoji: {
    fontSize: 156,
    textAlign: 'center',
    alignSelf: 'center',
  },
  emojiContainer: {
    overflow: 'hidden',
    justifyContent: 'center',
  },
});

export default DynamicSnapPointExample;

[heejongahn]

Also, there seems to be some kind of a timing issue with this version it there's a modal stack (i.e. when another modal should be opened again after a modal gets dismissed).

This is "MODAL > Stack Modals" screen from example app. You can see that sometimes both C and B gets dismissed by clicking C, and sometimes the entire stack just goes away.

stacks.mov

[gorhom]

@heejongahn thanks for debugging these issues 👍

for your first issue i notice that you provide animationEasing as an unstable variable which cause the issue, try providing it like this

  const animationEasing = useMemo(() => Easing.out(Easing.quad), []);

[gorhom]

for the stack modals, i think we will need to create another issue for it and debug it from there

[heejongahn]

for your first issue i notice that you provide animationEasing as an unstable variable which cause the issue, try providing it like this

Okay, I'll also update a example screen and send a PR.

for the stack modals, i think we will need to create another issue for it and debug it from there

When I checked, it seemed that the issue only happens with this branch, but not with master branch. So I thought we wouldn't want to merge this before also resolving the stack modal issue (as this might break currently well-functioning codes), and wasn't sure if filing another issue for this unmerged PR would be appropriate.

If you'd like to merge this first, then I'm happy to create another issue! Either way, please let me know.

[gorhom]

@heejongahn good finding !

okay i'll investigate it more

[gorhom]

@heejongahn could you try the last commit :)

[heejongahn]

Unfortunately, seems like same thing happens with both the example app and my app 😿

[gorhom]

@heejongahn thanks alot for debugging this issue, please try the last commit , it should resolve the issue which was introduce by the first commit of this pr 😅

[heejongahn]

I'm not the one to be thanked... I really appreciate your time on this issue! 👍

Now the example app works fine, but my app's still suffering from the same issue. I've added some log statements for BottomSheet / BottomSheetModal methods...

not-ok.mov

Few things I noticed:

• When the second modal dismisses and first modal gets restored, dismiss and restore happens in right order, but handleSnap (caused by restore?) happens before handleClose (caused by dismiss?)
• first BottomSheetModal's handleRestore is being called twice, and for the second time, isMinimized.current is false, thus no snapTo happens.

Not sure if this would help, but this is a log when you click backdrop to dismiss the second modal:

ok.mov

I'd really like to help yet I'm afraid I cannot pinpoint what the exact reproducing condition is, I can only suspect that it has something to do with timing issue... Please let me know if there's any way I could help. Thanks again for your effort.

[gorhom]

I'm not the one to be thanked... I really appreciate your time on this issue! 👍
glad to hear that 🎉

your use-case seems odd, could you share a reproachable sample code from your app or the exact same implementation on /examples ? that would help a lot

[heejongahn]

I'm trying to come up with the reproduction. Not there yet, but getting closer.

So for my example: Second modal is select menu. Pressing one of them changes the value that first modal depends on, so eventually it makes the first modal re-render.

e.g. pseudo-code:

function FirstModal() {
  const [firstModalInnerState, setFirstModalInnerState] = useState(defaultValue);

  const onPressSecondModalItem = useCallback((newValue: numbe) => {
    setFirstModalInnerState(newValue);
  }, [])

  // ...

  return <>
    // ...
    <MySelect
      currentValue={firstModalInnerState}
      onPress={presentSecondModal}
    />
    <SecondModal
      onPressItem={onPressSecondModalItem}
      { /* other props */}
    />
  </>;
}

I tried commenting out the setFirstModalInnerState part from my app, so that pressing second modal's item should not trigger first modal's re-render. The problem was gone!

So I think for the stack case, it also has something to do with the not only closing but also restoring modal getting rendered. However I wasn't able to repro it within the example app (for now).

I'll post here when I could come up with the minial reproducible repo.

[gorhom]

hi @heejongahn , could you have a look at this snack

https://snack.expo.io/@gorhom/issue-205

[kickbk]

@gorhom just installed the fix and tested. So far no crashes 👍 Will update it something goes wrong.

[heejongahn]

@gorhom I tried to, but the snack crashes with this error (which is odd, App.js seems fine):

Expected ref to be a function, a string, an object returned by React.createRef(), or null.

I apologize that I've been bit busy at the work so didn't really have time to find exact reproduction.

If there are folks waiting for this PR, and if my usecase is odd enough that merging this wouldn't cause similar issues for most users, I think merging & releasing this PR first might be a good call.

I'll try to follow-up with another issue with more information on my app's issue when I found the reproduction condition. Again, thanks for your effort and time!

[theohdv]

Same for me, i've tried the fix and tested it. Everything OK !

[kickbk]

@heejongahn the snack does crash, but for an unrelated reason. Try testing locally by updating your node modules with the fix branch "@gorhom/bottom-sheet": "https://github.com/gorhom/react-native-bottom-sheet#fix/prevent-snapping-when-closing",.
I also think it's good to merge.

[gorhom]

@theohdv , @kickbk thanks for testing this pr <3

@heejongahn you are right, the snack was using the publish package not this branch ,,, my bad 😅

you could copy the code and run it on using this branch and it should work fine

[gorhom]

i will try to investigate #240 if it could be fix with this pr - since they are related somehow - but it is not , i will merge this one first 👍

[gorhom]

found the issue of #240, it is not related to this one , however i already work on the solution and it will be pushed on another pr

[gorhom]

this should be fixed with v2.0.7 🎉