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.

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

Disable usesFontLeading for NSLayoutManager on iOS to fix baseline alignment issue on some fonts #27195

Closed

Conversation

pahmed
Copy link

@pahmed pahmed commented Nov 11, 2019

Summary

Fixes #27137

This PR fixes an issue on iOS where RCTTextView height is not calculated as it should for some fonts where font leading attributed is not equal to zero, which results in wrong baseline alignment behaviour.

The fix for this is by setting usesFontLeading property of NSLayoutManager to NO, which results is a layout behavior that is similar to UILabel

Probably the documentation for usesFontLeading describes why UILabel has a different (correct) layout behavior in that case

// By default, a layout manager will use leading as specified by the font. However, this is not appropriate for most UI text, for which a fixed leading is usually specified by UI layout guidelines. These methods allow the use of the font's leading to be turned off.

Changelog

[iOS] [Fixed] - Fix RCTTextView layout issue that happens on some font with leading attribute not equal to zero, which causes wrong base-alignment layout

Test Plan

Below are the test results before and after the change, and comparing that to native UILabel behavior.

The test is done with using system font and custom font (GothamNarrow-Medium) and font size 50

GothamNarrow-Medium.otf.zip

const App: () => React$Node = () => {
  return (
    <View style={{flex: 1, margin: 40, flexDirection: 'row', justifyContent: 'center', alignItems: 'baseline'}}>
      <View style={{width: 30, height: 30, backgroundColor: 'lightgray'}} />
      <Text style={{fontSize: 50, backgroundColor: 'green', fontFamily: 'GothamNarrow-Medium'}}>{'Settings'}</Text>
    </View>
  );
};

Before the fix

Screenshot 2019-11-11 at 16 53 26


After the fix

Screenshot 2019-11-11 at 16 55 11


Using UILabel

Screenshot 2019-11-11 at 16 59 28

…where text is not aligned correctly for some fonts (with `leading` not equal to zero) facebook#27137
@facebook-github-bot
Copy link
Contributor

Hi pahmed! 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 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 to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@pahmed pahmed changed the title Disable usesFontLeading for NSLayoutManager on iOS to fix an issue Disable usesFontLeading for NSLayoutManager on iOS to fix baseline alignment issue on some font Nov 11, 2019
@pahmed pahmed changed the title Disable usesFontLeading for NSLayoutManager on iOS to fix baseline alignment issue on some font Disable usesFontLeading for NSLayoutManager on iOS to fix baseline alignment issue on some fonts Nov 11, 2019
@react-native-bot react-native-bot added Platform: iOS iOS applications. Bug labels Nov 11, 2019
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 11, 2019
@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by Ahmed Ibrahim in 5d08aab.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 27, 2020
osdnk pushed a commit to osdnk/react-native that referenced this pull request Mar 9, 2020
…alignment issue on some fonts (facebook#27195)

Summary:
Fixes facebook#27137

This PR fixes an issue on iOS where RCTTextView height is not calculated as it should for some fonts where font `leading` attributed is not equal to zero, which results in wrong baseline alignment behaviour.

The fix for this is by setting `usesFontLeading` property of `NSLayoutManager` to `NO`, which results is a layout behavior that is similar to `UILabel`

Probably the documentation for `usesFontLeading` describes why UILabel has a different (correct) layout behavior in that case
> // By default, a layout manager will use leading as specified by the font.  However, this is not appropriate for most UI text, for which a fixed leading is usually specified by UI layout guidelines.  These methods allow the use of the font's leading to be turned off.

## Changelog

[iOS] [Fixed] - Fix RCTTextView layout issue that happens on some font with `leading` attribute not equal to zero, which causes wrong base-alignment layout
Pull Request resolved: facebook#27195

Test Plan:
Below are the test results before and after the change, and comparing that to native UILabel behavior.

The test is done with using system font and custom font (`GothamNarrow-Medium`) and font size 50

[GothamNarrow-Medium.otf.zip](https://github.com/facebook/react-native/files/3832143/GothamNarrow-Medium.otf.zip)

```js
const App: () => React$Node = () => {
  return (
    <View style={{flex: 1, margin: 40, flexDirection: 'row', justifyContent: 'center', alignItems: 'baseline'}}>
      <View style={{width: 30, height: 30, backgroundColor: 'lightgray'}} />
      <Text style={{fontSize: 50, backgroundColor: 'green', fontFamily: 'GothamNarrow-Medium'}}>{'Settings'}</Text>
    </View>
  );
};
```

-------
### Before the fix

<img width="962" alt="Screenshot 2019-11-11 at 16 53 26" src="https://user-images.githubusercontent.com/5355138/68601049-dd778780-04a3-11ea-879e-cc7b4eb2af95.png">

-----
### After the fix
<img width="944" alt="Screenshot 2019-11-11 at 16 55 11" src="https://user-images.githubusercontent.com/5355138/68601180-1d3e6f00-04a4-11ea-87bc-61c6fa2cdb18.png">

-----
### Using `UILabel`
<img width="805" alt="Screenshot 2019-11-11 at 16 59 28" src="https://user-images.githubusercontent.com/5355138/68601487-b2d9fe80-04a4-11ea-9a0f-c025c7753c24.png">

Differential Revision: D19576556

Pulled By: shergin

fbshipit-source-id: 4eaafdab963c3f53c461884c581e205e6426718a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Baseline alignment does not work as expected with some font on iOS
5 participants