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 snapToPosition function call, right away after the mount. #1623

Open
wants to merge 2 commits into
base: v4
Choose a base branch
from

Conversation

beqramo
Copy link
Contributor

@beqramo beqramo commented Nov 8, 2023

Fixes: #1294

Motivation

When snapToPosition gets called right away after a mount, animatedContainerHeight has an initial value of -999, if the prop didn't get passed. It is happening as the package can't calculate height right away on the mount. in simple terms onLayout function takes time to be called and to update the value of animatedContainerHeight

That means that snapToPosition causes some problems.
To fix the issue, we need to wait for the update of the value of animatedContainerHeight and then continue calculating the nextPosition value from normalizeSnapPoint function.

Because of that, I added a listener if I see that animatedContainerHeight still has an initial value. after I receive the update, I'm following the same process as it was before.

I was forced to update the Reanimated package due to the recent addition of the listener feature. I've been using the updated Reanimated package in my production app for a while now, and it's been working seamlessly, so it shouldn't be a problem.

Thank you.

@gorhom gorhom self-assigned this Nov 26, 2023
@gorhom gorhom added the v4 Written in Reanimated v2 label Nov 26, 2023
package.json Show resolved Hide resolved

/**
* mark the new position as temporary.
* Checks whether the library has measured the container's height or if it was passed from props.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not early exit this method , and ensure to listen to isLayoutCalculated ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe exiting from a function in that manner is correct. The caller has requested to open the bottom sheet, and the caller doesn't have any information on whether the package is ready or undergoing some heavy operations.

Regarding listening to isLayoutCalculated, if I understood it correctly, it seems more appropriate. I will update it after we know precisely whether there is a problem or not.

For more details on these matters, please refer to the comment section below.

@gorhom
Copy link
Owner

gorhom commented Nov 26, 2023

thanks @beqramo for submitting this PR, but i think it is not about layout not being ready, isLayoutCalculated is responsible to trigger the on mount animation, which waits till container height to be calculated before it become truthful

@gorhom
Copy link
Owner

gorhom commented Nov 26, 2023

could you please provide a reproducible sample code of the issue ?

@henrymoulton
Copy link

Noticed Reanimated 3 has an unreleased PR related to a race condition software-mansion/react-native-reanimated#5224 - wonder if there's a relation to this issue.

@beqramo
Copy link
Contributor Author

beqramo commented Nov 27, 2023

Hi, thanks for your feedback, @gorhom.

I will try to provide more context before presenting the conclusions:

When snapToPosition is called from the ref right away after mount:

  useEffect(() => {
        sheetRef.current?.snapToPosition(400);
  }, []);

At that time, SOMETIMES, the value of isLayoutCalculated is false. Why?

The value of isLayoutCalculated changes after the onLayout callback gets called (when the component is rendered and we have sizes and other details about the container).
We are facing some kind of race condition; sometimes, onLayout is getting called first, and isLayoutCalculated and consecutively, animatedContainerHeight do have correct values.
It is like, in short, which one will get called first? useEffect or onLayout?

What are the solutions? (Sorted from worse to best solution)

  1. To have some kind of callback prop like "onReady" that will indicate the parent component that the bottom-sheet is ready.
  2. Return a specific value like "not_ready" when it will be triggered from the ref, right away after mount.
  3. Take ownership of the problem from the library side; the library needs to automatically wait for the completion of all height calculation processes.

Reproducible sample code:
I first faced this problem on my app. When searching for solutions, I found that another developer already had the same problem.

Other than that I will drop my code snippet too:

  const sheetRef = useRef<BottomSheet>(null);
  
  useEffect(() => {
        // call right after mount
        sheetRef.current?.snapToPosition(400);
  }, []);
  
return  <BottomSheet
      ref={sheetRef}
      snapPoints={[10]}
      index={ -1}
   >
      <Column mt={7} px={5} stretch>
             {/* children here*/}
      </Column>
    </BottomSheet>

Thank you.

Hope I will not waste your time.

@beqramo
Copy link
Contributor Author

beqramo commented Nov 27, 2023

@henrymoulton Thanks for the info,
I think that one is a different issue from this.
From the funny side: I think I was facing that issue too and I'm happy that it is resolved and you informed me about it, waiting for the release.

Thank you

@henrymoulton
Copy link

@beqramo can you confirm it still happens if you're using Reanimated V2?

@beqramo
Copy link
Contributor Author

beqramo commented Jan 8, 2024

Hi @henrymoulton,

I am currently using my fork, and I haven't encountered the issue. Additionally, I am utilizing reanimated version 3. If you wish to test it, the problem might be more noticeable on low-end devices, particularly Android.

The issue you mentioned seems slightly different here. The problem lies in the synchronization of the onLayout and snapToPosition functions. The challenge arises because the first function is not synchronous, and its trigger can be delayed. That creates problem if snapToPosition gets called right away on parent component mount.

Thank you.

@MBach
Copy link

MBach commented Jun 4, 2024

Seems interesting! I'd like to see this PR merged if possible

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

Successfully merging this pull request may close these issues.

[v4] | [v2] snapToPosition opening bottomsheet to maximum screen height and snapToIndex not working
4 participants