DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

WIP: Fix missing invalidation if owner has undefined dimensions #1017

Draft
woehrl01 wants to merge 2 commits from woehrl01/fix-invalidate-owner-size into main
pull from: woehrl01/fix-invalidate-owner-size
merge into: DaddyFrosty:main
Conversation 8 Commits 2 Files Changed 3 +41 -1
woehrl01 commented 2020-06-11 12:26:38 -07:00 (Migrated from github.com)
Copy Link

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.

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.
nickolas-pohilets commented 2020-06-11 15:16:31 -07:00 (Migrated from github.com)
Copy Link

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

Would similar issue also happen when owner size changes from one defined size to another?
woehrl01 commented 2020-06-12 03:34:04 -07:00 (Migrated from github.com)
Copy Link

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.

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 commented 2022-10-21 21:09:09 -07:00 (Migrated from github.com)
Copy Link

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

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 commented 2022-10-22 00:28:37 -07:00 (Migrated from github.com)
Copy Link

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

@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!
❤️ 2
woehrl01 commented 2022-10-23 11:38:32 -07:00 (Migrated from github.com)
Copy Link

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

@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`]( https://github.com/facebook/yoga/blob/5dd33acc912711c599250e3533b07c732de503ee/yoga/YGLayout.h#L42) 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 commented 2022-10-28 01:11:39 -07:00 (Migrated from github.com)
Copy Link

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

@woehrl01 does this bug also happen if `padding` or `borderWidth` were set to undefined - or is it _just_ width and height?
woehrl01 commented 2022-10-28 04:04:48 -07:00 (Migrated from github.com)
Copy Link

@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 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 commented 2022-10-28 04:48:06 -07:00 (Migrated from github.com)
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

No - I was just wondering. It just seemed strange to me that only width/height would trigger this, but not other layout-affecting properties
This pull request has changes conflicting with the target branch.
  • tests/YGRelayoutTest.cpp
  • yoga/YGLayout.h
  • yoga/Yoga.cpp
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin woehrl01/fix-invalidate-owner-size:woehrl01/fix-invalidate-owner-size
git checkout woehrl01/fix-invalidate-owner-size
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
Algorithm
CLA Signed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1017
No description provided.
Delete Branch "woehrl01/fix-invalidate-owner-size"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?