Skip to content

Commit eb9993f

Browse files
Om-singhaIfacebook-github-bot
authored andcommitted
Keep cross axis margins and relative insets of items in wrapping containers
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
1 parent 79c6c23 commit eb9993f

1 file changed

Lines changed: 38 additions & 21 deletions

File tree

‎packages/react-native/ReactCommon/yoga/yoga/algorithm/CalculateLayout.cpp‎

Lines changed: 38 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2325,6 +2325,9 @@ static void calculateLayoutImpl(
23252325
float leadPerLine = 0;
23262326
float currentLead = leadingPaddingAndBorderCross;
23272327
float extraSpacePerLine = 0;
2328+
// With wrap-reverse the cross positions set here are mirrored further
2329+
// below, so an item's flex end margin is the one that leads here.
2330+
const bool isWrapReverse = node->style().flexWrap() == Wrap::WrapReverse;
23282331

23292332
const float unclampedCrossDim = sizingModeCrossDim == SizingMode::StretchFit
23302333
? availableInnerCrossDim + paddingAndBorderAxisCross
@@ -2443,43 +2446,51 @@ static void calculateLayoutImpl(
24432446
continue;
24442447
}
24452448
if (child->style().positionType() != PositionType::Absolute) {
2449+
const float flexStartMargin = child->style().computeFlexStartMargin(
2450+
crossAxis, direction, availableInnerWidth);
2451+
const float flexEndMargin = child->style().computeFlexEndMargin(
2452+
crossAxis, direction, availableInnerWidth);
2453+
const float leadingCrossMargin =
2454+
isWrapReverse ? flexEndMargin : flexStartMargin;
2455+
const float trailingCrossMargin =
2456+
isWrapReverse ? flexStartMargin : flexEndMargin;
2457+
const float relativePositionCross = child->relativePosition(
2458+
crossAxis, direction, availableInnerCrossDim);
2459+
24462460
switch (resolveChildAlignment(node, child)) {
24472461
case Align::Start:
24482462
case Align::End:
24492463
// Not yet implemented
24502464
break;
24512465
case Align::FlexStart: {
24522466
child->setLayoutPosition(
2453-
currentLead +
2454-
child->style().computeFlexStartPosition(
2455-
crossAxis, direction, availableInnerWidth),
2467+
currentLead + leadingCrossMargin + relativePositionCross,
24562468
flexStartEdge(crossAxis));
24572469
break;
24582470
}
24592471
case Align::FlexEnd: {
24602472
child->setLayoutPosition(
2461-
currentLead + lineHeight -
2462-
child->style().computeFlexEndMargin(
2463-
crossAxis, direction, availableInnerWidth) -
2473+
currentLead + lineHeight - trailingCrossMargin -
24642474
child->getLayout().measuredDimension(
2465-
dimension(crossAxis)),
2475+
dimension(crossAxis)) +
2476+
relativePositionCross,
24662477
flexStartEdge(crossAxis));
24672478
break;
24682479
}
24692480
case Align::Center: {
2470-
float childHeight =
2471-
child->getLayout().measuredDimension(dimension(crossAxis));
2481+
const float childOuterCrossDim =
2482+
child->getLayout().measuredDimension(dimension(crossAxis)) +
2483+
leadingCrossMargin + trailingCrossMargin;
24722484

24732485
child->setLayoutPosition(
2474-
currentLead + (lineHeight - childHeight) / 2,
2486+
currentLead + (lineHeight - childOuterCrossDim) / 2 +
2487+
leadingCrossMargin + relativePositionCross,
24752488
flexStartEdge(crossAxis));
24762489
break;
24772490
}
24782491
case Align::Stretch: {
24792492
child->setLayoutPosition(
2480-
currentLead +
2481-
child->style().computeFlexStartMargin(
2482-
crossAxis, direction, availableInnerWidth),
2493+
currentLead + leadingCrossMargin + relativePositionCross,
24832494
flexStartEdge(crossAxis));
24842495

24852496
// Remeasure child with the line height as it as been only
@@ -2527,11 +2538,7 @@ static void calculateLayoutImpl(
25272538
case Align::Baseline: {
25282539
child->setLayoutPosition(
25292540
currentLead + maxAscentForCurrentLine -
2530-
calculateBaseline(child) +
2531-
child->style().computeFlexStartPosition(
2532-
FlexDirection::Column,
2533-
direction,
2534-
availableInnerCrossDim),
2541+
calculateBaseline(child) + relativePositionCross,
25352542
PhysicalEdge::Top);
25362543

25372544
break;
@@ -2643,14 +2650,24 @@ static void calculateLayoutImpl(
26432650
}
26442651

26452652
// As we only wrapped in normal direction yet, we need to reverse the
2646-
// positions on wrap-reverse.
2653+
// positions on wrap-reverse. A relative inset is a physical offset, so it is
2654+
// taken out before reversing and added back afterwards.
26472655
if (performLayout && node->style().flexWrap() == Wrap::WrapReverse) {
26482656
for (auto child : node->getLayoutChildren()) {
26492657
if (child->style().positionType() != PositionType::Absolute) {
2658+
const float relativePositionCross =
2659+
child->style().display() == Display::None
2660+
? 0.0f
2661+
: child->relativePosition(
2662+
crossAxis, direction, availableInnerCrossDim);
2663+
const float positionInFlow =
2664+
child->getLayout().position(flexStartEdge(crossAxis)) -
2665+
relativePositionCross;
26502666
child->setLayoutPosition(
26512667
node->getLayout().measuredDimension(dimension(crossAxis)) -
2652-
child->getLayout().position(flexStartEdge(crossAxis)) -
2653-
child->getLayout().measuredDimension(dimension(crossAxis)),
2668+
positionInFlow -
2669+
child->getLayout().measuredDimension(dimension(crossAxis)) +
2670+
relativePositionCross,
26542671
flexStartEdge(crossAxis));
26552672
}
26562673
}

0 commit comments

Comments
 (0)