Skip to content

Commit 3f1b021

Browse files
Bartol Karuzafacebook-github-bot
Bartol Karuza
authored andcommitted
Inverted descent/ascent Android prioritisation to match iOS lineHeight 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
1 parent b9f21dc commit 3f1b021

File tree

2 files changed

+111
-12
lines changed

2 files changed

+111
-12
lines changed

ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java

+22-12
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
1-
// Copyright 2004-present Facebook. All Rights Reserved.
1+
/**
2+
* Copyright (c) 2015-present, Facebook, Inc.
3+
* All rights reserved.
4+
*
5+
* This source code is licensed under the BSD-style license found in the
6+
* LICENSE file in the root directory of this source tree. An additional grant
7+
* of patent rights can be found in the PATENTS file in the same directory.
8+
*/
29

310
package com.facebook.react.views.text;
411

@@ -27,16 +34,16 @@ public void chooseHeight(
2734
// This is more complicated that I wanted it to be. You can find a good explanation of what the
2835
// FontMetrics mean here: http://stackoverflow.com/questions/27631736.
2936
// The general solution is that if there's not enough height to show the full line height, we
30-
// will prioritize in this order: ascent, descent, bottom, top
37+
// will prioritize in this order: descent, ascent, bottom, top
3138

32-
if (-fm.ascent > mHeight) {
33-
// Show as much ascent as possible
34-
fm.top = fm.ascent = -mHeight;
35-
fm.bottom = fm.descent = 0;
39+
if (fm.descent > mHeight) {
40+
// Show as much descent as possible
41+
fm.bottom = fm.descent = Math.min(mHeight, fm.descent);
42+
fm.top = fm.ascent = 0;
3643
} else if (-fm.ascent + fm.descent > mHeight) {
37-
// Show all ascent, and as much descent as possible
38-
fm.top = fm.ascent;
39-
fm.bottom = fm.descent = mHeight + fm.ascent;
44+
// Show all descent, and as much ascent as possible
45+
fm.bottom = fm.descent;
46+
fm.top = fm.ascent = -mHeight + fm.descent;
4047
} else if (-fm.ascent + fm.bottom > mHeight) {
4148
// Show all ascent, descent, as much bottom as possible
4249
fm.top = fm.ascent;
@@ -45,10 +52,13 @@ public void chooseHeight(
4552
// Show all ascent, descent, bottom, as much top as possible
4653
fm.top = fm.bottom - mHeight;
4754
} else {
48-
// Show proportionally additional ascent and top
55+
// Show proportionally additional ascent / top & descent / bottom
4956
final int additional = mHeight - (-fm.top + fm.bottom);
50-
fm.top -= additional;
51-
fm.ascent -= additional;
57+
58+
fm.top -= additional / 2;
59+
fm.ascent -= additional / 2;
60+
fm.descent += additional / 2;
61+
fm.bottom += additional / 2;
5262
}
5363
}
5464
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
package com.facebook.react.views.text;
2+
3+
import android.graphics.Paint;
4+
5+
import static org.fest.assertions.api.Assertions.assertThat;
6+
import org.junit.Test;
7+
import org.junit.runner.RunWith;
8+
import org.powermock.core.classloader.annotations.PowerMockIgnore;
9+
import org.robolectric.RobolectricTestRunner;
10+
11+
@RunWith(RobolectricTestRunner.class)
12+
@PowerMockIgnore({"org.mockito.*", "org.robolectric.*", "android.*"})
13+
public class CustomLineHeightSpanTest {
14+
15+
@Test
16+
public void shouldIncreaseAllMetricsProportionally() {
17+
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(22);
18+
Paint.FontMetricsInt fm = new Paint.FontMetricsInt();
19+
fm.top = -10;
20+
fm.ascent = -5;
21+
fm.descent = 5;
22+
fm.bottom = 10;
23+
customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm);
24+
assertThat(fm.top).isEqualTo(-11);
25+
assertThat(fm.ascent).isEqualTo(-6);
26+
assertThat(fm.descent).isEqualTo(6);
27+
assertThat(fm.bottom).isEqualTo(11);
28+
}
29+
30+
@Test
31+
public void shouldReduceTopFirst() {
32+
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(19);
33+
Paint.FontMetricsInt fm = new Paint.FontMetricsInt();
34+
fm.top = -10;
35+
fm.ascent = -5;
36+
fm.descent = 5;
37+
fm.bottom = 10;
38+
customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm);
39+
assertThat(fm.top).isEqualTo(-9);
40+
assertThat(fm.ascent).isEqualTo(-5);
41+
assertThat(fm.descent).isEqualTo(5);
42+
assertThat(fm.bottom).isEqualTo(10);
43+
}
44+
45+
@Test
46+
public void shouldReduceBottomSecond() {
47+
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(14);
48+
Paint.FontMetricsInt fm = new Paint.FontMetricsInt();
49+
fm.top = -10;
50+
fm.ascent = -5;
51+
fm.descent = 5;
52+
fm.bottom = 10;
53+
customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm);
54+
assertThat(fm.top).isEqualTo(-5);
55+
assertThat(fm.ascent).isEqualTo(-5);
56+
assertThat(fm.descent).isEqualTo(5);
57+
assertThat(fm.bottom).isEqualTo(9);
58+
}
59+
60+
@Test
61+
public void shouldReduceAscentThird() {
62+
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(9);
63+
Paint.FontMetricsInt fm = new Paint.FontMetricsInt();
64+
fm.top = -10;
65+
fm.ascent = -5;
66+
fm.descent = 5;
67+
fm.bottom = 10;
68+
customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm);
69+
assertThat(fm.top).isEqualTo(-4);
70+
assertThat(fm.ascent).isEqualTo(-4);
71+
assertThat(fm.descent).isEqualTo(5);
72+
assertThat(fm.bottom).isEqualTo(5);
73+
}
74+
75+
@Test
76+
public void shouldReduceDescentLast() {
77+
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(4);
78+
Paint.FontMetricsInt fm = new Paint.FontMetricsInt();
79+
fm.top = -10;
80+
fm.ascent = -5;
81+
fm.descent = 5;
82+
fm.bottom = 10;
83+
customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm);
84+
assertThat(fm.top).isEqualTo(0);
85+
assertThat(fm.ascent).isEqualTo(0);
86+
assertThat(fm.descent).isEqualTo(4);
87+
assertThat(fm.bottom).isEqualTo(4);
88+
}
89+
}

0 commit comments

Comments
 (0)