Add letter spacing for Android versions >= Lollipop (5.0). #13877

zezhouliu · 2017-05-09T21:25:01Z

Thanks for submitting a PR! Please read these instructions carefully:

Explain the motivation for making this change.
Provide a test plan demonstrating that the code is solid.
Match the code formatting of the rest of the codebase.
Target the master branch, NOT a "stable" branch.

Motivation (required)

What existing problem does the pull request solve?

This PR makes enables lineSpacing on Android. It is based on #9420, differing only with respect to minor library import references.

We have merged this into Airbnb's react-native fork: airbnb#13

Test Plan (required)

Android fonts on Airbnb app

Android fonts specific use cases:

Next Steps

Sign the CLA, if you haven't already.

Small pull requests are much easier to review and more likely to get merged. Make sure the PR does only one thing, otherwise please split it.

Make sure all tests pass on both Travis and Circle CI. PRs that break tests are unlikely to be merged.

For more info, see the "Pull Requests" section of our "Contributing" guidelines.

facebook-github-bot · 2017-05-09T21:25:12Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

facebook-github-bot · 2017-05-09T21:29:41Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

ide

Can you update Text.js's docblock for letterSpacing so that the property is not marked as iOS-only anymore?

Also if you have time it'd be nice to add a letterSpacing demo to the UIExplorer app if there isn't one there already -- that makes it easy to compare the behavior side-by-side with iOS.

ide · 2017-05-10T01:27:28Z

ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextView.java

+          setLetterSpacing((float)0.0);
+        } else {
+          //calculate EM from proper font pixels
+          setLetterSpacing(1+(mLetterSpacing-PixelUtil.toDIPFromPixel(fontSize))/PixelUtil.toDIPFromPixel(fontSize));


Could you explain what this is doing?

Added a comment inline, and pushed an update.

ide · 2017-05-10T01:28:06Z

ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextView.java

+      if (!FloatUtil.floatsEqual(mLetterSpacing, nextLetterSpacing)) {
+        mLetterSpacing = nextLetterSpacing;
+        if(Float.isNaN(mLetterSpacing)) {
+          setLetterSpacing((float)0.0);


style: space between the cast and 0.0 (like the setPadding call above)

thanks, I fixed this!

janicduplessis · 2017-05-10T01:45:07Z

There's another similar PR for letter spacing, we might want to merge our efforts here to get it merged #13199

zezhouliu · 2017-05-17T23:18:23Z

Hi @ide: thanks for taking a look at this PR.

I made the changes that you suggested and also added an example to the explorer. I based it off

react-native/RNTester/js/TextExample.ios.js

Line 387 in bd00456

title: 'Letter Spacing',

I'm getting a CI error on travis: 'React/RCTBridge+Private.h' file not found Do you know how I can resolve this?

sesm · 2017-07-06T13:27:51Z

@zezhouliu @ide
Guys, what can community do to help you get this PR merged?
What is the primary PR for this issue now, this one or #13199?

piotrek1543

Nice work, but as @sesm still wondering which solution is more actual.

RWOverdijk · 2017-08-23T21:29:51Z

👍

facebook-github-bot · 2017-08-29T17:02:39Z

@zezhouliu I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

yenda · 2017-10-19T12:27:42Z

@zezhouliu thanks for this PR ! Can you resolve the merge conflicts ?

@ide it seems that the changes you requested have been address is there still something ?

shockdesign · 2017-11-13T02:16:49Z

@zezhouliu Have you had a chance to update this? I was just working on my own version of this and realised you've already done the hard yards..

motiz88 · 2017-12-30T20:49:33Z

Some issues with this PR as it stands (from the drive-by perspective of a RN user interested in this feature; I'm not a maintainer, or even a contributor yet - I can pretty much only semi-confidently read the RN source code at the moment, but I have spent time tinkering with letter spacing elsewhere):

It appears to need a non-trivial update to be mergeable, in light of 6114f86 in which a lot of the Text implementation has shifted.
It seems to assume font sizes are ints (easily fixable in itself).
It seems to hardcode an assumption that a ReactTextUpdate only has a single font size, which may be at odds with Text nodes being nestable etc. Presumably a correct implementation would instead wrap the content in a custom Span that uses Paint.getTextSize() to normalise a letter spacing value stored in points (DIP/SP depending on allowFontScaling) and pass it into
Paint.setLetterSpacing(float).
I am personally not sold on the points-to-em conversion formula (paraphrased for clarity):
1 + (letterSpacingPoints - fontSizeDIP) / fontSizeDIP - even putting aside DIP vs SP (allowFontScaling mentioned above), why doesn't it work out to 0 if letterSpacing is 0? But I could be missing something here.

motiz88 · 2018-02-20T09:13:01Z

@zezhouliu Want to close this to keep the focus on #17398?

Summary: `letterSpacing` is completely missing from RN Android at the moment. I've reviewed the `letterSpacing` implementations in #13199, #13877 and #16801 (that all seem to have stalled) and managed to put together an improved one based on #13199, updated to merge cleanly post 6114f86, that resolves the [issues](#13199 (comment)) I've identified with that code. I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats: - As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable? - The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference. - I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers. - The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly. - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`. - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least. - I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place). - Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here. Note comment re: unit tests above; RNTester screenshots follow. | iOS (existing functionality, amended test) | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> | facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review. [ANDROID] [FEATURE] [Text] - Implemented letterSpacing Closes #17398 Reviewed By: mdvacca Differential Revision: D6837718 Pulled By: hramos fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c

hramos · 2018-03-01T20:44:15Z

#17398 landed.

Summary: `letterSpacing` is completely missing from RN Android at the moment. I've reviewed the `letterSpacing` implementations in facebook#13199, facebook#13877 and facebook#16801 (that all seem to have stalled) and managed to put together an improved one based on facebook#13199, updated to merge cleanly post facebook@6114f86, that resolves the [issues](facebook#13199 (comment)) I've identified with that code. I believe this is the closest PR yet to a correct implementation of this feature, with a few caveats: - As with the other PRs, this only works on Android >= 5.0 (silently falling back to no letter spacing on older versions). Is this acceptable for a RN feature, in general? Would a dev mode warning be desirable? - The other PRs seem to have explored the space of potential solutions to the layout issue ([Android renders space _around_ glyphs](https://issuetracker.google.com/issues/37079859), iOS to the _right_ of each one) and come up empty, so I've opted to merely document the difference. - I have neither updated nor tested the "Flat" UI implementation - everything compiles but I've taken [this comment](facebook#12770 (comment)) to mean there's no point in trying to wade through it on my own right now; I'm happy to tackle it if given some pointers. - The implementation in `ReactEditText` is only there to handle the placeholder text, as `ReactBaseTextShadowNode` already affects the input control's contents correctly. - I'm not sure whether `<TextInput>` is meant to respect `allowFontScaling`; I've taken my cue here from `ReactTextInputManager.setFontSize()`, and used the same units (SP) to interpret the value in `ReactEditText.setLetterSpacingPt()`. - I'm not sure whether `<TextInput>` is even meant to support `letterSpacing` - it doesn't actually work on iOS. I'm not going to be able to handle the Objective-C side of this, not as part of this PR at least. - I have not added unit tests to `ReactTextTest` - is this desirable? I see that some other props such as `lineHeight` aren't covered there (unless I'm not looking in the right place). - Overall, I'm new to this codebase, so it's likely I've missed something not mentioned here. Note comment re: unit tests above; RNTester screenshots follow. | iOS (existing functionality, amended test) | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458459-c8d59498-edcb-11e7-8c8f-e7426f723886.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458473-2a1ca368-edcc-11e7-9ce6-30c6d3a48660.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458481-6c60a36e-edcc-11e7-9af5-9734dd722ced.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458486-8b3cdcf8-edcc-11e7-974b-25c6085fa674.png width=300> | | iOS _(not implemented, test not in this branch)_ | Android (new functionality & test) | | - | - | | <img src=https://user-images.githubusercontent.com/2246565/34458492-d69a77be-edcc-11e7-896f-21212621dbee.png width=300> | <img src=https://user-images.githubusercontent.com/2246565/34458490-b3a1139e-edcc-11e7-88c8-79d4430d1514.png width=300> | facebook/react-native-website#105 - this docs PR is edited slightly from what's in `TextStylePropTypes` here; happy to align either one to the other after a review. [ANDROID] [FEATURE] [Text] - Implemented letterSpacing Closes facebook#17398 Reviewed By: mdvacca Differential Revision: D6837718 Pulled By: hramos fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c

Add linespacing for versions >= Lollipop (5.0).

b4247fc

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels May 9, 2017

ide changed the title ~~Add linespacing for versions >= Lollipop (5.0).~~ Add letter spacing for Android versions >= Lollipop (5.0). May 10, 2017

ide suggested changes May 10, 2017

View reviewed changes

Alex Liu added 3 commits May 17, 2017 16:08

Update documentation for letterSpacing (no longer iOS only).

c4e0fa2

Add TextExamples for Android letter spacing.

9ce8181

Address PR comments in ReactTextView.

ed7ce85

shergin self-requested a review May 31, 2017 00:13

shergin added the Android label May 31, 2017

sesm mentioned this pull request Jul 6, 2017

[Android] Implement letterSpacing option #13199

Closed

piotrek1543 reviewed Jul 28, 2017

View reviewed changes

christoph-jerolimov added a commit to christoph-jerolimov/react-native-letterspacing-example that referenced this pull request Aug 26, 2017

Switch branch for facebook/react-native#13877

ec2f249

yenda mentioned this pull request Sep 18, 2017

Letter-spacing on Android status-im/status-mobile#1879

Closed

shockdesign mentioned this pull request Nov 13, 2017

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

Closed

motiz88 mentioned this pull request Dec 31, 2017

Implement letterSpacing on Android >= 5.0 #17398

Closed

hramos closed this Mar 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add letter spacing for Android versions >= Lollipop (5.0). #13877

Add letter spacing for Android versions >= Lollipop (5.0). #13877

zezhouliu commented May 9, 2017 •

edited

Loading

facebook-github-bot commented May 9, 2017

facebook-github-bot commented May 9, 2017

ide left a comment

ide May 10, 2017

zezhouliu May 17, 2017

ide May 10, 2017

zezhouliu May 17, 2017

janicduplessis commented May 10, 2017

zezhouliu commented May 17, 2017

sesm commented Jul 6, 2017

piotrek1543 left a comment •

edited

Loading

RWOverdijk commented Aug 23, 2017

facebook-github-bot commented Aug 29, 2017

yenda commented Oct 19, 2017

shockdesign commented Nov 13, 2017

motiz88 commented Dec 30, 2017

motiz88 commented Feb 20, 2018

hramos commented Mar 1, 2018

Add letter spacing for Android versions >= Lollipop (5.0). #13877

Add letter spacing for Android versions >= Lollipop (5.0). #13877

Conversation

zezhouliu commented May 9, 2017 • edited Loading

Motivation (required)

Test Plan (required)

Android fonts on Airbnb app

Android fonts specific use cases:

Next Steps

facebook-github-bot commented May 9, 2017

facebook-github-bot commented May 9, 2017

ide left a comment

Choose a reason for hiding this comment

ide May 10, 2017

Choose a reason for hiding this comment

zezhouliu May 17, 2017

Choose a reason for hiding this comment

ide May 10, 2017

Choose a reason for hiding this comment

zezhouliu May 17, 2017

Choose a reason for hiding this comment

janicduplessis commented May 10, 2017

zezhouliu commented May 17, 2017

sesm commented Jul 6, 2017

piotrek1543 left a comment • edited Loading

Choose a reason for hiding this comment

RWOverdijk commented Aug 23, 2017

facebook-github-bot commented Aug 29, 2017

yenda commented Oct 19, 2017

shockdesign commented Nov 13, 2017

motiz88 commented Dec 30, 2017

motiz88 commented Feb 20, 2018

hramos commented Mar 1, 2018

zezhouliu commented May 9, 2017 •

edited

Loading

piotrek1543 left a comment •

edited

Loading