react-native Fix single line text not fully rendered

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 https://github.com/facebook/react-native/issues/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:

React Native Minimum Reproducible Example (RNTesterAppShared.js)
Android Minimum Reproducible Example (CustomTextView.java, branch)

The android minimum reproducible example:

Reproduces the react-native issue using the same implementation of ReactTextShadowNode onMeasure, which calculates the wrong text width with StaticLayout (screenshot)
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:

Dec 03 '23 06:12 fabOnReact

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 --

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: 44f9371f246932215627a7ea01fbedf5c13e3019 Branch: main

Dec 03 '23 06:12 analysis-bot

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

Dec 05 '23 10:12 facebook-github-bot

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

Jan 16 '24 02:01 fabOnReact

Thanks @dmytrorykun, could we land this PR? Thanks

Feb 28 '24 00:02 fabOnReact

Mar 05 '24 09:03 fabOnReact

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.

Mar 10 '24 08:03 fabOnReact

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

Mar 11 '24 10:03 facebook-github-bot

@fabOnReact there is a new comment in the corresponding issue https://github.com/facebook/react-native/issues/39722?fbclid=IwAR0G5MSTKkiCcsfqddRDZ-CkLgCEMKER76cBlJvLGQJIOEZa8wqSqGQ_hRE#issuecomment-1992302836

Mar 14 '24 20:03 dmytrorykun

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

May 20 '24 15:05 cipolleschi

Is this still being worked on / reviewed? Wondering if I need to use a workaround for all the text in my app or if a fix for this issue is likely to be merged before we ship.

Aug 06 '24 19:08 johnsimeroth

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

Sorry about missing this.

This is NOT related to measurement issue that i was previously working on.

@fabOnReact Please also attach Output of npx react-native info.

I saw this issues on https://github.com/Expensify/App/issues/43068 is closed and mentioned it's no longer reproducible.

Do you mind retesting your test cases https://gist.github.com/fabOnReact/6a58c713d32ce5bb9c164b6906abac12?permalink_comment_id=4956212 with latest React Native version with New Architecture Enabled ?

Aug 07 '24 18:08 realsoelynn

@realsoelynn

Do you mind retesting your test cases https://gist.github.com/fabOnReact/6a58c713d32ce5bb9c164b6906abac12?permalink_comment_id=4956212 with latest React Native version with New Architecture Enabled ?

I tested again the test case with the latest React Native main branch and new architecture enabled.

CLICK TO OPEN SOURCECODE

import * as React from 'react';
import {StyleSheet, Text, TextInput, View} from 'react-native';

export default function App() {
  const email =
    'From [email protected]  From [email protected]';
  return (
    <>
      <View style={styles.container}>
        <View style={styles.flexBrokenStyle}>
          <Text
            style={styles.parentText}
            numberOfLines={1}
          >
            {email}
          </Text>
        </View>
      </View>
    </>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    padding: 8,
    backgroundColor: 'yellow',
  },
  flexBrokenStyle: {
    flexDirection: 'row',
  },
  parentText: {
    backgroundColor: 'red',
  },
});

I added a log statement in TextLayoutManager.java and verified that we are running on the new architecture.

The width is calculated with TextLayoutManager (newArchitecture). The text does not take the full width, because TextLayoutManager calculates the wrong width.

Metro output shows Fabric is enabled.

I saw this issues on https://github.com/Expensify/App/issues/43068 is closed and mentioned it's no longer reproducible.

The PR was not created to fix Issue https://github.com/Expensify/App/issues/43068. The Expensify Issue is https://github.com/Expensify/App/issues/21219.

Please also attach Output of npx react-native info.

OUTPUT OF NPX REACT-NATIVE INFO

info Fetching system and libraries information...
System:
  OS: macOS 14.6
  CPU: (10) arm64 Apple M1 Pro
  Memory: 176.52 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.15.1
    path: ~/.nvm/versions/node/v20.15.1/bin/node
  Yarn:
    version: 1.22.21
    path: /opt/homebrew/bin/yarn
  npm:
    version: 10.7.0
    path: ~/.nvm/versions/node/v20.15.1/bin/npm
  Watchman:
    version: 2024.07.15.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.14.3
    path: /Users/fabriziobertoglio/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK: Not Found
IDEs:
  Android Studio: 2023.3 AI-233.14808.21.2331.11709847
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.12
    path: /usr/bin/javac
  Ruby:
    version: 2.7.5
    path: /Users/fabriziobertoglio/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.73.4
    wanted: 0.73.4
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: true
iOS:
  hermesEnabled: true
  newArchEnabled: true

➜  expensify git:(main) npx react-native info
info Fetching system and libraries information...
System:
  OS: macOS 14.6
  CPU: (10) arm64 Apple M1 Pro
  Memory: 145.38 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.15.1
    path: ~/.nvm/versions/node/v20.15.1/bin/node
  Yarn:
    version: 1.22.21
    path: /opt/homebrew/bin/yarn
  npm:
    version: 10.7.0
    path: ~/.nvm/versions/node/v20.15.1/bin/npm
  Watchman:
    version: 2024.07.15.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.14.3
    path: /Users/fabriziobertoglio/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK: Not Found
IDEs:
  Android Studio: 2023.3 AI-233.14808.21.2331.11709847
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.12
    path: /usr/bin/javac
  Ruby:
    version: 2.7.5
    path: /Users/fabriziobertoglio/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.73.4
    wanted: 0.73.4
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: true
iOS:
  hermesEnabled: true
  newArchEnabled: true

Aug 10 '24 10:08 fabOnReact

The problem is that the created layout renders the text with line break enabled and what we end up measuring is just the first line which does not include all the text (since the rest wrapped). IOW we are measuring the text as if numberOfLines=undefined.

Nov 03 '24 23:11 s77rt

The suggested solution causes a regression since all Texts with numberOfLines={1} will take full width

Before	After

<Text numberOfLines={1} style={{backgroundColor: 'red', alignSelf: 'flex-start', color: 'white'}}>
    1{'\n'}
</Text>

Nov 03 '24 23:11 s77rt

The easiest solution that come to mind is to compare the layout's number of lines (layout.getLineCount()) with the specified numberOfLines, if it's greater, then the text is wider than the view, set calculatedWidth = width. However this have the same regression above since we can enter \n to increase the number of lines but not necessary exceeding the view's width.

I think what we can do is take the number of lines that contain only text and stop at the first one that is just a line break (so we can differentiate between a line wrap and line break). Edit: not going with this solution exactly but somehow similar, see referenced PR below

Nov 03 '24 23:11 s77rt

An alternate solution is to render the text without line break (setLineBreakConfig) but I got the app crashed trying to work with this one.

Edit: This won't fix the problem if numberOfLines > 1

Nov 03 '24 23:11 s77rt

commit 550b0c0 landed, can we close this?

Nov 21 '24 16:11 cipolleschi

react-native react-native copied to clipboard

Fix single line text not fully rendered

Summary:

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

What is the root cause of that problem?

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

Changelog:

Test Plan:

react-native
react-native copied to clipboard