Repository navigation
Keep cross axis margins and relative insets of items in wrapping containers - #2045
Om-singhaI wants to merge 1 commit into
Conversation
…ainers The multi line alignment pass (STEP 8) dropped the relative inset for `stretch`, `center` and `flex-end`, and for `flex-start` it used the inset in place of the leading margin. `center` also ignored margins, so an item with a one sided cross margin was centered on its border box. Baseline layouts run the same pass without wrapping, so they were affected too. Compute the leading margin, trailing margin and relative inset once per item and use them in every alignment, including baseline, which now honors `bottom` and skips insets on static items like the single line path does. Percentage insets on `flex-start` items now resolve against the container's cross size, as they do without wrapping. `flex-wrap: wrap-reverse` mirrors these positions afterwards, so there the flex end margin is the leading one, which keeps each margin on the edge it was set on. The inset is taken out before mirroring and added back afterwards so it keeps its physical direction. Fixes react#1856 Fixes react#1067
facebook-github-tools
Bot
added
the
Shared with Meta
|
@javache has imported this pull request. If you are a Meta employee, you can view this in D127132637. |
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
Thanks for digging into this, and for the clear before/after table. The fix looks right to me. I built it locally, the full suite passes, and I compared a few wrap layouts against main.
My concern isn't the change itself but what it does to existing layouts. I left a couple of questions inline.
| case Align::End: | ||
| // Not yet implemented | ||
| break; | ||
| case Align::FlexStart: { |
There was a problem hiding this comment.
This is the right fix, but it moves items that apps have been laying out with since 3.0. I compared main and this PR with a small Yoga test: a row with flexWrap: 'wrap', alignItems: 'flex-start' and children with margin: 4, which is a common setup in RN. On main the children on the first line sit at top 0; with this PR they move to 4, and the second line moves from 38 to 42. center with a one-sided margin and wrap-reverse with stretch move too.
React Native runs Yoga with Errata::All by default. Would it make sense to put the old behavior behind a new Errata flag, as you offered in the summary? That way RN keeps its current layout under the classic conformance, and strict gets the spec behavior.
|
|
||
| // The second line spans top 40 to 100. The item keeps its margin and moves | ||
| // 10 down for its top inset under every alignment. | ||
| TEST(YogaTest, wrap_row_keeps_cross_axis_margin_and_inset) { |
There was a problem hiding this comment.
The expected values here are worked out by hand from the spec. Could we add these cases as gentest fixtures instead, for example in YGFlexWrapTest.html? Then the expected values come from Chrome, and we get the Java and JS tests for free.
| isWrapReverse ? flexEndMargin : flexStartMargin; | ||
| const float trailingCrossMargin = | ||
| isWrapReverse ? flexStartMargin : flexEndMargin; | ||
| const float relativePositionCross = child->relativePosition( |
There was a problem hiding this comment.
The summary says every case now uses the leading and trailing margins. What about margin: auto on the cross axis? In a row with flexWrap: 'wrap', an item with marginTop: 'auto' in a 50pt line lands at top 0, but at 40 without wrap, because this pass overwrites what STEP 7 computed. It's the same on main, so it's not a regression from this PR. Could you open an issue for it, or handle it here if it's small?
Fixes #1856
Fixes #1067
In wrapping containers, STEP 8 (multi line alignment) of
CalculateLayout.cpprepositions items on the cross axis and drops part of each box. In #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 #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 #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 #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.