[Om-singhaI]

Related: #1856, #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 #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 Fix mismatched cases of inlineStart/End and flexStart/End #1561.
• stretch, center and flex-end never add the relative inset.
• center centers 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-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.

New configs keep the previous positions through the erratum below, so this applies once the erratum is cleared.

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, erratum cleared	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 #1067 repro goes from top 50 to 100.

Errata

These positions have been in place for a long time, so the previous behavior is kept behind a new erratum, YGErrataWrapCrossAlignmentIgnoresMarginAndInset (1 << 5). When it is set, STEP 8 and the wrap-reverse mirroring use their previous offsets, and the main column of the table above is what you get. It is set by default on new configs, like MinSizeUndefinedInsteadOfAuto and FlexFirstPassUsesRunningTotals, and YGErrataAll and YGErrataClassic include it, so existing layouts keep their positions. Clearing the bit, or setting YGErrataNone, gives the spec behavior.

The enum bindings (C++, Kotlin and TypeScript) are regenerated with enums.py.

Tests

The spec cases are gentest fixtures, eleven in YGFlexWrapTest.html and one in YGAlignItemsTest.html, with expected values from Chrome and generated C++, Java and JS tests. They cover row and column, LTR and RTL, wrap and wrap-reverse (including one sided margins), baseline, static items, a percentage inset and both issue repros.

Generated tests use a plain new config, which now carries the erratum, so gentest has a new data-errata attribute: a fixture names a YGErrata value and its generated tests set it on the config. The twelve fixtures use data-errata="None", and every other fixture generates the same output as before. All 12 generated C++ tests fail on the base commit, and also fail on this branch without the attribute.

tests/YGFlexWrapCrossAlignmentTest.cpp keeps 4 hand written errata cases that pin the previous positions under the erratum, including a wrapping row with align-items: flex-start and items with margin: 4, whose lines stay at top 0 and 38 on a new config and under YGErrataAll, and move to 4 and 42 when the bit is cleared or under YGErrataNone. The positions they expect under the erratum are the ones main gives, and all 4 fail without the erratum checks in CalculateLayout.cpp.

With the change, all 893 C++ tests pass (877 plus 12 generated plus 4). Laying out 20000 random trees, every position on this branch matches the base commit exactly on a new config and under YGErrataAll, and under YGErrataNone every position matches the first commit of this PR.

Not covered here: auto cross margins in wrapping containers and baseline rows, which STEP 8 also ignores on main (#2048).

Changelog:
[General][Fixed] - Keep cross axis margins and relative insets of items in wrapping containers when YGErrataWrapCrossAlignmentIgnoresMarginAndInset is cleared (it is set by default, so existing layouts do not change)

[meta-codesync]

@javache has imported this pull request. If you are a Meta employee, you can view this in D127132637.

[cipolleschi]

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.

[cipolleschi]

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.

[Om-singhaI]

Good idea, done in 75fbc35. I added YGErrataWrapCrossAlignmentIgnoresMarginAndInset (1 << 5). When it is set, STEP 8 and the wrap-reverse mirroring use their previous offsets. New configs leave it unset, and YGErrataAll and YGErrataClassic include it, so RN keeps its current layout under classic and strict gets the spec behavior. I can also set it on new configs if you prefer.

Your example now has a test: under YGErrataAll the lines stay at top 0 and 38, and under YGErrataNone they move to 4 and 42. I also laid out 20000 random trees with YGErrataAll on the base commit and on this branch, and every position matched. The enum bindings are regenerated with enums.py.

[cipolleschi]

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.

[Om-singhaI]

Done in 191a3ae. The cases are now fixtures, eleven in YGFlexWrapTest.html and the baseline row in YGAlignItemsTest.html, and gentest wrote the C++, Java and JS tests from them in headless Chrome. They use align-self, so one tree covers all four alignments. The twelve generated C++ tests pass on this branch and all fail without the fix. I removed the hand written spec tests they replace and kept the erratum tests hand written, since Chrome can't produce the previous layout.

[cipolleschi]

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?

[Om-singhaI]

Good catch, and I see the same on main. STEP 8 sets each item's cross position from its alignment and resolves auto margins to 0, so it overwrites the offset STEP 7 computed. In your setup the item lands at 0 under flex-start and stretch and at 20 under center, instead of 40. Rows with a baseline aligned item run this pass too, so they lose the margin even without wrap.

Fixing it means splitting the line's free space between the auto margins in STEP 8. That changes those layouts as well, so it would need its own errata coverage. I'd rather keep this PR to margins and insets, so I opened #2048 with the repro.

[cipolleschi]

Thanks for the thorough follow-up, all three threads are addressed. I rebuilt the branch and checked the erratum and the fix:

• Over 30k random trees (multi-line, baseline, gap, all align-content values, static, absolute and display none items, RTL), YGErrataAll and YGErrataClassic match the base commit exactly.
• Without the erratum, a single-line wrapping container with align-content: stretch now places items on the cross axis exactly like nowrap in all 20k fuzzed cases (row/column, LTR/RTL, wrap/wrap-reverse, every alignment, px, percentage and static insets). Base mismatches in about 77% of them.

One request: since the erratum keeps the current layout exactly, could you turn it on by default for new configs, as you offered? That way the behavior is preserved for everyone using Yoga, not only for YGErrataAll and YGErrataClassic, and anyone who wants the spec behavior clears the bit. It also matches the precedent: MinSizeUndefinedInsteadOfAuto and FlexFirstPassUsesRunningTotals are both in Config::errata_'s default ("Default configs carry the errata that preserve legacy geometry", YGConfigTest.cpp). Without it, every consumer on a default config gets the layout change. Concretely: add it to the default in Config.h, clear it in YGConfigTest along with the other two, and update the enums.py comment and the PR summary to say it's set by default on new configs.

With that, this looks good to me.

[Om-singhaI]

Thanks, done in fa28ffa: the erratum is now on by default in Config.h, cleared in YGConfigTest with the other two, and described as a default in enums.py and the summary.

Generated tests use a plain new config, so the twelve fixtures needed a way to opt out. c75df94 adds a data-errata attribute to gentest for that; every other fixture generates the same output. All 884 C++ tests pass.

[cipolleschi]

hey @Om-singhaI, i'm trying to merge this, but can you rebase on top of master?

[Om-singhaI]

Rebased on main, so the branch is my five commits on top of ff54581 with no merge commit. They applied without conflicts. The erratum stays in the default errata, so the new constexpr default config matches YGConfigNew() and the new YGConfigTest cases pass. All 893 C++ tests pass locally.