Skip to content

Commit 9fe20d4

Browse files
NickGerlemanfacebook-github-bot
authored andcommitted
Remove Measurement Placeholder Logic from Text ShadowNodes (#51465)
Summary: Pull Request resolved: #51465 Prior to D63303709 AttributedString could not represent formatting on an empty string, and so some text content was forcefully added to empty strings during measurement. This is problematic in combination with Facsimile, where we directly render the layout we used during measurement, since empty text now has a random "I" in it. Android's TextLayoutManager already knows how to interpret `baseTextAttributes`, and the placeholder is not needed. Other platforms should be updated to do the same, but that may be non-trivial to validate everywhere. This diff removes logic from ShadowNodes to always inset a placeholder, and instead shims it in platform TextLayoutManagers which have not yet been updated to use `BaseTextAttributes`. That way, we don't force placeholders during measurement, and different platforms can incrementally unjank their code. Changelog: [Internal] Reviewed By: rshest Differential Revision: D74770916 fbshipit-source-id: 7cf19db1a9a5cf68137bbff81b14ce5288235b2b
1 parent 0cd6a29 commit 9fe20d4

File tree

6 files changed

+58
-73
lines changed

6 files changed

+58
-73
lines changed
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
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 <react/renderer/attributedstring/AttributedString.h>
11+
12+
namespace facebook::react {
13+
14+
/**
15+
* Prior to D63303709 AttributedString could not represent formatting on an
16+
* empty string, and so some text content was forcefully added to empty strings
17+
* during measurement. Usages of this function should be replaced with
18+
* formatting based off of baseTextAttributes.
19+
*/
20+
inline AttributedString ensurePlaceholderIfEmpty_DO_NOT_USE(
21+
const AttributedString& attributedString) {
22+
if (attributedString.isEmpty()) {
23+
AttributedString placeholder{attributedString};
24+
placeholder.appendFragment(
25+
{.string = "I",
26+
.textAttributes = attributedString.getBaseTextAttributes(),
27+
.parentShadowView = {}});
28+
return placeholder;
29+
}
30+
31+
return attributedString;
32+
}
33+
34+
} // namespace facebook::react

packages/react-native/ReactCommon/react/renderer/components/text/BaseTextShadowNode.h

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,6 @@ class BaseTextShadowNode {
5656
const ShadowNode& parentNode,
5757
AttributedString& outAttributedString,
5858
Attachments& outAttachments);
59-
60-
/**
61-
* Returns a character used to measure empty strings in native platforms.
62-
*/
63-
inline static std::string getEmptyPlaceholder() {
64-
return "I";
65-
}
6659
};
6760

6861
} // namespace facebook::react

packages/react-native/ReactCommon/react/renderer/components/text/ParagraphShadowNode.cpp

Lines changed: 3 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -244,19 +244,6 @@ Size ParagraphShadowNode::measureContent(
244244
auto content =
245245
getContentWithMeasuredAttachments(layoutContext, layoutConstraints);
246246

247-
auto attributedString = content.attributedString;
248-
if (attributedString.isEmpty()) {
249-
// Note: `zero-width space` is insufficient in some cases (e.g. when we
250-
// need to measure the "height" of the font).
251-
// TODO T67606511: We will redefine the measurement of empty strings as
252-
// part of T67606511
253-
auto string = BaseTextShadowNode::getEmptyPlaceholder();
254-
auto textAttributes = TextAttributes::defaultTextAttributes();
255-
textAttributes.fontSizeMultiplier = layoutContext.fontSizeMultiplier;
256-
textAttributes.apply(getConcreteProps().textAttributes);
257-
attributedString.appendFragment({string, textAttributes, {}});
258-
}
259-
260247
TextLayoutContext textLayoutContext{
261248
.pointScaleFactor = layoutContext.pointScaleFactor,
262249
.surfaceId = getSurfaceId(),
@@ -267,7 +254,7 @@ Size ParagraphShadowNode::measureContent(
267254
TextLayoutManagerExtended tme(*textLayoutManager_);
268255

269256
auto preparedLayout = tme.prepareLayout(
270-
attributedString,
257+
content.attributedString,
271258
content.paragraphAttributes,
272259
textLayoutContext,
273260
layoutConstraints);
@@ -287,7 +274,7 @@ Size ParagraphShadowNode::measureContent(
287274

288275
auto size = textLayoutManager_
289276
->measure(
290-
AttributedStringBox{attributedString},
277+
AttributedStringBox{content.attributedString},
291278
content.paragraphAttributes,
292279
textLayoutContext,
293280
layoutConstraints)
@@ -304,21 +291,8 @@ Float ParagraphShadowNode::baseline(
304291
LayoutConstraints{size, size, layoutMetrics.layoutDirection};
305292
auto content =
306293
getContentWithMeasuredAttachments(layoutContext, layoutConstraints);
307-
auto attributedString = content.attributedString;
308-
309-
if (attributedString.isEmpty()) {
310-
// Note: `zero-width space` is insufficient in some cases (e.g. when we
311-
// need to measure the "height" of the font).
312-
// TODO T67606511: We will redefine the measurement of empty strings as
313-
// part of T67606511
314-
auto string = BaseTextShadowNode::getEmptyPlaceholder();
315-
auto textAttributes = TextAttributes::defaultTextAttributes();
316-
textAttributes.fontSizeMultiplier = layoutContext.fontSizeMultiplier;
317-
textAttributes.apply(getConcreteProps().textAttributes);
318-
attributedString.appendFragment({string, textAttributes, {}});
319-
}
320294

321-
AttributedStringBox attributedStringBox{attributedString};
295+
AttributedStringBox attributedStringBox{content.attributedString};
322296

323297
if constexpr (TextLayoutManagerExtended::supportsLineMeasurement()) {
324298
auto lines =

packages/react-native/ReactCommon/react/renderer/components/textinput/BaseTextInputShadowNode.h

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -228,26 +228,20 @@ class BaseTextInputShadowNode : public ConcreteViewShadowNode<
228228
return AttributedStringBox{attributedString};
229229
}
230230

231-
// For measurement purposes, we want to make sure that there's at least a
232-
// single character in the string so that the measured height is greater
233-
// than zero. Otherwise, empty TextInputs with no placeholder don't
234-
// display at all.
235-
// TODO T67606511: We will redefine the measurement of empty strings as part
236-
// of T67606511
237231
AttributedString getPlaceholderAttributedString(
238232
const LayoutContext& layoutContext) const {
239233
const auto& props = BaseShadowNode::getConcreteProps();
240234

241235
AttributedString attributedString;
242-
auto placeholderString = !props.placeholder.empty()
243-
? props.placeholder
244-
: BaseTextShadowNode::getEmptyPlaceholder();
245-
auto textAttributes =
246-
props.getEffectiveTextAttributes(layoutContext.fontSizeMultiplier);
247-
attributedString.appendFragment(
248-
{.string = std::move(placeholderString),
249-
.textAttributes = textAttributes,
250-
.parentShadowView = {}});
236+
attributedString.setBaseTextAttributes(
237+
props.getEffectiveTextAttributes(layoutContext.fontSizeMultiplier));
238+
239+
if (!props.placeholder.empty()) {
240+
attributedString.appendFragment(
241+
{.string = props.placeholder,
242+
.textAttributes = attributedString.getBaseTextAttributes(),
243+
.parentShadowView = {}});
244+
}
251245
return attributedString;
252246
}
253247
};

packages/react-native/ReactCommon/react/renderer/components/textinput/platform/android/react/renderer/components/androidtextinput/AndroidTextInputShadowNode.cpp

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,6 @@ Size AndroidTextInputShadowNode::measureContent(
6060
attributedString = getPlaceholderAttributedString(layoutContext);
6161
}
6262

63-
if (attributedString.isEmpty() && getStateData().mostRecentEventCount != 0) {
64-
return {.width = 0, .height = 0};
65-
}
66-
6763
auto textSize = textLayoutManager_
6864
->measure(
6965
AttributedStringBox{attributedString},
@@ -217,27 +213,20 @@ AttributedString AndroidTextInputShadowNode::getMostRecentAttributedString(
217213
: reactTreeAttributedString);
218214
}
219215

220-
// For measurement purposes, we want to make sure that there's at least a
221-
// single character in the string so that the measured height is greater
222-
// than zero. Otherwise, empty TextInputs with no placeholder don't
223-
// display at all.
224-
// TODO T67606511: We will redefine the measurement of empty strings as part
225-
// of T67606511
226216
AttributedString AndroidTextInputShadowNode::getPlaceholderAttributedString(
227217
const LayoutContext& layoutContext) const {
228218
const auto& props = BaseShadowNode::getConcreteProps();
229219

230220
AttributedString attributedString;
231-
auto placeholderString = !props.placeholder.empty()
232-
? props.placeholder
233-
: BaseTextShadowNode::getEmptyPlaceholder();
234-
auto textAttributes = TextAttributes::defaultTextAttributes();
235-
textAttributes.fontSizeMultiplier = layoutContext.fontSizeMultiplier;
236-
textAttributes.apply(props.textAttributes);
237-
attributedString.appendFragment(
238-
{.string = std::move(placeholderString),
239-
.textAttributes = textAttributes,
240-
.parentShadowView = ShadowView(*this)});
221+
attributedString.setBaseTextAttributes(
222+
props.getEffectiveTextAttributes(layoutContext.fontSizeMultiplier));
223+
224+
if (!props.placeholder.empty()) {
225+
attributedString.appendFragment(
226+
{.string = props.placeholder,
227+
.textAttributes = attributedString.getBaseTextAttributes(),
228+
.parentShadowView = {}});
229+
}
241230
return attributedString;
242231
}
243232

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#import "TextLayoutManager.h"
99
#import "RCTTextLayoutManager.h"
1010

11+
#import <react/renderer/attributedstring/PlaceholderAttributedString.h>
1112
#import <react/renderer/telemetry/TransactionTelemetry.h>
1213
#import <react/utils/ManagedObjectWrapper.h>
1314

@@ -36,7 +37,7 @@
3637

3738
switch (attributedStringBox.getMode()) {
3839
case AttributedStringBox::Mode::Value: {
39-
auto &attributedString = attributedStringBox.getValue();
40+
auto attributedString = ensurePlaceholderIfEmpty_DO_NOT_USE(attributedStringBox.getValue());
4041

4142
measurement = textMeasureCache_.get(
4243
{attributedString, paragraphAttributes, layoutConstraints}, [&](const TextMeasureCacheKey &key) {
@@ -92,7 +93,7 @@
9293
const Size &size) const
9394
{
9495
react_native_assert(attributedStringBox.getMode() == AttributedStringBox::Mode::Value);
95-
const auto &attributedString = attributedStringBox.getValue();
96+
auto attributedString = ensurePlaceholderIfEmpty_DO_NOT_USE(attributedStringBox.getValue());
9697

9798
RCTTextLayoutManager *textLayoutManager = (RCTTextLayoutManager *)unwrapManagedObject(nativeTextLayoutManager_);
9899

0 commit comments

Comments
 (0)