From 74e54cbcc408a8bbdd70f47cc8728d30cdc0d299 Mon Sep 17 00:00:00 2001 From: Stein Strindhaug Date: Tue, 13 Feb 2018 13:31:50 -0800 Subject: [PATCH] Fix: incorrect line-height calculation 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 https://github.com/facebook/react-native/issues/10712#issuecomment-359382137 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 https://github.com/facebook/react-native/pull/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 https://github.com/facebook/react-native/pull/17952 Differential Revision: D6980333 Pulled By: hramos fbshipit-source-id: 0a501358cfbf7f139fca46056d0d972b1daf6ae3 --- .../views/text/CustomLineHeightSpan.java | 10 ++++---- .../views/text/CustomLineHeightSpanTest.java | 23 ++++++++++++++++--- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java index 18645e56d..abba786b1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java @@ -55,10 +55,12 @@ public class CustomLineHeightSpan implements LineHeightSpan { // 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; + fm.descent = fm.bottom; } } } diff --git a/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java b/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java index 5d3919105..eadb7d354 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java @@ -13,7 +13,7 @@ import org.robolectric.RobolectricTestRunner; public class CustomLineHeightSpanTest { @Test - public void shouldIncreaseAllMetricsProportionally() { + public void evenLineHeightShouldIncreaseAllMetricsProportionally() { CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(22); Paint.FontMetricsInt fm = new Paint.FontMetricsInt(); fm.top = -10; @@ -21,10 +21,27 @@ public class CustomLineHeightSpanTest { 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