Add support for `display: contents` #46584

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

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 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

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

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

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

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).

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

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

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

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

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

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

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 · 2024-09-24T09:25:37Z

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.

That was indeed a local issue.

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.

That was caused by a few checks still using yogaStyle().display() instead of the aliased field. Should be fixed by 41a224f

j-piasecki · 2024-09-30T09:16:48Z

Hey @NickGerleman, has there been any progress on this by chance?

NickGerleman

All of the bits building on the Yoga change look good to me! I will pull a couple bits of this into that change, because we will get exhaustive switch/case errors without (and some toolchain doesn't classify the iterator as an input_iterator? Need to check what's going on for that one...).

Should be able to merge the Yoga change soon, now that facebook/yoga@5687182 landed.

facebook-github-bot · 2024-10-18T06:14:36Z

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

NickGerleman · 2024-10-18T07:00:06Z

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

+  if (yogaNode_.style().display() == yoga::Display::Contents) {
+    ShadowNode::traits_.set(ShadowNodeTraits::ForceFlattenView);
+  } else {
+    ShadowNode::traits_.unset(ShadowNodeTraits::ForceFlattenView);
+  }


Could we fold this into updateYogaProps instead to make lifecycle clearer?

Done in cf58940

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

NickGerleman · 2024-10-18T09:11:23Z

Would you mind rebasing on #47035 again? That change now has:

The checked in version of the Yoga absolute positoning change
Your Yoga change, with a minor tweak to fix build on older stdlibc++
A couple of the changes from this diff, needed to be checked in at the same time as the Yoga change internally, so that everything gels.

react-native-bot · 2024-10-18T15:24:49Z

	Fails
🚫	Danger failed to run `dangerfile.js`.

Error ReferenceError

validateChangelog is not defined
ReferenceError: validateChangelog is not defined
    at Object.<anonymous> (dangerfile.js:48:18)
    at Module._compile (node:internal/modules/cjs/loader:1364:14)
    at requireFromString (/home/runner/work/react-native/react-native/node_modules/require-from-string/index.js:28:4)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:161:68
    at step (/home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:52:23)
    at Object.next (/home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:33:53)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:27:71
    at new Promise (<anonymous>)
    at __awaiter (/home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:23:12)
    at runDangerfileEnvironment (/home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:118:132)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:181:38
    at step (/home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:44:23)
    at Object.next (/home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:25:53)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:19:71
    at new Promise (<anonymous>)
    at __awaiter (/home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:15:12)
    at Object.executeRuntimeEnvironment (/home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:144:88)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/commands/danger-runner.js:101:47
    at step (/home/runner/work/react-native/react-native/node_modules/danger/distribution/commands/danger-runner.js:34:23)
    at Object.next (/home/runner/work/react-native/react-native/node_modules/danger/distribution/commands/danger-runner.js:15:53)
    at fulfilled (/home/runner/work/react-native/react-native/node_modules/danger/distribution/commands/danger-runner.js:6:58)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Dangerfile

43|     'This will require a manual import by a Facebook employee.';
44|   warn(`${title} - <i>${idea}</i>`);
45| }
46| 
47| // Provides advice if a test plan is missing.
--------------------^
48| const includesTestPlan =
49|   danger.github.pr.body &&
50|   danger.github.pr.body.toLowerCase().includes('## test plan');
51| if (!includesTestPlan && !isFromPhabricator) {

Generated by 🚫 dangerJS against cf58940

facebook-github-bot · 2024-10-18T18:53:59Z

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

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

@NickGerleman merged this pull request in e7a3f47.

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

j-piasecki commented Sep 20, 2024

View reviewed changes

...onents/textinput/platform/ios/react/renderer/components/iostextinput/TextInputShadowNode.cpp Outdated Show resolved Hide resolved

j-piasecki commented Sep 20, 2024

View reviewed changes

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp Outdated Show resolved Hide resolved

j-piasecki commented Sep 20, 2024

View reviewed changes

packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp Outdated Show resolved Hide resolved

NickGerleman reviewed Sep 20, 2024

View reviewed changes

j-piasecki force-pushed the @@jpiasecki/display-contents-poc branch from 1bdc2a6 to 13de3ae Compare October 16, 2024 10:50

j-piasecki mentioned this pull request Oct 16, 2024

Add support for display: contents style #47035

Closed

j-piasecki marked this pull request as ready for review October 16, 2024 10:57

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

NickGerleman approved these changes Oct 18, 2024

View reviewed changes

NickGerleman reviewed Oct 18, 2024

View reviewed changes

j-piasecki added 5 commits October 18, 2024 17:19

Support Contents enum

3fa8e2d

Add trait that forces host views to be flattened

a6a4e5a

Set the new flag on nodes with display: contents

84abfb1

Update type

0d193a3

Move trait setting to updateYogaProps

cf58940

j-piasecki force-pushed the @@jpiasecki/display-contents-poc branch from 13de3ae to cf58940 Compare October 18, 2024 15:24

facebook-github-bot closed this in e7a3f47 Oct 19, 2024

facebook-github-bot added the Merged This PR has been merged. label Oct 19, 2024

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 •

edited

Loading

NickGerleman left a comment

j-piasecki commented Sep 23, 2024

j-piasecki commented Sep 23, 2024 •

edited

Loading

j-piasecki commented Sep 24, 2024

j-piasecki commented Sep 30, 2024

NickGerleman left a comment

facebook-github-bot commented Oct 18, 2024

NickGerleman Oct 18, 2024

j-piasecki Oct 18, 2024

NickGerleman commented Oct 18, 2024 •

edited

Loading

react-native-bot commented Oct 18, 2024

facebook-github-bot commented Oct 18, 2024

facebook-github-bot commented Oct 19, 2024

Add support for display: contents #46584

Add support for display: contents #46584

Conversation

j-piasecki commented Sep 20, 2024 • edited Loading

Summary

Changelog:

Test Plan:

NickGerleman left a comment

Choose a reason for hiding this comment

j-piasecki commented Sep 23, 2024

j-piasecki commented Sep 23, 2024 • edited Loading

j-piasecki commented Sep 24, 2024

j-piasecki commented Sep 30, 2024

NickGerleman left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 18, 2024

NickGerleman Oct 18, 2024

Choose a reason for hiding this comment

j-piasecki Oct 18, 2024

Choose a reason for hiding this comment

NickGerleman commented Oct 18, 2024 • edited Loading

react-native-bot commented Oct 18, 2024

Error ReferenceError

Dangerfile

facebook-github-bot commented Oct 18, 2024

facebook-github-bot commented Oct 19, 2024

Add support for `display: contents` #46584

Add support for `display: contents` #46584

j-piasecki commented Sep 20, 2024 •

edited

Loading

j-piasecki commented Sep 23, 2024 •

edited

Loading

NickGerleman commented Oct 18, 2024 •

edited

Loading