From 9f5bdd7b497427506dab753ed12017900cb5ffe7 Mon Sep 17 00:00:00 2001 From: Misha Greenberg Date: Fri, 18 Aug 2017 15:25:12 -0700 Subject: [PATCH] Size height of Android Text component based on includeFontPadding style Summary: Overview - This PR resolves the issue described in #14606. This PR makes Text components take into account the includeFontPadding property when calculating their size. Background - Currently, on Android, when includeFontPadding is set to false, the React Text component does not adjust its height. This makes it difficult to lay out other components at a precise spacing relative to a Text component. iOS calculates the height of a UILabel based on the font's descent + ascent. Android lets you choose whether to calculate the height of a TextView based on the font's top + bottom (includeFontPadding=true) or ascent + descent (includeFontPadding=false). In order for a text component to be the same size on iOS and Android (relative to the rest of the layout in points and dips), one should set includeFontPadding=false on Android - but the React Text component needs to take this property into account when sizing itself for this to work. Please see this stack overflow post for a visual explanation of the difference between a font's ascent/descent and top/bottom - https://stackoverflow.com/questions/27631736/meaning-of-top-ascent-baseline-descent-bottom-and-leading-in-androids-font Testing - Please see the attached screenshots to see the height difference of a Text component with this PR when includeFontPadding is true vs false. The font I am using has an ascent + descent = em-size so that the height of the Text component will be equal to the font-size for a single line of text. This is to clearly show the additional height that includeFontPadding=true adds to the Text component. For Text components that are styled in the same way, When includeFontPadding=true, height = ~29.714 dips When includeFontPadding=false, height= 24 dips includefontpaddingtrue includefontpaddingfalse Closes https://github.com/facebook/react-native/pull/14609 Reviewed By: AaaChiuuu Differential Revision: D5587602 Pulled By: achen1 fbshipit-source-id: 6d2f12ba72ec7462676645519cd27820279278eb --- .../java/com/facebook/react/flat/RCTText.java | 10 ++++++++-- .../com/facebook/react/uimanager/ViewProps.java | 1 + .../react/views/text/ReactTextShadowNode.java | 16 +++++++++++----- .../react/views/text/ReactTextViewManager.java | 2 +- 4 files changed, 21 insertions(+), 8 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/RCTText.java b/ReactAndroid/src/main/java/com/facebook/react/flat/RCTText.java index e1027817191..4e07754ff82 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/RCTText.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/RCTText.java @@ -57,6 +57,7 @@ import com.facebook.react.uimanager.annotations.ReactProp; private float mSpacingAdd = 0.0f; private int mNumberOfLines = Integer.MAX_VALUE; private int mAlignment = Gravity.NO_GRAVITY; + private boolean mIncludeFontPadding = true; public RCTText() { setMeasureFunction(this); @@ -94,7 +95,7 @@ import com.facebook.react.uimanager.annotations.ReactProp; (int) Math.ceil(width), widthMode, TextUtils.TruncateAt.END, - true, + mIncludeFontPadding, mNumberOfLines, mNumberOfLines == 1, text, @@ -158,7 +159,7 @@ import com.facebook.react.uimanager.annotations.ReactProp; (int) Math.ceil(right - left), YogaMeasureMode.EXACTLY, TextUtils.TruncateAt.END, - true, + mIncludeFontPadding, mNumberOfLines, mNumberOfLines == 1, mText, @@ -223,6 +224,11 @@ import com.facebook.react.uimanager.annotations.ReactProp; notifyChanged(true); } + @ReactProp(name = ViewProps.INCLUDE_FONT_PADDING, defaultBoolean = true) + public void setIncludeFontPadding(boolean includepad) { + mIncludeFontPadding = includepad; + } + @Override /* package */ void updateNodeRegion( float left, diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java index e3a6fb02462..fe71f1d2dba 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java @@ -92,6 +92,7 @@ public class ViewProps { public static final String TEXT_BREAK_STRATEGY = "textBreakStrategy"; public static final String ALLOW_FONT_SCALING = "allowFontScaling"; + public static final String INCLUDE_FONT_PADDING = "includeFontPadding"; public static final String BORDER_WIDTH = "borderWidth"; public static final String BORDER_LEFT_WIDTH = "borderLeftWidth"; diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java index 97408da5a27..4ceee69cf21 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java @@ -258,12 +258,12 @@ public class ReactTextShadowNode extends LayoutShadowNode { Layout.Alignment.ALIGN_NORMAL, 1.f, 0.f, - true); + mIncludeFontPadding); } else { layout = StaticLayout.Builder.obtain(text, 0, text.length(), textPaint, hintWidth) .setAlignment(Layout.Alignment.ALIGN_NORMAL) .setLineSpacing(0.f, 1.f) - .setIncludePad(true) + .setIncludePad(mIncludeFontPadding) .setBreakStrategy(mTextBreakStrategy) .setHyphenationFrequency(Layout.HYPHENATION_FREQUENCY_NORMAL) .build(); @@ -280,7 +280,7 @@ public class ReactTextShadowNode extends LayoutShadowNode { 1.f, 0.f, boring, - true); + mIncludeFontPadding); } else { // Is used for multiline, boring text and the width is known. @@ -292,12 +292,12 @@ public class ReactTextShadowNode extends LayoutShadowNode { Layout.Alignment.ALIGN_NORMAL, 1.f, 0.f, - true); + mIncludeFontPadding); } else { layout = StaticLayout.Builder.obtain(text, 0, text.length(), textPaint, (int) width) .setAlignment(Layout.Alignment.ALIGN_NORMAL) .setLineSpacing(0.f, 1.f) - .setIncludePad(true) + .setIncludePad(mIncludeFontPadding) .setBreakStrategy(mTextBreakStrategy) .setHyphenationFrequency(Layout.HYPHENATION_FREQUENCY_NORMAL) .build(); @@ -351,6 +351,7 @@ public class ReactTextShadowNode extends LayoutShadowNode { private boolean mIsUnderlineTextDecorationSet = false; private boolean mIsLineThroughTextDecorationSet = false; + private boolean mIncludeFontPadding = true; /** * mFontStyle can be {@link Typeface#NORMAL} or {@link Typeface#ITALIC}. @@ -559,6 +560,11 @@ public class ReactTextShadowNode extends LayoutShadowNode { } } + @ReactProp(name = ViewProps.INCLUDE_FONT_PADDING, defaultBoolean = true) + public void setIncludeFontPadding(boolean includepad) { + mIncludeFontPadding = includepad; + } + @ReactProp(name = ViewProps.TEXT_DECORATION_LINE) public void setTextDecorationLine(@Nullable String textDecorationLineString) { mIsUnderlineTextDecorationSet = false; 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 77591a8153d..392ee1d30c3 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 @@ -152,7 +152,7 @@ public class ReactTextViewManager extends BaseViewManager