Skip to content

Fix stale layout when a child switches to display: contents after insertion - #2028

Open
kwy404 wants to merge 1 commit into
react:mainfrom
kwy404:fix-contents-display-after-insert
Open

kwy404 wants to merge 1 commit into
react:mainfrom
kwy404:fix-contents-display-after-insert

Conversation

@kwy404

@kwy404 kwy404 commented Sep 25, 2026

Copy link
Copy Markdown

Summary

Each node caches how many of its children use display: contents (contentsChildrenCount_, added in #1726). The count is updated in insertChild, removeChild, replaceChild and setChildren, but not when the display of a child that is already attached changes.

So if YGNodeStyleSetDisplay(child, YGDisplayContents) is called after the child was inserted, the owner still reports hasContentsChildren() == false. The layout algorithm then skips cleanupContentsNodesRecursively for it, and the contents node keeps isDirty == true after layout. When a descendant of that node changes later, markDirtyAndPropagate stops at the contents node because it is already dirty, the root is never marked dirty, and the next YGNodeCalculateLayout returns the cached layout.

Minimal repro:

YGNodeRef root = YGNodeNew();
YGNodeStyleSetWidth(root, 100);
YGNodeStyleSetHeight(root, 100);
YGNodeRef child = YGNodeNew();
YGNodeInsertChild(root, child, 0);
YGNodeRef grandchild = YGNodeNew();
YGNodeStyleSetWidth(grandchild, 10);
YGNodeStyleSetHeight(grandchild, 10);
YGNodeInsertChild(child, grandchild, 0);

YGNodeStyleSetDisplay(child, YGDisplayContents);
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
// YGNodeIsDirty(child) is still true

YGNodeStyleSetWidth(grandchild, 20);
// YGNodeIsDirty(root) is false
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
// YGNodeLayoutGetWidth(grandchild) is 10, expected 20

This affects any binding that mutates nodes in place (the JS and Java bindings both go through YGNodeStyleSetDisplay). Setting the display before inserting the child works, which is what the existing tests do.

The fix recomputes the owner's count in YGNodeStyleSetDisplay when a node that has an owner switches to or from display: contents.

Changelog: [General][Fixed] - Fix stale layout when a child switches to display: contents after being inserted

Test Plan

Added dirty_propagation_through_child_set_to_display_contents to tests/YGDirtyMarkingTest.cpp. Without the fix, all three of its assertions fail (the contents node stays dirty, the root is not marked dirty, and the grandchild keeps width 10). With the fix it passes.

Ran the full C++ suite (unit_tests.bat, MSVC 2022 + Ninja): 855 tests passed.

Checked formatting of the changed files with the repo's clang-format version (21.1.2): clang-format --dry-run --Werror yoga/YGNodeStyle.cpp tests/YGDirtyMarkingTest.cpp reports no changes.

…ertion

Each node caches how many of its children use `display: contents`, but
the count is only updated when children are inserted, removed or
replaced. Setting `display: contents` on a child that is already
attached left the count at zero, so the owner skipped
`cleanupContentsNodesRecursively` and the contents node stayed dirty
after layout. Dirty marking from its children then stopped at that node
and never reached the root, so later style changes were ignored.

Recompute the owner's count in `YGNodeStyleSetDisplay` when a node with
an owner switches to or from `display: contents`.
@meta-cla meta-cla Bot added the CLA Signed label Sep 25, 2026
@facebook-github-tools facebook-github-tools Bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 25, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant