From e3f4a7ba0975741231c2e1e0b16ed05887df0b94 Mon Sep 17 00:00:00 2001 From: David Vacca Date: Wed, 13 May 2020 00:39:56 -0700 Subject: [PATCH] Fix precision of TextInlineViews in Android Summary: TextInlineViews in Android was incorrectly converting values to from float to int, this produced to loose precision and to render incomplete texts in some components. This diff changed the types from int to float, avoiding loose precision. The impact of this bug is not that high because in the conversion to int we were using Math.ceil(), which was already rounding the result to the next pixel. changeLog: [Android][Fixed] Fix precision of TextInlineViews in Fabric Android Reviewed By: JoshuaGross, shergin Differential Revision: D21541159 fbshipit-source-id: 4741ab96964c35af1c1b7d3e821e505ecef2efce --- .../com/facebook/react/fabric/FabricUIManager.java | 2 +- .../react/fabric/mounting/MountingManager.java | 2 +- .../java/com/facebook/react/uimanager/ViewManager.java | 2 +- .../react/views/slider/ReactSliderManager.java | 2 +- .../react/views/switchview/ReactSwitchManager.java | 2 +- .../react/views/text/ReactTextViewManager.java | 2 +- .../facebook/react/views/text/TextLayoutManager.java | 7 ++++--- ReactCommon/fabric/attributedstring/conversions.h | 4 ++-- .../platform/android/TextLayoutManager.cpp | 10 +++++----- 9 files changed, 17 insertions(+), 16 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index e5239c8db98..d23cfcef3b1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -464,7 +464,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { float maxWidth, float minHeight, float maxHeight, - @Nullable int[] attachmentsPositions) { + @Nullable float[] attachmentsPositions) { ReactContext context = rootTag < 0 ? mReactApplicationContext : mReactContextForRootTag.get(rootTag); return mMountingManager.measure( diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index 79966769fd7..6d249b1fd08 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -519,7 +519,7 @@ public class MountingManager { @NonNull YogaMeasureMode widthMode, float height, @NonNull YogaMeasureMode heightMode, - @Nullable int[] attachmentsPositions) { + @Nullable float[] attachmentsPositions) { return mViewManagerRegistry .get(componentName) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java index 0447427d3f0..e912d83f3f1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java @@ -318,7 +318,7 @@ public abstract class ViewManager YogaMeasureMode widthMode, float height, YogaMeasureMode heightMode, - @Nullable int[] attachmentsPositions) { + @Nullable float[] attachmentsPositions) { return 0; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/slider/ReactSliderManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/slider/ReactSliderManager.java index 2bab0c05320..1cc148fe0d2 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/slider/ReactSliderManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/slider/ReactSliderManager.java @@ -259,7 +259,7 @@ public class ReactSliderManager extends SimpleViewManager YogaMeasureMode widthMode, float height, YogaMeasureMode heightMode, - @Nullable int[] attachmentsPositions) { + @Nullable float[] attachmentsPositions) { SeekBar reactSlider = new ReactSlider(context, null, STYLE); final int spec = View.MeasureSpec.makeMeasureSpec( diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/switchview/ReactSwitchManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/switchview/ReactSwitchManager.java index f06596336a6..c2196f3ff14 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/switchview/ReactSwitchManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/switchview/ReactSwitchManager.java @@ -210,7 +210,7 @@ public class ReactSwitchManager extends SimpleViewManager YogaMeasureMode widthMode, float height, YogaMeasureMode heightMode, - @Nullable int[] attachmentsPositions) { + @Nullable float[] attachmentsPositions) { ReactSwitch view = new ReactSwitch(context); view.setShowText(false); int measureSpec = View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED); diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java index c3e248bc33b..7984406e0e7 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java @@ -120,7 +120,7 @@ public class ReactTextViewManager YogaMeasureMode widthMode, float height, YogaMeasureMode heightMode, - @Nullable int[] attachmentsPositions) { + @Nullable float[] attachmentsPositions) { return TextLayoutManager.measureText( context, diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java index f98035c179b..41d69d8bb2e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java @@ -216,7 +216,7 @@ public class TextLayoutManager { float height, YogaMeasureMode heightYogaMeasureMode, ReactTextViewManagerCallback reactTextViewManagerCallback, - @Nullable int[] attachmentsPositions) { + @Nullable float[] attachmentsPositions) { // TODO(5578671): Handle text direction (see View#getTextDirectionHeuristic) TextPaint textPaint = sTextPaintInstance; @@ -234,6 +234,7 @@ public class TextLayoutManager { if (text == null) { throw new IllegalStateException("Spannable element has not been prepared in onBeforeLayout"); } + BoringLayout.Metrics boring = BoringLayout.isBoring(text, textPaint); float desiredWidth = boring == null ? Layout.getDesiredWidth(text, textPaint) : Float.NaN; @@ -412,9 +413,9 @@ public class TextLayoutManager { // The attachment array returns the positions of each of the attachments as attachmentsPositions[attachmentPosition] = - (int) Math.ceil(PixelUtil.toSPFromPixel(placeholderTopPosition)); + PixelUtil.toSPFromPixel(placeholderTopPosition); attachmentsPositions[attachmentPosition + 1] = - (int) Math.ceil(PixelUtil.toSPFromPixel(placeholderLeftPosition)); + PixelUtil.toSPFromPixel(placeholderLeftPosition); attachmentIndex++; } } diff --git a/ReactCommon/fabric/attributedstring/conversions.h b/ReactCommon/fabric/attributedstring/conversions.h index 774913dcd32..3046e8f7983 100644 --- a/ReactCommon/fabric/attributedstring/conversions.h +++ b/ReactCommon/fabric/attributedstring/conversions.h @@ -611,9 +611,9 @@ inline folly::dynamic toDynamic(const AttributedString &attributedString) { if (fragment.isAttachment()) { dynamicFragment["isAttachment"] = true; dynamicFragment["width"] = - (int)fragment.parentShadowView.layoutMetrics.frame.size.width; + fragment.parentShadowView.layoutMetrics.frame.size.width; dynamicFragment["height"] = - (int)fragment.parentShadowView.layoutMetrics.frame.size.height; + fragment.parentShadowView.layoutMetrics.frame.size.height; } dynamicFragment["textAttributes"] = toDynamic(fragment.textAttributes); fragments.push_back(dynamicFragment); diff --git a/ReactCommon/fabric/textlayoutmanager/platform/android/TextLayoutManager.cpp b/ReactCommon/fabric/textlayoutmanager/platform/android/TextLayoutManager.cpp index 5a1aab94199..52fda0f7cd8 100644 --- a/ReactCommon/fabric/textlayoutmanager/platform/android/TextLayoutManager.cpp +++ b/ReactCommon/fabric/textlayoutmanager/platform/android/TextLayoutManager.cpp @@ -50,7 +50,7 @@ TextMeasurement TextLayoutManager::doMeasure( } } auto env = Environment::current(); - auto attachmentPositions = env->NewIntArray(attachmentsCount * 2); + auto attachmentPositions = env->NewFloatArray(attachmentsCount * 2); static auto measure = jni::findClassStatic("com/facebook/react/fabric/FabricUIManager") @@ -64,7 +64,7 @@ TextMeasurement TextLayoutManager::doMeasure( jfloat, jfloat, jfloat, - jintArray)>("measure"); + jfloatArray)>("measure"); auto minimumSize = layoutConstraints.minimumSize; auto maximumSize = layoutConstraints.maximumSize; @@ -93,7 +93,7 @@ TextMeasurement TextLayoutManager::doMeasure( maximumSize.height, attachmentPositions)); - jint *attachmentData = env->GetIntArrayElements(attachmentPositions, 0); + jfloat *attachmentData = env->GetFloatArrayElements(attachmentPositions, 0); auto attachments = TextMeasurement::Attachments{}; if (attachmentsCount > 0) { @@ -104,8 +104,8 @@ TextMeasurement TextLayoutManager::doMeasure( if (fragment["isAttachment"] == true) { float top = attachmentData[attachmentIndex * 2]; float left = attachmentData[attachmentIndex * 2 + 1]; - float width = fragment["width"].getInt(); - float height = fragment["height"].getInt(); + float width = (float)fragment["width"].getDouble(); + float height = (float)fragment["height"].getDouble(); auto rect = facebook::react::Rect{{left, top}, facebook::react::Size{width, height}};