From 03390d7c8764d62b8a29ac8b0fab0d54d98e55be Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Tue, 9 Feb 2021 22:40:17 -0800 Subject: [PATCH] Differ: fix confusion between flattening and concreteness of views Summary: During earlier testing I didn't fully realize that Android disables a core bit of View Flattening: Views can be concrete or non-concrete; and their children can be flattened or not. None of these properties are mutually exclusive with each other. Except on Android - that functionality is currently disabled. A View can be either flattened and non-concrete, or non-flat and concrete. So there are some flattening edge-cases hit on iOS but not Android, due to the larger state-space on iOS. To test, I forced Android to align with iOS and tested; and then tested on iOS; and ensured no mounting errors, assertions, or crashes were hit during some specific tests. Changelog: [Internal] Differential Revision: D26298872 fbshipit-source-id: 2f0f78127a7bf057c7cf109005f1dae74f0ff6ba --- .../renderer/mounting/Differentiator.cpp | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/ReactCommon/react/renderer/mounting/Differentiator.cpp b/ReactCommon/react/renderer/mounting/Differentiator.cpp index 5545674c3e2..79879a5dce2 100644 --- a/ReactCommon/react/renderer/mounting/Differentiator.cpp +++ b/ReactCommon/react/renderer/mounting/Differentiator.cpp @@ -1220,18 +1220,9 @@ static void calculateShadowViewMutationsV2( } } - // We handled this case above. We fall through to check concreteness - // of old/new view to remove/insert create/delete above, and then bail - // out here. - if (oldChildPair.flattened != newChildPair.flattened) { - newInsertedPairs.erase(insertedIt); - oldIndex++; - continue; - } - // old and new child pairs are both either flattened or unflattened at // this point. If they're not views, we don't need to update subtrees. - if (oldChildPair.isConcreteView) { + if (oldChildPair.isConcreteView && newChildPair.isConcreteView) { // TODO: do we always want to remove here? There are cases where we // might be able to remove this to prevent unnecessary // removes/inserts in cases of (un)flattening + reorders? @@ -1245,7 +1236,8 @@ static void calculateShadowViewMutationsV2( oldChildPair.shadowView, newChildPair.shadowView)); } } - if (!oldChildPair.flattened && + + if (!oldChildPair.flattened && !newChildPair.flattened && oldChildPair.shadowNode != newChildPair.shadowNode) { // Update subtrees auto oldGrandChildPairs = @@ -1313,9 +1305,16 @@ static void calculateShadowViewMutationsV2( newChildPair.shadowView, newChildPair.mountIndex)); } + + // `inOtherTree` is only set to true during flattening/unflattening of + // parent. If the parent isn't (un)flattened, this will always be `false`, + // even if the node is in the other (old) tree. In this case, we expect + // the node to be removed from `newInsertedPairs` when we later encounter + // it in this loop. if (!newChildPair.inOtherTree) { newInsertedPairs.insert({newChildPair.shadowView.tag, &newChildPair}); } + newIndex++; }