Repository navigation
Conversation
…ainers Summary: Fixes react/yoga#1856 Fixes react/yoga#1067 In wrapping containers, STEP 8 (multi line alignment) of `CalculateLayout.cpp` repositions items on the cross axis and drops part of each box. In react/yoga#1856 the container is a column (Yoga's default), so `left` is on the cross axis. - `flex-start` uses `computeFlexStartPosition` (the inset) where `computeFlexStartMargin` belongs. v2.0.1 used the margin, so this is a 3.0 regression from react/yoga#1561. - `stretch`, `center` and `flex-end` never add the relative inset. - `center` centers the border box instead of the margin box, which is react/yoga#1067. Every case now uses the leading margin, trailing margin and `relativePosition`. `wrap-reverse` mirrors positions afterwards, so it leads with the flex end margin and applies the inset after mirroring, keeping each margin on its edge and `top: 10` moving the item down. Also, a percentage inset on a `flex-start` item now resolves against the container's cross size, as without wrap (`top: 10%` in a 100x200 row: 10 to 20), and baseline rows, which run this pass even without wrap, honor `bottom` and ignore insets on static items. Column container, padding 10, `flex-wrap: wrap`, default `align-content: flex-start`, item with margin 5 and `left: 10` (item left; expected worked out from the spec): | `align-items` | main | this PR | expected | |---|---|---|---| | `flex-start` | 20 | 25 | 25 | | `stretch` | 15 | 25 | 25 | | `center` | 15 | 25 | 25 | | `flex-end` | 15 | 25 | 25 | | `flex-start`, no inset | 10 | 15 | 15 | The react/yoga#1067 repro goes from top 50 to 100. I added `tests/YGFlexWrapCrossAlignmentTest.cpp`: 9 hand written gtest cases like the other non generated tests, covering row and column, LTR and RTL, `wrap` and `wrap-reverse` (including one sided margins), baseline, static items and both issue repros. All 9 fail without the `CalculateLayout.cpp` change; with it, all 877 tests pass (868 plus 9). I can move them into gentest fixtures or add an `Errata` flag if you prefer. X-link: react/yoga#2045 Differential Revision: D127132637 Pulled By: javache
meta-cla
Bot
added
the
CLA Signed
|
@javache has exported this pull request. If you are a Meta employee, you can view the originating Diff in D127132637. |
Visual regressionNo visual differences at android: no capture |
This branch has not been deployed
This file contains hidden or 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
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:
Fixes react/yoga#1856
Fixes react/yoga#1067
In wrapping containers, STEP 8 (multi line alignment) of
CalculateLayout.cpprepositions items on the cross axis and drops part of each box. In react/yoga#1856 the container is a column (Yoga's default), soleftis on the cross axis.flex-startusescomputeFlexStartPosition(the inset) wherecomputeFlexStartMarginbelongs. v2.0.1 used the margin, so this is a 3.0 regression from Fix mismatched cases of inlineStart/End and flexStart/End yoga#1561.stretch,centerandflex-endnever add the relative inset.centercenters the border box instead of the margin box, which is [React-Native]: Combining flexWrap, alignItems & wrap leads to unwanted alignment yoga#1067.Every case now uses the leading margin, trailing margin and
relativePosition.wrap-reversemirrors positions afterwards, so it leads with the flex end margin and applies the inset after mirroring, keeping each margin on its edge andtop: 10moving the item down.Also, a percentage inset on a
flex-startitem now resolves against the container's cross size, as without wrap (top: 10%in a 100x200 row: 10 to 20), and baseline rows, which run this pass even without wrap, honorbottomand ignore insets on static items.Column container, padding 10,
flex-wrap: wrap, defaultalign-content: flex-start, item with margin 5 andleft: 10(item left; expected worked out from the spec):align-itemsflex-startstretchcenterflex-endflex-start, no insetThe react/yoga#1067 repro goes from top 50 to 100.
I added
tests/YGFlexWrapCrossAlignmentTest.cpp: 9 hand written gtest cases like the other non generated tests, covering row and column, LTR and RTL,wrapandwrap-reverse(including one sided margins), baseline, static items and both issue repros. All 9 fail without theCalculateLayout.cppchange; with it, all 877 tests pass (868 plus 9). I can move them into gentest fixtures or add anErrataflag if you prefer.X-link: react/yoga#2045
Differential Revision: D127132637
Pulled By: javache