software-mansion / react-native-reanimated

React Native's Animated library reimplemented
https://docs.swmansion.com/react-native-reanimated/
MIT License
9.1k stars 1.32k forks source link

fix(Android, Paper): Lost update on render #6685

Open tjzel opened 2 weeks ago

tjzel commented 2 weeks ago

Summary

Fixes https://github.com/software-mansion/react-native-reanimated/issues/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 https://github.com/software-mansion/react-native-reanimated/issues/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 commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

@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 commented 6 days ago

@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?

joe-sam commented 4 days ago

Could this be the underlying issue that views are intentionally delayed while mounting, and that there appears to be a retry mechanism already in place at least on Android(Paper) synchronise dispatching of view commands through RuntimeScheduler #47604 .

tjzel commented 3 days ago

@joe-sam I hope I'm not wrong, but I think ViewCommands apply to things like https://reactnative.dev/docs/legacy/direct-manipulation#other-native-methods, which I believe we don't utilize under the hood for animated style updates.

cc @tomekzaw to correct me here if I'm mistaken