Clean up random absolute position setting during alignment #1725
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request was exported from Phabricator. Differential Revision: D64244949 |
NickGerleman
added a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 11, 2024
Summary: X-link: facebook/yoga#1725 The legacy (wrong) absolute positioning path positions in two places, including work that is definitely always overwritten in the new absolute layout path. This came up before for position: static, but we didn't clean this up at the time. This code is also now leading display: contents impl being more annoying. Let's move everything in the legacy path to the same place at least, so earlier code can just deal with items in flow (as their steps should be doing), and we can reason about this code not doing anything for the modern (much less strange, and more correct). This behavior Changelog: [Internal] Differential Revision: D64244949
NickGerleman
pushed a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 15, 2024
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
NickGerleman
added a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 15, 2024
…46984) Summary: X-link: facebook/yoga#1725 The legacy (wrong) absolute positioning path positions in two places, including work that is definitely always overwritten in the new absolute layout path. This came up before for position: static, but we didn't clean this up at the time. This code is also now leading display: contents impl being more annoying. Let's move everything in the legacy path to the same place at least, so earlier code can just deal with items in flow (as their steps should be doing), and we can reason about this code not doing anything for the modern (much less strange, and more correct). This behavior Changelog: [Internal] Reviewed By: joevilches Differential Revision: D64244949
NickGerleman
pushed a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 15, 2024
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
added a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 17, 2024
Summary: X-link: facebook/yoga#1725 The legacy (wrong) absolute positioning path positions in two places, including work that is definitely always overwritten in the new absolute layout path. This came up before for position: static, but we didn't clean this up at the time. This code is also now leading display: contents impl being more annoying. This diff tries to converge to the more spec correct implementation of positioning here, that also only happens in one place. Previous path would potentially also incorrectly justify when `justify-content` was non-default, but not handled in the previous few cases? We don't have access to the flexLine at this point later, and apart from the existing tests now passing I reused the new correct logic for justification (spec says we should position child as if its the only child in the container https://www.w3.org/TR/css-flexbox-1/#abspos-items). I did not try removing `AbsolutePercentAgainstInnerSize` which I suspect would be pretty breaking. Changelog: [General][Breaking] - More spec compliant absolute positioning Reviewed By: joevilches Differential Revision: D64244949
NickGerleman
added a commit
to NickGerleman/yoga
that referenced
this pull request
Oct 17, 2024
Summary: X-link: facebook/react-native#46984 The legacy (wrong) absolute positioning path positions in two places, including work that is definitely always overwritten in the new absolute layout path. This came up before for position: static, but we didn't clean this up at the time. This code is also now leading display: contents impl being more annoying. This diff tries to converge to the more spec correct implementation of positioning here, that also only happens in one place. Previous path would potentially also incorrectly justify when `justify-content` was non-default, but not handled in the previous few cases? We don't have access to the flexLine at this point later, and apart from the existing tests now passing I reused the new correct logic for justification (spec says we should position child as if its the only child in the container https://www.w3.org/TR/css-flexbox-1/#abspos-items). I did not try removing `AbsolutePercentAgainstInnerSize` which I suspect would be pretty breaking. Changelog: [General][Breaking] - More spec compliant absolute positioning Reviewed By: joevilches Differential Revision: D64244949
NickGerleman
force-pushed
the
export-D64244949
branch
from
e879203
to
6cd7d33
Compare
|
This pull request was exported from Phabricator. Differential Revision: D64244949 |
NickGerleman
added a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 18, 2024
Summary: X-link: facebook/yoga#1725 The legacy (wrong) absolute positioning path positions in two places, including work that is definitely always overwritten in the new absolute layout path. This came up before for position: static, but we didn't clean this up at the time. This code is also now leading display: contents impl being more annoying. This diff tries to converge to the more spec correct implementation of positioning here, that also only happens in one place. Previous path would potentially also incorrectly justify when `justify-content` was non-default, but not handled in the previous few cases? We don't have access to the flexLine at this point later, and apart from the existing tests now passing I reused the new correct logic for justification (spec says we should position child as if its the only child in the container https://www.w3.org/TR/css-flexbox-1/#abspos-items). I added a new, more scoped errata `AbsolutePositionWithoutInsetsExcludesPadding` to preserve some of the legacy behavior that showed as very breaking. I also did not try removing `AbsolutePercentAgainstInnerSize` which I suspect would be more breaking than this change. Changelog: [General][Breaking] - More spec compliant absolute positioning Reviewed By: joevilches Differential Revision: D64244949
Summary: X-link: facebook/react-native#46984 The legacy (wrong) absolute positioning path positions in two places, including work that is definitely always overwritten in the new absolute layout path. This came up before for position: static, but we didn't clean this up at the time. This code is also now leading display: contents impl being more annoying. This diff tries to converge to the more spec correct implementation of positioning here, that also only happens in one place. Previous path would potentially also incorrectly justify when `justify-content` was non-default, but not handled in the previous few cases? We don't have access to the flexLine at this point later, and apart from the existing tests now passing I reused the new correct logic for justification (spec says we should position child as if its the only child in the container https://www.w3.org/TR/css-flexbox-1/#abspos-items). I added a new, more scoped errata `AbsolutePositionWithoutInsetsExcludesPadding` to preserve some of the legacy behavior that showed as very breaking. I also did not try removing `AbsolutePercentAgainstInnerSize` which I suspect would be more breaking than this change. Changelog: [General][Breaking] - More spec compliant absolute positioning Reviewed By: joevilches Differential Revision: D64244949
NickGerleman
force-pushed
the
export-D64244949
branch
from
6cd7d33
to
c90d19c
Compare
|
This pull request was exported from Phabricator. Differential Revision: D64244949 |
|
This pull request has been merged in 5687182. |
facebook-github-bot
pushed a commit
to facebook/litho
that referenced
this pull request
Oct 18, 2024
Summary: X-link: facebook/react-native#46984 X-link: facebook/yoga#1725 The legacy (wrong) absolute positioning path positions in two places, including work that is definitely always overwritten in the new absolute layout path. This came up before for position: static, but we didn't clean this up at the time. This code is also now leading display: contents impl being more annoying. This diff tries to converge to the more spec correct implementation of positioning here, that also only happens in one place. Previous path would potentially also incorrectly justify when `justify-content` was non-default, but not handled in the previous few cases? We don't have access to the flexLine at this point later, and apart from the existing tests now passing I reused the new correct logic for justification (spec says we should position child as if its the only child in the container https://www.w3.org/TR/css-flexbox-1/#abspos-items). I added a new, more scoped errata `AbsolutePositionWithoutInsetsExcludesPadding` to preserve some of the legacy behavior that showed as very breaking. I also did not try removing `AbsolutePercentAgainstInnerSize` which I suspect would be more breaking than this change. Changelog: [General][Breaking] - More spec compliant absolute positioning Reviewed By: joevilches Differential Revision: D64244949 fbshipit-source-id: ca97570e0de82e8f0424a0912adfd0b05254559e
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Oct 18, 2024
Summary: Pull Request resolved: #46984 X-link: facebook/yoga#1725 The legacy (wrong) absolute positioning path positions in two places, including work that is definitely always overwritten in the new absolute layout path. This came up before for position: static, but we didn't clean this up at the time. This code is also now leading display: contents impl being more annoying. This diff tries to converge to the more spec correct implementation of positioning here, that also only happens in one place. Previous path would potentially also incorrectly justify when `justify-content` was non-default, but not handled in the previous few cases? We don't have access to the flexLine at this point later, and apart from the existing tests now passing I reused the new correct logic for justification (spec says we should position child as if its the only child in the container https://www.w3.org/TR/css-flexbox-1/#abspos-items). I added a new, more scoped errata `AbsolutePositionWithoutInsetsExcludesPadding` to preserve some of the legacy behavior that showed as very breaking. I also did not try removing `AbsolutePercentAgainstInnerSize` which I suspect would be more breaking than this change. Changelog: [General][Breaking] - More spec compliant absolute positioning Reviewed By: joevilches Differential Revision: D64244949 fbshipit-source-id: ca97570e0de82e8f0424a0912adfd0b05254559e
NickGerleman
pushed a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 18, 2024
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
pushed a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 18, 2024
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
pushed a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 18, 2024
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
pushed a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 18, 2024
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
pushed a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 18, 2024
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
pushed a commit
to NickGerleman/react-native
that referenced
this pull request
Oct 18, 2024
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
pushed a commit
that referenced
this pull request
Oct 19, 2024
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:#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 #1725 Changelog: [Internal] Pull Request resolved: #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
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Oct 19, 2024
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
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Oct 19, 2024
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
facebook-github-bot
pushed a commit
to facebook/litho
that referenced
this pull request
Oct 19, 2024
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
The legacy (wrong) absolute positioning path positions in two places, including work that is definitely always overwritten in the new absolute layout path.
This came up before for position: static, but we didn't clean this up at the time. This code is also now leading display: contents impl being more annoying.
Let's move everything in the legacy path to the same place at least, so earlier code can just deal with items in flow (as their steps should be doing), and we can reason about this code not doing anything for the modern (much less strange, and more correct).
This behavior
Changelog: [Internal]
Differential Revision: D64244949