Skip to content

Commit

Permalink
Fix case where absolute nodes would sometimes not be cloned
Browse files Browse the repository at this point in the history
Summary:
X-link: facebook/react-native#45240

X-link: facebook/yoga#1675

There was a bug where some crash would happen if a tree was cloned that had static/absolute parent/child pair inside it. This was because we were no longer calling `cloneChildrenIfNeeded` on the static parent, but would still layout the absolute child. So that child's owner would be stale and have new layout. In React Native this would lead to a failed assert which causes the crash.

The fix here is to clone the children of static nodes during `layoutAbsoluteDescendants` so that we guarantee the node is either cloned if it is going to have new layout.

Changelog: [Internal]

Reviewed By: NickGerleman

Differential Revision: D59175629

fbshipit-source-id: 4d110a08ba5368704327d5ab69a8695b28e746f4
  • Loading branch information
joevilches authored and facebook-github-bot committed Jul 2, 2024
1 parent 0c75986 commit 92aff30
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 14 deletions.
42 changes: 29 additions & 13 deletions lib/yoga/src/main/cpp/yoga/algorithm/AbsoluteLayout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ void layoutAbsoluteChild(
containingBlockHeight);
}

void layoutAbsoluteDescendants(
bool layoutAbsoluteDescendants(
yoga::Node* containingNode,
yoga::Node* currentNode,
SizingMode widthSizingMode,
Expand All @@ -486,6 +486,7 @@ void layoutAbsoluteDescendants(
float currentNodeTopOffsetFromContainingBlock,
float containingNodeAvailableInnerWidth,
float containingNodeAvailableInnerHeight) {
bool hasNewLayout = false;
for (auto child : currentNode->getChildren()) {
if (child->style().display() == Display::None) {
continue;
Expand Down Expand Up @@ -514,6 +515,8 @@ void layoutAbsoluteDescendants(
currentDepth,
generationCount);

hasNewLayout = hasNewLayout || child->getHasNewLayout();

/*
* At this point the child has its position set but only on its the
* parent's flexStart edge. Additionally, this position should be
Expand Down Expand Up @@ -571,6 +574,13 @@ void layoutAbsoluteDescendants(
} else if (
child->style().positionType() == PositionType::Static &&
!child->alwaysFormsContainingBlock()) {
// We may write new layout results for absolute descendants of "child"
// which are positioned relative to the current containing block instead
// of their parent. "child" may not be dirty, or have new constraints, so
// absolute positioning may be the first time during this layout pass that
// we need to mutate these descendents. Make sure the path of
// nodes to them is mutable before positioning.
child->cloneChildrenIfNeeded();
const Direction childDirection =
child->resolveDirection(currentNodeDirection);
// By now all descendants of the containing block that are not absolute
Expand All @@ -582,19 +592,25 @@ void layoutAbsoluteDescendants(
currentNodeTopOffsetFromContainingBlock +
child->getLayout().position(PhysicalEdge::Top);

layoutAbsoluteDescendants(
containingNode,
child,
widthSizingMode,
childDirection,
layoutMarkerData,
currentDepth + 1,
generationCount,
childLeftOffsetFromContainingBlock,
childTopOffsetFromContainingBlock,
containingNodeAvailableInnerWidth,
containingNodeAvailableInnerHeight);
hasNewLayout = hasNewLayout ||
layoutAbsoluteDescendants(
containingNode,
child,
widthSizingMode,
childDirection,
layoutMarkerData,
currentDepth + 1,
generationCount,
childLeftOffsetFromContainingBlock,
childTopOffsetFromContainingBlock,
containingNodeAvailableInnerWidth,
containingNodeAvailableInnerHeight);

if (hasNewLayout) {
child->setHasNewLayout(hasNewLayout);
}
}
}
return hasNewLayout;
}
} // namespace facebook::yoga
3 changes: 2 additions & 1 deletion lib/yoga/src/main/cpp/yoga/algorithm/AbsoluteLayout.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ void layoutAbsoluteChild(
uint32_t depth,
uint32_t generationCount);

void layoutAbsoluteDescendants(
// Returns if some absolute descendant has new layout
bool layoutAbsoluteDescendants(
yoga::Node* containingNode,
yoga::Node* currentNode,
SizingMode widthSizingMode,
Expand Down

0 comments on commit 92aff30

Please sign in to comment.