Skip to content

Commit

Permalink
Fix: incorrect line-height calculation
Browse files Browse the repository at this point in the history
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
  • Loading branch information
strindhaug authored and facebook-github-bot committed Feb 13, 2018
1 parent 331cc79 commit 74e54cb
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,12 @@ public void chooseHeight(
// 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;
// Round up for the negative values and down for the positive values (arbritary choice)
// So that bottom - top equals additional even if it's an odd number.
fm.top -= Math.ceil(additional / 2.0f);
fm.bottom += Math.floor(additional / 2.0f);
fm.ascent = fm.top;

This comment has been minimized.

Copy link
@rikur

rikur Sep 8, 2019

Why was the / 2 removed here altogether?

fm.descent = fm.bottom;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,35 @@
public class CustomLineHeightSpanTest {

@Test
public void shouldIncreaseAllMetricsProportionally() {
public void evenLineHeightShouldIncreaseAllMetricsProportionally() {
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(22);
Paint.FontMetricsInt fm = new Paint.FontMetricsInt();
fm.top = -10;
fm.ascent = -5;
fm.descent = 5;
fm.bottom = 10;
customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm);
// Since line height is even it should be equally added to top and bottom.
assertThat(fm.top).isEqualTo(-11);
assertThat(fm.ascent).isEqualTo(-6);
assertThat(fm.descent).isEqualTo(6);
assertThat(fm.ascent).isEqualTo(-11);
assertThat(fm.descent).isEqualTo(11);
assertThat(fm.bottom).isEqualTo(11);
assertThat(fm.bottom - fm.top).isEqualTo(22);
}

@Test
public void oddLineHeightShouldAlsoWork() {
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(23);
Paint.FontMetricsInt fm = new Paint.FontMetricsInt();
fm.top = -10;
fm.ascent = -5;
fm.descent = 5;
fm.bottom = 10;
customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm);
// Only test that the sum is correct so the implementation
// is free to add the odd value either on top or bottom.
assertThat(fm.descent - fm.ascent).isEqualTo(23);
assertThat(fm.bottom - fm.top).isEqualTo(23);
}

@Test
Expand Down

1 comment on commit 74e54cb

@katieisnell
Copy link

Choose a reason for hiding this comment

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

Unfortunately I still observe this issue on version 0.59.5.

Please sign in to comment.