Inverted descent/ascent Android prioritisation to match iOS lineHeight behaviour

[bartolkaruza]

Motivation

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.

Test Plan

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

AFTER

iOS

Release Notes

[ANDROID] [BUGFIX] [Text] - Fix the lineHeight behaviour on Android to match iOS

[bartolkaruza]

@andreicoman11 Would you be able to review this PR as the original author of CustomLineHeightSpan?

[bartolkaruza]

@shergin Hi, any chance you could review this PR? Thanks

[bartolkaruza]

@hramos This PR is a small consistency fix, with a big positive impact; bringing Android and iOS behaviour in line and fixing broken behaviour on Android.
Before this fix text rendering on Android is just plain broken and there are multiple parties that I know of that are maintaining forks for this fix alone. Try setting the lineHeight to a a value close to 1x the fontSize and you will see the issue right away.
I'm having trouble getting this reviewed though. Could you help me/us out by getting someone's attention for a review? Thank you :)

[andreicoman11]

Hi, thanks for taking a swing at this, this definitely looks like something that is worth fixing.
Reviewing this code is a bit difficult though, since there are a number of different cases that all need to be tackled in a different way. That is the reason why the initial implementation was split into a couple of if-else statements. They make it easy to understand what cases there are, when and how are they handled.
The new implementation, while more elegant and less repetitive, is more difficult to understand. Do you mind changing it back to it's previous implementation style?
The tests are great, thanks for adding them!

[bartolkaruza]

@andreicoman11 Thanks for taking a look. I've changed the fix to be in the same style as your original implementation. It should be easier to review.
The reason I changed the style of implementation was that I wanted an implementation that could easily handle a re-ordering of the priorities. Considering this will not likely happen anytime soon after this PR, let's just keep it simple. 👍

[andreicoman11]

Thanks, looks good!

[facebook-github-bot]

@andreicoman11 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[andreicoman11]

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

[dickie81]

@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:

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>```

[dickie81]

Restoring fm.ascent -= additional; gives me this behaviour:

[dickie81]

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:

[bartolkaruza]

@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.

[bartolkaruza]

@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:

Original implementation before PR:

Implementation before review changes:

Proposed change by @dickie81 :

[dickie81]

@bartolkaruza: Sorry, I was not very clear when I proposed the change. I only meant for the code in the last else statement to be changed:

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;

  CustomLineHeightSpan(float height) {
    this.mHeight = (int) Math.ceil(height);
  }

  @Override
  public void chooseHeight(
          CharSequence text,
          int start,
          int end,
          int spanstartv,
          int v,
          Paint.FontMetricsInt fm) {
    // 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: descent, ascent, bottom, top

    if (-fm.ascent > mHeight) {
      // Show as much descent as possible
      fm.bottom = fm.descent = mHeight;
      fm.top = fm.ascent = 0;
    } else if (-fm.ascent + fm.descent > mHeight) {
      // Show all descent, and as much ascent as possible
      fm.bottom = fm.descent;
      fm.top = fm.ascent = -mHeight + fm.descent;
    } 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 / 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;
    }
  }
}

[dickie81]

This is the result I get with the above code:

[dickie81]

@bartolkaruza: It still seems your implementation before the review changes handled scenarios when line height is less than font sizes better.

[bartolkaruza]

There was a bug still in the low lineHeight case and that fix combined with @dickie81's proposed proportional change leads to the below desired results:

@andreicoman11 The PR is ready now, the fixes and the license header have been pushed and all tests are passing.

[facebook-github-bot]

@andreicoman11 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[strindhaug]

Why are you using integers? This will cause rounding errors since aditional is an int and 2 is an int so anything that is not divisible by two will be rounded down. (See: #10712 (comment) )

[bartolkaruza]

Hi, for us it has solved the problems in the design completely, but I see that there is still something wrong for your design case. Maybe you can open up a new issue for this and see if you can get the attention of other developers, linking to these issues, to get your specific case fixed. Even better to open a new issue, with a PR that fixes it.