Skip to content

Commit 26beed2

Browse files
committed
subtract rounded edges in pixel space when rounding layout results
Summary: **problem:** `roundLayoutResultsToPixelGrid()` derives a node's width and height as the difference of two independently rounded absolute edges. `roundValueToPixelGrid()` divides the rounded value back by `pointScaleFactor` and narrows it to `float` before returning, so the subtraction operates on two values that each already carry a representation error, and that error leaks into the resulting dimension. It only surfaces when `pointScaleFactor` makes the division inexact — `n/2` is dyadic and always exactly representable, `n/3` almost never is — and when the absolute coordinates are large enough for one float ULP to be significant. Sweeping 1001 grid-aligned offsets around 3800pt with `pointScaleFactor` 3, a node measured at exactly 288.0 is committed 287.999755859375 at 192 of them, one ULP short at that magnitude. This defeats a guarantee the function makes deliberately, and which its own comment states: "If a node has a custom measure function we never want to round down its size as this could lead to unwanted text truncation." Each edge is rounded in the intended direction; the subtraction of the two narrowed operands is not. The consequence is visible in React Native. A multi-line `<Text>` far down a long list commits a height a fraction of a point below what it measured. iOS TextKit applies a strict "does this line fit in the remaining height" test, so that shortfall makes it drop the entire trailing line, rendering visually truncated text with blank space left in its place while `onLayout` and `onTextLayout` both report the complete measurement. It reproduces on 3x screens only, tracks scroll position rather than content, and survives remounting — all consistent with the arithmetic. Measured on a minimal repro (iPhone 17 simulator, 20 fresh launches per arm, same instrumentation, only this change differing): 19 truncated renders before, 0 after, with the committed height going from 287.999755859375 to exactly 288.000000000000. **fix:** round in pixel space, where a grid-aligned value is an exact integer, so the subtraction is exact and a single narrowing happens at the end. `roundValueToPixelGridScaled()` returns the rounded value still scaled by `pointScaleFactor`, and `pixelGridValueToPoints()` converts back once. The public `roundValueToPixelGrid()` keeps its exact signature and behaviour as a thin wrapper, so `YGRoundValueToPixelGrid()` and the `Cache.cpp` callers are unaffected. regression: `YGRoundingMeasureFuncTest.rounding_measured_size_is_never_rounded_down_at_large_offsets` sweeps the offsets described above and asserts the committed height is never below the measured one. It fails on main and passes with this change. Note the exact comparison: `ASSERT_FLOAT_EQ` tolerates 4 ULPs and would not catch a 1-ULP shortfall. The full suite passes (843/843). ## Changelog: [General] [Fixed] - Layout dimensions are no longer rounded below their measured size at large offsets on non-integer pixel scales
1 parent 433d463 commit 26beed2

3 files changed

Lines changed: 138 additions & 20 deletions

File tree

‎tests/YGRoundingMeasureFuncTest.cpp‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@
88
#include <gtest/gtest.h>
99
#include <yoga/Yoga.h>
1010

11+
#include <iomanip>
12+
#include <limits>
13+
1114
static YGSize _measureFloor(
1215
YGNodeConstRef /*node*/,
1316
float width,
@@ -137,3 +140,68 @@ TEST(
137140

138141
YGConfigFree(config);
139142
}
143+
static YGSize _measureExactMultipleOfLineHeight(
144+
YGNodeConstRef /*node*/,
145+
float /*width*/,
146+
YGMeasureMode /*widthMode*/,
147+
float /*height*/,
148+
YGMeasureMode /*heightMode*/) {
149+
// 12 lines of 24pt: a height that is exactly representable and exactly on the pixel grid at any
150+
// scale factor, so any shortfall in the committed height comes from rounding, not from the input.
151+
return YGSize{
152+
300.0f,
153+
288.0f,
154+
};
155+
}
156+
157+
// A node with a measure function must never be committed a size smaller than it measured — the
158+
// rounding code calls this out explicitly ("we never want to round down its size as this could lead
159+
// to unwanted text truncation"), and forces ceil/floor on the node's edges to guarantee it.
160+
//
161+
// That guarantee used to be defeated by how the dimension was derived. Rounding each edge back to
162+
// points narrows it to float, and the dimension is the difference of two such edges, so each
163+
// operand's representation error leaked into the result. It only surfaced when pointScaleFactor made
164+
// the conversion inexact (n/2 is dyadic and always exact, n/3 almost never is) and when the absolute
165+
// coordinates were large enough for one float ULP to matter. A node measured at exactly 288.0 could
166+
// then be committed 287.999755859375 — enough for a platform text engine applying a strict "does this
167+
// line still fit" test to silently drop an entire trailing line.
168+
//
169+
// Note the exact comparison: ASSERT_FLOAT_EQ tolerates 4 ULPs and would not catch a 1-ULP shortfall.
170+
TEST(YogaTest, rounding_measured_size_is_never_rounded_down_at_large_offsets) {
171+
const float pointScaleFactor = 3.0f;
172+
const float measuredHeight = 288.0f;
173+
174+
// Sweep pixel-grid-aligned offsets through a range where one float ULP is significant.
175+
for (int scaledOffset = 11000; scaledOffset <= 12000; scaledOffset++) {
176+
const float offset = static_cast<float>(scaledOffset) / pointScaleFactor;
177+
178+
YGConfigRef config = YGConfigNew();
179+
YGConfigSetPointScaleFactor(config, pointScaleFactor);
180+
181+
YGNodeRef root = YGNodeNewWithConfig(config);
182+
YGNodeStyleSetWidth(root, 400.0f);
183+
YGNodeStyleSetHeight(root, offset + 1000.0f);
184+
185+
YGNodeRef root_child0 = YGNodeNewWithConfig(config);
186+
YGNodeStyleSetWidth(root_child0, 400.0f);
187+
YGNodeStyleSetHeight(root_child0, offset);
188+
YGNodeInsertChild(root, root_child0, 0);
189+
190+
YGNodeRef root_child1 = YGNodeNewWithConfig(config);
191+
YGNodeSetMeasureFunc(root_child1, _measureExactMultipleOfLineHeight);
192+
YGNodeInsertChild(root, root_child1, 1);
193+
194+
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
195+
196+
// Printed at full float precision: the shortfall is a single ULP, so the default formatting
197+
// would render both values as "288" and hide the difference.
198+
const float committedHeight = YGNodeLayoutGetHeight(root_child1);
199+
ASSERT_EQ(measuredHeight, committedHeight)
200+
<< std::setprecision(std::numeric_limits<float>::max_digits10)
201+
<< "measured height " << measuredHeight << " was committed as " << committedHeight
202+
<< " at offset " << offset;
203+
204+
YGNodeFreeRecursive(root);
205+
YGConfigFree(config);
206+
}
207+
}

‎yoga/algorithm/PixelGrid.cpp‎

Lines changed: 59 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,18 @@
1212

1313
namespace facebook::yoga {
1414

15-
float roundValueToPixelGrid(
15+
// Rounds `value` to the pixel grid and returns the result *in pixel space* (i.e. still multiplied by
16+
// `pointScaleFactor`), where a grid-aligned value is always an exact integer.
17+
//
18+
// Callers that need a difference of two rounded values must subtract in this space rather than convert each
19+
// operand back to points first: `scaledValue / pointScaleFactor` is generally not representable (for a 3x
20+
// screen it almost never is), and narrowing each operand to `float` before subtracting leaks that
21+
// representation error into the result. The error grows with the magnitude of the operands, so for a node far
22+
// down a long scrolling list it becomes large enough to matter — a height of exactly 288.0 points can come
23+
// back as 287.999755859375, which is enough for a text node to lose an entire trailing line when the platform
24+
// text engine checks whether the last line still fits. Subtracting two exact integers first, and narrowing
25+
// once at the end, keeps the returned dimension exactly grid-aligned.
26+
double roundValueToPixelGridScaled(
1627
const double value,
1728
const double pointScaleFactor,
1829
const bool forceCeil,
@@ -57,6 +68,25 @@ float roundValueToPixelGrid(
5768
? 1.0
5869
: 0.0);
5970
}
71+
return scaledValue;
72+
}
73+
74+
float roundValueToPixelGrid(
75+
const double value,
76+
const double pointScaleFactor,
77+
const bool forceCeil,
78+
const bool forceFloor) {
79+
const double scaledValue =
80+
roundValueToPixelGridScaled(value, pointScaleFactor, forceCeil, forceFloor);
81+
return (std::isnan(scaledValue) || std::isnan(pointScaleFactor))
82+
? YGUndefined
83+
: (float)(scaledValue / pointScaleFactor);
84+
}
85+
86+
// Converts a pixel-space value produced by `roundValueToPixelGridScaled()` back to points.
87+
static float pixelGridValueToPoints(
88+
const double scaledValue,
89+
const double pointScaleFactor) {
6090
return (std::isnan(scaledValue) || std::isnan(pointScaleFactor))
6191
? YGUndefined
6292
: (float)(scaledValue / pointScaleFactor);
@@ -86,13 +116,15 @@ void roundLayoutResultsToPixelGrid(
86116
// size as this could lead to unwanted text truncation.
87117
const bool textRounding = node->getNodeType() == NodeType::Text;
88118

89-
node->setLayoutPosition(
90-
roundValueToPixelGrid(nodeLeft, pointScaleFactor, false, textRounding),
91-
PhysicalEdge::Left);
119+
const double scaledLeft =
120+
roundValueToPixelGridScaled(nodeLeft, pointScaleFactor, false, textRounding);
121+
const double scaledTop =
122+
roundValueToPixelGridScaled(nodeTop, pointScaleFactor, false, textRounding);
92123

93124
node->setLayoutPosition(
94-
roundValueToPixelGrid(nodeTop, pointScaleFactor, false, textRounding),
95-
PhysicalEdge::Top);
125+
pixelGridValueToPoints(scaledLeft, pointScaleFactor), PhysicalEdge::Left);
126+
node->setLayoutPosition(
127+
pixelGridValueToPoints(scaledTop, pointScaleFactor), PhysicalEdge::Top);
96128

97129
// We multiply dimension by scale factor and if the result is close to the
98130
// whole number, we don't have any fraction To verify if the result is close
@@ -106,25 +138,32 @@ void roundLayoutResultsToPixelGrid(
106138
const bool hasFractionalHeight =
107139
!yoga::inexactEquals(round(scaledNodeHeight), scaledNodeHeight);
108140

141+
// The dimensions are derived as the difference of two rounded absolute edges. Both operands are exact
142+
// integers in pixel space, so subtracting there and narrowing once yields an exactly grid-aligned
143+
// dimension; converting each edge back to points first and subtracting in `float` would not.
144+
const double scaledAbsoluteLeft =
145+
roundValueToPixelGridScaled(absoluteNodeLeft, pointScaleFactor, false, textRounding);
146+
const double scaledAbsoluteRight = roundValueToPixelGridScaled(
147+
absoluteNodeRight,
148+
pointScaleFactor,
149+
(textRounding && hasFractionalWidth),
150+
(textRounding && !hasFractionalWidth));
151+
152+
const double scaledAbsoluteTop =
153+
roundValueToPixelGridScaled(absoluteNodeTop, pointScaleFactor, false, textRounding);
154+
const double scaledAbsoluteBottom = roundValueToPixelGridScaled(
155+
absoluteNodeBottom,
156+
pointScaleFactor,
157+
(textRounding && hasFractionalHeight),
158+
(textRounding && !hasFractionalHeight));
159+
109160
node->getLayout().setDimension(
110161
Dimension::Width,
111-
roundValueToPixelGrid(
112-
absoluteNodeRight,
113-
pointScaleFactor,
114-
(textRounding && hasFractionalWidth),
115-
(textRounding && !hasFractionalWidth)) -
116-
roundValueToPixelGrid(
117-
absoluteNodeLeft, pointScaleFactor, false, textRounding));
162+
pixelGridValueToPoints(scaledAbsoluteRight - scaledAbsoluteLeft, pointScaleFactor));
118163

119164
node->getLayout().setDimension(
120165
Dimension::Height,
121-
roundValueToPixelGrid(
122-
absoluteNodeBottom,
123-
pointScaleFactor,
124-
(textRounding && hasFractionalHeight),
125-
(textRounding && !hasFractionalHeight)) -
126-
roundValueToPixelGrid(
127-
absoluteNodeTop, pointScaleFactor, false, textRounding));
166+
pixelGridValueToPoints(scaledAbsoluteBottom - scaledAbsoluteTop, pointScaleFactor));
128167
}
129168

130169
for (yoga::Node* child : node->getChildren()) {

‎yoga/algorithm/PixelGrid.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,17 @@
1212

1313
namespace facebook::yoga {
1414

15+
// Round a point value to the nearest physical pixel based on DPI
16+
// (pointScaleFactor), returning the result in pixel space (still scaled by
17+
// pointScaleFactor), where a grid-aligned value is an exact integer. Use this,
18+
// rather than subtracting two `roundValueToPixelGrid()` results, whenever a
19+
// difference of two rounded values is needed.
20+
double roundValueToPixelGridScaled(
21+
double value,
22+
double pointScaleFactor,
23+
bool forceCeil,
24+
bool forceFloor);
25+
1526
// Round a point value to the nearest physical pixel based on DPI
1627
// (pointScaleFactor)
1728
float roundValueToPixelGrid(

0 commit comments

Comments
 (0)