Add errata supporting changes to position: static #1434
Conversation
|
This pull request was exported from Phabricator. Differential Revision: D50506915 |
|
FYI @nicoburns in case Taffy wants to support |
|
Oo, yeah this has been requested (DioxusLabs/taffy#212), and is definitely needed if one wants to render a "default web div". @joevilches I'd be interested in what your implementation strategy for this is. In particular, have you thought about:
Also, I think some |
|
Caching is a good callout. Right now some of the refactoring is to pass containing block when recursing during layout, instead of just parent dimensions. Containing block dimensions might need to be added as cache keys, like owner size is right now. |
Yeah, that would probably work. Actually, I figured that what we call "parent size" in Taffy might be called "owner size" in Yoga precisely because for this use case. I think that the containing block dimensions could probably be used instead of the "parent size" in the cache key (bearing in mind that for non-absolutely positioned nodes the parent is the containing block). Which would be nice, to avoid bloating the cache size. |
|
The history of “owner” is in this commit: f0edefd The history here is that the React Native Fabric renderer keeps a Yoga node per ShadowNode, but Fabric ShadowNodes are immutable after being laid out. https://reactnative.dev/architecture/render-pipeline This clashes with Yoga’s existing public API, which is oriented around nodes being mutated throughout their lifetime. Fabric took a dependency on Yoga internals, to build this different model, with some of it making its way into Yoga itself. I’ve been thinking about how we can move that boundary into something public in the future. Out of probably a couple dozen of Yoga using frameworks in our monorepo, Fabric is the only one using the private APIs. All others are now set up to only have visibility to public headers. |
|
Hmm... a node being able to have more than one parent doesn't sound like a fantastic idea. I'm guessing this is to allow for some kind of copy-on-write semantics on the Fabric side? I'd need to look at the code, but off the top of my head it seems to me that:
Long term, it would probably be good to make Yoga's algorithms generic over the node type they operate over, and make them operate directly on ShadowNodes in Fabric. |
|
Copy-on-write is correct. Fabric treats a node owned by a different parent as one that cannot be mutated, so we lazily create new writable Yoga nodes during layout. Layout information is copied to the ShadowNode, but original Yoga node needs to be cloned when creating new ShadowNode revision. This also clones the nodes layout cache. Then Fabric will dirty on node prop change. Shadow trees are allowed to be created on multiple threads, and multiple revisions can exist, derived from a single ancestor revision. There are a bunch of ideas on how to refactor this eventually. |
|
@nicoburns Yeah what Nick said is the gist of it. Basically just plan to pass down containing block information in the calculate layout functions, haven't planned much else beyond that. Caching was not on my mind initially but I will keep an eye out for it after the discussion here. I plan on getting started with these things this week, we shall see how much progress I make :) |
Summary: X-link: facebook/yoga#1434 I will use this errata to gate my changes that actually make position: static behave like the web. We have future plans to make position: relative the default again but users could still have declared certain nodes as position: static, so I think this is needed regardless. Reviewed By: NickGerleman Differential Revision: D50506915
Summary: I am about to embark on supporting `position: static` in Yoga. The enum exists already (and is the default position type, lol) but does not actually do anything and just behaves like `position: relative`. My approach here is to write a bunch of tests to test for the various behaviors of static positions and then develop on Yoga afterwards to get those tests passing. To do this, we need to make a few changes to the gentest files as there is not support for adding `position: static` at the moment: * Make it so that the gentest code can physically write `YGPositionTypeStatic` if it encounters `position: static` in the style * Make it so that gentest.js knows that Yoga's default is actually static. This way the code generated in the tests will actually label nodes for non default values * Explicitly label the position type even when it is not declared in the style prop (with the exception of the default) * Regenerate all the tests Additionally I added the first, basic test: making sure insets do nothing on a statically positioned element. Differential Revision: https://www.internalfb.com/diff/D50437855?entry_point=27 fbshipit-source-id: f4c9516020f9484513a8d58ec2b7a7814b6f93d9
Differential Revision: https://www.internalfb.com/diff/D50475939?entry_point=27 fbshipit-source-id: a1d68d4272a1a4ef17f2fabc2bf71d5fd9c2e4a5
Summary: Pull Request resolved: facebook#1434 X-link: facebook/react-native#41130 I will use this errata to gate my changes that actually make position: static behave like the web. We have future plans to make position: relative the default again but users could still have declared certain nodes as position: static, so I think this is needed regardless. Reviewed By: NickGerleman Differential Revision: D50506915 fbshipit-source-id: 3a72743a30710c64b8eecfe8e08e613f46e62112
|
This pull request was exported from Phabricator. Differential Revision: D50506915 |
joevilches
force-pushed
the
export-D50506915
branch
from
8fc9853
to
39562f6
Compare
Summary: X-link: facebook/yoga#1434 X-link: facebook/react-native#41130 I will use this errata to gate my changes that actually make position: static behave like the web. We have future plans to make position: relative the default again but users could still have declared certain nodes as position: static, so I think this is needed regardless. Reviewed By: NickGerleman Differential Revision: D50506915 fbshipit-source-id: b0d9e6883167de6ff002352c9288053324464cb9
Summary: X-link: facebook/yoga#1434 Pull Request resolved: #41130 I will use this errata to gate my changes that actually make position: static behave like the web. We have future plans to make position: relative the default again but users could still have declared certain nodes as position: static, so I think this is needed regardless. Reviewed By: NickGerleman Differential Revision: D50506915 fbshipit-source-id: b0d9e6883167de6ff002352c9288053324464cb9
|
This pull request has been merged in 52ae53a. |
Summary: X-link: facebook/yoga#1434 Pull Request resolved: facebook#41130 I will use this errata to gate my changes that actually make position: static behave like the web. We have future plans to make position: relative the default again but users could still have declared certain nodes as position: static, so I think this is needed regardless. Reviewed By: NickGerleman Differential Revision: D50506915 fbshipit-source-id: b0d9e6883167de6ff002352c9288053324464cb9
Summary:
X-link: facebook/react-native#41130
I will use this errata to gate my changes that actually make position: static behave like the web. We have future plans to make position: relative the default again but users could still have declared certain nodes as position: static, so I think this is needed regardless.
Differential Revision: D50506915