Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing invalidation if owner has undefined dimensions #1017

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

woehrl01
Copy link
Contributor

@woehrl01 woehrl01 commented Jun 11, 2020

Fixes #1003 by adding additional values, which keep track if the owner size has been undefined before. Being undefined lead to different calculations for min/max constraints thus, in that specific case leading to the wrong behavior.

Please also see my concerns posted in the issue.

@woehrl01 woehrl01 changed the title fix missing invalidation if owner has undefinied dims Fix missing invalidation if owner has undefined dimensions Jun 11, 2020
@nickolas-pohilets
Copy link

Would similar issue also happen when owner size changes from one defined size to another?

@woehrl01 woehrl01 marked this pull request as draft June 12, 2020 10:31
@woehrl01
Copy link
Contributor Author

It should also work in that case. As the missing parent size leads to not taking min/max values into account. I'll have a second look later, maybe we have a different fix, without the new variables here, too. Until then I'll keep this PR as draft. The alternative as described in the issue is still the best approach for now.

@NickGerleman
Copy link
Contributor

NickGerleman commented Oct 22, 2022

Hey @woehrl01, I think we want to import this, and think it might supersede the current code for YGUseExperimentalWebFlexBasis. @rozele root-caused a long-running issue in an app we care about to this issue #1161.

We're more generally looking to start migrating the React Native ecosystem off of legacy incorrect behaviors, but I think we should put this behind a flag. That way we can experiment, to know if it will be a breaking change we should leave behind a gate, or if its relatively safe to enable out of the box.

Size impact per-node can be mitigated by embedding into the flags bitset I think. We may need to add another byte to it soon. There is also a flag I have been meaning to remove from there (for super expensive whole-tree layout diffing).

I can pick this up, but wanted to give a heads up first 🙂.

@woehrl01
Copy link
Contributor Author

@NickGerleman it's awesome to see the latest effort you put into the library.

I fully agree on your approach. Feel free to pick this up!

@woehrl01
Copy link
Contributor Author

woehrl01 commented Oct 23, 2022

@NickGerleman I just had a closer look into the current layout of the YGLayout struct. I think you can put those two additional flags also into the following position by replacing lastOwnerDirection with a flag, similar to directionOffset + didUseLegacyFlagOffset. This would be even nicer as you have one field for flags and the other field for storing latest layout conditions. This won't change the per node size.

@jacobp100
Copy link

@woehrl01 does this bug also happen if padding or borderWidth were set to undefined - or is it just width and height?

@woehrl01
Copy link
Contributor Author

@jacobp100 Are you talking about padding or borderWidth on the owner? This fix is mainly if the root owner changes, where you usually can't set those properties at all. If you modify padding on a child item, the dirty flag is set and gets propagated. Do you have a specific issue/test case?

@jacobp100
Copy link

No - I was just wondering. It just seemed strange to me that only width/height would trigger this, but not other layout-affecting properties

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants