Skip to content

Commit 708f380

Browse files
Bartlomiej Bloniarzfacebook-github-bot
Bartlomiej Bloniarz
authored and
facebook-github-bot
committed
Keep non-layout animations on the synchronous path while another view animates layout (#58772)
Summary: The shared animation backend decided per surface whether a frame's animated props go through a shadow tree commit or straight to the mounted views. As soon as one view animated a layout prop, every animated view on that surface went through the commit path for the whole animation, including views that only animate `transform` or `opacity`. Besides the extra commit work, on Android this moved those views from `updatePropsSynchronously` to regular mounts. There the synchronous mount props cache (`overrideBySynchronousMountPropsAtMountingAndroid`) replaced the incoming transform with its last synchronously written value, so the view froze while the other view's layout animation ran and jumped when it ended. The decision is now made per view. `SurfaceUpdates` holds a surface's mutations keyed by view tag, and `applySurfaceUpdates` splits them into views with layout updates, which go through `commitUpdates`, and the rest, which are applied synchronously. `AnimationMutation` and `AnimationMutations` move to `AnimationMutation.h`, still included by `AnimationBackend.h`. `AnimatedPropsRegistry::update` reads the frame's batches instead of the merged per-surface map. ## Changelog: [General] [Fixed] - A view animating only non-layout props on the shared animation backend no longer freezes while another view on the surface animates a layout prop Differential Revision: D122570616
1 parent c63744d commit 708f380

15 files changed

Lines changed: 177 additions & 128 deletions

‎packages/react-native/Libraries/Animated/__tests__/AnimatedBackend-itest.js‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,72 @@ test('animate marginLeft layout prop', () => {
7777
);
7878
});
7979

80+
// A layout animation on one view must not push the other views of the
81+
// surface through a shadow tree commit: their non-layout props keep taking
82+
// the direct path to the mounted views.
83+
test('non-layout props stay on the direct path while another view animates layout', () => {
84+
const movingRef = createRef<HostInstance>();
85+
86+
let _translateX;
87+
let _translateXAnimation;
88+
let _siblingHeight;
89+
let _siblingHeightAnimation;
90+
91+
function MyApp() {
92+
const translateX = useAnimatedValue(0);
93+
const siblingHeight = useAnimatedValue(10);
94+
_translateX = translateX;
95+
_siblingHeight = siblingHeight;
96+
return (
97+
<View collapsable={false}>
98+
<Animated.View
99+
ref={movingRef}
100+
style={{width: 100, height: 100, transform: [{translateX}]}}
101+
/>
102+
<Animated.View style={{width: 100, height: siblingHeight}} />
103+
</View>
104+
);
105+
}
106+
107+
const root = Fantom.createRoot();
108+
109+
Fantom.runTask(() => {
110+
root.render(<MyApp />);
111+
});
112+
113+
Fantom.runTask(() => {
114+
_translateXAnimation = Animated.timing(_translateX, {
115+
toValue: 100,
116+
duration: 200,
117+
useNativeDriver: true,
118+
}).start();
119+
_siblingHeightAnimation = Animated.timing(_siblingHeight, {
120+
toValue: 110,
121+
duration: 200,
122+
useNativeDriver: true,
123+
}).start();
124+
});
125+
126+
Fantom.unstable_produceFramesForDuration(100);
127+
128+
// The sibling's height went through a commit; the transform did not.
129+
expect(root.getRenderedOutput({props: ['height']}).toJSX()).toEqual(
130+
<rn-view>
131+
<rn-view key={0} height="100" />
132+
<rn-view key={1} height="60" />
133+
</rn-view>,
134+
);
135+
expect(
136+
Fantom.unstable_getDirectManipulationProps(nullthrows(movingRef.current))
137+
.transform,
138+
).toEqual([{translateX: 50}]);
139+
140+
Fantom.runTask(() => {
141+
_translateXAnimation?.stop();
142+
_siblingHeightAnimation?.stop();
143+
});
144+
});
145+
80146
test('animated opacity', () => {
81147
let _opacity;
82148
let _opacityAnimation;

‎packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.cpp‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,25 +12,20 @@
1212
namespace facebook::react {
1313

1414
void AnimatedPropsRegistry::update(
15-
const std::unordered_map<SurfaceId, SurfaceUpdates>& surfaceUpdates) {
15+
const std::vector<AnimationMutations>& batches) {
1616
auto lock = std::lock_guard(mutex_);
17-
for (const auto& [surfaceId, updates] : surfaceUpdates) {
18-
auto contextIt = surfaceContexts_.find(surfaceId);
19-
if (contextIt == surfaceContexts_.end()) {
20-
continue;
21-
}
22-
auto& surfaceContext = contextIt->second;
23-
auto& pendingMap = surfaceContext.pendingMap;
24-
auto& pendingFamilies = surfaceContext.pendingFamilies;
25-
26-
auto& updatesMap = updates.propsMap;
27-
auto& updatesFamilies = updates.families;
28-
29-
for (auto& family : updatesFamilies) {
30-
pendingFamilies.insert(family);
31-
}
32-
33-
for (auto& [tag, animatedProps] : updatesMap) {
17+
for (const auto& mutations : batches) {
18+
for (const auto& mutation : mutations.batch) {
19+
const auto& family = mutation.family;
20+
auto contextIt = surfaceContexts_.find(family->getSurfaceId());
21+
if (contextIt == surfaceContexts_.end()) {
22+
continue;
23+
}
24+
auto& surfaceContext = contextIt->second;
25+
auto& pendingMap = surfaceContext.pendingMap;
26+
surfaceContext.pendingFamilies.insert(family);
27+
const auto tag = mutation.tag;
28+
const auto& animatedProps = mutation.props;
3429
auto it = pendingMap.find(tag);
3530
if (it == pendingMap.end()) {
3631
it = pendingMap.insert_or_assign(tag, std::make_unique<PropsSnapshot>())

‎packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.h‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include <react/renderer/uimanager/UIManager.h>
1616
#include <react/renderer/uimanager/UIManagerCommitHook.h>
1717
#include "AnimatedProps.h"
18+
#include "AnimationMutation.h"
1819

1920
namespace facebook::react {
2021

@@ -29,17 +30,11 @@ struct SurfaceContext {
2930
std::unordered_set<std::shared_ptr<const ShadowNodeFamily>> pendingFamilies, families;
3031
};
3132

32-
struct SurfaceUpdates {
33-
std::unordered_set<std::shared_ptr<const ShadowNodeFamily>> families;
34-
std::unordered_map<Tag, AnimatedProps> propsMap;
35-
bool hasLayoutUpdates{false};
36-
};
37-
3833
using SnapshotMap = std::unordered_map<Tag, std::unique_ptr<PropsSnapshot>>;
3934

4035
class AnimatedPropsRegistry {
4136
public:
42-
void update(const std::unordered_map<SurfaceId, SurfaceUpdates> &surfaceUpdates);
37+
void update(const std::vector<AnimationMutations> &batches);
4338
void initializeSurface(SurfaceId surfaceId);
4439
void clear(SurfaceId surfaceId);
4540
void clearOnSurfaceStop(SurfaceId surfaceId);

‎packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.cpp‎

Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -80,14 +80,10 @@ void AnimationBackend::unpackMutations(
8080
std::unordered_map<SurfaceId, SurfaceUpdates>& surfaceUpdates,
8181
std::set<SurfaceId>& asyncFlushSurfaces) {
8282
for (auto& mutation : mutations.batch) {
83-
const auto family = mutation.family;
84-
react_native_assert(family != nullptr);
85-
86-
auto& [families, updates, hasLayoutUpdates] =
87-
surfaceUpdates[family->getSurfaceId()];
88-
hasLayoutUpdates |= mutation.hasLayoutUpdates;
89-
families.insert(family);
90-
updates[mutation.tag] = std::move(mutation.props);
83+
react_native_assert(mutation.family != nullptr);
84+
auto& updates = surfaceUpdates[mutation.family->getSurfaceId()];
85+
const auto tag = mutation.tag;
86+
updates.insert_or_assign(tag, std::move(mutation));
9187
}
9288

9389
asyncFlushSurfaces.merge(mutations.asyncFlushSurfaces);
@@ -96,23 +92,34 @@ void AnimationBackend::unpackMutations(
9692
void AnimationBackend::applySurfaceUpdates(
9793
std::unordered_map<SurfaceId, SurfaceUpdates>& surfaceUpdates,
9894
const std::set<SurfaceId>& asyncFlushSurfaces) {
99-
animatedPropsRegistry_->update(surfaceUpdates);
100-
10195
for (auto& [surfaceId, updates] : surfaceUpdates) {
102-
if (updates.hasLayoutUpdates) {
103-
commitUpdates(surfaceId, updates);
104-
} else {
105-
synchronouslyUpdateProps(updates.propsMap);
96+
SurfaceUpdates layoutUpdates;
97+
std::unordered_map<Tag, AnimatedProps> directProps;
98+
for (auto& [tag, mutation] : updates) {
99+
if (mutation.hasLayoutUpdates) {
100+
layoutUpdates.emplace(tag, std::move(mutation));
101+
} else {
102+
directProps.emplace(tag, std::move(mutation.props));
103+
}
104+
}
105+
if (!layoutUpdates.empty()) {
106+
commitUpdates(surfaceId, layoutUpdates);
107+
}
108+
if (!directProps.empty()) {
109+
synchronouslyUpdateProps(directProps);
106110
}
107111
}
108112

109113
requestAsyncFlushForSurfaces(asyncFlushSurfaces);
110114
}
111115

112-
void AnimationBackend::applyMutations(AnimationMutations mutations) {
116+
void AnimationBackend::applyMutations(std::vector<AnimationMutations> batches) {
117+
animatedPropsRegistry_->update(batches);
113118
std::unordered_map<SurfaceId, SurfaceUpdates> surfaceUpdates;
114119
std::set<SurfaceId> asyncFlushSurfaces;
115-
unpackMutations(mutations, surfaceUpdates, asyncFlushSurfaces);
120+
for (auto& mutations : batches) {
121+
unpackMutations(mutations, surfaceUpdates, asyncFlushSurfaces);
122+
}
116123
applySurfaceUpdates(surfaceUpdates, asyncFlushSurfaces);
117124
}
118125

@@ -124,13 +131,13 @@ void AnimationBackend::onAnimationFrame(AnimationTimestamp timestamp) {
124131
callbacksCopy = callbacks;
125132
}
126133

127-
std::unordered_map<SurfaceId, SurfaceUpdates> surfaceUpdates;
128-
std::set<SurfaceId> asyncFlushSurfaces;
129-
for (auto& callbackWithId : callbacksCopy) {
130-
auto mutations = callbackWithId.callback(timestamp);
131-
unpackMutations(mutations, surfaceUpdates, asyncFlushSurfaces);
134+
// Sized up front rather than grown: MSVC's std::set move isn't noexcept, so
135+
// growing a vector of AnimationMutations would try to copy move-only props.
136+
std::vector<AnimationMutations> batches(callbacksCopy.size());
137+
for (size_t i = 0; i < callbacksCopy.size(); ++i) {
138+
batches[i] = callbacksCopy[i].callback(timestamp);
132139
}
133-
applySurfaceUpdates(surfaceUpdates, asyncFlushSurfaces);
140+
applyMutations(std::move(batches));
134141
}
135142

136143
CallbackId AnimationBackend::start(const Callback& callback) {
@@ -169,20 +176,23 @@ void AnimationBackend::trigger() {
169176

170177
void AnimationBackend::pushAnimationMutations(const Callback& callback) {
171178
auto timestamp = animationChoreographer_->now();
172-
auto mutations = callback(timestamp);
173-
applyMutations(std::move(mutations));
179+
std::vector<AnimationMutations> batches(1);
180+
batches[0] = callback(timestamp);
181+
applyMutations(std::move(batches));
174182
}
175183

176184
void AnimationBackend::commitUpdates(
177185
SurfaceId surfaceId,
178-
SurfaceUpdates& surfaceUpdates) {
186+
SurfaceUpdates& updates) {
179187
auto uiManager = uiManager_.lock();
180188
if (!uiManager) {
181189
return;
182190
}
183191

184-
auto& surfaceFamilies = surfaceUpdates.families;
185-
auto& updates = surfaceUpdates.propsMap;
192+
std::unordered_set<std::shared_ptr<const ShadowNodeFamily>> surfaceFamilies;
193+
for (const auto& [tag, mutation] : updates) {
194+
surfaceFamilies.insert(mutation.family);
195+
}
186196

187197
uiManager->getShadowTreeRegistry().visit(
188198
surfaceId, [&surfaceFamilies, &updates](const ShadowTree& shadowTree) {
@@ -198,7 +208,8 @@ void AnimationBackend::commitUpdates(
198208
auto newProps = ShadowNodeFragment::propsPlaceholder();
199209
if (surfaceFamilies.contains(
200210
shadowNode.getFamilyShared())) {
201-
auto& animatedProps = updates.at(shadowNode.getTag());
211+
auto& animatedProps =
212+
updates.at(shadowNode.getTag()).props;
202213
newProps = cloneProps(animatedProps, shadowNode);
203214
}
204215
return shadowNode.clone(

‎packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.h‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,22 +21,16 @@
2121
#include "AnimatedPropsRegistry.h"
2222
#include "AnimationBackendCommitHook.h"
2323
#include "AnimationChoreographer.h"
24+
#include "AnimationMutation.h"
2425

2526
namespace facebook::react {
2627

2728
class AnimationBackend;
2829

29-
struct AnimationMutation {
30-
Tag tag;
31-
std::shared_ptr<const ShadowNodeFamily> family;
32-
AnimatedProps props;
33-
bool hasLayoutUpdates{false};
34-
};
35-
36-
struct AnimationMutations {
37-
std::vector<AnimationMutation> batch;
38-
std::set<SurfaceId> asyncFlushSurfaces;
39-
};
30+
// A frame's mutations on one surface, by view. Views with layout updates go
31+
// through a shadow tree commit, the rest is applied directly to the mounted
32+
// views.
33+
using SurfaceUpdates = std::unordered_map<Tag, AnimationMutation>;
4034

4135
using Callback = std::function<AnimationMutations(AnimationTimestamp)>;
4236

@@ -74,7 +68,7 @@ class AnimationBackend : public UIManagerAnimationBackend {
7468
void applySurfaceUpdates(
7569
std::unordered_map<SurfaceId, SurfaceUpdates> &surfaceUpdates,
7670
const std::set<SurfaceId> &asyncFlushSurfaces);
77-
void applyMutations(AnimationMutations mutations);
71+
void applyMutations(std::vector<AnimationMutations> batches);
7872
std::vector<CallbackWithId> callbacks;
7973
std::shared_ptr<AnimatedPropsRegistry> animatedPropsRegistry_;
8074
std::shared_ptr<AnimationChoreographer> animationChoreographer_;

‎packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationMutation.h‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
#pragma once
9+
10+
#include <react/cxxstableapi/FrameworksGuard.h>
11+
12+
#include <react/renderer/core/ReactPrimitives.h>
13+
#include <react/renderer/core/ShadowNodeFamily.h>
14+
#include <memory>
15+
#include <set>
16+
#include <vector>
17+
#include "AnimatedProps.h"
18+
19+
namespace facebook::react {
20+
21+
struct AnimationMutation {
22+
Tag tag;
23+
std::shared_ptr<const ShadowNodeFamily> family;
24+
AnimatedProps props;
25+
bool hasLayoutUpdates{false};
26+
};
27+
28+
struct AnimationMutations {
29+
std::vector<AnimationMutation> batch;
30+
std::set<SurfaceId> asyncFlushSurfaces;
31+
};
32+
33+
} // namespace facebook::react

‎scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -710,6 +710,7 @@ using facebook::react::SizeAsTuple = std::tuple<facebook::react::Float, facebook
710710
using facebook::react::SnapshotMap = std::unordered_map<facebook::react::Tag, std::unique_ptr<facebook::react::PropsSnapshot>>;
711711
using facebook::react::StatePipe = std::function<void(const facebook::react::StateUpdate& stateUpdate)>;
712712
using facebook::react::SurfaceId = int32_t;
713+
using facebook::react::SurfaceUpdates = std::unordered_map<facebook::react::Tag, facebook::react::AnimationMutation>;
713714
using facebook::react::Tag = int32_t;
714715
using facebook::react::TelemetryClock = std::chrono::steady_clock;
715716
using facebook::react::TelemetryDuration = std::chrono::nanoseconds;
@@ -1580,7 +1581,7 @@ class facebook::react::AnimatedPropsRegistry {
15801581
public void clear(facebook::react::SurfaceId surfaceId);
15811582
public void clearOnSurfaceStop(facebook::react::SurfaceId surfaceId);
15821583
public void initializeSurface(facebook::react::SurfaceId surfaceId);
1583-
public void update(const std::unordered_map<facebook::react::SurfaceId, facebook::react::SurfaceUpdates>& surfaceUpdates);
1584+
public void update(const std::vector<facebook::react::AnimationMutations>& batches);
15841585
}
15851586

15861587
class facebook::react::AnimationBackend : public facebook::react::UIManagerAnimationBackend {
@@ -8112,12 +8113,6 @@ struct facebook::react::SurfaceContext {
81128113
public std::unordered_set<std::shared_ptr<const facebook::react::ShadowNodeFamily>> pendingFamilies;
81138114
}
81148115

8115-
struct facebook::react::SurfaceUpdates {
8116-
public bool hasLayoutUpdates;
8117-
public std::unordered_map<facebook::react::Tag, facebook::react::AnimatedProps> propsMap;
8118-
public std::unordered_set<std::shared_ptr<const facebook::react::ShadowNodeFamily>> families;
8119-
}
8120-
81218116
struct facebook::react::SystraceSection : public facebook::react::DummyTraceSection {
81228117
template <typename... ConvertsToStringPiece>
81238118
public SystraceSection(const char* name, ConvertsToStringPiece &&... args);

‎scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,7 @@ using facebook::react::SizeAsTuple = std::tuple<facebook::react::Float, facebook
709709
using facebook::react::SnapshotMap = std::unordered_map<facebook::react::Tag, std::unique_ptr<facebook::react::PropsSnapshot>>;
710710
using facebook::react::StatePipe = std::function<void(const facebook::react::StateUpdate& stateUpdate)>;
711711
using facebook::react::SurfaceId = int32_t;
712+
using facebook::react::SurfaceUpdates = std::unordered_map<facebook::react::Tag, facebook::react::AnimationMutation>;
712713
using facebook::react::Tag = int32_t;
713714
using facebook::react::TelemetryClock = std::chrono::steady_clock;
714715
using facebook::react::TelemetryDuration = std::chrono::nanoseconds;
@@ -1575,7 +1576,7 @@ class facebook::react::AnimatedPropsRegistry {
15751576
public void clear(facebook::react::SurfaceId surfaceId);
15761577
public void clearOnSurfaceStop(facebook::react::SurfaceId surfaceId);
15771578
public void initializeSurface(facebook::react::SurfaceId surfaceId);
1578-
public void update(const std::unordered_map<facebook::react::SurfaceId, facebook::react::SurfaceUpdates>& surfaceUpdates);
1579+
public void update(const std::vector<facebook::react::AnimationMutations>& batches);
15791580
}
15801581

15811582
class facebook::react::AnimationBackend : public facebook::react::UIManagerAnimationBackend {
@@ -7872,12 +7873,6 @@ struct facebook::react::SurfaceContext {
78727873
public std::unordered_set<std::shared_ptr<const facebook::react::ShadowNodeFamily>> pendingFamilies;
78737874
}
78747875

7875-
struct facebook::react::SurfaceUpdates {
7876-
public bool hasLayoutUpdates;
7877-
public std::unordered_map<facebook::react::Tag, facebook::react::AnimatedProps> propsMap;
7878-
public std::unordered_set<std::shared_ptr<const facebook::react::ShadowNodeFamily>> families;
7879-
}
7880-
78817876
struct facebook::react::SystraceSection : public facebook::react::DummyTraceSection {
78827877
template <typename... ConvertsToStringPiece>
78837878
public SystraceSection(const char* name, ConvertsToStringPiece &&... args);

0 commit comments

Comments
 (0)