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

[Android] fontSize styles on nested Text elements aren't always applied #33418

Closed
artemiswkearney opened this issue Mar 14, 2022 · 7 comments
Closed
Labels
Needs: Triage 🔍 Platform: Android Android applications. Resolution: Fixed A PR that fixes this issue has been merged.

Comments

@artemiswkearney
Copy link

Description

On Android devices, large Text components will sometimes fail to apply the fontSize property of nested Text component's styles, resulting in text rendering with the parent's font size.
image
These lines of text all have the same style, but render at different sizes.

(Haven't tested whether this applies to things besides fontSize. When scrolling through the text, the bug seems periodic, happening to the last few Texts out of every group of some larger number.)

Version

0.67.3 (originally observed on 0.64.1)

Output of npx react-native info

info Fetching system and libraries information...
System:
    OS: Windows 10 10.0.22000
    CPU: (12) x64 Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
    Memory: 2.65 GB / 15.76 GB
  Binaries:
    Node: 17.7.1 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.5 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 8.5.2 - C:\Program Files\nodejs\npm.CMD
    Watchman: Not Found
  SDKs:
    Android SDK: Not Found
    Windows SDK: Not Found
  IDEs:
    Android Studio: Not Found
    Visual Studio: 16.10.31402.337 (Visual Studio Community 2019), 15.9.28307.1033 (Visual Studio Community 2017)
  Languages:
    Java: 1.8.0_282 - C:\Program Files\OpenJDK\openjdk-8u282-b08\bin\javac.EXE
  npmPackages:
    @react-native-community/cli: Not Found
    react: 17.0.2 => 17.0.2
    react-native: 0.67.3 => 0.67.3
    react-native-windows: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

Steps to reproduce

  1. Render a large number of Text components, each with their own style props including fontSize values, inside a Text component with a different fontSize
  2. Deploy your app to an Android device
  3. Observe that some of the text is rendered with the wrong font size

Snack, code example, screenshot, or link to a repository

https://snack.expo.dev/IbMg-oQqi
Measures the height of various Texts using onLayout events, then computes how far off the height of a large nested Text is from its expected height. Constants at the top of the file:

  • USE_DIFFERENT_HEIGHTS: If true, the newlines in between phrases will be rendered at a smaller font size. Seems to make the bug happen at lower phrase counts, but isn't required.
  • NUM_PHRASES: The number of phrases to render. On my test phone and on the Snack Android device, 52 seems to be the minimum number to reproduce with USE_DIFFERENT_HEIGHTS off, 37 with it on. Use higher values like 200 to see the periodic effect mentioned earlier.
  • THROW_IF_WRONG_HEIGHT: If true, throws an error if the height is too far from the expected value. Otherwise, just console.logs about it.
@artemiswkearney
Copy link
Author

Also occurs for lineHeight.

@fai9al7dad
Copy link

same problem here

@cubuspl42
Copy link
Contributor

I created a MCVE for this issue. It's a simple app that contains multiple nodes with a simple text "abc".

const text = "abc";

function App(): JSX.Element {
  return (
    <SafeAreaView>
      {section(5)}
      {section(10)}
      {section(15)}
      {section(20)}
      {section(84)}
      {section(85)}
      {section(86)}
      {section(87)}
      {section(88)}
    </SafeAreaView>
  );
}

const section = (size: number) => (
  <View>
    <Text>Section with {size * text.length} characters</Text>
    <Text style={{
      "color": "red",
    }}>
      {[...Array(size).keys()].map(innerText)}
    </Text>
  </View>
);

const innerText = (index: number) => (
  <Text
    key={index}
    style={{
      "color": "blue",
      "fontWeight": "bold"
    }}
  >
    {text}
  </Text>
);

image

We can see that the styling starts behaving unexpectedly when the whole text becomes longer than 255 characters.

I started to investigate this issue in React Native's source, and that number is by no means accidental. I'll post more results soon.

@Szymon20000
Copy link

I checked the pr and it makes a lot of sense.
Also tested it in RNTester on paper arch and wasn't able to spot any difference.
As I'm not the author of the original code so I'm not able to tell what was the reason for using priorities this way.
However, I guess the purpose was to make sure that children operations are executed after parents.
I don't know any example of span operations that would be broken by this approach.

I'm not sure though if we really need to use priority. From what I see spans are sorted first by priority then by insertion order. I guess it wasn't the case when the code was created.

aosp-mirror/platform_frameworks_base@fa05ba0
Screenshot 2023-04-03 at 17 51 52

Maybe we can set the same priority for span operations and insertion order will handle the rest (we also need to call Collections.reverse(ops) as we want to apply parents first)?

@cubuspl42
Copy link
Contributor

@Szymon20000 Thanks for your thoughts. Doesn't this belong to the PR, though?

@Szymon20000
Copy link

It does. A bit tired today!
Copied!

@cortinico cortinico added the Resolution: PR Submitted A pull request with a fix has been provided. label May 25, 2023
facebook-github-bot pushed a commit that referenced this issue May 26, 2023
Summary:
Attempting to fix the issue #33418

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] Fixed inconsistent styling for text nodes with many children

Pull Request resolved: #36656

Test Plan:
No test plan yet. I'd like to ask for help with creating one.

##

Putting template aside, I'd like to ask for a review of the approach I'm suggesting.

React Native as-is (at least in some cases) [messes up the styles](#33418 (comment)) for text nodes with more than 85 children, just like that.

![image](https://user-images.githubusercontent.com/2590174/227981778-7ef6e7e1-00ee-4f67-bcf1-d452183ea33d.png)

All of this text should be blue.

The root cause is that code (on Android) assumes it can assign as many `Spannable` span priority values as we'd like, while in reality, it has to be packed in an 8-bit-long section of the flags mask. So 255 possible values. In the scenario I produced, React generates three spans per text node, and for 85 text nodes, it sums up to 255. For each span, a new priority value is assigned.

As I understand it, we don't need that many priority values. If I'm not mistaken, these priorities are crucial only for ensuring that nested styles have precedence over the outer ones. I'm proposing to calculate the priority value "vertically" (based on the node's depth in the tree) not "horizontally" (based on its position).

It would be awesome if some core engineer familiar with `ReactAndroid` shared their experience with this module, especially if there are any known cases when we _know_ that we'd like to create overlapping spans fighting over the same aspects of the style.

Reviewed By: cortinico

Differential Revision: D46094200

Pulled By: NickGerleman

fbshipit-source-id: aae195c71684fe50469a1ee1bd30625cbfc3622f
@Pranav-yadav Pranav-yadav added Resolution: Fixed A PR that fixes this issue has been merged. and removed Resolution: PR Submitted A pull request with a fix has been provided. labels May 26, 2023
@Pranav-yadav
Copy link
Contributor

Looks like PR #36656 has been merged 🎉. Thank you @cubuspl42 🙏.


@artemiswkearney Thanks, for reporting this 🙏
Closing this issue as PR #36656 w/ fix has been merged.

If you think this issue is not resolved or if any other concerns, please let us know.👍

kelset pushed a commit that referenced this issue Jun 13, 2023
Summary:
Attempting to fix the issue #33418

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] Fixed inconsistent styling for text nodes with many children

Pull Request resolved: #36656

Test Plan:
No test plan yet. I'd like to ask for help with creating one.

##

Putting template aside, I'd like to ask for a review of the approach I'm suggesting.

React Native as-is (at least in some cases) [messes up the styles](#33418 (comment)) for text nodes with more than 85 children, just like that.

![image](https://user-images.githubusercontent.com/2590174/227981778-7ef6e7e1-00ee-4f67-bcf1-d452183ea33d.png)

All of this text should be blue.

The root cause is that code (on Android) assumes it can assign as many `Spannable` span priority values as we'd like, while in reality, it has to be packed in an 8-bit-long section of the flags mask. So 255 possible values. In the scenario I produced, React generates three spans per text node, and for 85 text nodes, it sums up to 255. For each span, a new priority value is assigned.

As I understand it, we don't need that many priority values. If I'm not mistaken, these priorities are crucial only for ensuring that nested styles have precedence over the outer ones. I'm proposing to calculate the priority value "vertically" (based on the node's depth in the tree) not "horizontally" (based on its position).

It would be awesome if some core engineer familiar with `ReactAndroid` shared their experience with this module, especially if there are any known cases when we _know_ that we'd like to create overlapping spans fighting over the same aspects of the style.

Reviewed By: cortinico

Differential Revision: D46094200

Pulled By: NickGerleman

fbshipit-source-id: aae195c71684fe50469a1ee1bd30625cbfc3622f
Kudo pushed a commit to expo/react-native that referenced this issue Jun 15, 2023
Summary:
Attempting to fix the issue facebook#33418

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] Fixed inconsistent styling for text nodes with many children

Pull Request resolved: facebook#36656

Test Plan:
No test plan yet. I'd like to ask for help with creating one.

##

Putting template aside, I'd like to ask for a review of the approach I'm suggesting.

React Native as-is (at least in some cases) [messes up the styles](facebook#33418 (comment)) for text nodes with more than 85 children, just like that.

![image](https://user-images.githubusercontent.com/2590174/227981778-7ef6e7e1-00ee-4f67-bcf1-d452183ea33d.png)

All of this text should be blue.

The root cause is that code (on Android) assumes it can assign as many `Spannable` span priority values as we'd like, while in reality, it has to be packed in an 8-bit-long section of the flags mask. So 255 possible values. In the scenario I produced, React generates three spans per text node, and for 85 text nodes, it sums up to 255. For each span, a new priority value is assigned.

As I understand it, we don't need that many priority values. If I'm not mistaken, these priorities are crucial only for ensuring that nested styles have precedence over the outer ones. I'm proposing to calculate the priority value "vertically" (based on the node's depth in the tree) not "horizontally" (based on its position).

It would be awesome if some core engineer familiar with `ReactAndroid` shared their experience with this module, especially if there are any known cases when we _know_ that we'd like to create overlapping spans fighting over the same aspects of the style.

Reviewed By: cortinico

Differential Revision: D46094200

Pulled By: NickGerleman

fbshipit-source-id: aae195c71684fe50469a1ee1bd30625cbfc3622f
(cherry picked from commit 73f4a78)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Triage 🔍 Platform: Android Android applications. Resolution: Fixed A PR that fixes this issue has been merged.
Projects
None yet
Development

No branches or pull requests

7 participants