[ANDROID] [FIXED] Fixed random styling for text nodes with many children #36656
Conversation
|
Hi @cubuspl42! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
This branch is based on 0.71.4 because:
That having said, the bug is still present on We can discuss rebasing this these changes on |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
8 similar comments
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
I am currently on vacation but can give some tentative feedback.
|
|
@NickGerleman Thanks for the early review! That helps a lot. Ad. 2. I know about the ongoing Fabric thing but as a downstream React Native 0.71.4 user, that's a song of the future for me 😀 I'll figure that out and will try to apply the changes there too. But I think I would need some help with building the Ad 3. That sounds like a great thing to check and I'd like to check that. But, to be honest, I don't fully understand how that translates to React components terms. Would you be able to share one example of a Ad 4. You're right, that should be possible and sounds good. By the way, do you know any way to calm the bot down? |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
8 similar comments
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
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 I started from user messages containing hundreds of markdown tags, went through debugging the HTML library, then entered React Native, then I think I reached my limit. 😅 I don't know if this sorting can be relied on. Does it happen on all Android versions supported by React Native? Is it guaranteed that it won't change (again?) in the future? It would be great to have a quote from the API documentation that at least implies that the order actually matters. Otherwise, I don't know. I'm not sure if dropping priorities altogether is a good idea. |
|
Hard to check unfortunately. I think we can do a hybrid approach though. |
|
@NickGerleman What do you think about comments by @Szymon20000? |
cubuspl42
changed the title
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
...ive/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java
Show resolved
Hide resolved
|
@NickGerleman merged this pull request in dcb4eb0. |
|
@NickGerleman Thanks for merging this! When can I expect this to be included in a release? Does this have any chance of being included in 0.72.x, or will I have to wait till 0.73? |
|
You can make a pick request here: reactwg/react-native-releases#54 |
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.  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.  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)
Attempting to fix the issue #33418
Changelog
[ANDROID] Fixed inconsistent styling for text nodes with many children
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 for text nodes with more than 85 children, just like that.
All of this text should be blue.
The root cause is that code (on Android) assumes it can assign as many
Spannablespan 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
ReactAndroidshared 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.