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(Android, Paper): Lost update on render #6685

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

Conversation

tjzel
Copy link
Collaborator

@tjzel tjzel commented Nov 10, 2024

Summary

Fixes #6677

Due to the React Native Android's batching of Native Views creation, sometimes the update from Reanimated for a component might come before the corresponding view was actually created. This used to crash the app which was fixed in:

However, the above fix didn't address the fact the update was lost. This pull request re-applies first Animated Style update so then the update would be applied on the second frame, instead of not at all.

Test plan

Test it on the reproduction in #6677

@bartlomiejbloniarz Do you think it can affect Layout Animations?

Note

This should probably be handled in Android specific native code but I failed there to force the UIManager to create the View when we have an update pending.

@joe-sam
Copy link

joe-sam commented Nov 13, 2024

Possibly the updater function argument for useAnimatedStyle/useAnimatedProps should just include previous style/prop argument and leave it to the end user of the API to decide whether to set the animation for style/prop or not.
If the component is not created yet the callback argument prevStyleOrProp should be undefined instead of throwing some unknown view tag error which cannot be handled elsewhere in the stack.

useAnimatedStyleOrProps ( (prevStyleOrProp : Style | Props | undefined) => { return nextStyleOrProp } , ...)

This updates applied to the later frame may not be idempotent, associative or distributive.
The flags will also need to be cleaned up on unmounting.

@amadeus
Copy link
Contributor

amadeus commented Nov 13, 2024

Going to test a version of this change on our beta app to see if it fixes some of the issues we've been seeing.

I did have to modify this change though, as it was crashing, ultimately the change I made (unclear if this was correct or not) was to wrap the new styleUpdater call in a runOnUI

@tjzel
Copy link
Collaborator Author

tjzel commented Nov 19, 2024

@joe-sam Could you elaborate?

Possibly the updater function argument for useAnimatedStyle/useAnimatedProps should just include previous style/prop argument and leave it to the end user of the API to decide whether to set the animation for style/prop or not. If the component is not created yet the callback argument prevStyleOrProp should be undefined instead of throwing some unknown view tag error which cannot be handled elsewhere in the stack.

Why would you need to access the previous style? How would that help with this problem? The issue here is that React gives us a viewTag for a View that wasn't yet created natively - and we cannot check that at the moment of scheduling the update.

By the way, the case you described can be handled with useDerivedValue if I understand correctly.


@amadeus it doesn't make sense to me that you needed to wrap it in runOnUI - that call is already executed on the UI Runtime. Can you provide some more context about that crash? I don't want to merge it if there are related crashes reported.

@amadeus
Copy link
Contributor

amadeus commented Nov 19, 2024

@tjzel specifically these lines: https://github.com/software-mansion/react-native-reanimated/pull/6685/files#diff-eec756c87121a5eb74826bffb2eafdd06a5b46924e14c729cdfc5b99a7dbcd94R536-R545

Those aren't running on ui.

Also FWIW, this is a patch on version 3.16.0, which maybe has differences from the main branch here?

Comment on lines +536 to +544
requestAnimationFrame(() =>
styleUpdater(
shareableViewDescriptors,
updater,
remoteState,
areAnimationsActive,
isAnimatedProps
)
);
Copy link
Member

Choose a reason for hiding this comment

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

We should consider resolving this in native code - similar to the fixes we've already implemented for iOS:

The current solution seems a bit undeterministic and could potentially cause some unexpected blinking 🤔

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.

useAnimatedStyle gets out of sync with SharedValue modified inside useEffect on Android
4 participants