Add support for `display: contents` #46584

j-piasecki · 2024-09-20T10:20:59Z

Summary:

This PR adds support for display: contents property. The approach it takes to implement is to make all YogaLayoutableShadowNodes with the property set to be skipped when building the Yoga tree, like so (green node is one with display: contents):

  graph TD
  A((A))
  B((B))
  C((C))
  D((D))
  E((E))

  A'((A'))
  C'((C'))
  D'((D'))
  E'((E'))

subgraph Shadow Tree
  A --> B
  A --> E
  B --> C
  B --> D
end

subgraph Yoga Tree
  A' --> C'
  A' --> D'
  A' --> E'
end

  style B fill:#050

To accomplish this, the way YogaLayoutableShadowNode stores its layoutable children. Before this PR it had a list of all direct children that were also YogaLayoutableShadowNode but this PR breaks the assumption that there is a 1:1 relation between Yoga tree and YogaLayoutableShadowNodes. To take this into account, I've changed the list of direct children to a list of pairs - one of the nodes in the pair is the direct child of the node and the other is a node that's a direct child of the owners YogaNode. The two pointers differ only if the child YogaLayoutableShadowNode has display: contents set, otherwise they point to the same node. In the above example the list for the node A would look like this: [(B, C), (B, D), (E, E)], where the first node represents a direct child of the shadow node, and the second node represents the owner of the direct child of yoga node. Due to this change, the logic of all methods referencing the list had to be updated to reflect this change.

ViewShadowNode and ConcreteViewShadowNode were updated to always be flattened when they have display: contents set. TextInputShadowNode was updated to set Hidden trait when having display: contents set as according to the spec it should be treated as display: none.

In most places in YogaLayoutableShadowNodes the logic itself didn't change - instead of operating directly on the node runForEveryConcreteSubtree is used and instead of appending the node directly to yogaLayoutableChildren_ a pair is added.

Unless the node has display: contents set runForEveryConcreteSubtree will be executed only for that node, otherwise it will recursively traverse the subtree and run the provided lambda for every non-contents subtree found:

  graph TD
  A((A))
  B((B))
  C((C))
  D((D))
  E((E))

  A --> B
  A --> E
  B --> C
  B --> D


  style A fill:#050
  style B fill:#050

If the method were to be called with the node A in the above tree, the lambda would be executed for nodes C, D, and E.

Significant changes were made to the replaceChild method. Previously it was always called with a direct child of the node, so the base method could have been used. Now it may be called with a descendant that's not a direct child. To accomodate that, the logic is split in two branches:

If the node to replace is a direct child, the logic is unchanged
If the node isn't a direct child, cloneTree method is used on an ancestor that's a direct child of the node which child is being replaced. cloneTree is responsible to replace the node in the subtree, which then is being inserted into the node using the base replaceChild method. In the above graph, if replaceChild were called on node A with node C' node B would be cloned in a way that would replace C with C' resulting in subtree B' being created. Then B would be replaced with B' using base replaceChild method.

After the tree structure is updated, it may be necessary to also update yogaLayoutableChildren_ as the subtree structure may have been changed. Previously it had two branches:

If the new child is not YogaLayoutableShadowNode, erase it
If it is YogaLayoutableShadowNode, replace it in place in the list

After the change, replacing a child may result in multiple nodes in yogaLayoutableChildren_ being changed. The split into two branches remains:

If the new child is not YogaLayoutableShadowNode, erase every pair that contained the old node from the list
If it is, iterate over all subtrees that are layout-meaningful (i.e. non display: contents) and replace all the nodes in place. If there are more nodes than before, they will be inserted in order so that entries sharing the direct child pointer are clustered together. If there are less nodes than before, the excess nodes will be erased from the list.

Similarly, there were some changes necessary in cloneChildInPlace method - previously it was cloning a node at specified index and replacing it. Now, since yogaLayoutableChildren_ don't have to be direct children of the node it needs to use cloneTree method to clone entire subtree that contains the relevant node.

Notes:

I'm not exactly sure what's the best way to fully separate Display::Contents from Yoga since the two places where display property is stored are the yoga node and props which hold yoga styles.
I thought that adding a feature flag for this feature after the initial review may be a good idea to improve clarity, let me know if you don't agree and I will implement the feature flag first.

Changelog:

[GENERAL] [ADDED] - Added support for display: contents

Test Plan:

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.

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>
  );
}

j-piasecki · 2024-09-20T10:45:06Z

...onents/textinput/platform/ios/react/renderer/components/iostextinput/TextInputShadowNode.cpp

+void TextInputShadowNode::initialize() {
+  if (yogaNode_.style().display() == yoga::Display::Contents) {
+    traits_.set(ShadowNodeTraits::Trait::Hidden);
+  } else {
+    traits_.unset(ShadowNodeTraits::Trait::Hidden);
+  }
+}


The same approach wasn't working on Android, I assume that it may be related to this

react-native/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp

Lines 217 to 223 in 151008f

#ifndef ANDROID

// T153547836: Disabled on Android because the mounting infrastructure

// is not fully ready yet.

if (childShadowNode.getTraits().check(ShadowNodeTraits::Trait::Hidden)) {

continue;

}

#endif

j-piasecki · 2024-09-20T10:46:40Z

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

+    yogaNode_.setChildren({});
+  }
+
+  if (fragment.children || (!isDisplayContents && wasDisplayContents)) {


If the node had display: contents but doesn't anymore, its children need to be updated since the list wasn't initialized and they were mounted higher up the tree.

j-piasecki · 2024-09-20T10:48:54Z

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

+    // TODO: Why does this not use `ShadowNodeFragment::statePlaceholder()` like
+    // `adoptYogaChild()`?
+    clonedChildNode = childNode.clone(
+        {ShadowNodeFragment::propsPlaceholder(),
+          ShadowNodeFragment::childrenPlaceholder(),
+          childNode.getState()});


Not really related, but I think the answer is related to #44796. TL;DR cloning a node with a state placeholder will replace the state in the node with the most recently committed one.

NickGerleman

Thank you for all of the perseverance here 😅. Still trying to wrap my head around all the changes here.

The bookkeeping here around mutation and adding/removing the prop seems to add more a complexity than I had hoped. Do you think this would be any simpler if we went the route of properly teaching Yoga about display: contents? RN would still need to have knowledge of it, but we get to avoid some of the tricky scenarios here (but need to teach Yoga during layout traversal).

NickGerleman · 2024-09-20T22:01:17Z

packages/react-native/ReactCommon/yoga/yoga/YGEnums.h

@@ -38,7 +38,8 @@ YG_ENUM_DECL(
 YG_ENUM_DECL(
    YGDisplay,
    YGDisplayFlex,
-    YGDisplayNone)
+    YGDisplayNone,
+    YGDisplayContents)


I'm not exactly sure what's the best way to fully separate Display::Contents from Yoga since the two places where display property is stored are the yoga node and props which hold yoga styles

The Props structure holding a yoga::Style is something I plan to change in the future more broadly. These should ideally not be coupled together.

We have some extra props on YogaStylableProps not part of yoga::Style that we flatten when transferring props to Yoga node. Maybe we could back display prop by field, and ignore the one on the Yoga style for now during props part?

Is this close to what you had in mind?

NickGerleman · 2024-09-20T22:04:45Z

...t/platform/android/react/renderer/components/androidtextinput/AndroidTextInputShadowNode.cpp

+  if (yogaNode_.style().display() == yoga::Display::Contents) {
+    yogaNode_.style().setDisplay(yoga::Display::None);
+  }


Could you link to spec about why we do this? Though, I'm not sure if it is safe to mutate Yoga node at this point. Maybe makes sense to have some sort of trait to control this which is read when we are transferring props to Yoga node.

https://drafts.csswg.org/css-display/#unbox-html

I assumed that TextInput would correspond to input.

NickGerleman · 2024-09-20T22:10:19Z

packages/react-native/ReactCommon/react/renderer/components/view/ViewShadowNode.cpp

+    if (viewProps.yogaStyle.display() == yoga::Display::Contents) {
+        formsStackingContext = false;
+        formsView = false;
+    }


I think ChildrenFormStackingContext could still cause the view to be materialized here. Also think this logic may not run for non-view-derived but still container components. Think we might need to expose this to Differentiator in different way.

I think you're right. I've moved the logic to Differentiator and added a new trait that allows a view to be force-flattened in 624969b. What do you think about this approach?

NickGerleman · 2024-09-20T22:13:41Z

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

@@ -149,12 +139,33 @@ YogaLayoutableShadowNode::YogaLayoutableShadowNode(
        static_cast<const YogaLayoutableShadowNode&>(sourceShadowNode)
            .yogaTreeHasBeenConfigured_;
  }
+
+  bool wasDisplayContents = hasDisplayContentsStyle();


Can we derive this from the source shadow node, instead of the temp state of the Yoga node?

It's obvious now that you've pointed it out, done in bc1984f

NickGerleman · 2024-09-20T22:14:55Z

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

+  if (!getTraits().check(ShadowNodeTraits::Trait::LeafYogaNode) && !isDisplayContents) {
+    for (auto& child : getChildren()) {
+      if (auto layoutableChild = std::dynamic_pointer_cast<const YogaLayoutableShadowNode>(child)) {
+        runForEveryConcreteSubtree(layoutableChild, [&](const YogaLayoutableShadowNode::Shared subtreeRoot) {


Suggested change

runForEveryConcreteSubtree(layoutableChild, [&](const YogaLayoutableShadowNode::Shared subtreeRoot) {

runForEveryConcreteSubtree(layoutableChild, [&](const YogaLayoutableShadowNode::Shared& subtreeRoot) {

Done in bc1984f

NickGerleman · 2024-09-20T22:16:04Z

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

+  bool isDisplayContents = hasDisplayContentsStyle();
+
+  if (!getTraits().check(ShadowNodeTraits::Trait::LeafYogaNode) && !isDisplayContents) {
+    for (auto& child : getChildren()) {
+      if (auto layoutableChild = std::dynamic_pointer_cast<const YogaLayoutableShadowNode>(child)) {
+        runForEveryConcreteSubtree(layoutableChild, [&](const YogaLayoutableShadowNode::Shared subtreeRoot) {
+          yogaLayoutableChildren_.push_back({
+            .directChild = layoutableChild,
+            .yogaChild = subtreeRoot,
+          });
+        });
+      }
+    }
+  }
+
+  if (isDisplayContents) {
+    yogaNode_.setChildren({});
+  }


Could we fold this logic into updateYogaChildren? Perhaps passing previous shadow node, if we need to know before/after?

We can but we would need to call it every time instead of when a fragment contains new children. Without either the current logic or updating updateYogaChildren the list would be empty for clones of the node.

After looking at it closer though I realized that I could remove iterating over subrees from there as it always happens before the method is called (c14b801)

NickGerleman · 2024-09-20T22:17:15Z

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

+  if (hasDisplayContentsStyle()) {
+    return;
+  }


It feels like a logic error if we try to adopt here. Could we assert instead?

Also a nit, we should add this after the preconditions below

Done in bc1984f and dd283e0.

I think early return was a leftover from when not every cloning path was updated to consider display: contents

NickGerleman · 2024-09-20T22:21:45Z

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.h

+  /*
+   * Describes a shadow node which is a direct child in the Yoga tree,
+   * not necessarily in the shadow tree. In most cases directChild == yogaChild,
+   * the exception being when directChild has its display set to `contents`.
+   * In that case yogaChild points to a ShadowNode whose yogaNode is a direct
+   * child of this one's.
+   */
+  struct ChildPair {
+    Shared directChild;
+    Shared yogaChild;
+  };


Could you remind me why we need to store this state, vs deriving it during iteration?

This way there's no need to check whether a child has display: contents set on it before doing anything on an item from the list. Otherwise, it would need to be checked first whether the node from the list is a direct child or not. In some cases, where access to the direct child is required (when replacing children), the direct child would need to be found.

Having access to both allows to just directly refer to the node that's needed at the time.

NickGerleman · 2024-09-20T22:23:41Z

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

 void YogaLayoutableShadowNode::layout(LayoutContext layoutContext) {
  // Reading data from a dirtied node does not make sense.
  react_native_assert(!yogaNode_.isDirty());

-  for (auto childYogaNode : yogaNode_.getChildren()) {
+  for (auto child : yogaLayoutableChildren_) {


nit: don't copy shared_ptr by value for each iteration here (previous version was copying raw ptr)

Done in bc1984f

NickGerleman · 2024-09-20T22:27:39Z

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

+  for (const auto &child : getChildren()) {
+    if (const auto& layoutableChild = std::dynamic_pointer_cast<const YogaLayoutableShadowNode>(child)) {
+      if (layoutableChild->hasDisplayContentsStyle()) {
+        auto& contentsNode = shadowNodeFromContext(&layoutableChild->yogaNode_);
+        contentsNode.setContentsLayoutMetrics(layoutContext);
+      }
+    }
+  }


Why do we need to do this if we still iterate through children in layout()?

Layout info is only propagated for nodes that are present in the Yoga tree. Since nodes with display: contents are not present there, they were effectively skipped.

j-piasecki · 2024-09-23T11:50:11Z

Do you think this would be any simpler if we went the route of properly teaching Yoga about display: contents? RN would still need to have knowledge of it, but we get to avoid some of the tricky scenarios here (but need to teach Yoga during layout traversal).

I only have a high-level idea of how Yoga works but I imagine the approach would be relatively similar - instead of removing the nodes from the Yoga tree like in this PR, we would need to skip them during layout. Though, it's hard to say where it fits on the easier said than done scale. I think that the main thing it would simplify is the cloning logic, as it would stay relatively (or even entirely) unchanged.

j-piasecki · 2024-09-23T15:41:06Z

Seems like the approach with setting display to none on TextInput doesn't work after the changes I've made related to the new trait.

I've also started seeing crashes immediately on startup on Android but this may be a local issue as it doesn't matter which branch I'm running on.

j-piasecki added 24 commits September 20, 2024 09:05

Initial implementation

bbb8bc2

Fix nested contents

d0f00d2

Restore updating

7885ce4

It crashes less often

afc7cba

It crashes even less

b3bf79c

Even less crashes

f8b1d3b

It's still broken

c0d1128

example

e1589ee

It works?

0a5e790

I think it works

7f84b76

It should be better

b958baf

Remove unused var

a8f7cef

Formatting

668fabd

Don't build a children list many times

c99cba3

Update Podfile.lock

e0084fc

Update type

ddf1125

Renames

0ff4c6d

Extract variable

cfb4a4a

Move check to helper method

25d5827

Update setting traits

0bf9de7

Use the same cocoapods version, restore app

1a660f4

Treat contents as none on Android

e55e4b1

Indent

a224757

Fix typo

fd4be74

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Software Mansion Partner: Software Mansion Partner labels Sep 20, 2024

Modify yoga children in place

dfde5af

j-piasecki commented Sep 20, 2024

View reviewed changes

NickGerleman reviewed Sep 20, 2024

View reviewed changes

j-piasecki added 5 commits September 23, 2024 10:00

Apply changes from review

bc1984f

Add a new trait that forces views to be flattened

624969b

Simplify updateYogaChildren

c14b801

Replace check with assert

dd283e0

Decouple display: contents from yoga

e9f4140

Fix Android build

287b912

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `display: contents` #46584

Add support for `display: contents` #46584

j-piasecki commented Sep 20, 2024

j-piasecki Sep 20, 2024

j-piasecki Sep 20, 2024

j-piasecki Sep 20, 2024

NickGerleman left a comment

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

NickGerleman Sep 20, 2024

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

NickGerleman Sep 20, 2024

j-piasecki Sep 23, 2024

j-piasecki commented Sep 23, 2024

j-piasecki commented Sep 23, 2024

	#ifndef ANDROID
	// T153547836: Disabled on Android because the mounting infrastructure
	// is not fully ready yet.
	if (childShadowNode.getTraits().check(ShadowNodeTraits::Trait::Hidden)) {
	continue;
	}
	#endif

	runForEveryConcreteSubtree(layoutableChild, [&](const YogaLayoutableShadowNode::Shared subtreeRoot) {
	runForEveryConcreteSubtree(layoutableChild, [&](const YogaLayoutableShadowNode::Shared& subtreeRoot) {

Add support for display: contents #46584

Are you sure you want to change the base?

Add support for display: contents #46584

Conversation

j-piasecki commented Sep 20, 2024

Summary:

Changelog:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickGerleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-piasecki commented Sep 23, 2024

j-piasecki commented Sep 23, 2024

Add support for `display: contents` #46584

Add support for `display: contents` #46584