-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/** | ||
* Copyright (c) 2015-present, Facebook, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the BSD-style license found in the | ||
* LICENSE file in the root directory of this source tree. An additional grant | ||
* of patent rights can be found in the PATENTS file in the same directory. | ||
*/ | ||
|
||
package com.facebook.react.views.text; | ||
|
||
import android.annotation.TargetApi; | ||
import android.os.Build; | ||
import android.text.TextPaint; | ||
import android.text.style.MetricAffectingSpan; | ||
|
||
import com.facebook.infer.annotation.Assertions; | ||
|
||
/** | ||
* A {@link MetricAffectingSpan} that allows to set the letter spacing | ||
* on the selected text span. | ||
*/ | ||
@TargetApi(Build.VERSION_CODES.LOLLIPOP) | ||
public class CustomLetterSpacingSpan extends MetricAffectingSpan { | ||
|
||
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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. 😄 |
||
mLetterSpacing = letterSpacing; | ||
} | ||
|
||
@Override | ||
public void updateDrawState(TextPaint paint) { | ||
paint.setLetterSpacing(mLetterSpacing); | ||
} | ||
|
||
@Override | ||
public void updateMeasureState(TextPaint paint) { | ||
paint.setLetterSpacing(mLetterSpacing); | ||
} | ||
} |
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)