Skip to content

Commit faebbfa

Browse files
Yonah Karpfacebook-github-bot
authored andcommitted
Yoga | dirty YGNode when removing it from parent
Summary: ## Context When a child node is removed from its parent via YGNodeRemoveChild, the child's calculated layout is cleared (setLayout({})) but the child node is not marked dirty. This causes issues when the view is removed views and later reused with the same layout values. the layout values were cleared (resulting in NaN/0), but since the layout values are the same for the newly reattached node, it isn't dirtied. However, the user would expect it to be dirty, as: a) a fully new node would be dirty by default. b) They just set layout **Example**: - Node is removed from the view and it's layout is cleared. - Node is reattatched and set with height/width that happens to be same as previous - Checking if the view is dirty unexpectedly gives false, despite user setting a "new" value. - Pulling height/width gives 0, as layout hasn't been recalculated yet ## This Diff Marks the removed child node as dirty when clearing its layout in YGNodeRemoveChild, ensuring that subsequent layout calculations will properly recalculate the child's layout values Reviewed By: NickGerleman Differential Revision: D92280506
1 parent 526a220 commit faebbfa

3 files changed

Lines changed: 90 additions & 0 deletions

File tree

tests/YGDirtyMarkingTest.cpp

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,3 +249,58 @@ TEST(YogaTest, dirty_node_only_if_undefined_values_gets_set_to_undefined) {
249249

250250
YGNodeFreeRecursive(root);
251251
}
252+
253+
TEST(YogaTest, dirty_removed_child_node) {
254+
YGNodeRef root = YGNodeNew();
255+
YGNodeStyleSetWidth(root, 100);
256+
YGNodeStyleSetHeight(root, 100);
257+
258+
YGNodeRef child = YGNodeNew();
259+
YGNodeStyleSetWidth(child, 50);
260+
YGNodeStyleSetHeight(child, 50);
261+
YGNodeInsertChild(root, child, 0);
262+
263+
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
264+
265+
EXPECT_FALSE(YGNodeIsDirty(child));
266+
267+
YGNodeRemoveChild(root, child);
268+
269+
// Child should be marked dirty after removal so layout is recalculated
270+
// when the child is reused (e.g., in a recycling view system)
271+
EXPECT_TRUE(YGNodeIsDirty(child));
272+
273+
YGNodeFree(child);
274+
YGNodeFreeRecursive(root);
275+
}
276+
277+
TEST(YogaTest, dirty_removed_child_nodes_when_removing_all) {
278+
YGNodeRef root = YGNodeNew();
279+
YGNodeStyleSetWidth(root, 100);
280+
YGNodeStyleSetHeight(root, 100);
281+
282+
YGNodeRef child0 = YGNodeNew();
283+
YGNodeStyleSetWidth(child0, 50);
284+
YGNodeStyleSetHeight(child0, 25);
285+
YGNodeInsertChild(root, child0, 0);
286+
287+
YGNodeRef child1 = YGNodeNew();
288+
YGNodeStyleSetWidth(child1, 50);
289+
YGNodeStyleSetHeight(child1, 25);
290+
YGNodeInsertChild(root, child1, 1);
291+
292+
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
293+
294+
EXPECT_FALSE(YGNodeIsDirty(child0));
295+
EXPECT_FALSE(YGNodeIsDirty(child1));
296+
297+
YGNodeRemoveAllChildren(root);
298+
299+
// All children should be marked dirty after removal
300+
EXPECT_TRUE(YGNodeIsDirty(child0));
301+
EXPECT_TRUE(YGNodeIsDirty(child1));
302+
303+
YGNodeFree(child0);
304+
YGNodeFree(child1);
305+
YGNodeFreeRecursive(root);
306+
}

tests/YGNodeChildTest.cpp

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,3 +33,36 @@ TEST(YogaTest, reset_layout_when_child_removed) {
3333
YGNodeFreeRecursive(root);
3434
YGNodeFreeRecursive(root_child0);
3535
}
36+
37+
TEST(YogaTest, removed_child_can_be_reused_with_valid_layout) {
38+
YGNodeRef root = YGNodeNew();
39+
YGNodeStyleSetWidth(root, 200);
40+
YGNodeStyleSetHeight(root, 200);
41+
42+
YGNodeRef child = YGNodeNew();
43+
YGNodeStyleSetWidth(child, 100);
44+
YGNodeStyleSetHeight(child, 100);
45+
YGNodeInsertChild(root, child, 0);
46+
47+
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
48+
49+
ASSERT_EQ(100, YGNodeLayoutGetWidth(child));
50+
ASSERT_EQ(100, YGNodeLayoutGetHeight(child));
51+
52+
// Remove child - layout should be cleared and child marked dirty
53+
YGNodeRemoveChild(root, child);
54+
55+
ASSERT_TRUE(YGFloatIsUndefined(YGNodeLayoutGetWidth(child)));
56+
ASSERT_TRUE(YGFloatIsUndefined(YGNodeLayoutGetHeight(child)));
57+
ASSERT_TRUE(YGNodeIsDirty(child));
58+
59+
// Reinsert the child and recalculate - layout should be valid again
60+
YGNodeInsertChild(root, child, 0);
61+
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
62+
63+
ASSERT_EQ(100, YGNodeLayoutGetWidth(child));
64+
ASSERT_EQ(100, YGNodeLayoutGetHeight(child));
65+
ASSERT_FALSE(YGNodeIsDirty(child));
66+
67+
YGNodeFreeRecursive(root);
68+
}

yoga/YGNode.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ void YGNodeRemoveChild(
177177
if (owner == childOwner) {
178178
excludedChild->setLayout({}); // layout is no longer valid
179179
excludedChild->setOwner(nullptr);
180+
excludedChild->setDirty(true); // invalidate cache
180181
}
181182
owner->markDirtyAndPropagate();
182183
}
@@ -198,6 +199,7 @@ void YGNodeRemoveAllChildren(const YGNodeRef ownerRef) {
198199
yoga::Node* oldChild = owner->getChild(i);
199200
oldChild->setLayout({}); // layout is no longer valid
200201
oldChild->setOwner(nullptr);
202+
oldChild->setDirty(true); // invalidate cache
201203
}
202204
owner->clearChildren();
203205
owner->markDirtyAndPropagate();

0 commit comments

Comments
 (0)