diff --git a/tests/YGFlexShrinkAspectRatioBugTest.cpp b/tests/YGFlexShrinkAspectRatioBugTest.cpp new file mode 100644 index 0000000000..e2d886dc18 --- /dev/null +++ b/tests/YGFlexShrinkAspectRatioBugTest.cpp @@ -0,0 +1,120 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// Regression test for https://github.com/react/yoga/issues/2038 +// Sibling case to YGFlexShrinkBorderBugTest (issue #1665): an item whose flex +// basis comes from an aspectRatio, and which is the only shrinkable item on an +// overflowing line, is measured at ~8.3e9 instead of being clamped to 0. +// +// Mechanism: distributeFreeSpaceFirstPass freezes the item and subtracts its +// contribution from totalFlexShrinkScaledFactors. The compiler contracts that +// multiply-subtract into a fused multiply-add using the unrounded product, so +// it does not cancel against the float accumulator built in calculateFlexLine +// and leaves a residual of 2^-19. distributeFreeSpaceSecondPass then divides +// the remaining free space by that residual. The absolute 1e-6f epsilon guard +// added for #1665 does not catch it, because the residual's magnitude scales +// with the basis and shrink factor. + +#include +#include + +namespace { + +// root(350x350) > row(flexDirection: row) > [ box(aspectRatio, flexShrink), sibling(600x23) ] +// Returns the box; the caller owns the tree via rootOut. +YGNodeRef buildOverflowingRow(YGConfigRef config, YGNodeRef* rootOut, float shrink) { + YGNodeRef root = YGNodeNewWithConfig(config); + YGNodeStyleSetWidth(root, 350.0f); + YGNodeStyleSetHeight(root, 350.0f); + + YGNodeRef row = YGNodeNewWithConfig(config); + YGNodeStyleSetFlexDirection(row, YGFlexDirectionRow); + YGNodeInsertChild(root, row, 0); + + // The only shrinkable item on the line. Its flex basis is derived from the + // aspect ratio and the stretched cross size (23 * 22/24 = 21.083...), which is + // what makes the precision asymmetry observable. + YGNodeRef box = YGNodeNewWithConfig(config); + YGNodeStyleSetAspectRatio(box, 22.0f / 24.0f); + YGNodeStyleSetFlexShrink(box, shrink); + YGNodeInsertChild(row, box, 0); + + // Overflows the row so remainingFreeSpace is negative. flexShrink defaults to + // 0, so this item cannot shrink and the box is the sole candidate. + YGNodeRef sibling = YGNodeNewWithConfig(config); + YGNodeStyleSetWidth(sibling, 600.0f); + YGNodeStyleSetHeight(sibling, 23.0f); + YGNodeInsertChild(row, sibling, 1); + + *rootOut = root; + return box; +} + +} // namespace + +TEST(YGFlexShrinkAspectRatioBug, aspect_ratio_sole_shrinkable_item_on_overflowing_line) { + YGConfigRef config = YGConfigNew(); + YGNodeRef root = nullptr; + YGNodeRef box = buildOverflowingRow(config, &root, 3.0f); + + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); + + // Unshrunk the box would be 21.083 wide; shrinking can only reduce it. + EXPECT_GE(YGNodeLayoutGetWidth(box), 0.0f); + EXPECT_LE(YGNodeLayoutGetWidth(box), 22.0f) + << "shrinking item is astronomically large (bug)"; + // aspectRatio derives the cross axis from the main axis, so a runaway width + // propagates to the height as well. + EXPECT_LE(YGNodeLayoutGetHeight(box), 24.0f) + << "runaway propagated to the cross axis via aspectRatio"; + + YGNodeFreeRecursive(root); + YGConfigFree(config); +} + +// flexShrink 3 is not special. Whether a given factor diverges depends on where +// its product with the 55/3 basis lands in the float mantissa, so cover a +// spread. Values 1 and 2 already clamp correctly on the unfixed code; the rest +// do not. +TEST(YGFlexShrinkAspectRatioBug, bounded_across_shrink_factors) { + const float shrinkFactors[] = { + 1.0f, 2.0f, 3.0f, 6.0f, 9.0f, 11.0f, 12.0f, 19.0f, 22.0f, 999.0f}; + + for (float shrink : shrinkFactors) { + YGConfigRef config = YGConfigNew(); + YGNodeRef root = nullptr; + YGNodeRef box = buildOverflowingRow(config, &root, shrink); + + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); + + EXPECT_LE(YGNodeLayoutGetWidth(box), 22.0f) + << "flexShrink " << shrink << " produced a runaway width"; + EXPECT_GE(YGNodeLayoutGetWidth(box), 0.0f) + << "flexShrink " << shrink << " produced a negative width"; + + YGNodeFreeRecursive(root); + YGConfigFree(config); + } +} + +// Guards the basis itself: with no shrinking the box must keep its +// aspect-ratio-derived size, so a change here would mean the basis moved rather +// than the distribution misbehaving. +TEST(YGFlexShrinkAspectRatioBug, unshrinkable_item_keeps_aspect_ratio_basis) { + YGConfigRef config = YGConfigNew(); + YGNodeRef root = nullptr; + YGNodeRef box = buildOverflowingRow(config, &root, 0.0f); + + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); + + // 23 * 22/24 = 21.083..., rounded to the default pixel grid. + EXPECT_FLOAT_EQ(21.0f, YGNodeLayoutGetWidth(box)); + EXPECT_FLOAT_EQ(23.0f, YGNodeLayoutGetHeight(box)); + + YGNodeFreeRecursive(root); + YGConfigFree(config); +} diff --git a/yoga/algorithm/CalculateLayout.cpp b/yoga/algorithm/CalculateLayout.cpp index e8998c0693..f56e86638f 100644 --- a/yoga/algorithm/CalculateLayout.cpp +++ b/yoga/algorithm/CalculateLayout.cpp @@ -1104,15 +1104,13 @@ static float distributeFreeSpaceSecondPass( if (flexShrinkScaledFactor != 0) { float childSize = YGUndefined; - // Use a relative epsilon guard instead of exact equality: after the - // first pass removes all constrained items, - // totalFlexShrinkScaledFactors may be near-zero rather than exactly 0 - // due to floating-point cancellation. Dividing by a near-zero value - // produces a gigantic childSize that overwhelms the min/max clamp. - const float shrinkFactorMagnitude = - std::abs(flexLine.layout.totalFlexShrinkScaledFactors); - if (yoga::isDefined(flexLine.layout.totalFlexShrinkScaledFactors) && - shrinkFactorMagnitude < 1e-6f) { + // Once the first pass has frozen every shrinkable item there is nothing + // left to distribute proportionally. Test that with the item count + // rather than by comparing totalFlexShrinkScaledFactors against zero. + // Float accumulation is not exact -- (a + b) - a - b need not be 0 even + // for identical operands -- so any absolute epsilon here is a guess; + // the previous 1e-6f guard missed a real 1.9e-6 residual (#2038). + if (flexLine.layout.numberOfShrinkableItems == 0) { childSize = childFlexBasis + flexShrinkScaledFactor; } else { childSize = childFlexBasis + @@ -1336,9 +1334,22 @@ static void distributeFreeSpaceFirstPass( // resulting in the item's size calculation being identical in the // first and second passes. deltaFreeSpace += boundMainSize - childFlexBasis; + // Round to float before subtracting. Left inline, the compiler may + // contract this multiply-subtract into a fused multiply-add that + // uses the unrounded product, so it does not cancel against the + // float accumulator built in calculateFlexLine and leaves a residual + // (2^-19 for an aspect-ratio basis) which + // distributeFreeSpaceSecondPass then divides the remaining free space + // by. + const float excludedShrinkScaledFactor = + -currentLineChild->resolveFlexShrink() * + currentLineChild->getLayout().computedFlexBasis.unwrap(); flexLine.layout.totalFlexShrinkScaledFactors -= - (-currentLineChild->resolveFlexShrink() * - currentLineChild->getLayout().computedFlexBasis.unwrap()); + excludedShrinkScaledFactor; + if (excludedShrinkScaledFactor != 0 && + flexLine.layout.numberOfShrinkableItems > 0) { + flexLine.layout.numberOfShrinkableItems--; + } } } } else if ( diff --git a/yoga/algorithm/FlexLine.cpp b/yoga/algorithm/FlexLine.cpp index dc0a300add..5423f4826e 100644 --- a/yoga/algorithm/FlexLine.cpp +++ b/yoga/algorithm/FlexLine.cpp @@ -28,6 +28,7 @@ FlexLine calculateFlexLine( float sizeConsumed = 0.0f; float totalFlexGrowFactors = 0.0f; float totalFlexShrinkScaledFactors = 0.0f; + size_t numberOfShrinkableItems = 0; size_t numberOfAutoMargins = 0; yoga::Node* firstElementInLine = nullptr; @@ -94,8 +95,12 @@ FlexLine calculateFlexLine( // Unlike the grow factor, the shrink factor is scaled relative to the // child dimension. - totalFlexShrinkScaledFactors += -child->resolveFlexShrink() * + const float shrinkScaledFactor = -child->resolveFlexShrink() * child->getLayout().computedFlexBasis.unwrap(); + totalFlexShrinkScaledFactors += shrinkScaledFactor; + if (shrinkScaledFactor != 0) { + numberOfShrinkableItems++; + } } itemsInFlow.push_back(child); @@ -118,6 +123,7 @@ FlexLine calculateFlexLine( .layout = FlexLineRunningLayout{ totalFlexGrowFactors, totalFlexShrinkScaledFactors, + numberOfShrinkableItems, }}; } diff --git a/yoga/algorithm/FlexLine.h b/yoga/algorithm/FlexLine.h index 89983ab95d..f3541a4c9e 100644 --- a/yoga/algorithm/FlexLine.h +++ b/yoga/algorithm/FlexLine.h @@ -23,6 +23,13 @@ struct FlexLineRunningLayout { // line. This is decremented as free space is distributed. float totalFlexShrinkScaledFactors{0.0f}; + // Number of items on the line which can still shrink, i.e. whose scaled + // shrink factor is non-zero. Kept in lockstep with + // totalFlexShrinkScaledFactors as items are frozen so that "no shrinkable + // items remain" can be tested exactly rather than by comparing an accumulated + // float against zero. + size_t numberOfShrinkableItems{0}; + // The amount of available space within inner dimensions of the line which may // still be distributed. float remainingFreeSpace{0.0f};