Skip to content

Commit 5e40a17

Browse files
fabriziocuccimeta-codesync[bot]
authored andcommitted
fix(ios): prevent pixel-grid rounding from clipping text (#57698)
Summary: Pull Request resolved: #57698 Re-lands the iOS text measurement rounding fix from PR #54260 / D85438723, which was reverted by D86791267. This version adds unit test coverage and moves the rounding into a helper that can be tested. On iOS, text measurement is rounded up to the device pixel grid. Converting the measured size from double to float and then rounding it to the pixel grid in Yoga can lose precision for a value that sits on a pixel boundary. That value can round down instead of up and clip the final line (height) or the trailing glyph (width). Fixes #53450. The fix adds a small epsilon before ceil on both width and height. This matches the approach the legacy architecture has used since D7074168 (a534672). The rounding now lives in a helper, internal_roundTextMeasurementToPixelGrid, so it can be covered by unit tests. The helper keeps the internal_ prefix so it stays out of the public C++ API snapshot. On the earlier revert: D85438723 was backed out after wildeMarketplaceScreenshot-e2e.js failed. Running that test on an OnDemand shows what actually happens. It passes on trunk. With this change the ad renders one physical pixel taller (height 44px to 45px, width unchanged). The top 44 rows are byte-for-byte identical to the baseline and the extra row is blank, so the content renders correctly and the failure is only the pixel-exact size check rejecting a 1px-different image. This is expected for the epsilon and is resolved by re-capturing the baseline, which is what D7074168 did in its own commit. The test is also independently flaky (test infra has flagged it, and it renders autoplaying video and carousels), which is a separate reason its earlier auto-rebaseline looked wrong. Rendering verification (wildeMarketplaceScreenshot, config browse_dynamic_image_layer_frame_overlay_high_res_sr_video), captured on an OnDemand. This is the element-scoped capture the test compares (the mp_feed_ad_item ad cell), not a full screen. The two are identical except for one blank pixel row at the bottom. | Before (trunk, 274x44) | After (this change, 274x45) | | --- | --- | | {F1993025868} | {F1993025869} | Changelog: [iOS] [Fixed] - Prevent the final line or trailing glyph of text from being clipped due to pixel-grid rounding precision loss (#53450) Reviewed By: cipolleschi Differential Revision: D113691958 fbshipit-source-id: 5824ac5efa6c01d630aaaa16cbb9181061e841a1
1 parent 98dc7d6 commit 5e40a17

4 files changed

Lines changed: 125 additions & 2 deletions

File tree

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
#pragma once
9+
10+
#include <cmath>
11+
12+
#include <react/renderer/graphics/Size.h>
13+
14+
namespace facebook::react {
15+
16+
/*
17+
* Rounds a raw text measurement up to the device pixel grid.
18+
*
19+
* A small epsilon is added to each dimension before ceil, so a value that sits
20+
* effectively on a physical-pixel boundary gains one extra physical pixel
21+
* instead of being rounded down. This works around precision loss when a
22+
* measurement is converted from double to float and then rounded to the pixel
23+
* grid in Yoga: without it, such a value can round down and clip the final line
24+
* (height) or trailing glyph (width) of text. Dimensions that already have
25+
* sub-pixel headroom are unaffected. See facebook/react-native issue #53450;
26+
* this mirrors the legacy architecture fix in D7074168, which applied the same
27+
* epsilon to both dimensions.
28+
*
29+
* This helper lives in its own header only so the unit test in
30+
* `textlayoutmanager:tests` can exercise it directly. In production it is used
31+
* only by RCTTextLayoutManager.mm, and the `internal_` prefix keeps it out of
32+
* the public C++ API snapshot.
33+
*/
34+
inline Size internal_roundTextMeasurementToPixelGrid(Size size, Float pointScaleFactor)
35+
{
36+
constexpr auto kEpsilon = static_cast<Float>(0.001);
37+
return Size{
38+
.width = std::ceil((size.width + kEpsilon) * pointScaleFactor) / pointScaleFactor,
39+
.height = std::ceil((size.height + kEpsilon) * pointScaleFactor) / pointScaleFactor,
40+
};
41+
}
42+
43+
} // namespace facebook::react

packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#import <React/NSTextStorage+FontScaling.h>
1515
#import <React/RCTUtils.h>
1616
#import <react/featureflags/ReactNativeFeatureFlags.h>
17+
#import <react/renderer/textlayoutmanager/TextMeasurementRounding.h>
1718
#import <react/utils/ManagedObjectWrapper.h>
1819
#import <react/utils/SimpleThreadSafeCache.h>
1920

@@ -581,8 +582,11 @@ - (TextMeasurement)_measureTextStorage:(NSTextStorage *)textStorage
581582
size.height = enumeratedLinesHeight;
582583
}
583584

584-
size = (CGSize){ceil(size.width * layoutContext.pointScaleFactor) / layoutContext.pointScaleFactor,
585-
ceil(size.height * layoutContext.pointScaleFactor) / layoutContext.pointScaleFactor};
585+
facebook::react::Size roundedSize = facebook::react::internal_roundTextMeasurementToPixelGrid(
586+
{.width = static_cast<facebook::react::Float>(size.width),
587+
.height = static_cast<facebook::react::Float>(size.height)},
588+
layoutContext.pointScaleFactor);
589+
size = CGSize{roundedSize.width, roundedSize.height};
586590

587591
NSRange visibleGlyphRange = [layoutManager glyphRangeForTextContainer:textContainer];
588592

packages/react-native/ReactCommon/react/renderer/textlayoutmanager/tests/TextLayoutManagerTest.cpp

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@
77

88
#include <gtest/gtest.h>
99

10+
#include <cmath>
11+
1012
#include <react/renderer/textlayoutmanager/TextMeasureCache.h>
13+
#include <react/renderer/textlayoutmanager/TextMeasurementRounding.h>
1114

1215
using namespace facebook::react;
1316

@@ -97,3 +100,47 @@ TEST(TextLayoutManagerTest, pointScaleFactorAffectsPreparedTextCacheHash) {
97100
std::hash<PreparedTextCacheKey>{}(lhs),
98101
std::hash<PreparedTextCacheKey>{}(rhs));
99102
}
103+
104+
// Tests for internal_roundTextMeasurementToPixelGrid (the pixel-grid rounding
105+
// used by text measurement). A small epsilon is added before ceil so a
106+
// dimension that lands exactly on a pixel boundary gains one physical pixel of
107+
// slack (avoiding double->float->Yoga precision from clipping the last line or
108+
// trailing glyph), while dimensions with sub-pixel headroom are left untouched.
109+
// pointScaleFactor 3 (an @3x screen) is used so one physical pixel is 1/3 pt.
110+
// These lock in the behavior of D7074168 / PR #54260 in the new architecture,
111+
// with the unit coverage that change lacked.
112+
namespace {
113+
constexpr Float kScale = 3.0;
114+
long heightInPixels(Size raw) {
115+
return std::lround(
116+
internal_roundTextMeasurementToPixelGrid(raw, kScale).height * kScale);
117+
}
118+
long widthInPixels(Size raw) {
119+
return std::lround(
120+
internal_roundTextMeasurementToPixelGrid(raw, kScale).width * kScale);
121+
}
122+
} // namespace
123+
124+
// Height exactly on a pixel boundary gains one extra physical pixel (60 -> 61)
125+
// so the final line is not clipped after layout rounding.
126+
TEST(TextMeasurementRoundingTest, addsSlackWhenHeightOnPixelBoundary) {
127+
EXPECT_EQ(heightInPixels(Size{100.0, 20.0}), 61);
128+
}
129+
130+
// Width exactly on a pixel boundary likewise gains one extra physical pixel so
131+
// the trailing glyph is not clipped (matches the legacy both-dimensions fix).
132+
TEST(TextMeasurementRoundingTest, addsSlackWhenWidthOnPixelBoundary) {
133+
EXPECT_EQ(widthInPixels(Size{20.0, 100.0}), 61);
134+
}
135+
136+
// Height with sub-pixel headroom must NOT be inflated: 20.1pt -> 60.3px ->
137+
// 61px, never 62. The epsilon only nudges boundary values, so text is not made
138+
// taller than needed.
139+
TEST(TextMeasurementRoundingTest, doesNotInflateHeightWithSubpixelHeadroom) {
140+
EXPECT_EQ(heightInPixels(Size{100.0, 20.1}), 61);
141+
}
142+
143+
// Width with sub-pixel headroom must NOT be inflated either: 20.1pt -> 61px.
144+
TEST(TextMeasurementRoundingTest, doesNotInflateWidthWithSubpixelHeadroom) {
145+
EXPECT_EQ(widthInPixels(Size{20.1, 100.0}), 61);
146+
}

packages/rn-tester/js/examples/Text/TextSharedExamples.js

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,27 @@ const styles = StyleSheet.create({
242242
},
243243
});
244244

245+
function LastLineClippingExample(): React.Node {
246+
// Wrapped paragraphs at assorted font sizes are prone to the final line being
247+
// clipped by one physical pixel after layout rounding (issue #53450). The
248+
// border makes any clipping of the last line visible.
249+
return (
250+
<View>
251+
{[11, 12, 13, 14, 15, 16, 17].map(fontSize => (
252+
<View
253+
key={fontSize}
254+
style={{borderWidth: 1, borderColor: '#999999', marginBottom: 6}}>
255+
<Text style={{fontSize}}>
256+
{`(${fontSize}px) This sentence wraps across multiple lines so the ` +
257+
'final line sits near a pixel boundary and must render fully, ' +
258+
'without being cut off.'}
259+
</Text>
260+
</View>
261+
))}
262+
</View>
263+
);
264+
}
265+
245266
export default [
246267
{
247268
title: 'Empty Text',
@@ -277,4 +298,12 @@ export default [
277298
description: 'Shows the a11y behavior of Text with role="link"',
278299
render: TextWithLinkRoleExample,
279300
},
301+
{
302+
title: 'Wrapped text last-line clipping',
303+
name: 'wrappedLastLineClipping',
304+
description:
305+
'The final line of wrapped text must render fully, not clipped by one ' +
306+
'physical pixel after layout rounding (issue #53450).',
307+
render: LastLineClippingExample,
308+
},
280309
] as ReadonlyArray<RNTesterModuleExample>;

0 commit comments

Comments
 (0)