From 1d2d7cdb2825b200ac0463d565ddfd1cd524f7b0 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Fri, 28 Apr 2023 12:13:00 +0200 Subject: [PATCH 1/2] De-duplicate SetSpanOperation class --- .../views/text/ReactBaseTextShadowNode.java | 25 ---------------- .../react/views/text/SetSpanOperation.java | 29 +++++++++++++++++++ .../react/views/text/TextLayoutManager.java | 26 ----------------- .../text/TextLayoutManagerMapBuffer.java | 26 ----------------- 4 files changed, 29 insertions(+), 77 deletions(-) create mode 100644 packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/SetSpanOperation.java diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java index 4fd3a8165f4294..18d26e8c865a42 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java @@ -70,31 +70,6 @@ public abstract class ReactBaseTextShadowNode extends LayoutShadowNode { protected @Nullable ReactTextViewManagerCallback mReactTextViewManagerCallback; - private static class SetSpanOperation { - protected int start, end; - protected ReactSpan what; - - SetSpanOperation(int start, int end, ReactSpan what) { - this.start = start; - this.end = end; - this.what = what; - } - - public void execute(SpannableStringBuilder sb, int priority) { - // All spans will automatically extend to the right of the text, but not the left - except - // for spans that start at the beginning of the text. - int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE; - if (start == 0) { - spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE; - } - - spanFlags &= ~Spannable.SPAN_PRIORITY; - spanFlags |= (priority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY; - - sb.setSpan(what, start, end, spanFlags); - } - } - private static void buildSpannedFromShadowNode( ReactBaseTextShadowNode textShadowNode, SpannableStringBuilder sb, diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/SetSpanOperation.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/SetSpanOperation.java new file mode 100644 index 00000000000000..83be910a25216f --- /dev/null +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/SetSpanOperation.java @@ -0,0 +1,29 @@ +package com.facebook.react.views.text; + +import android.text.Spannable; +import android.text.SpannableStringBuilder; + +class SetSpanOperation { + protected int start, end; + protected ReactSpan what; + + SetSpanOperation(int start, int end, ReactSpan what) { + this.start = start; + this.end = end; + this.what = what; + } + + public void execute(SpannableStringBuilder sb, int priority) { + // All spans will automatically extend to the right of the text, but not the left - except + // for spans that start at the beginning of the text. + int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE; + if (start == 0) { + spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE; + } + + spanFlags &= ~Spannable.SPAN_PRIORITY; + spanFlags |= (priority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY; + + sb.setSpan(what, start, end, spanFlags); + } +} diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java index 040171ed008225..639fd1ad467fec 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java @@ -561,30 +561,4 @@ public static WritableArray measureLines( hyphenationFrequency); return FontMetricsUtil.getFontMetrics(text, layout, sTextPaintInstance, context); } - - // TODO T31905686: This class should be private - public static class SetSpanOperation { - protected int start, end; - protected ReactSpan what; - - public SetSpanOperation(int start, int end, ReactSpan what) { - this.start = start; - this.end = end; - this.what = what; - } - - public void execute(Spannable sb, int priority) { - // All spans will automatically extend to the right of the text, but not the left - except - // for spans that start at the beginning of the text. - int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE; - if (start == 0) { - spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE; - } - - spanFlags &= ~Spannable.SPAN_PRIORITY; - spanFlags |= (priority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY; - - sb.setSpan(what, start, end, spanFlags); - } - } } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java index 9216703108829a..05c5a1377cb5b2 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java @@ -588,30 +588,4 @@ public static WritableArray measureLines( hyphenationFrequency); return FontMetricsUtil.getFontMetrics(text, layout, sTextPaintInstance, context); } - - // TODO T31905686: This class should be private - public static class SetSpanOperation { - protected int start, end; - protected ReactSpan what; - - public SetSpanOperation(int start, int end, ReactSpan what) { - this.start = start; - this.end = end; - this.what = what; - } - - public void execute(Spannable sb, int priority) { - // All spans will automatically extend to the right of the text, but not the left - except - // for spans that start at the beginning of the text. - int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE; - if (start == 0) { - spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE; - } - - spanFlags &= ~Spannable.SPAN_PRIORITY; - spanFlags |= (priority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY; - - sb.setSpan(what, start, end, spanFlags); - } - } } From e1d34df6612589506490e1f4c86a6ca73442fd65 Mon Sep 17 00:00:00 2001 From: Jakub Trzebiatowski Date: Sun, 21 May 2023 19:44:39 +0200 Subject: [PATCH 2/2] Ensure not to exceed the Spanned priority limit ...as it led to totally random results for deep text node trees. Instead, assign high priority values to SetSpanOperations with the highest priority (which represent the nodes high in the tree), but clamp the priority 0 for SetSpanOperations with the lowest priority (which represent nodes closer to the leaves). This isn't everything that could be done in this area, but this is a relatively non-invasive change which fixes an obvious bitwise overflow bug. --- .../views/text/ReactBaseTextShadowNode.java | 10 +++---- .../react/views/text/SetSpanOperation.java | 28 +++++++++++++++++-- .../react/views/text/TextLayoutManager.java | 10 +++---- .../text/TextLayoutManagerMapBuffer.java | 10 +++---- 4 files changed, 41 insertions(+), 17 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java index 18d26e8c865a42..6bb75fb5b54b7e 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java @@ -256,8 +256,9 @@ protected Spannable spannedFromShadowNode( // While setting the Spans on the final text, we also check whether any of them are inline views // or images. - int priority = 0; - for (SetSpanOperation op : ops) { + for (int priorityIndex = 0; priorityIndex < ops.size(); priorityIndex++) { + final SetSpanOperation op = ops.get(ops.size() - priorityIndex - 1); + boolean isInlineImage = op.what instanceof TextInlineImageSpan; if (isInlineImage || op.what instanceof TextInlineViewPlaceholderSpan) { int height; @@ -284,9 +285,8 @@ protected Spannable spannedFromShadowNode( } // Actual order of calling {@code execute} does NOT matter, - // but the {@code priority} DOES matter. - op.execute(sb, priority); - priority++; + // but the {@code priorityIndex} DOES matter. + op.execute(sb, priorityIndex); } textShadowNode.mTextAttributes.setHeightOfTallestInlineViewOrImage( diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/SetSpanOperation.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/SetSpanOperation.java index 83be910a25216f..8b5ad7fd8e6292 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/SetSpanOperation.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/SetSpanOperation.java @@ -2,8 +2,14 @@ import android.text.Spannable; import android.text.SpannableStringBuilder; +import android.text.Spanned; + +import com.facebook.common.logging.FLog; class SetSpanOperation { + private static final String TAG = "SetSpanOperation"; + static final int SPAN_MAX_PRIORITY = Spanned.SPAN_PRIORITY >> Spanned.SPAN_PRIORITY_SHIFT; + protected int start, end; protected ReactSpan what; @@ -13,7 +19,14 @@ class SetSpanOperation { this.what = what; } - public void execute(SpannableStringBuilder sb, int priority) { + /** + * @param sb builder + * @param priorityIndex index of this operation in the topological sorting which puts operations + * with higher priority before operations with lower priority. + */ + public void execute(SpannableStringBuilder sb, int priorityIndex) { + assert priorityIndex >= 0; + // All spans will automatically extend to the right of the text, but not the left - except // for spans that start at the beginning of the text. int spanFlags = Spannable.SPAN_EXCLUSIVE_INCLUSIVE; @@ -21,8 +34,19 @@ public void execute(SpannableStringBuilder sb, int priority) { spanFlags = Spannable.SPAN_INCLUSIVE_INCLUSIVE; } + // Calculate priority, assigning the highest values to operations with the highest priority + final int priority = SPAN_MAX_PRIORITY - priorityIndex; + + if (priority < 0) { + FLog.w(TAG, "Text tree size exceeded the limit, styling may become unpredictable"); + } + + // If the computed priority doesn't fit in the flags, clamp it. The effect might not be correct + // in 100% of cases, but doing nothing (as we did in the past) leads to totally random results. + final int effectivePriority = Math.max(priority, 0); + spanFlags &= ~Spannable.SPAN_PRIORITY; - spanFlags |= (priority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY; + spanFlags |= (effectivePriority << Spannable.SPAN_PRIORITY_SHIFT) & Spannable.SPAN_PRIORITY; sb.setSpan(what, start, end, spanFlags); } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java index 639fd1ad467fec..25bfe983255cc7 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java @@ -223,12 +223,12 @@ private static Spannable createSpannableFromAttributedString( // TODO T31905686: add support for inline Images // While setting the Spans on the final text, we also check whether any of them are images. - int priority = 0; - for (SetSpanOperation op : ops) { + for (int priorityIndex = 0; priorityIndex < ops.size(); ++priorityIndex) { + final SetSpanOperation op = ops.get(ops.size() - priorityIndex - 1); + // Actual order of calling {@code execute} does NOT matter, - // but the {@code priority} DOES matter. - op.execute(sb, priority); - priority++; + // but the {@code priorityIndex} DOES matter. + op.execute(sb, priorityIndex); } if (reactTextViewManagerCallback != null) { diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java index 05c5a1377cb5b2..71703755597285 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManagerMapBuffer.java @@ -264,12 +264,12 @@ private static Spannable createSpannableFromAttributedString( // TODO T31905686: add support for inline Images // While setting the Spans on the final text, we also check whether any of them are images. - int priority = 0; - for (SetSpanOperation op : ops) { + for (int priorityIndex = 0; priorityIndex < ops.size(); ++priorityIndex) { + final SetSpanOperation op = ops.get(ops.size() - priorityIndex - 1); + // Actual order of calling {@code execute} does NOT matter, - // but the {@code priority} DOES matter. - op.execute(sb, priority); - priority++; + // but the {@code priorityIndex} DOES matter. + op.execute(sb, priorityIndex); } if (reactTextViewManagerCallback != null) {