Skip to content

Commit 73f4a78

Browse files
cubuspl42kelset
authored andcommitted
Fixed random styling for text nodes with many children (#36656)
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
1 parent dfc64d5 commit 73f4a78

File tree

4 files changed

+67
-92
lines changed

4 files changed

+67
-92
lines changed

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java

Lines changed: 5 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -69,31 +69,6 @@ public abstract class ReactBaseTextShadowNode extends LayoutShadowNode {
6969

7070
protected @Nullable ReactTextViewManagerCallback mReactTextViewManagerCallback;
7171

72-
private static class SetSpanOperation {
73-
protected int start, end;
74-
protected ReactSpan what;
75-
76-
SetSpanOperation(int start, int end, ReactSpan what) {
77-
this.start = start;
78-
this.end = end;
79-
this.what = what;
80-
}
81-
82-
public void execute(SpannableStringBuilder sb, int priority) {
83-
// All spans will automatically extend to the right of the text, but not the left - except
84-
// for spans that start at the beginning of the text.
85-
int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE;
86-
if (start == 0) {
87-
spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE;
88-
}
89-
90-
spanFlags &= ~Spannable.SPAN_PRIORITY;
91-
spanFlags |= (priority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY;
92-
93-
sb.setSpan(what, start, end, spanFlags);
94-
}
95-
}
96-
9772
private static void buildSpannedFromShadowNode(
9873
ReactBaseTextShadowNode textShadowNode,
9974
SpannableStringBuilder sb,
@@ -276,8 +251,9 @@ protected Spannable spannedFromShadowNode(
276251

277252
// While setting the Spans on the final text, we also check whether any of them are inline views
278253
// or images.
279-
int priority = 0;
280-
for (SetSpanOperation op : ops) {
254+
for (int priorityIndex = 0; priorityIndex < ops.size(); priorityIndex++) {
255+
final SetSpanOperation op = ops.get(ops.size() - priorityIndex - 1);
256+
281257
boolean isInlineImage = op.what instanceof TextInlineImageSpan;
282258
if (isInlineImage || op.what instanceof TextInlineViewPlaceholderSpan) {
283259
int height;
@@ -304,9 +280,8 @@ protected Spannable spannedFromShadowNode(
304280
}
305281

306282
// Actual order of calling {@code execute} does NOT matter,
307-
// but the {@code priority} DOES matter.
308-
op.execute(sb, priority);
309-
priority++;
283+
// but the {@code priorityIndex} DOES matter.
284+
op.execute(sb, priorityIndex);
310285
}
311286

312287
textShadowNode.mTextAttributes.setHeightOfTallestInlineViewOrImage(
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
package com.facebook.react.views.text;
2+
3+
import android.text.Spannable;
4+
import android.text.SpannableStringBuilder;
5+
import android.text.Spanned;
6+
import com.facebook.common.logging.FLog;
7+
8+
class SetSpanOperation {
9+
private static final String TAG = "SetSpanOperation";
10+
static final int SPAN_MAX_PRIORITY = Spanned.SPAN_PRIORITY >> Spanned.SPAN_PRIORITY_SHIFT;
11+
12+
protected int start, end;
13+
protected ReactSpan what;
14+
15+
SetSpanOperation(int start, int end, ReactSpan what) {
16+
this.start = start;
17+
this.end = end;
18+
this.what = what;
19+
}
20+
21+
/**
22+
* @param sb builder
23+
* @param priorityIndex index of this operation in the topological sorting which puts operations
24+
* with higher priority before operations with lower priority.
25+
*/
26+
public void execute(SpannableStringBuilder sb, int priorityIndex) {
27+
assert priorityIndex >= 0;
28+
29+
// All spans will automatically extend to the right of the text, but not the left - except
30+
// for spans that start at the beginning of the text.
31+
int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE;
32+
if (start == 0) {
33+
spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE;
34+
}
35+
36+
// Calculate priority, assigning the highest values to operations with the highest priority
37+
final int priority = SPAN_MAX_PRIORITY - priorityIndex;
38+
39+
if (priority < 0) {
40+
FLog.w(TAG, "Text tree size exceeded the limit, styling may become unpredictable");
41+
}
42+
43+
// If the computed priority doesn't fit in the flags, clamp it. The effect might not be correct
44+
// in 100% of cases, but doing nothing (as we did in the past) leads to totally random results.
45+
final int effectivePriority = Math.max(priority, 0);
46+
47+
spanFlags &= ~Spannable.SPAN_PRIORITY;
48+
spanFlags |= (effectivePriority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY;
49+
50+
sb.setSpan(what, start, end, spanFlags);
51+
}
52+
}

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

Lines changed: 5 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -213,12 +213,12 @@ private static Spannable createSpannableFromAttributedString(
213213

214214
// TODO T31905686: add support for inline Images
215215
// While setting the Spans on the final text, we also check whether any of them are images.
216-
int priority = 0;
217-
for (SetSpanOperation op : ops) {
216+
for (int priorityIndex = 0; priorityIndex < ops.size(); ++priorityIndex) {
217+
final SetSpanOperation op = ops.get(ops.size() - priorityIndex - 1);
218+
218219
// Actual order of calling {@code execute} does NOT matter,
219-
// but the {@code priority} DOES matter.
220-
op.execute(sb, priority);
221-
priority++;
220+
// but the {@code priorityIndex} DOES matter.
221+
op.execute(sb, priorityIndex);
222222
}
223223

224224
if (reactTextViewManagerCallback != null) {
@@ -551,30 +551,4 @@ public static WritableArray measureLines(
551551
hyphenationFrequency);
552552
return FontMetricsUtil.getFontMetrics(text, layout, sTextPaintInstance, context);
553553
}
554-
555-
// TODO T31905686: This class should be private
556-
public static class SetSpanOperation {
557-
protected int start, end;
558-
protected ReactSpan what;
559-
560-
public SetSpanOperation(int start, int end, ReactSpan what) {
561-
this.start = start;
562-
this.end = end;
563-
this.what = what;
564-
}
565-
566-
public void execute(Spannable sb, int priority) {
567-
// All spans will automatically extend to the right of the text, but not the left - except
568-
// for spans that start at the beginning of the text.
569-
int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE;
570-
if (start == 0) {
571-
spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE;
572-
}
573-
574-
spanFlags &= ~Spannable.SPAN_PRIORITY;
575-
spanFlags |= (priority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY;
576-
577-
sb.setSpan(what, start, end, spanFlags);
578-
}
579-
}
580554
}

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java

Lines changed: 5 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -227,12 +227,12 @@ private static Spannable createSpannableFromAttributedString(
227227

228228
// TODO T31905686: add support for inline Images
229229
// While setting the Spans on the final text, we also check whether any of them are images.
230-
int priority = 0;
231-
for (SetSpanOperation op : ops) {
230+
for (int priorityIndex = 0; priorityIndex < ops.size(); ++priorityIndex) {
231+
final SetSpanOperation op = ops.get(ops.size() - priorityIndex - 1);
232+
232233
// Actual order of calling {@code execute} does NOT matter,
233-
// but the {@code priority} DOES matter.
234-
op.execute(sb, priority);
235-
priority++;
234+
// but the {@code priorityIndex} DOES matter.
235+
op.execute(sb, priorityIndex);
236236
}
237237

238238
if (reactTextViewManagerCallback != null) {
@@ -570,30 +570,4 @@ public static WritableArray measureLines(
570570
hyphenationFrequency);
571571
return FontMetricsUtil.getFontMetrics(text, layout, sTextPaintInstance, context);
572572
}
573-
574-
// TODO T31905686: This class should be private
575-
public static class SetSpanOperation {
576-
protected int start, end;
577-
protected ReactSpan what;
578-
579-
public SetSpanOperation(int start, int end, ReactSpan what) {
580-
this.start = start;
581-
this.end = end;
582-
this.what = what;
583-
}
584-
585-
public void execute(Spannable sb, int priority) {
586-
// All spans will automatically extend to the right of the text, but not the left - except
587-
// for spans that start at the beginning of the text.
588-
int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE;
589-
if (start == 0) {
590-
spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE;
591-
}
592-
593-
spanFlags &= ~Spannable.SPAN_PRIORITY;
594-
spanFlags |= (priority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY;
595-
596-
sb.setSpan(what, start, end, spanFlags);
597-
}
598-
}
599573
}

0 commit comments

Comments
 (0)