-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Comments
Also occurs for |
same problem here |
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>
); 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. |
I checked the pr and it makes a lot of sense. 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 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)? |
@Szymon20000 Thanks for your thoughts. Doesn't this belong to the PR, though? |
It does. A bit tired today! |
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
Looks like PR #36656 has been merged 🎉. Thank you @cubuspl42 🙏. @artemiswkearney Thanks, for reporting this 🙏 If you think this issue is not resolved or if any other concerns, please let us know.👍 |
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
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)
Description
On Android devices, large
Text
components will sometimes fail to apply thefontSize
property of nestedText
component's styles, resulting in text rendering with the parent's font size.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 fewText
s out of every group of some larger number.)Version
0.67.3 (originally observed on 0.64.1)
Output of
npx react-native info
Steps to reproduce
Text
components, each with their ownstyle
props includingfontSize
values, inside aText
component with a differentfontSize
Snack, code example, screenshot, or link to a repository
https://snack.expo.dev/IbMg-oQqi
Measures the height of various
Text
s usingonLayout
events, then computes how far off the height of a large nestedText
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 withUSE_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, justconsole.log
s about it.The text was updated successfully, but these errors were encountered: