Skip to content
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.

Sign up for GitHub

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

Inverted descent/ascent Android prioritisation to match iOS lineHeight behaviour #16448

Closed
wants to merge 6 commits into from
Closed

Inverted descent/ascent Android prioritisation to match iOS lineHeight behaviour #16448

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
80f15bf
Inverted descent/ascent prioritization to match iOS
Oct 18, 2017
6a2dbb1
fix ascent in case lineHeight is bigger than fontMetrics
Oct 18, 2017
6ac5133
revert fix to match style of initial implementation
Nov 2, 2017
5dc3294
add license header
Nov 4, 2017
b063523
Revert the committed change in behaviour from previous commit
Nov 5, 2017
c7e373e
fixed issue for large lineHeight and small lineHeight cases
Nov 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 30 additions & 22 deletions ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,28 +27,36 @@ public void chooseHeight(
// This is more complicated that I wanted it to be. You can find a good explanation of what the
// FontMetrics mean here: http://stackoverflow.com/questions/27631736.
// The general solution is that if there's not enough height to show the full line height, we
// will prioritize in this order: ascent, descent, bottom, top

if (-fm.ascent > mHeight) {
// Show as much ascent as possible
fm.top = fm.ascent = -mHeight;
fm.bottom = fm.descent = 0;
} else if (-fm.ascent + fm.descent > mHeight) {
// Show all ascent, and as much descent as possible
fm.top = fm.ascent;
fm.bottom = fm.descent = mHeight + fm.ascent;
} else if (-fm.ascent + fm.bottom > mHeight) {
// Show all ascent, descent, as much bottom as possible
fm.top = fm.ascent;
fm.bottom = fm.ascent + mHeight;
} else if (-fm.top + fm.bottom > mHeight) {
// Show all ascent, descent, bottom, as much top as possible
fm.top = fm.bottom - mHeight;
} else {
// Show proportionally additional ascent and top
final int additional = mHeight - (-fm.top + fm.bottom);
fm.top -= additional;
fm.ascent -= additional;
Copy link

@dickie81 dickie81 Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@bartolkaruza & @andreicoman11:
I've been looking at this PR and noticed a possible regression. Line height seems to be (more) broken on wrapped lines of text when line height is greater than font size. It seems to be related to this line of code.

Here is an example of what I'm seeing:
screen shot 2017-11-03 at 11 50 19

JSX:

<View style={{backgroundColor: "yellow", paddingTop: 100, paddingBottom: 100}}>
    <Text style={{fontSize: 20, lineHeight: 60, backgroundColor: "blue", color: "white"}}>
        Fontsize 20 Lineheight 60 qwertyuiopasdfghjklzxcvbnm QWERTYUIOPASDFGHJKLZXCVBNM
        Fontsize 20 Lineheight 60 qwertyuiopasdfghjklzxcvbnm QWERTYUIOPASDFGHJKLZXCVBNM
    </Text>
</View>```

Copy link

Choose a reason for hiding this comment

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

Restoring fm.ascent -= additional; gives me this behaviour:
screen shot 2017-11-03 at 11 56 40

Copy link

Choose a reason for hiding this comment

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

Proposed fix to make Android consistent with iOS:

      // 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;

Giving following behaviour:
screen shot 2017-11-03 at 12 11 40

Copy link
Author

Choose a reason for hiding this comment

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

@dickie81 Thank you for looking out, I did not check the full range of test cases on the actual UI when changing the style back to the original style, leaning on the unit tests instead. I'll try out your approach against the cases I have locally for all the lineHeight scenario's and post the test setup so everyone can verify.

Copy link
Author

@bartolkaruza bartolkaruza Nov 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@dickie81 I've been testing the ui effects and there is definitely a regression in the case of bigger lineHeight and even with very small lineHeight something broke.

Unfortunately your proposed solution still causes cut-off at the bottom, when the lineHeight is small.

@andreicoman11 I'm looking into this more in depth, this PR is not ready yet as it stands now. I'll try to improve it to cover all scenario's.

Here is the testing setup that you can use to verify on your end: https://gist.github.com/bartolkaruza/b74f228636bb78fea00aa8bd82e3766d

And these are the 4 different implementations compared to iOS. Not one is quite right yet.

Current PR state after review changes:

new-style-bartolkaruza

Original implementation before PR:

original-andreicoman11

Implementation before review changes:

original-bartolkaruza

Proposed change by @dickie81 :

proposed-dickie81

// will prioritize in this order: descent, ascent, bottom, top

int height = mHeight;
int currentHeight = -fm.top + fm.bottom;
int delta = currentHeight - height;
if (delta > 0) {
int bottomSpaceAvailable = fm.bottom - fm.descent;
int topSpaceAvailable = -fm.top - -fm.ascent;
if (topSpaceAvailable > 0 && delta > 0) {
int topSpaceTaken = topSpaceAvailable;
if (delta < topSpaceTaken) {
topSpaceTaken = delta;
}
delta -= topSpaceTaken;
fm.top = fm.ascent - (topSpaceAvailable - topSpaceTaken);
}
if (bottomSpaceAvailable > 0) {
int bottomSpaceTaken = bottomSpaceAvailable;
if (delta < bottomSpaceAvailable) {
bottomSpaceTaken = delta;
}
delta -= bottomSpaceTaken;
fm.bottom = fm.descent + bottomSpaceAvailable - bottomSpaceTaken;
}
}

fm.top += delta;
if(delta > 0) {
fm.ascent = Math.max(fm.top, fm.ascent - delta);
}

}
}
74 changes: 74 additions & 0 deletions ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package com.facebook.react.views.text;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing the license header, mind adding this so we can land this change?


import android.graphics.Paint;

import static org.fest.assertions.api.Assertions.assertThat;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.powermock.core.classloader.annotations.PowerMockIgnore;
import org.robolectric.RobolectricTestRunner;

@RunWith(RobolectricTestRunner.class)
@PowerMockIgnore({"org.mockito.*", "org.robolectric.*", "android.*"})
public class CustomLineHeightSpanTest {

@Test
public void shouldIncreaseTop() {
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(21);
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);
assertThat(fm.top).isEqualTo(-11);
assertThat(fm.ascent).isEqualTo(-5);
assertThat(fm.descent).isEqualTo(5);
assertThat(fm.bottom).isEqualTo(10);
}

@Test
public void shouldReduceTopFirst() {
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(19);
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);
assertThat(fm.top).isEqualTo(-9);
assertThat(fm.ascent).isEqualTo(-5);
assertThat(fm.descent).isEqualTo(5);
assertThat(fm.bottom).isEqualTo(10);
}

@Test
public void shouldReduceBottomSecond() {
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(14);
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);
assertThat(fm.top).isEqualTo(-5);
assertThat(fm.ascent).isEqualTo(-5);
assertThat(fm.descent).isEqualTo(5);
assertThat(fm.bottom).isEqualTo(9);
}

@Test
public void shouldReduceAscentThird() {
CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(9);
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);
assertThat(fm.top).isEqualTo(-4);
assertThat(fm.ascent).isEqualTo(-4);
assertThat(fm.descent).isEqualTo(5);
assertThat(fm.bottom).isEqualTo(5);
}
}