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

Android subpixel text #19043

Closed
wants to merge 1 commit into from

Conversation

kevinresol
Copy link
Contributor

@kevinresol kevinresol commented Apr 27, 2018

Fixes inconsistency in custom font rendering in android. See explanation below.

Test Plan

I tested the following on my physical device (Sony Z5C) and on Android emulator on Mac.
The test app contains various fonts in the AvenirNext family, which are added to the app as .ttf format.

Before:

Note that the character "thickness" is inconsistent. Some characters looks thicker than other. See the screenshots below as an example:

  • Looks thicker: right half of the g and & as in "Region & Language" (list-view screen)
  • Looks thicker: w as in Change Password (list-view screen)
  • Looks vertically compressed: a and e as in "Message" in the bottom bar (list-view screen)
  • Looks thicker: A, N, V, v (paragraph screen)
  • Looks thinner: i (paragraph screen), it leaves extra space on its right hand side

screenshot_20180427-151129
screenshot_20180427-151419

After:

Every characters has consistent "weight" and looks nice:

screenshot_20180427-150835
screenshot_20180427-151639

Related PRs

None

Release Notes

[ANDROID] [ENHANCEMENT] [CustomStyleSpan] - Enable subpixel text rendering

@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 Apr 27, 2018
@facebook-github-bot
Copy link
Contributor

@kevinresol I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@kevinresol
Copy link
Contributor Author

no idea. but maybe @mdvacca? I see him reviewed some PR in the folder

@kevinresol
Copy link
Contributor Author

@mkonicek I see you were the original implementer. Could you give us some comment on this?

@kevinresol
Copy link
Contributor Author

What can I do to get this reviewed?

@kevinresol
Copy link
Contributor Author

@mkonicek @mdvacca sorry to bother but at least I would like to have some feedback/comments

@kevinresol
Copy link
Contributor Author

kevinresol commented Aug 13, 2018

or @andreicoman11 ? As I see you reviewed bfeaa6a

@kevinresol
Copy link
Contributor Author

@mkonicek @mdvacca @andreicoman11 can even anyone point me to a direction?

@kevinresol
Copy link
Contributor Author

@mkonicek @mdvacca @andreicoman11 could you please at least point me to some possibly responsible persons?

@kevinresol
Copy link
Contributor Author

1 similar comment
@kevinresol
Copy link
Contributor Author

@kevinresol
Copy link
Contributor Author

@pasqualeanatriello any idea I could get this reviewed?

@kevinresol
Copy link
Contributor Author

@hramos sorry to bother but I would like to have someone from facebook to review this, or can you at least give me some directions?

@kelset
Copy link
Contributor

kelset commented Sep 21, 2018

Hey Kevin sorry no one of the people you tagged have replied here - I'll try to find someone who can review this, since it's a one-line change I feel it would be a good-to-have in next releases 🤗

(sadly I can't re-run tests, can you rebase your commit to trigger a new one?)

EDIT: the flag https://developer.android.com/reference/android/graphics/Paint#setSubpixelText(boolean)

@kevinresol kevinresol force-pushed the android_subpixel_text branch from 317e100 to e5b6965 Compare September 21, 2018 13:21
@kevinresol
Copy link
Contributor Author

Updated.

@kelset
Copy link
Contributor

kelset commented Sep 24, 2018

Looks like the errors in CI are not related to your commit, let's try to merge it :)

Copy link
Contributor

@kelset kelset left a comment

Choose a reason for hiding this comment

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

Asked a bit around, got 2 positive feedbacks on this.

@kelset
Copy link
Contributor

kelset commented Sep 24, 2018

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 24, 2018
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.

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

@react-native-bot
Copy link
Collaborator

@kevinresol merged commit 65e4e67 into facebook:master.


Once this commit is added to a release, you will see the corresponding version tag below the description at 65e4e67. If the commit has a single master tag, it is not yet part of a release.

@facebook facebook locked as resolved and limited conversation to collaborators Sep 24, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 24, 2018
grabbou pushed a commit that referenced this pull request Oct 2, 2018
Summary:
<!--
  Required: Write your motivation here.
  If this PR fixes an issue, type "Fixes #issueNumber" to automatically close the issue when the PR is merged.
-->

Fixes inconsistency in custom font rendering in android. See explanation below.

<!--
  Required: Write your test plan here. If you changed any code, please provide us with
  clear instructions on how you verified your changes work. Bonus points for screenshots and videos!
-->

I tested the following on my physical device (Sony Z5C) and on Android emulator on Mac.
The test app contains various fonts in the AvenirNext family, which are added to the app as `.ttf` format.

Note that the character "thickness" is inconsistent. Some characters looks thicker than other. See the screenshots below as an example:
- Looks thicker: right half of the `g` and `&` as in "Region & Language" (list-view screen)
- Looks thicker: `w` as in `Change Password` (list-view screen)
- Looks vertically compressed: `a` and `e` as in "Message" in the bottom bar (list-view screen)
- Looks thicker: `A`, `N`, `V`, `v` (paragraph screen)
- Looks thinner: `i` (paragraph screen), it leaves extra space on its right hand side

![screenshot_20180427-151129](https://user-images.githubusercontent.com/1103788/39350754-ae9fafe2-4a31-11e8-8835-97280783bb95.png)
![screenshot_20180427-151419](https://user-images.githubusercontent.com/1103788/39350755-aecc9bba-4a31-11e8-8128-2d230f003dfa.png)

Every characters has consistent "weight" and looks nice:

![screenshot_20180427-150835](https://user-images.githubusercontent.com/1103788/39350753-ae698728-4a31-11e8-8a55-4e161c559b95.png)
![screenshot_20180427-151639](https://user-images.githubusercontent.com/1103788/39350756-aef95d44-4a31-11e8-820b-1a7dee77fabe.png)

<!--
  Does this PR require a documentation change?
  Create a PR at https://github.com/facebook/react-native-website and add a link to it here.
-->

None

<!--
  Required.
  Help reviewers and the release process by writing your own release notes. See below for an example.
-->

[ANDROID] [ENHANCEMENT] [CustomStyleSpan] - Enable subpixel text rendering

<!--
  **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

    CATEGORY
  [----------]      TYPE
  [ CLI      ] [-------------]    LOCATION
  [ DOCS     ] [ BREAKING    ] [-------------]
  [ GENERAL  ] [ BUGFIX      ] [ {Component} ]
  [ INTERNAL ] [ ENHANCEMENT ] [ {Filename}  ]
  [ IOS      ] [ FEATURE     ] [ {Directory} ]   |-----------|
  [ ANDROID  ] [ MINOR       ] [ {Framework} ] - | {Message} |
  [----------] [-------------] [-------------]   |-----------|

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Pull Request resolved: #19043

Differential Revision: D10008155

Pulled By: hramos

fbshipit-source-id: 7fe9bb0f9be80380947ff80eac522e3cfd6b22cf
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
<!--
  Required: Write your motivation here.
  If this PR fixes an issue, type "Fixes #issueNumber" to automatically close the issue when the PR is merged.
-->

Fixes inconsistency in custom font rendering in android. See explanation below.

<!--
  Required: Write your test plan here. If you changed any code, please provide us with
  clear instructions on how you verified your changes work. Bonus points for screenshots and videos!
-->

I tested the following on my physical device (Sony Z5C) and on Android emulator on Mac.
The test app contains various fonts in the AvenirNext family, which are added to the app as `.ttf` format.

Note that the character "thickness" is inconsistent. Some characters looks thicker than other. See the screenshots below as an example:
- Looks thicker: right half of the `g` and `&` as in "Region & Language" (list-view screen)
- Looks thicker: `w` as in `Change Password` (list-view screen)
- Looks vertically compressed: `a` and `e` as in "Message" in the bottom bar (list-view screen)
- Looks thicker: `A`, `N`, `V`, `v` (paragraph screen)
- Looks thinner: `i` (paragraph screen), it leaves extra space on its right hand side

![screenshot_20180427-151129](https://user-images.githubusercontent.com/1103788/39350754-ae9fafe2-4a31-11e8-8835-97280783bb95.png)
![screenshot_20180427-151419](https://user-images.githubusercontent.com/1103788/39350755-aecc9bba-4a31-11e8-8128-2d230f003dfa.png)

Every characters has consistent "weight" and looks nice:

![screenshot_20180427-150835](https://user-images.githubusercontent.com/1103788/39350753-ae698728-4a31-11e8-8a55-4e161c559b95.png)
![screenshot_20180427-151639](https://user-images.githubusercontent.com/1103788/39350756-aef95d44-4a31-11e8-820b-1a7dee77fabe.png)

<!--
  Does this PR require a documentation change?
  Create a PR at https://github.com/facebook/react-native-website and add a link to it here.
-->

None

<!--
  Required.
  Help reviewers and the release process by writing your own release notes. See below for an example.
-->

[ANDROID] [ENHANCEMENT] [CustomStyleSpan] - Enable subpixel text rendering

<!--
  **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

    CATEGORY
  [----------]      TYPE
  [ CLI      ] [-------------]    LOCATION
  [ DOCS     ] [ BREAKING    ] [-------------]
  [ GENERAL  ] [ BUGFIX      ] [ {Component} ]
  [ INTERNAL ] [ ENHANCEMENT ] [ {Filename}  ]
  [ IOS      ] [ FEATURE     ] [ {Directory} ]   |-----------|
  [ ANDROID  ] [ MINOR       ] [ {Framework} ] - | {Message} |
  [----------] [-------------] [-------------]   |-----------|

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Pull Request resolved: facebook#19043

Differential Revision: D10008155

Pulled By: hramos

fbshipit-source-id: 7fe9bb0f9be80380947ff80eac522e3cfd6b22cf
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants