fix: apply font size to ReactTextView to fix ellipsis cut

[BeeMargarida]

Summary:

This PR aims to fix #36350. In certain cases, when the text is cut due to numberOfLines, the ellipsis appear cut. This is actually an Android bug, which was reported on their side here.

This PR contains a workaround for it by applying the text size to the TextView directly instead of just the Spannable inside it. This solves all problems and it seems like it does not cause any regressions.

Changelog:

[ANDROID] [FIXED] - Fix ellipsis being cut on certain font sizes

Test Plan:

One piece of code where the problem could be replicated would be the one below, running on an Pixel 3A emulator.

<Text style={{padding: 27, fontSize: 30}} numberOfLines={1}>
   This text will be cut-off strangely in Android
</Text>

RN-tester of the problem:

Before	With the fix

RN-Tester comparison:

Before	With the fix
main.mp4	fix.mp4

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,719,508	-13,081
android	hermes	armeabi-v7a	8,030,246	-13,686
android	hermes	x86	9,207,090	-14,853
android	hermes	x86_64	9,060,466	-14,445
android	jsc	arm64-v8a	9,284,068	-13,721
android	jsc	armeabi-v7a	8,472,245	-14,312
android	jsc	x86	9,343,039	-15,488
android	jsc	x86_64	9,600,037	-15,076

Base commit: a310217
Branch: main

[javache]

Could you add the repro for this issue to RNTester as well and show before/after in your video?

[BeeMargarida]

@javache Updated the PR description with the before/after of the repro example, also added the repro to rn-tester

[NickGerleman]

Why are we using SP here?

[BeeMargarida]

I ended up basing this code in the snippet from TextAttributes.java.

Should it be removed?

[NickGerleman]

It looks like the code there is using it in conjunction with allowFontScaling to find font size. Not anything with adjustFontSizeToFit.

Can we reuse getEffectiveFontSize()? E.g. have the view manager or ShadowNode call setTextSize()?

[BeeMargarida]

Oh, you're right!

Can we reuse getEffectiveFontSize()?

Not directly because ReactTextView does not use TextAttributes. We could extract that logic and use a static method, but it would require passing mMaxFontSizeMultiplier and mAllowFontScaling (or just use defaults), which I'm not sure it would be the best, I'm not really sure what's the implications of this.

We could just remove the adjust logic and just use toPixelFromDIP, would it be best?

[NickGerleman]

I think it should be technically safe to set base test size on the container. We do that now with TextInput now as well for different reasons. Though ideally we'd fix this so that everything lays out consistently.

I tried to grep through some of the code for this at https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/widget/TextView.java out of curiosity. I wonder, do we need to be calling setEllipsize on the StaticLayout.Builder and/or other related APIs in RN?

[BeeMargarida]

I wonder, do we need to be calling setEllipsize on the StaticLayout.Builder and/or other related APIs in RN?

You mean, should we force the ellipsize call on RN size? I don't think it would make any difference. When I opened the bug on Android tracker - https://issuetracker.google.com/issues/278044456 - I used a similar setup of using a Spannable and calling the setEllipsize on the TextView. It seems like the total size of the spannable is not being taken into consideration when the ellipsis are being applied. I didn't go any deeper than that by debugging the Android codebase, so I'm not sure where the problem is.

[NickGerleman]

Ah, I missed that the cutoff happened in an out of the box TextView set to ellipsize with a Spannable.

I think the approach of setting metrics on the TextView itself makes sense then. Do we need to do the same for other metric affecting properties than size?

If you look at ReactEditText, I needed to do something similar for many style attributes that were part of the Spannable, but for different reasons.

[BeeMargarida]

I think the approach of setting metrics on the TextView itself makes sense then. Do we need to do the same for other metric affecting properties than size?

Perhaps it makes sense to make it similar to what was made in ReactEditText and apply attributes like fontSize, letterSpacing, fontFamily, etc, to the TextView instead of the spans. It's possible that letterSpacing can cause the same issue.

Update: checked and indeed it causes the same problem with the ellipsis when some letterSpacing values are applied. If letterSpacing is applied to the TextView, the problem is solved. So, it seems like the solution in this PR is incomplete. The question is what properties should be applied to the TextView: all the same ones that are now being applied to the ReactEditText?

If you look at ReactEditText, I needed to do something similar for many style attributes that were part of the Spannable, but for different reasons.

Yeah, I noticed that, I'm also investigating a bug related to line height in ReactEditText. One question that is not related to this bug, if you don't mind me asking: I noticed you are not removing the line height from the spans and applying to the EditText component (with lineSpacingExtra). Any specific reason for that?

[BeeMargarida]

I applied the fix for letter spacing, basically a similar solution to what's currently in ReactEditText.

[facebook-github-bot]

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@NickGerleman merged this pull request in 6d24ee1.