Skip to content

Commit

Permalink
Differ: ensure all ShadowViews generated by differ have correct Layou…
Browse files Browse the repository at this point in the history
…tMetrics

Summary:
While I think this was a very marginal bug with no known issues in the wild, incorrect layout values were sometimes being propagated to certain nodes. This would only occur during complex nested (un)flattening operations and may only impact node consistency, specifically with setting the "previous" ShadowView of mutation instructions, specifically REMOVE and DELETE. Even in rigorous testing I had trouble hitting this case and it didn't seem to impact the "next" values in CREATE, INSERT, or UPDATE.

The issue: previously `sliceChildShadowNodeViewPairsV2` assumed that the node it's operating on is a child of a non-flattened view, and the baseline origin is `{0,0}`. You can see when `sliceChildShadowNodeViewPairsRecursivelyV2` is called, a `layoutOffset` is passed in. If we ever got a list of a node that was in a flattened parent by calling `sliceChildShadowNodeViewPairsV2`, we would incorrectly assume that baseline layoutOffset for the node is `0,0`.

Now, we store the layoutOffset in the ShadowViewNodePair and can retrieve it when getting child pairs of a node.

Changelog: [internal]

Reviewed By: sammy-SC

Differential Revision: D27759380

fbshipit-source-id: a89756190a1cb377bcc55ff31799c2afbaecdaa9
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Apr 15, 2021
1 parent c22b874 commit d1b1e8b
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 5 deletions.
20 changes: 16 additions & 4 deletions ReactCommon/react/renderer/mounting/Differentiator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,16 @@ static void sliceChildShadowNodeViewPairsRecursivelyV2(
bool isConcreteView = shadowNodeIsConcrete(childShadowNode);
bool areChildrenFlattened = !childShadowNode.getTraits().check(
ShadowNodeTraits::Trait::FormsStackingContext);
Point storedOrigin = {};
if (areChildrenFlattened) {
storedOrigin = origin;
}
scope.push_back(
{shadowView, &childShadowNode, areChildrenFlattened, isConcreteView});
{shadowView,
&childShadowNode,
areChildrenFlattened,
isConcreteView,
storedOrigin});
pairList.push_back(&scope.back());

if (areChildrenFlattened) {
Expand All @@ -255,7 +263,8 @@ static void sliceChildShadowNodeViewPairsRecursivelyV2(
ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairsV2(
ShadowNode const &shadowNode,
ViewNodePairScope &scope,
bool allowFlattened) {
bool allowFlattened,
Point layoutOffset) {
auto pairList = ShadowViewNodePair::NonOwningList{};

if (!shadowNode.getTraits().check(
Expand All @@ -266,7 +275,7 @@ ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairsV2(
}

sliceChildShadowNodeViewPairsRecursivelyV2(
pairList, scope, {0, 0}, shadowNode);
pairList, scope, layoutOffset, shadowNode);

// Sorting pairs based on `orderIndex` if needed.
reorderInPlaceIfNeeded(pairList);
Expand All @@ -291,7 +300,10 @@ sliceChildShadowNodeViewPairsFromViewNodePair(
ViewNodePairScope &scope,
bool allowFlattened = false) {
return sliceChildShadowNodeViewPairsV2(
*shadowViewNodePair.shadowNode, scope, allowFlattened);
*shadowViewNodePair.shadowNode,
scope,
allowFlattened,
shadowViewNodePair.contextOrigin);
}

/*
Expand Down
3 changes: 2 additions & 1 deletion ReactCommon/react/renderer/mounting/Differentiator.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ ShadowViewMutation::List calculateShadowViewMutations(
ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairsV2(
ShadowNode const &shadowNode,
ViewNodePairScope &viewNodePairScope,
bool allowFlattened = false);
bool allowFlattened = false,
Point layoutOffset = {0, 0});

/*
* Generates a list of `ShadowViewNodePair`s that represents a layer of a
Expand Down
1 change: 1 addition & 0 deletions ReactCommon/react/renderer/mounting/ShadowView.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ struct ShadowViewNodePair final {
ShadowNode const *shadowNode;
bool flattened{false};
bool isConcreteView{true};
Point contextOrigin{0, 0};

size_t mountIndex{0};

Expand Down

0 comments on commit d1b1e8b

Please sign in to comment.