-
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
Lineheight behaves differently on ios and android #10712
Comments
Just ran into this issue, can confirm still exists |
Ditto, can confirm that I'm still seeing this on RN v0.45.0 (latest). It's not apparent to me if this is intended behavior based on the corresponding underlying native capabilities, or rather an unintentional discrepancy. I'd just like to add that it certainly makes achieving a consistent layout between both platforms challenging, in achieving parity with pre-designed screens. I've resorted to disabling Is this on anyone's radar by chance? |
I did a little test on CustomLineHeightSpan.java. It works on new pure Android project but not on RNTesterApp. I am still investigating what the different between ReactNative TextView and PureAndroid TextView. I would like to pair with ReactNative core team to work on this investigation (if possible). The idea is simple: use // Copyright 2004-present Facebook. All Rights Reserved.
package com.facebook.react.views.text;
import android.graphics.Paint;
import android.text.style.LineHeightSpan;
/**
* We use a custom {@link LineHeightSpan}, because `lineSpacingExtra` is broken. Details here:
* https://github.com/facebook/react-native/issues/7546
*/
public class CustomLineHeightSpan implements LineHeightSpan {
private final int mHeight;
private final int mSize;
CustomLineHeightSpan(float size, float height) {
this.mSize = (int) size;
this.mHeight = Math.max((int) height, mSize);
}
@Override
public void chooseHeight(
CharSequence text,
int start,
int end,
int spanstartv,
int v,
Paint.FontMetricsInt fm) {
int spacing = (mHeight - mSize) / 2;
fm.bottom = fm.descent + spacing;
fm.ascent = fm.descent - mSize;
fm.top = fm.descent - mSize - spacing;
}
} |
Checkout my proposal for CustomLineHeightSpan.java fixed. henrytao-me#1. |
Same problems with fonts on RN: 0.44.0. There are few issues as far as I know about this, or very similar behaviour. |
It's a big problem for me. I'm working on a project in which correct font rendering is critical. As a temporary workaround I have applied lineHeight bigger than fontSize. |
@henrytao-me it seems that you've made a pull-request against your own forked react-native repo whilst you should have made it against the Facebook repo. Making a pull-request against master is going to be the most effective way of getting some decent feedback. Please check up on #14611 |
Thanks @peterp for suggestion. Some how I missed this conversation. I will create new PR soon. |
Facing the same issues...any alternatives?? |
I've opened a PR fixing this issue. The reason I worked on a simpler solution than @henrytao-me's proposed solution is that the number of cases when working with font rendering is high and the only difference between Android and iOS rendering is the priority of ascent over descent. The change in #16448 touches only that and adds some unit test coverage for confidence that nothing else changes in the calculation. I'm hoping this will make it easier to review and land this fix soon. |
Thanks @bartolkaruza. I was busy recently on other stuffs. 🙇 |
@bartolkaruza that link does not seem to be correct, I guess it's #16448 ? |
Thank you @guidobouman, I've edited my reference as well. |
…t behaviour Summary: We noticed that on Android the lineHeight behaviour is different from iOS for built in fonts and custom fonts. The problem becomes visible when the lineHeight approaches the fontSize, showing a cut-off on the bottom of the TextView. This issue has been raised before in #10712. There is a mention of a PR with a fix in that issue, which has not been merged yet. This implementation is a less intrusive fix leaving the current lineHeight approach in place and fixing the discrepancy only. This proposed change prioritises ascent over descent for reduction, making the lineHeight functionality behave identical to iOS. There is no existing test covering the lineHeight property and its behaviour in the CustomLineHeightSpan. This PR contains new unit tests that covers the various scenario's for the lineHeight calculations. The original behaviour, before the change can against these unit tests. The case that fails is `shouldReduceAscentThird`, which can be made to succeed on the old code by changing the asserts to: ``` assertThat(fm.top).isEqualTo(-5); assertThat(fm.ascent).isEqualTo(-5); assertThat(fm.descent).isEqualTo(-4); assertThat(fm.bottom).isEqualTo(-4); ``` The unit test succeeds for the current implementation, which has the values for ascent and descent inverted. Below screenshots show before, after and iOS: BEFORE ![screen shot 2017-10-18 at 15 35 41](https://user-images.githubusercontent.com/1605731/31721688-58d7086a-b41a-11e7-8186-9a201e2acb01.png) AFTER ![screen shot 2017-10-18 at 15 37 02](https://user-images.githubusercontent.com/1605731/31721665-473cf86c-b41a-11e7-94d5-7a70eaf99889.png) iOS ![screen shot 2017-10-18 at 15 35 22](https://user-images.githubusercontent.com/1605731/31721712-707e30a6-b41a-11e7-9baa-f886a66837e6.png) [ANDROID] [BUGFIX] [Text] - Fix the lineHeight behaviour on Android to match iOS Closes #16448 Differential Revision: D6221854 Pulled By: andreicoman11 fbshipit-source-id: 7292f0f05f212d79678ac9d73e8a46bf93f1a7c6
…t behaviour Summary: We noticed that on Android the lineHeight behaviour is different from iOS for built in fonts and custom fonts. The problem becomes visible when the lineHeight approaches the fontSize, showing a cut-off on the bottom of the TextView. This issue has been raised before in facebook#10712. There is a mention of a PR with a fix in that issue, which has not been merged yet. This implementation is a less intrusive fix leaving the current lineHeight approach in place and fixing the discrepancy only. This proposed change prioritises ascent over descent for reduction, making the lineHeight functionality behave identical to iOS. There is no existing test covering the lineHeight property and its behaviour in the CustomLineHeightSpan. This PR contains new unit tests that covers the various scenario's for the lineHeight calculations. The original behaviour, before the change can against these unit tests. The case that fails is `shouldReduceAscentThird`, which can be made to succeed on the old code by changing the asserts to: ``` assertThat(fm.top).isEqualTo(-5); assertThat(fm.ascent).isEqualTo(-5); assertThat(fm.descent).isEqualTo(-4); assertThat(fm.bottom).isEqualTo(-4); ``` The unit test succeeds for the current implementation, which has the values for ascent and descent inverted. Below screenshots show before, after and iOS: BEFORE ![screen shot 2017-10-18 at 15 35 41](https://user-images.githubusercontent.com/1605731/31721688-58d7086a-b41a-11e7-8186-9a201e2acb01.png) AFTER ![screen shot 2017-10-18 at 15 37 02](https://user-images.githubusercontent.com/1605731/31721665-473cf86c-b41a-11e7-94d5-7a70eaf99889.png) iOS ![screen shot 2017-10-18 at 15 35 22](https://user-images.githubusercontent.com/1605731/31721712-707e30a6-b41a-11e7-9baa-f886a66837e6.png) [ANDROID] [BUGFIX] [Text] - Fix the lineHeight behaviour on Android to match iOS Closes facebook#16448 Differential Revision: D6221854 Pulled By: andreicoman11 fbshipit-source-id: 7292f0f05f212d79678ac9d73e8a46bf93f1a7c6
Any updates on getting this in a release? I saw it brought up in the 0.51 issue thread but it doesn't seem to have been released. Let me know if there's anything I can do to help speed things along. |
It is on the 0.52-stable branch. I think there is 0.52 rc you could point
to if you need it right now.
|
0.52 has been fully released! Should be good to go... |
Cool. I will check it soon. Back to react native now. |
I updated to 0.52 and it's a lot better, but still not quite correct. There is a weird issue with multiline text where depending on the relation between the font size and the line height the line-height is actually slightly shorter when one Text element spans multiple lines compared to the same number of Text elements with only one line (with no additional padding or margins) -- E.g. with My device has 2px pr pt so when i measure the screenshot i find the total height of the box with an explicit height of 288pt to be 576px as expected (288*2=576). But the 9 lines is for some reason only 567px which gives a total height of 283.5pt which divided by 9 gives an actual lineHeight of 31.5pt, measuring the distance between the bottom of the letters also gives the same lineHeight. While the single Text element with nine lines is even shorter at 526px which is 263pt, divided by 9 this gives an actual lineHeight of 29.22222... which is weird. But when i measure the distance between the lines it's only 58px = 29pt which means there is some magic padding of 1pt added top and bottom even though I explicitly set no padding. This is just one example fontsize/lineheight combo. Different combinations gives different results. Which makes me suspect some of it is caused by some weird rounding error. A larger font size makes the line-height even shorter. So perhaps the font size is somehow rounded up and then subtracted from the lineheight or something? Edit: I don't fully understand the code but all the explicit and implicit rounding/truncation going on, makes me think that this could be part of the issue. There probably should be added a test checking that the computed total height of a multi line element is equal to the line height specified times the number of lines (or at least within +/- 1 px). Is there a good reason why the calculations doesn't use floats all the way? Or at least for all the calculations? Snippet to reproduce: // style.js
export default StyleSheet.create({
container: {
flexDirection: 'column',
height: '100%',
},
bio: {
marginVertical: 0,
paddingVertical: 0,
margin: 0,
padding: 0,
fontSize: 16,
lineHeight: 32,
},
debug1: { backgroundColor: 'gray' },
debug2: { backgroundColor: 'pink' },
})
// Render
// ...
return (
<View style={styles.container}>
<ScrollView style={styles.descWrapper}>
<Text style={[styles.bio, { marginVertical: 50 }]}>Example:</Text>
<View style={{ flexDirection: 'row' }}>
<View style={{ flex: 1 }}>
<Text style={[styles.bio, styles.debug1, { height: 32 * 9 }]}>
E
</Text>
</View>
<View style={{ flex: 1 }}>
<Text style={[styles.bio, styles.debug2]}>A</Text>
<Text style={[styles.bio, styles.debug1]}>A</Text>
<Text style={[styles.bio, styles.debug2]}>A</Text>
<Text style={[styles.bio, styles.debug1]}>A</Text>
<Text style={[styles.bio, styles.debug2]}>A</Text>
<Text style={[styles.bio, styles.debug1]}>A</Text>
<Text style={[styles.bio, styles.debug2]}>A</Text>
<Text style={[styles.bio, styles.debug1]}>A</Text>
<Text style={[styles.bio, styles.debug2]}>A</Text>
</View>
<View style={{ flex: 1 }}>
<Text
style={[styles.bio, styles.debug2]}
numberOfLines={9}
ellipsizeMode="tail"
>
Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do
eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut
enim ad minim veniam, quis nostrud exercitation ullamco laboris
nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in
reprehenderit in voluptate velit esse cillum dolore eu fugiat
nulla pariatur. Excepteur sint occaecat cupidatat non proident,
sunt in culpa qui officia deserunt mollit anim id est laborum.
</Text>
</View>
</View>
</ScrollView>
</View>
); |
I looked a bit closer at the final else block: } else {
// Show proportionally additional ascent / top & descent / bottom
final int additional = mHeight - (-fm.top + fm.bottom);
fm.top -= additional / 2;
fm.ascent -= additional / 2;
fm.descent += additional / 2;
fm.bottom += additional / 2;
} And I think the integer division is definitely part of the problem: Let's say that (-fm.top + fm.bottom) happens to be 10.1 and mHeight is 40. Then this is divided by two which should have been 29.9/2 == 14.95, but again since So the total extra height added becomes only 28 even though the calculation really should have added 29.9. This 1.9 difference is probably later rounded up to exactly 2 when when the font is rendered as native pixels (since 1.9, 2.8 (2x) and 5.7 (3x) all rounds up). So this explains the random 1 or 2 pt missing in the multiple elements case that appears or disappears depending on the fontSize. But it doesn't completely explain the multiline issue. That's probably because the actual distance between the baselines isn't adjusted correctly either. Where is the code that adjusts the leading, height, lineSpaceMultiplier or whatever is used to control the distance between the baselines? |
Summary: There seems to be a rounding error in the android code for line height, so that for some fonts and at some combinations of line height and font size the actual height of the elements seems to be slightly too short. I've identified one issue that I mentioned here #10712 (comment) that could at least explain some of the problem. That when the line-height minus the original sum of the absolute value of top and bottom from the metrics, happens to be an odd number, the division by two causes a rounding error of 1, so that the actual line height is 1pt less than it should. The fix uses floating point division instead of integer division, and rounds (arbitrarily) the negative values up and the positive values down so that the total is still the correct for odd numbers. It turns out that only ascent and descent is used to give the actual line-height between lines in the same text-element. The top and bottom values are only used for padding the top and bottom of the text. So when the line-height is greater than the font size and the extra padding this PR sets the ascent and descent to the same value as the top and bottom respectively. I've renamed the shouldIncreaseAllMetricsProportionally test to evenLineHeightShouldIncreaseAllMetricsProportionally and added an extra assertion to check that bottom-top still equals the line height. Added another test oddLineHeightShouldAlsoWork that is similar but uses an odd number for the line height to test that it still works with odd numbers. This test only uses the sum of the values so that it's indifferent to what value the implementation chooses to round up or down. Improvement on #16448 Fix line-height calculation on Android. | Before | After | | ------------- |-------------| | ![without fix](https://user-images.githubusercontent.com/2144849/36150230-4404a0cc-10c3-11e8-8880-4ab84339c741.png) | ![actual fix](https://user-images.githubusercontent.com/2144849/36156620-eb496d0e-10d7-11e8-8bd1-1cb536a38fbf.png) | (All three columns have font size 16 and lineHeight: 32. The first one is has fixed height 9*32, the second is 9 Text elements, the last is one text element with lots of text limited to 9 lines, so they should be the same height. ) Closes #17952 Differential Revision: D6980333 Pulled By: hramos fbshipit-source-id: 0a501358cfbf7f139fca46056d0d972b1daf6ae3
This should be resolved now that 74e54cb landed. |
…t behaviour Summary: We noticed that on Android the lineHeight behaviour is different from iOS for built in fonts and custom fonts. The problem becomes visible when the lineHeight approaches the fontSize, showing a cut-off on the bottom of the TextView. This issue has been raised before in facebook#10712. There is a mention of a PR with a fix in that issue, which has not been merged yet. This implementation is a less intrusive fix leaving the current lineHeight approach in place and fixing the discrepancy only. This proposed change prioritises ascent over descent for reduction, making the lineHeight functionality behave identical to iOS. There is no existing test covering the lineHeight property and its behaviour in the CustomLineHeightSpan. This PR contains new unit tests that covers the various scenario's for the lineHeight calculations. The original behaviour, before the change can against these unit tests. The case that fails is `shouldReduceAscentThird`, which can be made to succeed on the old code by changing the asserts to: ``` assertThat(fm.top).isEqualTo(-5); assertThat(fm.ascent).isEqualTo(-5); assertThat(fm.descent).isEqualTo(-4); assertThat(fm.bottom).isEqualTo(-4); ``` The unit test succeeds for the current implementation, which has the values for ascent and descent inverted. Below screenshots show before, after and iOS: BEFORE ![screen shot 2017-10-18 at 15 35 41](https://user-images.githubusercontent.com/1605731/31721688-58d7086a-b41a-11e7-8186-9a201e2acb01.png) AFTER ![screen shot 2017-10-18 at 15 37 02](https://user-images.githubusercontent.com/1605731/31721665-473cf86c-b41a-11e7-94d5-7a70eaf99889.png) iOS ![screen shot 2017-10-18 at 15 35 22](https://user-images.githubusercontent.com/1605731/31721712-707e30a6-b41a-11e7-9baa-f886a66837e6.png) [ANDROID] [BUGFIX] [Text] - Fix the lineHeight behaviour on Android to match iOS Closes facebook#16448 Differential Revision: D6221854 Pulled By: andreicoman11 fbshipit-source-id: 7292f0f05f212d79678ac9d73e8a46bf93f1a7c6
Summary: There seems to be a rounding error in the android code for line height, so that for some fonts and at some combinations of line height and font size the actual height of the elements seems to be slightly too short. I've identified one issue that I mentioned here facebook#10712 (comment) that could at least explain some of the problem. That when the line-height minus the original sum of the absolute value of top and bottom from the metrics, happens to be an odd number, the division by two causes a rounding error of 1, so that the actual line height is 1pt less than it should. The fix uses floating point division instead of integer division, and rounds (arbitrarily) the negative values up and the positive values down so that the total is still the correct for odd numbers. It turns out that only ascent and descent is used to give the actual line-height between lines in the same text-element. The top and bottom values are only used for padding the top and bottom of the text. So when the line-height is greater than the font size and the extra padding this PR sets the ascent and descent to the same value as the top and bottom respectively. I've renamed the shouldIncreaseAllMetricsProportionally test to evenLineHeightShouldIncreaseAllMetricsProportionally and added an extra assertion to check that bottom-top still equals the line height. Added another test oddLineHeightShouldAlsoWork that is similar but uses an odd number for the line height to test that it still works with odd numbers. This test only uses the sum of the values so that it's indifferent to what value the implementation chooses to round up or down. Improvement on facebook#16448 Fix line-height calculation on Android. | Before | After | | ------------- |-------------| | ![without fix](https://user-images.githubusercontent.com/2144849/36150230-4404a0cc-10c3-11e8-8880-4ab84339c741.png) | ![actual fix](https://user-images.githubusercontent.com/2144849/36156620-eb496d0e-10d7-11e8-8bd1-1cb536a38fbf.png) | (All three columns have font size 16 and lineHeight: 32. The first one is has fixed height 9*32, the second is 9 Text elements, the last is one text element with lots of text limited to 9 lines, so they should be the same height. ) Closes facebook#17952 Differential Revision: D6980333 Pulled By: hramos fbshipit-source-id: 0a501358cfbf7f139fca46056d0d972b1daf6ae3
Summary: There seems to be a rounding error in the android code for line height, so that for some fonts and at some combinations of line height and font size the actual height of the elements seems to be slightly too short. I've identified one issue that I mentioned here facebook#10712 (comment) that could at least explain some of the problem. That when the line-height minus the original sum of the absolute value of top and bottom from the metrics, happens to be an odd number, the division by two causes a rounding error of 1, so that the actual line height is 1pt less than it should. The fix uses floating point division instead of integer division, and rounds (arbitrarily) the negative values up and the positive values down so that the total is still the correct for odd numbers. It turns out that only ascent and descent is used to give the actual line-height between lines in the same text-element. The top and bottom values are only used for padding the top and bottom of the text. So when the line-height is greater than the font size and the extra padding this PR sets the ascent and descent to the same value as the top and bottom respectively. I've renamed the shouldIncreaseAllMetricsProportionally test to evenLineHeightShouldIncreaseAllMetricsProportionally and added an extra assertion to check that bottom-top still equals the line height. Added another test oddLineHeightShouldAlsoWork that is similar but uses an odd number for the line height to test that it still works with odd numbers. This test only uses the sum of the values so that it's indifferent to what value the implementation chooses to round up or down. Improvement on facebook#16448 Fix line-height calculation on Android. | Before | After | | ------------- |-------------| | ![without fix](https://user-images.githubusercontent.com/2144849/36150230-4404a0cc-10c3-11e8-8880-4ab84339c741.png) | ![actual fix](https://user-images.githubusercontent.com/2144849/36156620-eb496d0e-10d7-11e8-8bd1-1cb536a38fbf.png) | (All three columns have font size 16 and lineHeight: 32. The first one is has fixed height 9*32, the second is 9 Text elements, the last is one text element with lots of text limited to 9 lines, so they should be the same height. ) Closes facebook#17952 Differential Revision: D6980333 Pulled By: hramos fbshipit-source-id: 0a501358cfbf7f139fca46056d0d972b1daf6ae3
Summary: There seems to be a rounding error in the android code for line height, so that for some fonts and at some combinations of line height and font size the actual height of the elements seems to be slightly too short. I've identified one issue that I mentioned here facebook#10712 (comment) that could at least explain some of the problem. That when the line-height minus the original sum of the absolute value of top and bottom from the metrics, happens to be an odd number, the division by two causes a rounding error of 1, so that the actual line height is 1pt less than it should. The fix uses floating point division instead of integer division, and rounds (arbitrarily) the negative values up and the positive values down so that the total is still the correct for odd numbers. It turns out that only ascent and descent is used to give the actual line-height between lines in the same text-element. The top and bottom values are only used for padding the top and bottom of the text. So when the line-height is greater than the font size and the extra padding this PR sets the ascent and descent to the same value as the top and bottom respectively. I've renamed the shouldIncreaseAllMetricsProportionally test to evenLineHeightShouldIncreaseAllMetricsProportionally and added an extra assertion to check that bottom-top still equals the line height. Added another test oddLineHeightShouldAlsoWork that is similar but uses an odd number for the line height to test that it still works with odd numbers. This test only uses the sum of the values so that it's indifferent to what value the implementation chooses to round up or down. Improvement on facebook#16448 Fix line-height calculation on Android. | Before | After | | ------------- |-------------| | ![without fix](https://user-images.githubusercontent.com/2144849/36150230-4404a0cc-10c3-11e8-8880-4ab84339c741.png) | ![actual fix](https://user-images.githubusercontent.com/2144849/36156620-eb496d0e-10d7-11e8-8bd1-1cb536a38fbf.png) | (All three columns have font size 16 and lineHeight: 32. The first one is has fixed height 9*32, the second is 9 Text elements, the last is one text element with lots of text limited to 9 lines, so they should be the same height. ) Closes facebook#17952 Differential Revision: D6980333 Pulled By: hramos fbshipit-source-id: 0a501358cfbf7f139fca46056d0d972b1daf6ae3
I faced this problem again in my RN 0.59.5 on Android 9.0 users. Users on Android 8.0 and 7.0 are working fine. iOS users are working fine too. Currently, I fixed the issue by providing
Hope this is helpful for anyone who is facing this issue. |
Not sure if other folks are experiencing this, and not sure if it's only with Expo for whatever reason, but |
Description
On ios:
On Android:
note how the on android it seems like the line height is only applied on top of the text, while on ios it's applied to both top and bottom. This is also an issue when you have lineheights < fontSize. On ios it trims from the top, but on android it'll trim from top and bottom, so the text doesn't line up and get's truncated differently.
Reproduction
RNplay demo: https://rnplay.org/apps/OJFGEg
Additional Information
The text was updated successfully, but these errors were encountered: