[Android] Fix: incorrect line-height calculation

[strindhaug]

Motivation

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.

Test Plan

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.

Related PRs

Improvement on #16448

Release Notes

Fix line-height calculation on Android.

Before	After

(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. )

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[pull-bot]

	Warnings
⚠️	📋 Release Notes - This PR may have incorrectly formatted Release Notes.

Generated by 🚫 dangerJS

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[strindhaug]

Not sure how I can actually test these changes in a project. Is there some quick and dirty way I can just replace the modified file and deploy it on my phone in an existing project; without having to do a full build?
(Something like replacing a class file somewhere and re-run react-native run-android, or symlink to my checkout of this repo or anything like that?)

Making a full build would be hard, since I'm having a lot of issues with getting the full build env running on my linux laptop.

[strindhaug]

I tried using only floats, but it seems everything expects it to be integers, so I decided to use rounding instead and add the remainder to the negative values.

[janicduplessis]

@strindhaug I think the easiest way to test is copy your changes under node_modules and run ./gradlew installArchives in node_modules/react-native, then rebuild your app.

[janicduplessis]

You can probably also test in the example app RN Tester, you can run it with ./gradlew RNTester:android:app:installDebug. Pre-requisites to build from source are listed here http://facebook.github.io/react-native/docs/android-building-from-source.html.

[strindhaug]

Finally I at least got the RNTester app working, so I could test my line-height bug demo.

It fixes the first problem of the multiple Text nodes being 1px too short, as I expected.
But while the multiline Text node is now 1px longer for each line, it's still 5px shorter than it should.
So there still is another bug somewhere making the multi-line line-height too short.

Before

Multi element line-height is 63px, multiline single element has line height 58px

After applying the changes from this PR:

Multi element line-height is 64px, multiline single element has line height 59px

[strindhaug]

I finally figured it out! The multiline version were shorter than it should because apparently the top and bottom values are only used for the extra padding at the top and bottom, the ascent and descent is the ones that actually control the internal line height.

Since in the last else section the desired line-height is larger than both the top+bottom and ascent+descent, it's safe to set the ascent and descent to the same values as top and bottom respectively. So that the resulting line-height actually matches the desired line-height:

[janicduplessis]

@strindhaug Awesome work, let me know if anything is unclear in the doc for building on linux, I'd like to make contributing as easy as possible.

[janicduplessis]

@facebook-github-bot shipit

[facebook-github-bot]

Something went wrong executing that command, @hramos could you take a look?

[facebook-github-bot]

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

[strindhaug]

@janicduplessis
Well, the instructions does seem to sort of work. When running the RNTester app the error messages about missing packages doesn't seem to matter (at least for those views I tested.)

And the building from source by adding it to an existing project using all those changes in gradle, does seem to build react-native itself successfully without any error messages during build; but it fails to complete because react-native-sentry fails:

:react-native-sentry:compileReleaseJavaWithJavac
:react-native-sentry:compileReleaseJavaWithJavac - is not incremental (e.g. outputs have changed, no previous execution, etc.).
/home/stein/zeppelin/arctic/arctic-circle/node_modules/react-native-sentry/android/src/main/java/io/sentry/RNSentryModule.java:252: error: incompatible types: ReadableArray cannot be converted to ReadableNativeArray
            addExceptionInterface(eventBuilder, exception.getString("type"), exception.getString("value"), stacktrace.getArray("frames"));
                                                                                                                              ^
Note: /home/stein/zeppelin/arctic/arctic-circle/node_modules/react-native-sentry/android/src/main/java/io/sentry/RNSentryModule.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Note: Some messages have been simplified; recompile with -Xdiags:verbose to get full output
1 error
:react-native-sentry:compileReleaseJavaWithJavac FAILED

FAILURE: Build failed with an exception.

So either my build of the tag v0.52.0 is not identical with the release version of v0.52.0, or the release version of react-native-sentry is not built with the correct version of react-native but as long as it's not trying to compile it it somehow works. (It might work without react-native-sentry, but it's a lot of work removing it completely, so I haven't bothered testing)

But I do find it suspicious that when I run ./gradlew build it spews out error messages about missing packages and then 59 of 262 tests fails because of java.lang.NoClassDefFoundError:

com.facebook.react.views.text.ReactTextTest > testTextDecorationLineUnderlineLineThroughApplied FAILED
    java.lang.NoClassDefFoundError at ReactTextTest.java:380

com.facebook.react.views.text.ReactTextTest > testNormalFontStyleApplied FAILED
    java.lang.NoClassDefFoundError at ReactTextTest.java:380

262 tests completed, 59 failed
:ReactAndroid:testDebugUnitTest FAILED

(Running ./gradlew installArchives gives the same missing packages errors but claims the build was successful)

I grepped the output to find all the missing packages:

$ cat gradlew.build.output.txt |grep -o "package .* does not exist"|sort|uniq
package android.support.v4.app does not exist
package android.support.v4.net does not exist
package android.support.v4.text does not exist
package android.support.v4.util does not exist
package android.support.v4.view does not exist
package com.facebook.common.executors does not exist
package com.facebook.common.logging does not exist
package com.facebook.common.references does not exist
package com.facebook.common.soloader does not exist
package com.facebook.datasource does not exist
package com.facebook.drawee.backends.pipeline does not exist
package com.facebook.imagepipeline.backends.okhttp3 does not exist
package com.facebook.imagepipeline.core does not exist
package com.facebook.imagepipeline.image does not exist
package com.facebook.imagepipeline.listener does not exist
package com.facebook.imagepipeline.request does not exist
package com.facebook.react.common.build does not exist
package com.facebook.soloader does not exist
package ImagePipelineConfig does not exist
package Pools does not exist
package SoLoaderShim does not exist

Either I've missed some step to install these, or maybe I'm not supposed to run those gradle tasks?