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

Jump to bottom

Android React Text Component includeFontPadding:false incorrect height #14606

Closed
mishagreenberg opened this issue Jun 19, 2017 · 5 comments
Closed

Android React Text Component includeFontPadding:false incorrect height #14606

mishagreenberg opened this issue Jun 19, 2017 · 5 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@mishagreenberg
Copy link

mishagreenberg commented Jun 19, 2017 •
edited by lelandrichardson

On Android, when includeFontPadding is set to false, the React Text component does not adjust its height. This makes it difficult to lay out other components at a precise spacing relative to a Text component.

For the native TextView component, the height is different depending on the value of includeFontPadding-

When includeFontPadding is set to true, the height of the TextView is based on abs(FontMetrics.top) + abs(FontMetrics.bottom).

When includeFontPadding to set to false, the height of the TextView is based on abs(FontMetrics.ascent) + abs(FontMetrics.descent).

This is happening because in ReactTextShadowNode.java, includeFontPadding is hardcoded to true when passing parameters to StaticLayout and BoringLayout. This seems like an easy fix.

An example from the measure function in ReactTextShadowNode.java -

private final YogaMeasureFunction mTextMeasureFunction = new YogaMeasureFunction() {

BoringLayout.make(
                text,
                textPaint,
                boring.width,
                Layout.Alignment.ALIGN_NORMAL,
                1.f,
                0.f,
                boring,
                true); // this parameter is includeFontPadding and should be based on the includeFontPadding value style of the component
...
};
@hramos hramos closed this as completed Jun 19, 2017
@dmueller39
Copy link

@hramos Can you provide a reason for closing the issue? Does it need more information?

@lelandrichardson
Copy link
Contributor

I'd like to reopen this issue as I believe font rendering remains to be one of the biggest reasons for cross-platform differences across iOS and Android and this is an important thing to explore.

@mishagreenberg mishagreenberg mentioned this issue Jun 19, 2017
Closed
@msand
Copy link
Contributor

msand commented Jun 20, 2017 •
edited

We're tracking similar issues in react-native-svg here: software-mansion/react-native-svg#362 and here software-mansion/react-native-svg#377

I've fixed most of the issues. Font rendering, ligatures, kerning, spacing etc. has a surprisingly big effect on the user / reading experience, especially if there are large amounts of text or larger font-sizes on headers and labels etc.

@mishagreenberg
Copy link
Author

#14609 is a PR to fix this issue

facebook-github-bot pushed a commit that referenced this issue Aug 18, 2017
@mgreenberg75 @facebook-github-bot
Size height of Android Text component based on includeFontPadding style
9f5bdd7 
Summary:
Overview -

This PR resolves the issue described in #14606. This PR makes Text components take into account the includeFontPadding property when calculating their size.

Background -

Currently, on Android, when includeFontPadding is set to false, the React Text component does not adjust its height. This makes it difficult to lay out other components at a precise spacing relative to a Text component.

iOS calculates the height of a UILabel based on the font's descent + ascent.

Android lets you choose whether to calculate the height of a TextView based on the font's top + bottom (includeFontPadding=true) or ascent + descent (includeFontPadding=false).

In order for a text component to be the same size on iOS and Android (relative to the rest of the layout in points and dips), one should set includeFontPadding=false on Android - but the React Text component needs to take this property into account when sizing itself for this to work.

Please see this stack overflow post for a visual explanation of the difference between a font's ascent/descent and top/bottom - https://stackoverflow.com/questions/27631736/meaning-of-top-ascent-baseline-descent-bottom-and-leading-in-androids-font

Testing -

Please see the attached screenshots to see the height difference of a Text component with this PR when includeFontPadding is true vs false.

The font I am using has an ascent + descent = em-size so that the height of the Text component will be equal to the font-size for a single line of text. This is to clearly show the additional height that includeFontPadding=true adds to the Text component.

For Text components that are styled in the same way,

When includeFontPadding=true, height = ~29.714 dips
When includeFontPadding=false, height= 24 dips

<img width="342" alt="includefontpaddingtrue" src="https://user-images.githubusercontent.com/1437344/27299391-3eec9de0-54fa-11e7-81d5-d0aeb40e8e27.png">

<img width="346" alt="includefontpaddingfalse" src="https://user-images.githubusercontent.com/1437344/27299401-45c95248-54fa-11e7-98d7-17dd152d3cb8.png">
Closes #14609

Reviewed By: AaaChiuuu

Differential Revision: D5587602

Pulled By: achen1

fbshipit-source-id: 6d2f12ba72ec7462676645519cd27820279278eb
@mishagreenberg
Copy link
Author

#14609 was merged

@facebook facebook locked as resolved and limited conversation to collaborators Aug 21, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Aug 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@hramos @msand @mishagreenberg @lelandrichardson @dmueller39 @react-native-bot