Skip to content

Commit

Permalink
Fix precision of TextInlineViews in Android
Browse files Browse the repository at this point in the history
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
  • Loading branch information
mdvacca authored and facebook-github-bot committed May 13, 2020
1 parent 774ebd9 commit e3f4a7b
Show file tree
Hide file tree
Showing 9 changed files with 17 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ private long measure(
float maxWidth,
float minHeight,
float maxHeight,
@Nullable int[] attachmentsPositions) {
@Nullable float[] attachmentsPositions) {
ReactContext context =
rootTag < 0 ? mReactApplicationContext : mReactContextForRootTag.get(rootTag);
return mMountingManager.measure(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ public long measure(
@NonNull YogaMeasureMode widthMode,
float height,
@NonNull YogaMeasureMode heightMode,
@Nullable int[] attachmentsPositions) {
@Nullable float[] attachmentsPositions) {

return mViewManagerRegistry
.get(componentName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ public long measure(
YogaMeasureMode widthMode,
float height,
YogaMeasureMode heightMode,
@Nullable int[] attachmentsPositions) {
@Nullable float[] attachmentsPositions) {
return 0;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ public long measure(
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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ public long measure(
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public long measure(
YogaMeasureMode widthMode,
float height,
YogaMeasureMode heightMode,
@Nullable int[] attachmentsPositions) {
@Nullable float[] attachmentsPositions) {

return TextLayoutManager.measureText(
context,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ public static long measureText(
float height,
YogaMeasureMode heightYogaMeasureMode,
ReactTextViewManagerCallback reactTextViewManagerCallback,
@Nullable int[] attachmentsPositions) {
@Nullable float[] attachmentsPositions) {

// TODO(5578671): Handle text direction (see View#getTextDirectionHeuristic)
TextPaint textPaint = sTextPaintInstance;
Expand All @@ -234,6 +234,7 @@ public static long measureText(
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;

Expand Down Expand Up @@ -412,9 +413,9 @@ public static long measureText(

// 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++;
}
}
Expand Down
4 changes: 2 additions & 2 deletions ReactCommon/fabric/attributedstring/conversions.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -64,7 +64,7 @@ TextMeasurement TextLayoutManager::doMeasure(
jfloat,
jfloat,
jfloat,
jintArray)>("measure");
jfloatArray)>("measure");

auto minimumSize = layoutConstraints.minimumSize;
auto maximumSize = layoutConstraints.maximumSize;
Expand Down Expand Up @@ -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) {
Expand All @@ -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}};
Expand Down

0 comments on commit e3f4a7b

Please sign in to comment.