Add support for `display: contents` style #1726

j-piasecki · 2024-10-14T12:56:14Z

Summary

This PR adds support for display: contents style by effectively skipping nodes with display: contents set during layout.

This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - display: contents allows nodes to be skipped, i.e.:

<div id="node1">
  <div id="node2" style="display: contents;">
    <div id="node3" />
  </div>
</div>

node3 will be laid out as if it were a child of node1.

Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces LayoutableChildren::Iterator which can traverse the subtree of a given node in a way that nodes with display: contents are replaced with their concrete children.

A tree like this:

flowchart TD
    A((A))
    B((B))
    C((C))
    D((D))
    E((E))
    F((F))
    G((G))
    H((H))
    I((I))
    J((J))

    A --> B
    A --> C
    B --> D
    B --> E
    C --> F
    D --> G
    F --> H
    G --> I
    H --> J

    style B fill:#050
    style C fill:#050
    style D fill:#050
    style H fill:#050
    style I fill:#050

would be laid out as if the green nodes (ones with display: contents) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries.

There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many display: contents nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex.

One more major change this PR introduces is cleanupContentsNodesRecursively. Since nodes with display: contents would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested contents nodes, would not be cloned, breaking doesOwn relation. All of this is handled in the new method which clones contents nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side.

Relies on #1725

Test plan

Added tests for display: contents based on existing tests for display: none and ensured that all the tests were passing.

vercel · 2024-10-14T12:56:19Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
yoga-website	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 18, 2024 3:30pm

NickGerleman

This is really excellent! Thanks for all the work you've done here. I know this has gone through a lot of iteration to get to this point, and I think we've landed on a really solid way to implement this in Yoga + RN.

I can see it incorporates #1725 as part of it. I might want to split while importing, to land that part first, in case it leads to any unexpected breakage.

It also looks like this generates some MSVC /W4 specific warnings. We should fix those before merging. https://github.com/facebook/yoga/actions/runs/11328004846/job/31512544728?pr=1726

NickGerleman · 2024-10-14T16:59:34Z

yoga/node/Node.h

@@ -31,6 +31,128 @@ namespace facebook::yoga {

 class YG_EXPORT Node : public ::YGNode {
 public:
+  class LayoutableChildren {


Could we put this in its own header?

Done in 7305320

NickGerleman · 2024-10-14T17:03:57Z

yoga/node/Node.h

+      const Node* node_;
+      size_t childIndex_;
+      size_t currentNodeIndex_{0};
+      Backtrack backtrack_;


Most of the time it will be an empty vector, so no heap allocation, but I wonder if this could refer to e.g. single parent iterator (and that iterator could point to parent), such that iteration only allocates memory on the stack.

I don't think this is easily achievable. I see two main problems with it:

incrementing the iterator would have to return a new instance with a pointer to the previous one when going deeper into the tree, we would need to prolong the lifetime of the previous iterator somehow, so that it's valid when actually backtracking.

modifying the "parent" iterator would possibly break backtracking logic. To prevent that, each iterator would need to keep a deep copy of all ancestors.

NickGerleman · 2024-10-14T17:06:24Z

yoga/node/Node.h

+
+   private:
+    const Node* node_;
+  };


Does this all work out if we add following below? As I'm sure you ran into, missing or incorrect signatures can lead to cryptic errors within standard library, so it's helpful to have something like this which can show whether iterator is valid implementation.

static_assert(std::input_iterator<LayoutableChildren::Iterator>);

Done in 5028c3e

yoga/node/Node.h

NickGerleman

LGTM. I will break apart the absolute positioning change from this and land first just to be safe (will try to merge Wednesday after a lot of branches cut).

facebook-github-bot · 2024-10-15T16:01:49Z

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 X-link: facebook/yoga#1726 Differential Revision: D64404340 Pulled By: NickGerleman

Summary: This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 X-link: facebook/yoga#1726 Test Plan: Added tests for `display: contents` based on existing tests for `display: none` and ensured that all the tests were passing. Differential Revision: D64404340 Pulled By: NickGerleman

NickGerleman · 2024-10-18T07:13:50Z

yoga/node/LayoutableChildren.h

+  };
+
+  LayoutableChildren(const T* node) : node_(node) {
+    static_assert(std::input_iterator<LayoutableChildren<T>::Iterator>);


Apparently there was a C++ 20 standard defect that led older stdlibc++ to require these to be default constructible 😮. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2325r3.html

Going to locally patch in a default ctor that acts as the sentinel/end node.

buck-out/v2/gen/fbsource/aaebd9ffd9ca7aaa/xplat/yoga/__yogaFbcode__/buck-private-headers/yoga/node/Node.h:150:12: note: in instantiation of member function 'facebook::yoga::LayoutableChildren<facebook::yoga::Node>::LayoutableChildren' requested here return LayoutableChildren(this); ^ buck-out/v2/gen/fbsource/aaebd9ffd9ca7aaa/xplat/yoga/__yogaFbcode__/buck-private-headers/yoga/node/LayoutableChildren.h:118:19: note: because 'LayoutableChildren<Node>::Iterator' does not satisfy 'input_iterator' static_assert(std::input_iterator<LayoutableChildren<T>::Iterator>); ^ fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/iterator_concepts.h:634:30: note: because 'facebook::yoga::LayoutableChildren<facebook::yoga::Node>::Iterator' does not satisfy 'input_or_output_iterator' concept input_iterator = input_or_output_iterator<_Iter> ^ fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/iterator_concepts.h:614:5: note: because 'facebook::yoga::LayoutableChildren<facebook::yoga::Node>::Iterator' does not satisfy 'weakly_incrementable' && weakly_incrementable<_Iter>; ^ fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/iterator_concepts.h:597:36: note: because 'facebook::yoga::LayoutableChildren<facebook::yoga::Node>::Iterator' does not satisfy 'default_initializable' concept weakly_incrementable = default_initializable<_Iter> ^ fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/concepts:143:37: note: because 'facebook::yoga::LayoutableChildren<facebook::yoga::Node>::Iterator' does not satisfy 'constructible_from' concept default_initializable = constructible_from<_Tp> ^ fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/concepts:139:30: note: because 'is_constructible_v<facebook::yoga::LayoutableChildren<facebook::yoga::Node>::Iterator>' evaluated to false = destructible<_Tp> && is_constructible_v<_Tp, _Args...>;

NickGerleman · 2024-10-18T07:30:31Z

yoga/node/Node.h

@@ -144,6 +146,24 @@ class YG_EXPORT Node : public ::YGNode {
    return children_.size();
  }

+  const LayoutableChildren getLayoutChildren() const {


The other children already do this, so makes sense to keep with the pattern (no change needed/requested), but it is bizarre that the methods which return mutable references to children are marked const.

NickGerleman · 2024-10-18T07:36:51Z

Edge case I didn't think of (not blocker), what happens if the root node has display: contents? I’m sure a bunch of external things would break on zero root node metrics, but I wonder if we would crash or do something more graceful.

Summary: This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 Changelog: [Internal] X-link: facebook/yoga#1726 Test Plan: Added tests for `display: contents` based on existing tests for `display: none` and ensured that all the tests were passing. Differential Revision: D64404340 Pulled By: NickGerleman

j-piasecki · 2024-10-18T15:03:32Z

Edge case I didn't think of (not blocker), what happens if the root node has display: contents? I’m sure a bunch of external things would break on zero root node metrics, but I wonder if we would crash or do something more graceful.

I think it would be effectively ignored since it's the parent that skips the children if they do have display: contents set.

Summary: This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 Changelog: [Internal] X-link: facebook/yoga#1726 Test Plan: Added tests for `display: contents` based on existing tests for `display: none` and ensured that all the tests were passing. Differential Revision: D64404340 Pulled By: NickGerleman

facebook-github-bot · 2024-10-19T05:09:04Z

@NickGerleman merged this pull request in 68bb234.

Summary: Pull Request resolved: #47035 This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 Changelog: [Internal] X-link: facebook/yoga#1726 Test Plan: Added tests for `display: contents` based on existing tests for `display: none` and ensured that all the tests were passing. Reviewed By: joevilches Differential Revision: D64404340 Pulled By: NickGerleman fbshipit-source-id: f6f6e9a6fad82873f18c8a0ead58aad897df5d09

Summary: This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:#50 style C fill:#50 style D fill:#50 style H fill:#50 style I fill:#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 X-link: facebook/yoga#1726 This PR adds a few things over the corresponding one in Yoga: - new value in the `DisplayType` enum - `Contents` - new `ShadowNodeTrait` - `ForceFlattenView`, that forces the node not to form a host view - updates TS types to include `display: contents` - aliases `display: contents` to `display: none` on the old architecture ## Changelog: [GENERAL] [ADDED] - Added support for `display: contents` Pull Request resolved: #46584 Test Plan: <details> <summary>So far I've been testing on relatively simple snippets like this one and on entirety of RNTester by inserting views with `display: contents` in random places and seeing if anything breaks.</summary> ```jsx import React from 'react'; import { Button, Pressable, SafeAreaView, ScrollView, TextInput, View, Text } from 'react-native'; export default function App() { const [toggle, setToggle] = React.useState(false); return ( <View style={{flex: 1, paddingTop: 100}}> <SafeAreaView style={{width: '100%', height: 200}}> <Pressable style={{width: 100, height: 100, backgroundColor: 'black'}} onPress={() => setToggle(!toggle)}> <ScrollView /> </Pressable> <View style={{display: 'flex', flexDirection: 'row', flex: 1, backgroundColor: 'magenta'}}> <SafeAreaView style={{ // display: 'contents', flex: 1, }}> <View style={{ display: 'contents', width: '100%', height: 200, }}> <View style={{ display: 'contents', flex: 1, }}> { toggle && <View style={{flex: 1, backgroundColor: 'yellow'}} /> } <View style={{flex: 1, backgroundColor: 'blue'}} /> <View style={{flex: 1, backgroundColor: 'cyan'}} /> </View> </View> </SafeAreaView> </View> {/* <View style={{width: 100, height: 100, backgroundColor: 'magenta', display: 'flex'}} /> */} <TextInput style={{width: 200, height: 100, backgroundColor: 'red', display: 'flex'}}> <Text style={{color: 'white'}}>Hello</Text> <Text style={{color: 'green'}}>World</Text> </TextInput> </SafeAreaView> </View> ); } ``` </details> Reviewed By: joevilches Differential Revision: D64584476 Pulled By: NickGerleman fbshipit-source-id: bec77b5087ff95f0980cf02274fbb2c8581eb3c0

Summary: X-link: facebook/react-native#47035 This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 Changelog: [Internal] X-link: facebook/yoga#1726 Reviewed By: joevilches Differential Revision: D64404340 Pulled By: NickGerleman fbshipit-source-id: f6f6e9a6fad82873f18c8a0ead58aad897df5d09

facebook-github-bot added CLA Signed Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 14, 2024

vercel bot deployed to Preview October 14, 2024 12:59 View deployment

NickGerleman reviewed Oct 14, 2024

View reviewed changes

vercel bot deployed to Preview October 15, 2024 12:49 View deployment

NickGerleman approved these changes Oct 15, 2024

View reviewed changes

NickGerleman mentioned this pull request Oct 15, 2024

Add support for display: contents style facebook/react-native#47035

Closed

j-piasecki mentioned this pull request Oct 16, 2024

Add support for display: contents facebook/react-native#46584

Closed

NickGerleman reviewed Oct 18, 2024

View reviewed changes

j-piasecki added 10 commits October 18, 2024 17:08

Add Display::Contents enum

1add6f3

Add display: contents support to gentest

32b651e

Add a basic test

aa88e2c

display: contents implementation

1e489d5

Implement an iterator for layoutable children

39a7cf1

Move cloning to cleanupContentsNodesRecursively

73d2355

Use the iterator for traversing children

20a20b0

Add tests

a636725

Use SIZE_MAX instead of -1

22e878c

Add iterator assert

e4b0650

j-piasecki added 4 commits October 18, 2024 17:08

Rename field

f17d03f

Move iterator to a separate file

26e3ad8

Remove additional assert

c36b197

Add a default constructor

8c57ed3

j-piasecki force-pushed the @jpiasecki/display-contents branch from 93cca87 to 8c57ed3 Compare October 18, 2024 15:09

vercel bot deployed to Preview October 18, 2024 15:12 View deployment

Update LayoutableChildren

a16ebd1

vercel bot deployed to Preview October 18, 2024 15:30 View deployment

facebook-github-bot closed this in 68bb234 Oct 19, 2024

facebook-github-bot added the Merged label Oct 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `display: contents` style #1726

Add support for `display: contents` style #1726

j-piasecki commented Oct 14, 2024

vercel bot commented Oct 14, 2024 •

edited

Loading

NickGerleman left a comment

NickGerleman Oct 14, 2024

j-piasecki Oct 15, 2024

NickGerleman Oct 14, 2024

j-piasecki Oct 15, 2024

NickGerleman Oct 14, 2024

j-piasecki Oct 15, 2024

NickGerleman left a comment

facebook-github-bot commented Oct 15, 2024

NickGerleman Oct 18, 2024 •

edited

Loading

NickGerleman Oct 18, 2024

NickGerleman commented Oct 18, 2024 •

edited

Loading

j-piasecki commented Oct 18, 2024

facebook-github-bot commented Oct 19, 2024

Add support for display: contents style #1726

Add support for display: contents style #1726

Conversation

j-piasecki commented Oct 14, 2024

Summary

Test plan

vercel bot commented Oct 14, 2024 • edited Loading

NickGerleman left a comment

Choose a reason for hiding this comment

NickGerleman Oct 14, 2024

Choose a reason for hiding this comment

j-piasecki Oct 15, 2024

Choose a reason for hiding this comment

NickGerleman Oct 14, 2024

Choose a reason for hiding this comment

j-piasecki Oct 15, 2024

Choose a reason for hiding this comment

NickGerleman Oct 14, 2024

Choose a reason for hiding this comment

j-piasecki Oct 15, 2024

Choose a reason for hiding this comment

NickGerleman left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 15, 2024

NickGerleman Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

NickGerleman Oct 18, 2024

Choose a reason for hiding this comment

NickGerleman commented Oct 18, 2024 • edited Loading

j-piasecki commented Oct 18, 2024

facebook-github-bot commented Oct 19, 2024

Add support for `display: contents` style #1726

Add support for `display: contents` style #1726

vercel bot commented Oct 14, 2024 •

edited

Loading

NickGerleman Oct 18, 2024 •

edited

Loading

NickGerleman commented Oct 18, 2024 •

edited

Loading