Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] LetterSpacing support for Android Lollipop and above (21) #16801

Closed
wants to merge 3 commits into from
Closed

[WIP] LetterSpacing support for Android Lollipop and above (21) #16801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b69db41
Added letterSpacing support to Android component of ReactTextView
shockdesign Nov 13, 2017
672b6e6
More updates to support this with latest ReactNative
shockdesign Nov 13, 2017
b3e7505
Small fixes
shockdesign Nov 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Libraries/Text/TextStylePropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ const TextStylePropTypes = {
textShadowRadius: ReactPropTypes.number,
textShadowColor: ColorPropType,
/**
* @platform ios
* Specifies the letter spacing (kerning). Defaults to 0.0 (no kerning) and is supported on iOS and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Kerning seems to be the wrong term per Wikipedia.

Letter-spacing should not be confused with kerning. [...] Kerning is a spacing adjustment of one or more specific pairs of adjacent characters [...]

* only on Android Lollipop (21) and greater.
*/
letterSpacing: ReactPropTypes.number,
lineHeight: ReactPropTypes.number,
Expand Down
14 changes: 14 additions & 0 deletions RNTester/js/TextExample.android.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,20 @@ class TextExample extends React.Component<{}> {
right right right right right right right right right right right right right
</Text>
</RNTesterBlock>
<RNTesterBlock title="Letter Spacing">
<Text style={{letterSpacing: 0}}>
letterSpacing = 0
</Text>
<Text style={{letterSpacing: 2, marginTop: 5}}>
letterSpacing = 2
</Text>
<Text style={{letterSpacing: 9, marginTop: 5}}>
letterSpacing = 9
</Text>
<Text style={{letterSpacing: -1, marginTop: 5}}>
letterSpacing = -1
</Text>
</RNTesterBlock>
<RNTesterBlock title="Unicode">
<View>
<View style={{flexDirection: 'row'}}>
Expand Down
13 changes: 13 additions & 0 deletions RNTester/js/TextInputExample.android.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,19 @@ exports.examples = [
);
}
},
{
title: 'Letter spacing',
platform: 'android',
render: function() {
return (
<View>
<TextInput lineSpacing={1.2}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably mean to use letterSpacing here.

placeholder="Line spacing of 1.2"
/>
</View>
);
}
},
{
title: 'Auto-expanding',
render: function() {
Expand Down
1 change: 1 addition & 0 deletions ReactAndroid/src/main/java/com/facebook/react/flat/RCTTextInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ public void onCollectExtraUpdates(UIViewOperationQueue uiViewOperationQueue) {
getPadding(Spacing.TOP),
getPadding(Spacing.END),
getPadding(Spacing.BOTTOM),
getLetterSpacing(),
UNSET);
// TODO: the Float.NaN should be replaced with the real line height see D3592781
uiViewOperationQueue.enqueueUpdateExtraData(getReactTag(), reactTextUpdate);
Expand Down
1 change: 1 addition & 0 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ public class ViewProps {
public static final String FONT_WEIGHT = "fontWeight";
public static final String FONT_STYLE = "fontStyle";
public static final String FONT_FAMILY = "fontFamily";
public static final String LETTER_SPACING = "letterSpacing";
public static final String LINE_HEIGHT = "lineHeight";
public static final String NEEDS_OFFSCREEN_ALPHA_COMPOSITING = "needsOffscreenAlphaCompositing";
public static final String NUMBER_OF_LINES = "numberOfLines";
Expand Down
18 changes: 18 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/text/ReactBaseTextShadowNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,8 @@ private static int parseNumericFontWeight(String fontWeightString) {
protected int mTextAlign = Gravity.NO_GRAVITY;
protected int mTextBreakStrategy =
(Build.VERSION.SDK_INT < Build.VERSION_CODES.M) ? 0 : Layout.BREAK_STRATEGY_HIGH_QUALITY;
protected float mLetterSpacing = 0.0f;
protected float mPreviousLetterSpacing = Float.NaN;

protected float mTextShadowOffsetDx = 0;
protected float mTextShadowOffsetDy = 0;
Expand Down Expand Up @@ -291,6 +293,14 @@ public float getEffectiveLineHeight() {
return useInlineViewHeight ? mHeightOfTallestInlineImage : mLineHeight;
}

private static float calculateLetterSpacing(float letterSpacing, int fontSize) {
if (Float.isNaN(letterSpacing) || letterSpacing == 0.0f) {
return 0.0f;
}

return letterSpacing / fontSize * 2.0f;
Copy link
Contributor

@motiz88 motiz88 Dec 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the approach taken in #13877 is a more sound one with respect to the unit conversions involved; see also my comment on setFontSize below.

}

// Return text alignment according to LTR or RTL style
private int getTextAlign() {
int textAlign = mTextAlign;
Expand All @@ -304,6 +314,13 @@ private int getTextAlign() {
return textAlign;
}

@ReactProp(name = ViewProps.LETTER_SPACING, defaultFloat = Float.NaN)
public void setLetterSpacing(float letterSpacing) {
mPreviousLetterSpacing = letterSpacing;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick (though I'm not an authority on this codebase): "previous" implies you're keeping an old state, but in fact you're just converting units for the same state; maybe something like mLetterSpacingPts and mLetterSpacingEms? (or mLetterSpacingInput and mLetterSpacing, following the pattern seen elsewhere in this file with e.g. mFontSize)

mLetterSpacing = calculateLetterSpacing(letterSpacing, mFontSize);
markUpdated();
}

@ReactProp(name = ViewProps.NUMBER_OF_LINES, defaultInt = UNSET)
public void setNumberOfLines(int numberOfLines) {
mNumberOfLines = numberOfLines == 0 ? UNSET : numberOfLines;
Expand Down Expand Up @@ -363,6 +380,7 @@ public void setFontSize(float fontSize) {
: (float) Math.ceil(PixelUtil.toPixelFromDIP(fontSize));
}
mFontSize = (int) fontSize;
mLetterSpacing = calculateLetterSpacing(mPreviousLetterSpacing, mFontSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still on the subject of unit conversions:

mFontSize is in pixels here, but letter spacing is given in points (presumably the same units as mFontSizeInput - SP or DIP depending on mAllowFontScaling). We need to pass ems to setLetterSpacing(); the correct calculation to yield that would seem to be letterSpacing / mFontSizeInput (handling UNSET gracefully somehow - is there a default font size defined somewhere?), since the font size is by definition the em size.

markUpdated();
}

Expand Down
3 changes: 2 additions & 1 deletion ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,11 @@ public void onCollectExtraUpdates(UIViewOperationQueue uiViewOperationQueue) {
getPadding(Spacing.TOP),
getPadding(Spacing.END),
getPadding(Spacing.BOTTOM),
getLetterSpacing(),
getTextAlign(),
mTextBreakStrategy
);
uiViewOperationQueue.enqueueUpdateExtraData(getReactTag(), reactTextUpdate);
}
}
}
}
9 changes: 9 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextUpdate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public class ReactTextUpdate {
private final float mPaddingTop;
private final float mPaddingRight;
private final float mPaddingBottom;
private final float mLetterSpacing;
private final int mTextAlign;
private final int mTextBreakStrategy;

Expand All @@ -42,6 +43,7 @@ public ReactTextUpdate(
float paddingTop,
float paddingEnd,
float paddingBottom,
float letterSpacing,
int textAlign) {
this(text,
jsEventCounter,
Expand All @@ -50,6 +52,7 @@ public ReactTextUpdate(
paddingTop,
paddingEnd,
paddingBottom,
letterSpacing,
textAlign,
Layout.BREAK_STRATEGY_HIGH_QUALITY);
}
Expand All @@ -62,6 +65,7 @@ public ReactTextUpdate(
float paddingTop,
float paddingEnd,
float paddingBottom,
float letterSpacing,
int textAlign,
int textBreakStrategy) {
mText = text;
Expand All @@ -71,6 +75,7 @@ public ReactTextUpdate(
mPaddingTop = paddingTop;
mPaddingRight = paddingEnd;
mPaddingBottom = paddingBottom;
mLetterSpacing = letterSpacing;
mTextAlign = textAlign;
mTextBreakStrategy = textBreakStrategy;
}
Expand Down Expand Up @@ -103,6 +108,10 @@ public float getPaddingBottom() {
return mPaddingBottom;
}

public float getLetterSpacing() {
return mLetterSpacing;
}

public int getTextAlign() {
return mTextAlign;
}
Expand Down
12 changes: 12 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public class ReactTextView extends TextView implements ReactCompoundView {
private int mDefaultGravityVertical;
private boolean mTextIsSelectable;
private float mLineHeight = Float.NaN;
private float mLetterSpacing = Float.NaN;
private int mTextAlign = Gravity.NO_GRAVITY;
private int mNumberOfLines = ViewDefaults.NUMBER_OF_LINES;
private TextUtils.TruncateAt mEllipsizeLocation = TextUtils.TruncateAt.END;
Expand Down Expand Up @@ -62,6 +63,17 @@ public void setText(ReactTextUpdate update) {
(int) Math.floor(update.getPaddingRight()),
(int) Math.floor(update.getPaddingBottom()));

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
float nextLetterSpacing = update.getLetterSpacing();
if (Float.isNaN(nextLetterSpacing)) {
nextLetterSpacing = 0.0f;
}

if(getLetterSpacing() != nextLetterSpacing) {
setLetterSpacing(nextLetterSpacing);
}
}

int nextTextAlign = update.getTextAlign();
if (mTextAlign != nextTextAlign) {
mTextAlign = nextTextAlign;
Expand Down
9 changes: 9 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import android.graphics.PorterDuff;
import android.graphics.Typeface;
import android.graphics.drawable.Drawable;
import android.os.Build;
import android.support.v4.content.ContextCompat;
import android.text.Editable;
import android.text.InputFilter;
Expand Down Expand Up @@ -419,6 +420,14 @@ public void setTextAlignVertical(ReactEditText view, @Nullable String textAlignV
}
}

@ReactProp(name = ViewProps.LETTER_SPACING, defaultFloat = 0.0f)
public void setLetterSpacing(ReactEditText view, float spacing) {
if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.LOLLIPOP) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering aloud: Is there a precedent for RN enabling an Android-version-dependent feature with neither a fallback nor a warning for older versions?

// Only set letter spacing on Android 21 and up devices, not available otherwise
view.setLetterSpacing(spacing);
}
}

@ReactProp(name = "inlineImageLeft")
public void setInlineImageLeft(ReactEditText view, @Nullable String resource) {
int id = ResourceDrawableIdHelper.getInstance().getResourceDrawableId(view.getContext(), resource);
Expand Down
1 change: 1 addition & 0 deletions ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputShadowNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ public void onCollectExtraUpdates(UIViewOperationQueue uiViewOperationQueue) {
getPadding(Spacing.TOP),
getPadding(Spacing.RIGHT),
getPadding(Spacing.BOTTOM),
getLetterSpacing(),
mTextAlign,
mTextBreakStrategy);
uiViewOperationQueue.enqueueUpdateExtraData(getReactTag(), reactTextUpdate);
Expand Down
14 changes: 14 additions & 0 deletions ...tAndroid/src/test/java/com/facebook/react/views/textinput/ReactTextInputPropertyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,20 @@ public void testTextAlign() {
assertThat(view.getGravity()).isEqualTo(defaultGravity);
}

@Test
public void testLetterSpacing() {
ReactEditText view = mManager.createViewInstance(mThemedContext);

mManager.updateProperties(view, buildStyles());
assertThat(view.getLetterSpacing()).isEqualTo(0.0);

mManager.updateProperties(view, buildStyles("numberOfLines", 0.5));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably mean letterSpacing here and below.

assertThat(view.getLetterSpacing()).isEqualTo(0.5);

mManager.updateProperties(view, buildStyles("numberOfLines", 1.2));
assertThat(view.getLetterSpacing()).isEqualTo(1.2);
}

@Test
public void testMaxLength() {
ReactEditText view = mManager.createViewInstance(mThemedContext);
Expand Down
6 changes: 3 additions & 3 deletions docs/text-style-props.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,9 @@ For best results also set `textAlignVertical` to `center`. Default is true.



| Type | Required | Platform |
| - | - | - |
| number | No | iOS |
| Type | Required |
| - | - |
| number | No |



Expand Down
2 changes: 1 addition & 1 deletion docs/text.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ default is `true`.

- **`fontVariant`**: array of enum('small-caps', 'oldstyle-nums', 'lining-nums', 'tabular-nums', 'proportional-nums') (_iOS_)

- **`letterSpacing`**: number (_iOS_)
- **`letterSpacing`**: number
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably prudent to amend this, perhaps to say something like (iOS, Android >= 5.0).


- **`textDecorationColor`**: [color](docs/colors.html) (_iOS_)

Expand Down