diff --git a/tests/YGRoundingMeasureFuncTest.cpp b/tests/YGRoundingMeasureFuncTest.cpp index c654a280ec..140a6a16b0 100644 --- a/tests/YGRoundingMeasureFuncTest.cpp +++ b/tests/YGRoundingMeasureFuncTest.cpp @@ -8,6 +8,9 @@ #include #include +#include +#include + static YGSize _measureFloor( YGNodeConstRef /*node*/, float width, @@ -137,3 +140,68 @@ TEST( YGConfigFree(config); } +static YGSize _measureExactMultipleOfLineHeight( + YGNodeConstRef /*node*/, + float /*width*/, + YGMeasureMode /*widthMode*/, + float /*height*/, + YGMeasureMode /*heightMode*/) { + // 12 lines of 24pt: a height that is exactly representable and exactly on the pixel grid at any + // scale factor, so any shortfall in the committed height comes from rounding, not from the input. + return YGSize{ + 300.0f, + 288.0f, + }; +} + +// A node with a measure function must never be committed a size smaller than it measured — the +// rounding code calls this out explicitly ("we never want to round down its size as this could lead +// to unwanted text truncation"), and forces ceil/floor on the node's edges to guarantee it. +// +// That guarantee used to be defeated by how the dimension was derived. Rounding each edge back to +// points narrows it to float, and the dimension is the difference of two such edges, so each +// operand's representation error leaked into the result. It only surfaced when pointScaleFactor made +// the conversion inexact (n/2 is dyadic and always exact, n/3 almost never is) and when the absolute +// coordinates were large enough for one float ULP to matter. A node measured at exactly 288.0 could +// then be committed 287.999755859375 — enough for a platform text engine applying a strict "does this +// line still fit" test to silently drop an entire trailing line. +// +// Note the exact comparison: ASSERT_FLOAT_EQ tolerates 4 ULPs and would not catch a 1-ULP shortfall. +TEST(YogaTest, rounding_measured_size_is_never_rounded_down_at_large_offsets) { + const float pointScaleFactor = 3.0f; + const float measuredHeight = 288.0f; + + // Sweep pixel-grid-aligned offsets through a range where one float ULP is significant. + for (int scaledOffset = 11000; scaledOffset <= 12000; scaledOffset++) { + const float offset = static_cast(scaledOffset) / pointScaleFactor; + + YGConfigRef config = YGConfigNew(); + YGConfigSetPointScaleFactor(config, pointScaleFactor); + + YGNodeRef root = YGNodeNewWithConfig(config); + YGNodeStyleSetWidth(root, 400.0f); + YGNodeStyleSetHeight(root, offset + 1000.0f); + + YGNodeRef root_child0 = YGNodeNewWithConfig(config); + YGNodeStyleSetWidth(root_child0, 400.0f); + YGNodeStyleSetHeight(root_child0, offset); + YGNodeInsertChild(root, root_child0, 0); + + YGNodeRef root_child1 = YGNodeNewWithConfig(config); + YGNodeSetMeasureFunc(root_child1, _measureExactMultipleOfLineHeight); + YGNodeInsertChild(root, root_child1, 1); + + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); + + // Printed at full float precision: the shortfall is a single ULP, so the default formatting + // would render both values as "288" and hide the difference. + const float committedHeight = YGNodeLayoutGetHeight(root_child1); + ASSERT_EQ(measuredHeight, committedHeight) + << std::setprecision(std::numeric_limits::max_digits10) + << "measured height " << measuredHeight << " was committed as " << committedHeight + << " at offset " << offset; + + YGNodeFreeRecursive(root); + YGConfigFree(config); + } +} diff --git a/yoga/algorithm/PixelGrid.cpp b/yoga/algorithm/PixelGrid.cpp index 61de2be2e8..dac169af75 100644 --- a/yoga/algorithm/PixelGrid.cpp +++ b/yoga/algorithm/PixelGrid.cpp @@ -12,7 +12,18 @@ namespace facebook::yoga { -float roundValueToPixelGrid( +// Rounds `value` to the pixel grid and returns the result *in pixel space* (i.e. still multiplied by +// `pointScaleFactor`), where a grid-aligned value is always an exact integer. +// +// Callers that need a difference of two rounded values must subtract in this space rather than convert each +// operand back to points first: `scaledValue / pointScaleFactor` is generally not representable (for a 3x +// screen it almost never is), and narrowing each operand to `float` before subtracting leaks that +// representation error into the result. The error grows with the magnitude of the operands, so for a node far +// down a long scrolling list it becomes large enough to matter — a height of exactly 288.0 points can come +// back as 287.999755859375, which is enough for a text node to lose an entire trailing line when the platform +// text engine checks whether the last line still fits. Subtracting two exact integers first, and narrowing +// once at the end, keeps the returned dimension exactly grid-aligned. +double roundValueToPixelGridScaled( const double value, const double pointScaleFactor, const bool forceCeil, @@ -57,6 +68,25 @@ float roundValueToPixelGrid( ? 1.0 : 0.0); } + return scaledValue; +} + +float roundValueToPixelGrid( + const double value, + const double pointScaleFactor, + const bool forceCeil, + const bool forceFloor) { + const double scaledValue = + roundValueToPixelGridScaled(value, pointScaleFactor, forceCeil, forceFloor); + return (std::isnan(scaledValue) || std::isnan(pointScaleFactor)) + ? YGUndefined + : (float)(scaledValue / pointScaleFactor); +} + +// Converts a pixel-space value produced by `roundValueToPixelGridScaled()` back to points. +static float pixelGridValueToPoints( + const double scaledValue, + const double pointScaleFactor) { return (std::isnan(scaledValue) || std::isnan(pointScaleFactor)) ? YGUndefined : (float)(scaledValue / pointScaleFactor); @@ -86,13 +116,15 @@ void roundLayoutResultsToPixelGrid( // size as this could lead to unwanted text truncation. const bool textRounding = node->getNodeType() == NodeType::Text; - node->setLayoutPosition( - roundValueToPixelGrid(nodeLeft, pointScaleFactor, false, textRounding), - PhysicalEdge::Left); + const double scaledLeft = + roundValueToPixelGridScaled(nodeLeft, pointScaleFactor, false, textRounding); + const double scaledTop = + roundValueToPixelGridScaled(nodeTop, pointScaleFactor, false, textRounding); node->setLayoutPosition( - roundValueToPixelGrid(nodeTop, pointScaleFactor, false, textRounding), - PhysicalEdge::Top); + pixelGridValueToPoints(scaledLeft, pointScaleFactor), PhysicalEdge::Left); + node->setLayoutPosition( + pixelGridValueToPoints(scaledTop, pointScaleFactor), PhysicalEdge::Top); // We multiply dimension by scale factor and if the result is close to the // whole number, we don't have any fraction To verify if the result is close @@ -106,25 +138,32 @@ void roundLayoutResultsToPixelGrid( const bool hasFractionalHeight = !yoga::inexactEquals(round(scaledNodeHeight), scaledNodeHeight); + // The dimensions are derived as the difference of two rounded absolute edges. Both operands are exact + // integers in pixel space, so subtracting there and narrowing once yields an exactly grid-aligned + // dimension; converting each edge back to points first and subtracting in `float` would not. + const double scaledAbsoluteLeft = + roundValueToPixelGridScaled(absoluteNodeLeft, pointScaleFactor, false, textRounding); + const double scaledAbsoluteRight = roundValueToPixelGridScaled( + absoluteNodeRight, + pointScaleFactor, + (textRounding && hasFractionalWidth), + (textRounding && !hasFractionalWidth)); + + const double scaledAbsoluteTop = + roundValueToPixelGridScaled(absoluteNodeTop, pointScaleFactor, false, textRounding); + const double scaledAbsoluteBottom = roundValueToPixelGridScaled( + absoluteNodeBottom, + pointScaleFactor, + (textRounding && hasFractionalHeight), + (textRounding && !hasFractionalHeight)); + node->getLayout().setDimension( Dimension::Width, - roundValueToPixelGrid( - absoluteNodeRight, - pointScaleFactor, - (textRounding && hasFractionalWidth), - (textRounding && !hasFractionalWidth)) - - roundValueToPixelGrid( - absoluteNodeLeft, pointScaleFactor, false, textRounding)); + pixelGridValueToPoints(scaledAbsoluteRight - scaledAbsoluteLeft, pointScaleFactor)); node->getLayout().setDimension( Dimension::Height, - roundValueToPixelGrid( - absoluteNodeBottom, - pointScaleFactor, - (textRounding && hasFractionalHeight), - (textRounding && !hasFractionalHeight)) - - roundValueToPixelGrid( - absoluteNodeTop, pointScaleFactor, false, textRounding)); + pixelGridValueToPoints(scaledAbsoluteBottom - scaledAbsoluteTop, pointScaleFactor)); } for (yoga::Node* child : node->getChildren()) { diff --git a/yoga/algorithm/PixelGrid.h b/yoga/algorithm/PixelGrid.h index f3bf79f98a..ecc23480fb 100644 --- a/yoga/algorithm/PixelGrid.h +++ b/yoga/algorithm/PixelGrid.h @@ -12,6 +12,17 @@ namespace facebook::yoga { +// Round a point value to the nearest physical pixel based on DPI +// (pointScaleFactor), returning the result in pixel space (still scaled by +// pointScaleFactor), where a grid-aligned value is an exact integer. Use this, +// rather than subtracting two `roundValueToPixelGrid()` results, whenever a +// difference of two rounded values is needed. +double roundValueToPixelGridScaled( + double value, + double pointScaleFactor, + bool forceCeil, + bool forceFloor); + // Round a point value to the nearest physical pixel based on DPI // (pointScaleFactor) float roundValueToPixelGrid(