Skip to content

Commit a2f932a

Browse files
zeyapfacebook-github-bot
authored andcommitted
Revisit timing to capture old/new nodes (#56877)
Summary: ## Changelog: [Internal] [Changed] - Revisit timing to capture old/new nodes Previously, when `applyViewTransitionName` is called from react reconciler, if there is active viewtransition, we decide it's applying to new node. This was incorrect because `applyViewTransitionName(old)` for next transition can be called when the previous transition is still ongoing. In fact `applyViewTransitionName(old)` is guaranteed to be called before mutationCallback in a transition while `applyViewTransitionName(new)` is inside mutationCallback. So that is a better criteria to separate old and new cases. Given mutationCallback is synchronous on JS thread we can just use a boolean flag to tell the timing. Given this interleaved situation, when we clean up resources (for new/old nodes) of a transition at its end, we should also avoid removing those for a pending transition. Introducing transitionId for this case. Reviewed By: Abbondanzo Differential Revision: D105214322
1 parent 6720760 commit a2f932a

11 files changed

Lines changed: 64 additions & 16 deletions

packages/react-native/ReactCommon/react/renderer/viewtransition/ViewTransitionModule.cpp

Lines changed: 43 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -86,14 +86,22 @@ void ViewTransitionModule::applyViewTransitionName(
8686
.size = layoutMetrics.frame.size,
8787
.pointScaleFactor = layoutMetrics.pointScaleFactor};
8888

89-
nameRegistry_[tag].insert(name);
90-
91-
// If applyViewTransitionName is called after transition started, this is the
92-
// "new" state (end snapshot). Otherwise, this is the "old" state (start
93-
// snapshot)
94-
if (!transitionStarted_) {
89+
// Calls outside mutationCallback are from the before-mutation phase (old
90+
// state for an upcoming transition). Assign a provisional next transition ID
91+
// so startViewTransitionEnd cleanup preserves these entries.
92+
auto currentTransitionId =
93+
insideMutationCallback_ ? activeTransitionId_ : activeTransitionId_ + 1;
94+
nameRegistry_[tag].names.insert(name);
95+
nameRegistry_[tag].transitionId = currentTransitionId;
96+
97+
// Old state: called outside mutationCallback (before-mutation phase).
98+
// New state: called inside mutationCallback (after-mutation phase).
99+
if (!insideMutationCallback_) {
95100
AnimationKeyFrameView oldView{
96-
.layoutMetrics = keyframeMetrics, .tag = tag, .surfaceId = surfaceId};
101+
.layoutMetrics = keyframeMetrics,
102+
.tag = tag,
103+
.surfaceId = surfaceId,
104+
.transitionId = currentTransitionId};
97105
oldLayout_[name] = oldView;
98106

99107
// Request the platform to capture a bitmap snapshot of the old view
@@ -147,7 +155,10 @@ void ViewTransitionModule::applyViewTransitionName(
147155

148156
} else {
149157
AnimationKeyFrameView newView{
150-
.layoutMetrics = keyframeMetrics, .tag = tag, .surfaceId = surfaceId};
158+
.layoutMetrics = keyframeMetrics,
159+
.tag = tag,
160+
.surfaceId = surfaceId,
161+
.transitionId = activeTransitionId_};
151162
newLayout_[name] = newView;
152163
}
153164
}
@@ -316,7 +327,7 @@ void ViewTransitionModule::cancelViewTransitionName(
316327

317328
void ViewTransitionModule::restoreViewTransitionName(
318329
const ShadowNode& shadowNode) {
319-
nameRegistry_[shadowNode.getTag()].merge(
330+
nameRegistry_[shadowNode.getTag()].names.merge(
320331
cancelledNameRegistry_[shadowNode.getTag()]);
321332
cancelledNameRegistry_.erase(shadowNode.getTag());
322333
}
@@ -387,13 +398,16 @@ void ViewTransitionModule::startViewTransition(
387398

388399
// Mark transition as started
389400
transitionStarted_ = true;
401+
activeTransitionId_ = ++transitionIdCounter_;
390402
pendingAnimationIds_.clear();
391403
onCompleteCallback_ = onCompleteCallback;
392404

393405
// Call mutation callback (including commitRoot, measureInstance,
394406
// applyViewTransitionName, createViewTransitionInstance for old & new)
395407
if (mutationCallback) {
408+
insideMutationCallback_ = true;
396409
mutationCallback();
410+
insideMutationCallback_ = false;
397411
}
398412

399413
applySnapshotsOnPseudoElementShadowNodes();
@@ -442,13 +456,28 @@ void ViewTransitionModule::suspendOnActiveViewTransition() {
442456
}
443457

444458
void ViewTransitionModule::startViewTransitionEnd() {
445-
for (const auto& [tag, names] : nameRegistry_) {
446-
for (const auto& name : names) {
447-
oldLayout_.erase(name);
448-
newLayout_.erase(name);
459+
auto finishedId = activeTransitionId_;
460+
461+
// Only clear layout and registry entries belonging to the finished
462+
// transition. A suspended transition's before-mutation phase may have
463+
// already written entries with a newer transitionId — preserve those.
464+
for (auto it = nameRegistry_.begin(); it != nameRegistry_.end();) {
465+
if (it->second.transitionId == finishedId) {
466+
for (const auto& name : it->second.names) {
467+
if (auto oit = oldLayout_.find(name);
468+
oit != oldLayout_.end() && oit->second.transitionId == finishedId) {
469+
oldLayout_.erase(oit);
470+
}
471+
if (auto nit = newLayout_.find(name);
472+
nit != newLayout_.end() && nit->second.transitionId == finishedId) {
473+
newLayout_.erase(nit);
474+
}
475+
}
476+
it = nameRegistry_.erase(it);
477+
} else {
478+
++it;
449479
}
450480
}
451-
nameRegistry_.clear();
452481
oldPseudoElementNodes_.clear();
453482

454483
// Clear any pending bitmap snapshots that were captured but never consumed.

packages/react-native/ReactCommon/react/renderer/viewtransition/ViewTransitionModule.h

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,17 +98,25 @@ class ViewTransitionModule : public UIManagerViewTransitionDelegate,
9898
AnimationKeyFrameViewLayoutMetrics layoutMetrics;
9999
Tag tag{0};
100100
SurfaceId surfaceId{0};
101+
uint32_t transitionId{0};
101102
};
102103

103104
private:
105+
uint32_t transitionIdCounter_{0};
106+
uint32_t activeTransitionId_{0};
107+
104108
// registry of layout of old/new views
105109
std::unordered_map<std::string, AnimationKeyFrameView> oldLayout_{};
106110
std::unordered_map<std::string, AnimationKeyFrameView> newLayout_{};
107-
// tag -> names registry, populated during applyViewTransitionName
111+
// tag -> (names, transitionId) registry, populated during applyViewTransitionName
108112
// Note that tag and name are not 1:1 mapping
109113
// - In some nested composition 2 names are mappped to the same tag
110114
// - tags of old and new views are mapped to the same name(s)
111-
std::unordered_map<Tag, std::unordered_set<std::string>> nameRegistry_{};
115+
struct NameRegistryEntry {
116+
std::unordered_set<std::string> names;
117+
uint32_t transitionId{0};
118+
};
119+
std::unordered_map<Tag, NameRegistryEntry> nameRegistry_{};
112120

113121
// used for cancel/restore viewTransitionName
114122
std::unordered_map<Tag, std::unordered_set<std::string>> cancelledNameRegistry_{};
@@ -138,6 +146,8 @@ class ViewTransitionModule : public UIManagerViewTransitionDelegate,
138146

139147
bool transitionStarted_{false};
140148

149+
bool insideMutationCallback_{false};
150+
141151
bool transitionReadyFinished_{false};
142152

143153
// When suspendNextTransition_ is true and a transition is active, the next

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5453,6 +5453,7 @@ struct facebook::react::ViewTransitionModule::AnimationKeyFrameView {
54535453
public facebook::react::SurfaceId surfaceId;
54545454
public facebook::react::Tag tag;
54555455
public facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics layoutMetrics;
5456+
public uint32_t transitionId;
54565457
}
54575458

54585459
struct facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics {

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5279,6 +5279,7 @@ struct facebook::react::ViewTransitionModule::AnimationKeyFrameView {
52795279
public facebook::react::SurfaceId surfaceId;
52805280
public facebook::react::Tag tag;
52815281
public facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics layoutMetrics;
5282+
public uint32_t transitionId;
52825283
}
52835284

52845285
struct facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics {

scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5444,6 +5444,7 @@ struct facebook::react::ViewTransitionModule::AnimationKeyFrameView {
54445444
public facebook::react::SurfaceId surfaceId;
54455445
public facebook::react::Tag tag;
54465446
public facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics layoutMetrics;
5447+
public uint32_t transitionId;
54475448
}
54485449

54495450
struct facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics {

scripts/cxx-api/api-snapshots/ReactAppleDebugCxx.api

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8031,6 +8031,7 @@ struct facebook::react::ViewTransitionModule::AnimationKeyFrameView {
80318031
public facebook::react::SurfaceId surfaceId;
80328032
public facebook::react::Tag tag;
80338033
public facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics layoutMetrics;
8034+
public uint32_t transitionId;
80348035
}
80358036

80368037
struct facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics {

scripts/cxx-api/api-snapshots/ReactAppleNewarchCxx.api

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7506,6 +7506,7 @@ struct facebook::react::ViewTransitionModule::AnimationKeyFrameView {
75067506
public facebook::react::SurfaceId surfaceId;
75077507
public facebook::react::Tag tag;
75087508
public facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics layoutMetrics;
7509+
public uint32_t transitionId;
75097510
}
75107511

75117512
struct facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics {

scripts/cxx-api/api-snapshots/ReactAppleReleaseCxx.api

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8022,6 +8022,7 @@ struct facebook::react::ViewTransitionModule::AnimationKeyFrameView {
80228022
public facebook::react::SurfaceId surfaceId;
80238023
public facebook::react::Tag tag;
80248024
public facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics layoutMetrics;
8025+
public uint32_t transitionId;
80258026
}
80268027

80278028
struct facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics {

scripts/cxx-api/api-snapshots/ReactCommonDebugCxx.api

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3892,6 +3892,7 @@ struct facebook::react::ViewTransitionModule::AnimationKeyFrameView {
38923892
public facebook::react::SurfaceId surfaceId;
38933893
public facebook::react::Tag tag;
38943894
public facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics layoutMetrics;
3895+
public uint32_t transitionId;
38953896
}
38963897

38973898
struct facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics {

scripts/cxx-api/api-snapshots/ReactCommonNewarchCxx.api

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3758,6 +3758,7 @@ struct facebook::react::ViewTransitionModule::AnimationKeyFrameView {
37583758
public facebook::react::SurfaceId surfaceId;
37593759
public facebook::react::Tag tag;
37603760
public facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics layoutMetrics;
3761+
public uint32_t transitionId;
37613762
}
37623763

37633764
struct facebook::react::ViewTransitionModule::AnimationKeyFrameViewLayoutMetrics {

0 commit comments

Comments
 (0)