Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: text cut off phenomenon on some Android devices like OPPO, Xiaomi and etc #37271

Closed
wants to merge 6 commits into from
Closed

fix: text cut off phenomenon on some Android devices like OPPO, Xiaomi and etc #37271

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
875c04e
fix: text cut off phenomenon on some Android devices like Xiaomi
May 5, 2023
f5444b7
fix: Fix NoSuchMethodError on Android api level below M
May 10, 2023
b924760
fix: Text component may get wrong height when the value of numberOfLi…
May 11, 2023
72eb600
fix: add setFallbackLineSpacing call
May 12, 2023
9123480
Merge branch 'facebook:main' into fix/text-cut
jcdhlzq May 19, 2023
aabea1b
Merge branch 'facebook:main' into fix/text-cut
jcdhlzq May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
215 changes: 131 additions & 84 deletions ...-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
import android.text.Spanned;
import android.text.StaticLayout;
import android.text.TextPaint;
import android.util.TypedValue;
import android.view.Gravity;
import android.view.ViewGroup;
import android.widget.TextView;
import androidx.annotation.Nullable;
import com.facebook.infer.annotation.Assertions;
Expand All @@ -32,6 +34,7 @@
import com.facebook.react.uimanager.UIViewOperationQueue;
import com.facebook.react.uimanager.annotations.ReactProp;
import com.facebook.react.uimanager.events.RCTEventEmitter;
import com.facebook.react.views.view.MeasureUtil;
import com.facebook.yoga.YogaBaselineFunction;
import com.facebook.yoga.YogaConstants;
import com.facebook.yoga.YogaDirection;
Expand Down Expand Up @@ -59,6 +62,130 @@ public class ReactTextShadowNode extends ReactBaseTextShadowNode {

private boolean mShouldNotifyOnTextLayout;

private TextView mInternalView = null;

@Override
public void setThemedContext(ThemedReactContext themedContext) {
super.setThemedContext(themedContext);

mInternalView = new TextView(themedContext);
mInternalView.setPadding(0, 0, 0, 0);
// This is needed to fix an android bug since 4.4.3 which will throw an NPE in measure,
// setting the layoutParams fixes it: https://code.google.com/p/android/issues/detail?id=75877
mInternalView.setLayoutParams(
new ViewGroup.LayoutParams(
ViewGroup.LayoutParams.WRAP_CONTENT, ViewGroup.LayoutParams.WRAP_CONTENT));
}

private long measureWithView(Spannable text, TextView textView, float width,
YogaMeasureMode widthMode, float height, YogaMeasureMode heightMode) {
textView.setText(text);
textView.setTextSize(TypedValue.COMPLEX_UNIT_PX, mTextAttributes.getEffectiveFontSize());

textView.setGravity(getTextAlign());
textView.setIncludeFontPadding(mIncludeFontPadding);
float paddingLeft = getPadding(Spacing.START);
float paddingTop = getPadding(Spacing.TOP);
float paddingRight = getPadding(Spacing.END);
float paddingBottom = getPadding(Spacing.BOTTOM);

if (paddingLeft != UNSET
&& paddingTop != UNSET
&& paddingRight != UNSET
&& paddingBottom != UNSET) {

textView.setPadding(
(int) Math.floor(paddingLeft),
(int) Math.floor(paddingTop),
(int) Math.floor(paddingRight),
(int) Math.floor(paddingBottom));
}
if (mNumberOfLines != UNSET) {
textView.setLines(mNumberOfLines);
}

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M
&& textView.getBreakStrategy() != mTextBreakStrategy) {
textView.setBreakStrategy(mTextBreakStrategy);
}
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O &&
textView.getJustificationMode() != mJustificationMode) {
textView.setJustificationMode(mJustificationMode);
}
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M &&
textView.getHyphenationFrequency() != mHyphenationFrequency) {
textView.setHyphenationFrequency(mHyphenationFrequency);
}
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
textView.setFallbackLineSpacing(true);
}

textView.measure(
MeasureUtil.getMeasureSpec(width, widthMode),
MeasureUtil.getMeasureSpec(height, heightMode));

Layout layout = textView.getLayout();

if (mAdjustsFontSizeToFit) {
int initialFontSize = mTextAttributes.getEffectiveFontSize();
int currentFontSize = mTextAttributes.getEffectiveFontSize();
// Minimum font size is 4pts to match the iOS implementation.
int minimumFontSize =
(int) Math.max(mMinimumFontScale * initialFontSize, PixelUtil.toPixelFromDIP(4));
while (currentFontSize > minimumFontSize
&& (mNumberOfLines != UNSET && layout.getLineCount() > mNumberOfLines
|| heightMode != YogaMeasureMode.UNDEFINED && layout.getHeight() > height)) {
// TODO: We could probably use a smarter algorithm here. This will require 0(n)
// measurements
// based on the number of points the font size needs to be reduced by.
currentFontSize = currentFontSize - (int) PixelUtil.toPixelFromDIP(1);

float ratio = (float) currentFontSize / (float) initialFontSize;
ReactAbsoluteSizeSpan[] sizeSpans =
text.getSpans(0, text.length(), ReactAbsoluteSizeSpan.class);
for (ReactAbsoluteSizeSpan span : sizeSpans) {
text.setSpan(
new ReactAbsoluteSizeSpan(
(int) Math.max((span.getSize() * ratio), minimumFontSize)),
text.getSpanStart(span),
text.getSpanEnd(span),
text.getSpanFlags(span));
text.removeSpan(span);
}
// make sure the placeholder content is also being measured
textView.setText(text);
textView.measure(
MeasureUtil.getMeasureSpec(width, widthMode),
MeasureUtil.getMeasureSpec(height, heightMode));
layout = textView.getLayout();
}
}

if (mShouldNotifyOnTextLayout) {
ThemedReactContext themedReactContext = getThemedContext();
WritableArray lines =
FontMetricsUtil.getFontMetrics(
text, layout, textView.getPaint(), themedReactContext);
WritableMap event = Arguments.createMap();
event.putArray("lines", lines);
if (themedReactContext.hasActiveCatalystInstance()) {
themedReactContext
.getJSModule(RCTEventEmitter.class)
.receiveEvent(getReactTag(), "topTextLayout", event);
} else {
ReactSoftExceptionLogger.logSoftException(
"ReactTextShadowNode",
new ReactNoCrashSoftException("Cannot get RCTEventEmitter, no CatalystInstance"));
}
}

if (mNumberOfLines != UNSET && mNumberOfLines < layout.getLineCount()) {
return YogaMeasureOutput.make(
layout.getWidth(), layout.getLineBottom(mNumberOfLines - 1));
} else {
return YogaMeasureOutput.make(layout.getWidth(), layout.getHeight());
}
}
private final YogaMeasureFunction mTextMeasureFunction =
new YogaMeasureFunction() {
@Override
Expand All @@ -73,90 +200,10 @@ public long measure(
mPreparedSpannableText,
"Spannable element has not been prepared in onBeforeLayout");

Layout layout = measureSpannedText(text, width, widthMode);

if (mAdjustsFontSizeToFit) {
int initialFontSize = mTextAttributes.getEffectiveFontSize();
int currentFontSize = mTextAttributes.getEffectiveFontSize();
// Minimum font size is 4pts to match the iOS implementation.
int minimumFontSize =
(int) Math.max(mMinimumFontScale * initialFontSize, PixelUtil.toPixelFromDIP(4));
while (currentFontSize > minimumFontSize
&& (mNumberOfLines != UNSET && layout.getLineCount() > mNumberOfLines
|| heightMode != YogaMeasureMode.UNDEFINED && layout.getHeight() > height)) {
// TODO: We could probably use a smarter algorithm here. This will require 0(n)
// measurements
// based on the number of points the font size needs to be reduced by.
currentFontSize = currentFontSize - (int) PixelUtil.toPixelFromDIP(1);

float ratio = (float) currentFontSize / (float) initialFontSize;
ReactAbsoluteSizeSpan[] sizeSpans =
text.getSpans(0, text.length(), ReactAbsoluteSizeSpan.class);
for (ReactAbsoluteSizeSpan span : sizeSpans) {
text.setSpan(
new ReactAbsoluteSizeSpan(
(int) Math.max((span.getSize() * ratio), minimumFontSize)),
text.getSpanStart(span),
text.getSpanEnd(span),
text.getSpanFlags(span));
text.removeSpan(span);
}
layout = measureSpannedText(text, width, widthMode);
}
}

if (mShouldNotifyOnTextLayout) {
ThemedReactContext themedReactContext = getThemedContext();
WritableArray lines =
FontMetricsUtil.getFontMetrics(
text, layout, sTextPaintInstance, themedReactContext);
WritableMap event = Arguments.createMap();
event.putArray("lines", lines);
if (themedReactContext.hasActiveReactInstance()) {
themedReactContext
.getJSModule(RCTEventEmitter.class)
.receiveEvent(getReactTag(), "topTextLayout", event);
} else {
ReactSoftExceptionLogger.logSoftException(
"ReactTextShadowNode",
new ReactNoCrashSoftException("Cannot get RCTEventEmitter, no CatalystInstance"));
}
}

final int lineCount =
mNumberOfLines == UNSET
? layout.getLineCount()
: Math.min(mNumberOfLines, layout.getLineCount());

// Instead of using `layout.getWidth()` (which may yield a significantly larger width for
// text that is wrapping), compute width using the longest line.
float layoutWidth = 0;
if (widthMode == YogaMeasureMode.EXACTLY) {
layoutWidth = width;
} else {
for (int lineIndex = 0; lineIndex < lineCount; lineIndex++) {
float lineWidth = layout.getLineWidth(lineIndex);
if (lineWidth > layoutWidth) {
layoutWidth = lineWidth;
}
}
if (widthMode == YogaMeasureMode.AT_MOST && layoutWidth > width) {
layoutWidth = width;
}
}

if (android.os.Build.VERSION.SDK_INT > android.os.Build.VERSION_CODES.Q) {
layoutWidth = (float) Math.ceil(layoutWidth);
}
float layoutHeight = height;
if (heightMode != YogaMeasureMode.EXACTLY) {
layoutHeight = layout.getLineBottom(lineCount - 1);
if (heightMode == YogaMeasureMode.AT_MOST && layoutHeight > height) {
layoutHeight = height;
}
}

return YogaMeasureOutput.make(layoutWidth, layoutHeight);
TextView textView =
Assertions.assertNotNull(mInternalView, "mInternalView cannot be null");

return measureWithView(text, textView, width, widthMode, height, heightMode);
Comment on lines +203 to +206
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yoga measures on a background thread. It's unsafe to access TextView here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, could you offer some points exactly about the unsafe access? If it means some cases like #17530 , I think they are different. Because EditText has a background by default while TextView doesn't.

    <style name="Widget.EditText">
        <item name="focusable">true</item>
        <item name="focusableInTouchMode">true</item>
        <item name="clickable">true</item>
        <item name="background">?attr/editTextBackground</item>
        <item name="textAppearance">?attr/textAppearanceMediumInverse</item>
        <item name="textColor">?attr/editTextColor</item>
        <item name="gravity">center_vertical</item>
        <item name="breakStrategy">simple</item>
        <item name="hyphenationFrequency">@dimen/config_preferredHyphenationFrequency</item>
        <item name="defaultFocusHighlightEnabled">false</item>
    </style>
    <style name="Widget.TextView">
        <item name="textAppearance">?attr/textAppearanceSmall</item>
        <item name="textSelectHandleLeft">?attr/textSelectHandleLeft</item>
        <item name="textSelectHandleRight">?attr/textSelectHandleRight</item>
        <item name="textSelectHandle">?attr/textSelectHandle</item>
        <item name="textEditPasteWindowLayout">?attr/textEditPasteWindowLayout</item>
        <item name="textEditNoPasteWindowLayout">?attr/textEditNoPasteWindowLayout</item>
        <item name="textEditSidePasteWindowLayout">?attr/textEditSidePasteWindowLayout</item>
        <item name="textEditSideNoPasteWindowLayout">?attr/textEditSideNoPasteWindowLayout</item>
        <item name="textEditSuggestionItemLayout">?attr/textEditSuggestionItemLayout</item>
        <item name="textEditSuggestionContainerLayout">?attr/textEditSuggestionContainerLayout</item>
        <item name="textEditSuggestionHighlightStyle">?attr/textEditSuggestionHighlightStyle</item>
        <item name="textCursorDrawable">?attr/textCursorDrawable</item>
        <item name="breakStrategy">high_quality</item>
        <item name="hyphenationFrequency">@dimen/config_preferredHyphenationFrequency</item>
    </style>

Moreover, There are many cases where YogaMeasureFunction measures in the same way like ReactSwitch, ProgressBar.

Copy link
Contributor

@NickGerleman NickGerleman Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the change is that with this PR, the ShadowNode now manages a new TextView on the background thread that is only used during layout?

So this is moving from laying out Spannable to laying out a background text element.

This is how TextInput works today on Paper, but Fabric, for both Text and TextInput, still measures off of the spannable/AttributedString.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the change is that with this PR, the ShadowNode now manages a new TextView on the background thread that is only used during layout?

So this is moving from laying out Spannable to laying out a background text element.

This is how TextInput works today on Paper, but Fabric, for both Text and TextInput, still measures off of the spannable/AttributedString.

That's right!

Copy link
Contributor

@NickGerleman NickGerleman Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing if we can dig up the context on why Fabric chose to avoid this style of measurement, but this is a pretty major change. Usually we'd want to A/B test something like this, but that isn't really a possibility because most of our internal apps do not run Paper-specific code. This also means we receive very limited test coverage, since this code is not exercised by tests against most of our own apps.

If it is possible to find the state we are missing in the layout, that would likely be more palatable. Do you know if this issue reproduces in Fabric?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickGerleman I'm also pretty concerned about memory here, as it means we now have two TextViews for every Text element on screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had A/B test on various devices with different ROMs and put it on PRT environment and No any problem or bad case feedback reported.
As for Fabric mode, we don't use that architecture yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickGerleman I'm also pretty concerned about memory here, as it means we now have two TextViews for every Text element on screen.

Later, maybe after a few days, I'll have a test about peak memory difference and explain the root cause for this phenomenon which is related to Paint.nGetRunAdvance, a native method which may be modified by different ROM vendors. For recent days, I am too busy to take care about this PR in time. Sorry about that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickGerleman I'm also pretty concerned about memory here, as it means we now have two TextViews for every Text element on screen.

The memory diff is about 1.2kb when measuring with TextView.
image

Copy link
Contributor Author

@jcdhlzq jcdhlzq Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing if we can dig up the context on why Fabric chose to avoid this style of measurement, but this is a pretty major change. Usually we'd want to A/B test something like this, but that isn't really a possibility because most of our internal apps do not run Paper-specific code. This also means we receive very limited test coverage, since this code is not exercised by tests against most of our own apps.

If it is possible to find the state we are missing in the layout, that would likely be more palatable. Do you know if this issue reproduces in Fabric?

@NickGerleman Yes, it reproduces when running on the app built with newArchEnabled=true.
image
image

}
};

Expand Down