-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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.
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
[Android] Implement letterSpacing option #13199
[Android] Implement letterSpacing option #13199
Conversation
You can just remove the |
Also maybe we should specify in the doc that it only works on android 5+ |
Thanks @janicduplessis for the hint. I removed the badge, added some documentation and rebased the branch. |
+1 need this! |
Sorry for the spam, but after 4w any feedback would be great. So after watching the git annotation of the changed files I decided to ping you.. 😏 /cc @janicduplessis @mkonicek @emilsjolander @astreet Latest push just rebased this PR again on master. |
Sorry for the delay we are a bit swamped in PRs as you can see. Feel free to ping us if we forget to check back. It looks good to me but would like a second review from someone at FB that is more familiar with that code since it will affect apps quite a lot. @AaaChiuuu could you have a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff looks really good, thanks! I'm concerned about letter spacing working slightly differently on the two platforms though, even with the work-around (the workaround also doesn't work if the text is inline). Can you think of other options? Can we at least automatically apply the negative margin by translating the text by half the increased letter spacing?
private final float mLetterSpacing; | ||
|
||
public CustomLetterSpacingSpan(float letterSpacing) { | ||
Assertions.assertCondition(!Float.isNaN(letterSpacing) && letterSpacing != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use SoftAssertions so that we throw a catchable Exception and not an Error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that this happen only if you change the internal code. This can not happen by any input from JSX. But I can change this when nothing other blocks this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still would like us to not use Errors when we can use Exceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Will change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having less points to discuss is the fastest way to get PR approved. 😄
I have thought about different solutions and also tried to implement such a "negative" margin. But it doesn't work and make other thinks must more complex. I think this is a good-for-now-but-not-perfect solution. Its up to the developers to apply different letterspacing inline and test them on both platforms. For us (and I hope for many other developers) this should work fine in headline and in 90% of the letterspacing cases with multiline text. I'm sorry I see currently no option that I can improve these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I just looked through the Span/Paint APIs and I think matching iOS behavior is too advanced for what they let us do :(
private final float mLetterSpacing; | ||
|
||
public CustomLetterSpacingSpan(float letterSpacing) { | ||
Assertions.assertCondition(!Float.isNaN(letterSpacing) && letterSpacing != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still would like us to not use Errors when we can use Exceptions
* is calculated based on your font size and the font family. To left-align a text similar | ||
* to iOS (or in different font sizes) you should add a Platform-specific negative layout attribute. | ||
* | ||
* iOS: The additional space will be added behind the character and is defined in points. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this implying that android and ios also use two different units? If so, can we make them use the same unit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that are different units. The problem is that the Android only supports the M-unit while iOS only supports points. Is it not possible (for me) to use the exakt same unit here. 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see two options then:
-
convert em to pt: unfortunately this is going to require measuring the width of an M in the current font/font size (and also recalculating it when the font/font size changes), and it would probably work quite poorly for any animations that adjust font size (not that I know of any).
-
Create two different props (letterSpacingIOS, letterSpacingAndroid).
I much prefer (1) but let me know what you think. I don't think having the same prop behave very differently on both platforms is a good solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like you, I would prefer option one.
Can you help me to calculating the exact width of the letter M in a performant way?
Did you see my current calculation in calculateLetterSpacing
which is a "high performant" 😏 way which calculates a good result in 90%... 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, when defining 5 as letter spacing, this means 5 points on iOS.
With the current implementation this means "letterSpacingInput / fontSize * 2f" M on Android.
With a fontSize of 14 dpi this means ~0.71 M. And because the "small M" needs round about 8-10 dpi* from left to right this results in 5-6 dpis "real letter spacing" on Android.
With a fontSize of 28 dpi this means ~0.36 M. And because the "bigger M" here needs round about 16-20 dpi* from left to right this results in 6-7 dpis "real letter spacing" on Android.
In our case (and I think in many more cases) this is much better than no letter spacing. 🙈 Also if I know, an addtional PR can improve this in the future. And I'm looking forward to anybody who can and will do this. S/he will get some free 🍻 from us, here in Cologne. 😄
*I does not measure the size exactly because it depends on the selected font family.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm fine siding with practicality here then. If you wanted to measure an 'M', you'd want to look at how we do it in our text shadow node: basically use a BoringLayout. But I'm also fine if you come up with some formula that at least factors in font size (looks like you already have something like that -- if so, add some more detailed docs for the prop about how we're emulating what iOS does, but it isn't perfect)
Can someone please get this merged? Letter spacing is a very big deal to many design conscious clients/designers. I would fix the merge conflict if I had authorization to. |
@jerolimov @astreet |
@jerolimov @astreet I agree with @Hunter-Dolan that this is a really really really big deal. |
@jerolimov 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. |
Bump. |
Because Android TextView#setLetterSpacing expects the letter spacing value in 'EM' unit, where one is defined by the width of the letter 'M'. Typical values for slight expansion will be around 0.05. Negative values will tighten text. So we use NaN as not defined value. This method calculates an 'EM' *approach* based on the input and the calculated font size. With common fonts this calculated value renders a similar spacing and layout to iOS.
I tested #13877 which doesn't work for me after rebasing it. For both branches I created a small test project:
I could take the idea from the responses above and PR 13877 to calculate the correct "M" value. But I still have the issue that Android split the additional letter spacing and add this space on the left and on the right of each character, where iOS add this only on the right side. Adding a negative padding (left and right) on Android (in |
@jerolimov Could you please add a small example to RNTester app? |
@jerolimov 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. |
@jerolimov @shergin What we can do in order to continue work on this? |
@jerolimov Could you please resolve conflicts? |
@jerolimov How is this solution different from #13199? How do you think, which is better? |
@jerolimov I've resolved the conflicts if it helps get things moving. Really looking forward to this landing. |
@astreet @shergin @jerolimov: We seem to be at an impasse here. Should I raise a new PR directly against facebook/react-native ? |
@dickie81 thanks for working on this, if it can get things to move forward please do ! |
I'd like to add my two cents here following #16801 (review) and #13877 (comment) in the hopes of getting this feature back on track. The usual disclaimer applies - I'm not a RN contributor (yet) or maintainer, just an interested user who has tinkered with letter spacing before.
I will have a stab at resolving conflicts with |
@jerolimov 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
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 facebook/react-native@6114f86, that resolves the [issues](facebook/react-native#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/react-native#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/react-native#17398 Reviewed By: mdvacca Differential Revision: D6837718 Pulled By: hramos fbshipit-source-id: 5c9d49e9cf4af6457b636416ce5fe15315aab72c
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
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
Motivation
Implement letterSpacing option on Android. This was requested multiple times. (See also #3485 #10622) - This PR is inspired by #9420 but also fixes the layout calculation which is required for multiline text with letter spacing.
The problem with letter spacing on Android is, in general, that the API TextView#setLetterSpacing expects the letter spacing value in "EM", where "1 EM" is defined by the width of the letter 'M'. From the documentation: "Typical values for slight expansion will be around 0.05. Negative values will tighten text." The react-native developer expectation was that the spacing is defined in points, similar to width, height, margin, padding, and so on.
This patch implements an "approach" and calculates the "EM" based on the font size.
I tested this with the default font and the layout was now "similar" to iOS, but doesn't match the iOS layout exactly.
Test Plan
Screenshot on iOS (before and after):
Screenshot on Android before this patch:
Screenshot on Android after this patch:
Sourcecode for this "beautiful app" is available here: https://gist.github.com/jerolimov/39e4fd29702b6f1ad55768907d92c33d
Documentation Question
How can I update the documentation and remove the "ios" badge on the
letterSpacing
option?