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.

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 single line text not fully rendered #41770

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Dec 3, 2023

Summary:

Please re-state the problem that we are trying to solve in this issue.

ReactTextShadowNode#measure calculates the wrong width for single line Text that has an ellipsis (fixes #39722).

What is the root cause of that problem?

React Native sets the wrong width in the ReactTextShadowNode onMeasure function for single line Text, demonstrated with:

The android minimum reproducible example:

  1. Reproduces the react-native issue using the same implementation of ReactTextShadowNode onMeasure, which calculates the wrong text width with StaticLayout (screenshot)
  2. Demonstrates that the default TextView implementation of onMeasure does not have this issue (screenshot) (the issue does not reproduce when commenting the implementation of onMeasure ). It is not the expected/intended behavior on Android.

What changes do you think we should make in order to solve the problem?

Set the correct width for single-line Text in the measure callback.

Changelog:

[ANDROID] [FIXED] - Fix single line text not fully rendered

Test Plan:

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 3, 2023
@analysis-bot
Copy link

analysis-bot commented Dec 3, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,558,619 +16,062
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,928,748 +16,192
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 44f9371
Branch: main

@fabOnReact
Copy link
Contributor Author

thanks a lot @dmytrorykun. When you do plan to merge this PR?

@fabOnReact
Copy link
Contributor Author

Thanks @dmytrorykun, could we land this PR? Thanks

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Forwarding an internal comment:

  • The premise that we need to use the simpler BoringLayout to get the max width text could take without breaking seems strange. StaticLayout has options to not line break (maybe we should be setting that for single line instead?)
  • This will leave the bug when there is a unicode, RTL, or other complicated character.
  • This code is duplicated into Fabric (TextLayoutManager.java and now some more variants), and this code does not change anything there. If has same issue, we introduce new Fabric regression to Paper. If it isn't there, it has a different solution probably.

Comment on lines 201 to 205
// StaticLayout#getLineWidth does not work with single-line text.
boolean overrideTextBreakStrategySingleLine =
boring == null
? false
: mNumberOfLines == 1 && !mAdjustsFontSizeToFit && boring.width > width;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about

Suggested change
// StaticLayout#getLineWidth does not work with single-line text.
boolean overrideTextBreakStrategySingleLine =
boring == null
? false
: mNumberOfLines == 1 && !mAdjustsFontSizeToFit && boring.width > width;
// StaticLayout#getLineWidth does not work with single-line text.
boolean overrideTextBreakStrategySingleLine =
boring != null && mNumberOfLines == 1 && !mAdjustsFontSizeToFit && boring.width > width;

and we avoid the ternary?

@fabOnReact fabOnReact changed the title Fix single line text not fully rendered on old architecture Fix single line text not fully rendered Mar 10, 2024
@fabOnReact
Copy link
Contributor Author

fabOnReact commented Mar 10, 2024

Thanks for the code review.

  • The premise that we need to use the simpler BoringLayout to get the max width text could take without breaking seems strange. StaticLayout has options to not line break (maybe we should be setting that for single line instead?)

I updated the solution to use StaticLayout instead of BoringLayout.

  • This will leave the bug when there is a unicode, RTL, or other complicated character.

The new solution supports not boring text, emoji, RTL, etc (test cases).

  • This code is duplicated into Fabric (TextLayoutManager.java and now some more variants), and this code does not change anything there. If has same issue, we introduce new Fabric regression to Paper. If it isn't there, it has a different solution probably.

I added the Fabric implementation and included tests of the functionalities on Fabric and Paper.

@fabOnReact fabOnReact marked this pull request as ready for review March 10, 2024 08:19
@facebook-github-bot
Copy link
Contributor

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

@dmytrorykun
Copy link
Contributor

@fabOnReact there is a new comment in the corresponding issue #39722

@cipolleschi
Copy link
Contributor

@realsoelynn this might be related to the measurement issues you are working on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

numberOfLines={1} and alignSelf: 'flex-start' when we set this text is break middle of in Android
5 participants