Sync JS-side AnimatedValue before invoking native-driver completion callback (#57021)#57021
Closed
fabriziocucci wants to merge 1 commit into
Closed
Sync JS-side AnimatedValue before invoking native-driver completion callback (#57021)#57021fabriziocucci wants to merge 1 commit into
fabriziocucci wants to merge 1 commit into
Conversation
|
@fabriziocucci has exported this pull request. If you are a Meta employee, you can view the originating Diff in D106940382. |
…allback (facebook#57021) Summary: `Animation.__startAnimationIfNative` invoked the user's `start({finished})` callback BEFORE syncing the JS-side `AnimatedValue` with the post-animation value reported by the native side. Any caller that read the `AnimatedValue` from inside the callback, or from a React re-render that the callback triggered, observed the **pre-animation** value, producing visual jumps. Concrete impact: an `<Animated.View>` whose `transform: [{scaleX}]` is driven by `value.interpolate({inputRange: [0, 1], outputRange: [0.5, 1]})` would render at `scaleX = interp(0) = 0.5` after the animation finished, instead of at `scaleX = interp(1) = 1`. The same pattern affects opacity, color and any other style derived from an animated value read during a re-render scheduled by the completion callback. This change reorders the native completion callback so `animatedValue.__onAnimatedValueUpdateReceived(value, offset)` runs BEFORE `this.__notifyAnimationEnd(result)`. The user's callback (and any re-render it schedules) now observes the post-animation JS value. The reorder is gated behind a new JS-only feature flag, `animatedShouldSyncValueBeforeStartCallback`, which defaults to `true` (the fix is on by default). Set the flag to `false` to opt out and restore the pre-fix ordering as a kill-switch. A Fantom integration test in `Animated-itest.js` exercises the exact scenario: starts a `useNativeDriver: true` `Animated.timing(0 -> 1)`, captures both `_value._value` and `value.interpolate(...).__getValue()` inside the `start({finished})` callback and asserts the value matches the flag state (pre-animation when off, post-animation when on). ## Behavior change to consider The reorder also changes the order in which JS-side observers of the `AnimatedValue` are notified relative to the `start({finished})` callback. This was confirmed empirically against the current and the proposed ordering. Before (flag off): 1. `start({finished})` callback fires 2. `AnimatedValue.addListener(...)` subscribers receive the post-animation value After (flag on): 1. `AnimatedValue.addListener(...)` subscribers receive the post-animation value 2. `start({finished})` callback fires For the vast majority of callers this is irrelevant or strictly better (observers and callback now agree on the same value). The flag defaults to `true` so the fix ships immediately; the flag itself stays as a kill-switch in case real-world callers turn out to depend on the previous ordering. Once adoption has been verified the flag can be removed entirely. Changelog: [General][Fixed] - Sync JS-side `Animated.Value` with the post-animation value before invoking `Animated.timing(...).start({finished})` callbacks so reads from inside the callback (or from React re-renders it triggers) observe the post-animation value rather than the pre-animation value. Gated behind a new JS-only feature flag, `animatedShouldSyncValueBeforeStartCallback`, defaulting to `true` (set to `false` to opt out). Reviewed By: javache, zeyap Differential Revision: D106940382
51e4be6 to
d4da5e0
Compare
|
This pull request has been merged in ee6958a. |
Collaborator
|
This pull request was successfully merged by @fabriziocucci in ee6958a When will my fix make it into a release? | How to file a pick request? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Animation.__startAnimationIfNativeinvoked the user'sstart({finished})callback BEFORE syncing the JS-side
AnimatedValuewith the post-animationvalue reported by the native side. Any caller that read the
AnimatedValuefrom inside the callback, or from a React re-render that the callback
triggered, observed the pre-animation value, producing visual jumps.
Concrete impact: an
<Animated.View>whosetransform: [{scaleX}]isdriven by
value.interpolate({inputRange: [0, 1], outputRange: [0.5, 1]})would render at
scaleX = interp(0) = 0.5after the animation finished,instead of at
scaleX = interp(1) = 1. The same pattern affects opacity,color and any other style derived from an animated value read during a
re-render scheduled by the completion callback.
This change reorders the native completion callback so
animatedValue.__onAnimatedValueUpdateReceived(value, offset)runs BEFOREthis.__notifyAnimationEnd(result). The user's callback (and any re-renderit schedules) now observes the post-animation JS value.
The reorder is gated behind a new JS-only feature flag,
animatedShouldSyncValueBeforeStartCallback, which defaults totrue(the fix is on by default). Set the flag to
falseto opt out andrestore the pre-fix ordering as a kill-switch.
A Fantom integration test in
Animated-itest.jsexercises the exactscenario: starts a
useNativeDriver: trueAnimated.timing(0 -> 1),captures both
_value._valueandvalue.interpolate(...).__getValue()inside the
start({finished})callback and asserts the value matches theflag state (pre-animation when off, post-animation when on).
Behavior change to consider
The reorder also changes the order in which JS-side observers of the
AnimatedValueare notified relative to thestart({finished})callback.This was confirmed empirically against the current and the proposed
ordering.
Before (flag off):
start({finished})callback firesAnimatedValue.addListener(...)subscribers receive the post-animation valueAfter (flag on):
AnimatedValue.addListener(...)subscribers receive the post-animation valuestart({finished})callback firesFor the vast majority of callers this is irrelevant or strictly better
(observers and callback now agree on the same value). The flag defaults
to
trueso the fix ships immediately; the flag itself stays as akill-switch in case real-world callers turn out to depend on the
previous ordering. Once adoption has been verified the flag can be
removed entirely.
Changelog:
[General][Fixed] - Sync JS-side
Animated.Valuewith the post-animation value before invokingAnimated.timing(...).start({finished})callbacks so reads from inside the callback (or from React re-renders it triggers) observe the post-animation value rather than the pre-animation value. Gated behind a new JS-only feature flag,animatedShouldSyncValueBeforeStartCallback, defaulting totrue(set tofalseto opt out).Reviewed By: javache, zeyap
Differential Revision: D106940382