From c3d072955024824d7ff6113fc9f642d25c462f16 Mon Sep 17 00:00:00 2001 From: David Vacca Date: Fri, 6 Mar 2020 23:04:54 -0800 Subject: [PATCH] Fix rendering of TextInput in Catalyst App Summary: This diff fixes the rendering of the TextInput RNTester examples in catalyst app. The root cause of the bug is that we were parsing the props of the text using the TextAttributeProps class, this is incorrect because TextAttributeProps is a holder of the C++ TextAttributes and not TextProps, but the name is confusing. As a consecuence there was some mistmaches of types during parsing and that was throwing an exception in some examples. I created the task T63643819 to refactor these classes to make this cleaner. I'll be working on T63643819 next week, now I want to unblock this bug. changelog: [internal] Reviewed By: JoshuaGross Differential Revision: D20320969 fbshipit-source-id: 7b47546ba4f34df2a7fa151ab200823ea2eeb696 --- .../views/text/ReactTextViewManager.java | 4 +- .../react/views/text/TextAttributeProps.java | 73 ++++++++----------- .../textinput/ReactTextInputManager.java | 4 +- 3 files changed, 33 insertions(+), 48 deletions(-) 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 e198a472a71..a5472c4339f 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 @@ -94,8 +94,6 @@ public class ReactTextViewManager view.getContext(), attributedString, mReactTextViewManagerCallback); view.setSpanned(spanned); - TextAttributeProps textViewProps = new TextAttributeProps(props); - int textBreakStrategy = getTextBreakStrategy(paragraphAttributes.getString("textBreakStrategy")); @@ -106,7 +104,7 @@ public class ReactTextViewManager spanned, state.hasKey("mostRecentEventCount") ? state.getInt("mostRecentEventCount") : -1, false, // TODO add this into local Data - textViewProps.getTextAlign(), + TextAttributeProps.getTextAlignment(props), textBreakStrategy, justificationMode); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextAttributeProps.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextAttributeProps.java index f6f5a6f1d1c..dbfea6bce1c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextAttributeProps.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextAttributeProps.java @@ -20,6 +20,9 @@ import com.facebook.react.uimanager.ReactStylesDiffMap; import com.facebook.react.uimanager.ViewProps; import com.facebook.yoga.YogaDirection; +// TODO: T63643819 refactor naming of TextAttributeProps to make explicit that this represents +// TextAttributes and not TextProps. As part of this refactor extract methods that don't belong to +// TextAttributeProps (e.g. TextAlign) public class TextAttributeProps { private static final String INLINE_IMAGE_PLACEHOLDER = "I"; @@ -36,8 +39,8 @@ public class TextAttributeProps { private static final int DEFAULT_TEXT_SHADOW_COLOR = 0x55000000; protected boolean mIsAttachment = false; - protected float mAttachmentWidth; - protected float mAttachmentHeight; + protected float mAttachmentWidth = Float.NaN; + protected float mAttachmentHeight = Float.NaN; protected float mLineHeight = Float.NaN; protected boolean mIsColorSet = false; protected boolean mAllowFontScaling = true; @@ -110,7 +113,6 @@ public class TextAttributeProps { setLineHeight(getFloatProp(ViewProps.LINE_HEIGHT, UNSET)); setLetterSpacing(getFloatProp(ViewProps.LETTER_SPACING, Float.NaN)); setAllowFontScaling(getBooleanProp(ViewProps.ALLOW_FONT_SCALING, true)); - setTextAlign(getStringProp(ViewProps.TEXT_ALIGN)); setFontSize(getFloatProp(ViewProps.FONT_SIZE, UNSET)); setColor(props.hasKey(ViewProps.COLOR) ? props.getInt(ViewProps.COLOR, 0) : null); setColor(props.hasKey("foregroundColor") ? props.getInt("foregroundColor", 0) : null); @@ -134,6 +136,31 @@ public class TextAttributeProps { setIsAttachment(getBooleanProp(ViewProps.IS_ATTACHMENT, false)); } + // TODO T63645393 add support for RTL + public static int getTextAlignment(ReactStylesDiffMap props) { + @Nullable + String textAlignPropValue = + props.hasKey(ViewProps.TEXT_ALIGN) ? props.getString(ViewProps.TEXT_ALIGN) : null; + int textAlignment; + + if ("justify".equals(textAlignPropValue)) { + textAlignment = Gravity.LEFT; + } else { + if (textAlignPropValue == null || "auto".equals(textAlignPropValue)) { + textAlignment = Gravity.NO_GRAVITY; + } else if ("left".equals(textAlignPropValue)) { + textAlignment = Gravity.LEFT; + } else if ("right".equals(textAlignPropValue)) { + textAlignment = Gravity.RIGHT; + } else if ("center".equals(textAlignPropValue)) { + textAlignment = Gravity.CENTER_HORIZONTAL; + } else { + throw new JSApplicationIllegalArgumentException("Invalid textAlign: " + textAlignPropValue); + } + } + return textAlignment; + } + private void setIsAttachment(boolean isAttachment) { mIsAttachment = isAttachment; } @@ -196,19 +223,6 @@ public class TextAttributeProps { return useInlineViewHeight ? mHeightOfTallestInlineImage : mLineHeight; } - // Return text alignment according to LTR or RTL style - public int getTextAlign() { - int textAlign = mTextAlign; - if (getLayoutDirection() == YogaDirection.RTL) { - if (textAlign == Gravity.RIGHT) { - textAlign = Gravity.LEFT; - } else if (textAlign == Gravity.LEFT) { - textAlign = Gravity.RIGHT; - } - } - return textAlign; - } - public void setNumberOfLines(int numberOfLines) { mNumberOfLines = numberOfLines == 0 ? UNSET : numberOfLines; } @@ -253,31 +267,6 @@ public class TextAttributeProps { } } - public void setTextAlign(@Nullable String textAlign) { - if ("justify".equals(textAlign)) { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { - mJustificationMode = Layout.JUSTIFICATION_MODE_INTER_WORD; - } - mTextAlign = Gravity.LEFT; - } else { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { - mJustificationMode = Layout.JUSTIFICATION_MODE_NONE; - } - - if (textAlign == null || "auto".equals(textAlign)) { - mTextAlign = Gravity.NO_GRAVITY; - } else if ("left".equals(textAlign)) { - mTextAlign = Gravity.LEFT; - } else if ("right".equals(textAlign)) { - mTextAlign = Gravity.RIGHT; - } else if ("center".equals(textAlign)) { - mTextAlign = Gravity.CENTER_HORIZONTAL; - } else { - throw new JSApplicationIllegalArgumentException("Invalid textAlign: " + textAlign); - } - } - } - public void setFontSize(float fontSize) { mFontSizeInput = fontSize; if (fontSize != UNSET) { @@ -446,7 +435,7 @@ public class TextAttributeProps { : -1; } - // TODO T31905686 remove this from here and add support to RTL + // TODO T63645393 remove this from here and add support to RTL private YogaDirection getLayoutDirection() { return YogaDirection.LTR; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java index 1b681118d64..703148030e8 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java @@ -1261,8 +1261,6 @@ public class ReactTextInputManager extends BaseViewManager