add support for native/downloadable fonts #23865

dulmandakh · 2019-03-12T15:37:00Z

Summary

Android API 26 and Android Support Library 26 added support for font resource type and native/downloadable fonts. It allows apps to easily download fonts from online providers, but also use of various font weights other than normal and bold, like medium. So it deprecated APIs for asset fonts, and should be removed in the future.

Advantages:

Just copy font files in res/font and use it specifying filename (without extension) in fontFamily
Define custom font-family using XML file (in res/font) and font files, it may have many weights and styles. See PR for example.
Define configuration to download fonts from online font providers, and use it.

See https://developer.android.com/guide/topics/ui/look-and-feel/fonts-in-xml and https://developer.android.com/guide/topics/ui/look-and-feel/downloadable-fonts

Changelog

[Android] [Changed] - add support for custom/downloadable fonts

Test Plan

CI is green and RNTester app's Text examples include Srisakdi font example.

dulmandakh · 2019-03-12T15:58:29Z

@hey99xx please review

hey99xx · 2019-03-15T03:48:33Z

ReactAndroid/src/main/java/com/facebook/react/views/text/CustomStyleSpan.java

+    int style,
+    int weight,
+    @Nullable String family,
+    Context context) {


Instead of duplicating the method above will minimal change, why not call apply(paint, style, weight, family, context.getAssets());.
I know you deprecated the method above, but until you can get rid of CustomStyleSpan(..., AssetManager) constructor I think it's a more concise solution.

Ah nevermind. Then you would call old ReactFontManager.getTypeface method again. Still, there might be a prettier way to refactor this. For example you can have a single method that takes both args:

private static void apply( Paint paint, int style, int weight, @Nullable String family, Context context, AssetManager assets) { // ... if (family != null) { ReactFontManager fontManager = ReactFontManager.getInstance(); typeface = context != null ? fontManager.getTypeface(family, want, context) : fontManager.getTypeface(family, want, assets); } else if (typeface != null) {

hey99xx · 2019-03-15T04:01:52Z

ReactAndroid/src/main/java/com/facebook/react/views/text/ReactFontManager.java

+    String fontFamilyName,
+    int style,
+    Context context) {
+    int fontId = context.getResources().getIdentifier(fontFamilyName, "font", context.getPackageName());


I don't know if the first priority should go to mFontCache (which has advantage of it can be changed programmatically) or to the static assets.

Also for static assets, I'd guess both getIdentifier and Typeface.create call can be slow, that's why there are caches in the first place. If you have 100s of textviews, you probably don't want to redo these computations that never change.

ResourcesCompat.getFont is documented as a blocking call. I don't know if that means it blocks for network or something else, but keep that in mind too. I don't have too much knowledge about the new font system, this is just looking up to javadoc, but I don't know if this would then work for downloadable fonts.

Maybe adding cache would be a good idea, like you said it'll improve performance for 100s texts.

hey99xx · 2019-03-15T04:06:38Z

Overall I really like the idea though. In our app we have fonts with 3+ different weights and have to hardcode asset names as fontFamily style. This brings us much closer to iOS that does not have this problem.

dulmandakh · 2019-03-15T04:39:04Z

@hey99xx yep, feature parity with iOS is the motivation for this PR. We have medium weights in our UI and I have to check platform and specify fontFamily on Android, but fontWeight on iOS. I have a concern that current API might be used somewhere in FB and changing it might break their apps, therefore created almost identical APIs.

dulmandakh · 2019-03-15T04:40:08Z

@hey99xx thank you for the comments. I hope that I'll get someone from FB to review and maybe remove deprecated APIs if they're not used at FB.

hey99xx · 2019-03-15T07:21:57Z

remove deprecated APIs if they're not used at FB.

That's why I didn't comment on public CustomStyleSpan, I learnt from previous PRs that FB is cautious on deprecating first then deleting. But for private static void apply, it makes less sense to keep it unless you think FB uses their own private methods by reflection.

facebook-github-bot

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

facebook-github-bot

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

react-native-bot · 2019-04-10T20:55:06Z

This pull request was successfully merged by @dulmandakh in f01c4e2.

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

Summary: Android API 26 and Android Support Library 26 added support for font resource type and native/downloadable fonts. It allows apps to easily download fonts from online providers, but also use of various font weights other than normal and bold, like medium. So it deprecated APIs for asset fonts, and should be removed in the future. Advantages: - Just copy font files in res/font and use it specifying filename (without extension) in fontFamily - Define custom font-family using XML file (in res/font) and font files, it may have many weights and styles. See PR for example. - Define configuration to download fonts from online font providers, and use it. See https://developer.android.com/guide/topics/ui/look-and-feel/fonts-in-xml and https://developer.android.com/guide/topics/ui/look-and-feel/downloadable-fonts [Android] [Changed] - add support for custom/downloadable fonts Pull Request resolved: facebook#23865 Differential Revision: D14506542 Pulled By: hramos fbshipit-source-id: 67ba3148fb4b548cdbc779213cf6c1b2c3baffd2

fkgozali · 2019-04-25T01:39:59Z

Heads up: we're currently analyzing this commit and we suspect it's causing some start up regression for some RN surfaces at Facebook. I'll comment on the code that might be causing it, but in the short term we'd very likely need to revert this commit. I'll provide suggested "fix", but it's actually not that simple.

fkgozali · 2019-04-25T01:45:22Z

ReactAndroid/src/main/java/com/facebook/react/views/text/ReactFontManager.java

+      );
+    }
+
+    int fontId = context.getResources().getIdentifier(fontFamilyName, "font", context.getPackageName());


So getIdentifier() is problematic in a big app that has many resources. This could contribute to non-trivial msec, just trying to crack open the resources, then looking it up from the APK. Rather than calling this for all scenarios, we'd instead build a "provider" pattern where:

each app decides if it wants to provide a custom fonts

if no provider is given, avoid asking getIdentifier() completely

if given, ask the provider for a resourceId (int), then use it

Now the implementation of the provider needs to be a switch statement for the best performance. Example:

int getCustomFontProvider(name) { switch(name) { case "mine": return R.font.mine; ...

Referencing R.font.* directly is fast and efficient, because it points exactly to an int resource Id. The only problem is, that each app needs to implement this code themselves, because each app has different set of custom fonts.

What do you think?

cc @cpojer @mdvacca

dulmandakh · 2019-04-25T01:58:31Z

How about checking custom fonts the old way first, then the new way later, so that it won't affect FB. It's just a workaround for FB. I think that custom font feature parity with iOS is very important for general public.

fkgozali · 2019-04-25T02:02:12Z

What do you mean by the “old way”? Regardless of whether parity is important, we’re trying to mitigate regression as the current priority

dulmandakh · 2019-04-25T02:06:35Z

When someone tried to use a custom font it'll check

R.font.* or font resources. New way
Scan in assets. Old way.

I'm proposing to reverse it, so FB will find fonts in assets and won't cause regression.

fkgozali · 2019-04-25T02:07:53Z

oh but I don’t think fb uses custom fonts (for RN) today, so we don’t want to look up any font

fkgozali · 2019-04-25T02:10:44Z

to satisfy all use cases, the provider model should work (fb wont have any provider, so fb isn’t affected, and RNTester will provide a provider, so custom fonts will work)

dulmandakh · 2019-04-25T02:39:50Z

Please feel free to revert the commit. I'll look for a generic solution and add you as reviewer.

dulmandakh · 2019-04-25T03:18:16Z

@fkgozali I would like to have a TypedArray resource where you list custom fonts to make them available in React Native, so it won't lookup for font resources and won't affect performance. iOS does the same, you have to list custom fonts in Info.plist.

fkgozali · 2019-04-25T18:34:39Z

FYI the revert is eb40b09

Summary: In #23865, RN introduced support for custom fonts the Android Way. But it introduced performance regression because it'll lookup for a font using getIdentifier() every time fontFamily changed. This PR fixes regression by requiring custom fonts to be listed in **fonts** array, and populating **mTypeCache** at first use using the list. [Android] [Changed] - Require custom fonts to list in **fonts** array. Fixes performance regression. Pull Request resolved: #24595 Reviewed By: mdvacca Differential Revision: D15184590 Pulled By: fkgozali fbshipit-source-id: e3feb2396609583ebc95101130186a1f5af931da

betoharres · 2019-07-24T18:47:14Z

Since this PR was reverted, is there any known issue/PR open about this bug? I'm currently having this problem and I'm not sure if I'm doing something wrong or if this bug still occurs.

info
  React Native Environment Info:
    System:
      OS: Linux 5.1 Arch Linux undefined
      CPU: (6) x64 Intel(R) Core(TM) i5-8400 CPU @ 2.80GHz
      Memory: 3.17 GB / 15.59 GB
      Shell: 5.7.1 - /usr/bin/zsh
    Binaries:
      Node: 11.15.0 - /usr/bin/node
      Yarn: 1.16.0 - /usr/bin/yarn
      npm: 6.10.0 - /usr/bin/npm
      Watchman: 4.9.0 - /usr/local/bin/watchman
    SDKs:
      Android SDK:
        API Levels: 26, 28, 29
        Build Tools: 26.0.3, 28.0.3, 29.0.0
        System Images: android-28 | Google Play Intel x86 Atom, android-29 | Google APIs Intel x86 Atom
    npmPackages:
      react: 16.8.3 => 16.8.3
      react-native: 0.59.8 => 0.59.8
    npmGlobalPackages:
      react-native-cli: 2.0.1

Thanks you =)

ScreamZ · 2019-09-09T13:32:56Z

Any news on that thread ? Where to follow since revert

chrisbobbe · 2020-02-07T18:30:52Z

Any news on that thread ? Where to follow since revert

It landed with fd6386a on #24595! 🙂

add support for downloadable fonts

d0a068d

dulmandakh requested a review from mdvacca March 12, 2019 15:54

react-native-bot added PR: Includes Changelog Platform: Android Android applications. labels Mar 12, 2019

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Partner labels Mar 12, 2019

dulmandakh requested review from cpojer and gengjiawen March 12, 2019 15:56

dulmandakh changed the title ~~add support for custom/downloadable fonts~~ add support for downloadable fonts Mar 12, 2019

dulmandakh mentioned this pull request Mar 13, 2019

Feature/gradle font sync #23869

Closed

dulmandakh changed the title ~~add support for downloadable fonts~~ add support for native/downloadable fonts Mar 13, 2019

react-native-bot added the PR: Includes Changelog label Mar 14, 2019

hramos removed the PR: Includes Changelog label Mar 14, 2019

hey99xx reviewed Mar 15, 2019

View reviewed changes

facebook-github-bot approved these changes Mar 18, 2019

View reviewed changes

dulmandakh added 2 commits March 19, 2019 10:37

Merge branch 'master' into android-fonts

649bae8

cleanup and typeface cache

6464616

cpojer mentioned this pull request Mar 19, 2019

Supporting custom fonts the Android Way #19736

Closed

fix indentation changes

67403f6

facebook-github-bot reviewed Mar 27, 2019

View reviewed changes

facebook-github-bot closed this in f01c4e2 Apr 10, 2019

react-native-bot added the Merged This PR has been merged. label Apr 10, 2019

hramos removed Partner labels Apr 24, 2019

dulmandakh deleted the android-fonts branch April 25, 2019 01:43

fkgozali reviewed Apr 25, 2019

View reviewed changes

dulmandakh mentioned this pull request Apr 25, 2019

custom fonts support The Android Way #24595

Closed

saifahn mentioned this pull request Sep 6, 2019

Improve font handling and consistency jolocom/smartwallet-app#1442

Closed

chrisbobbe mentioned this pull request Feb 7, 2020

Solution found: Newer Unicode emojis don't render on older Android devices zulip/zulip-mobile#3885

Open

jsamr mentioned this pull request Nov 27, 2020

Support cascading styles meliorence/react-native-render-html#437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for native/downloadable fonts #23865

add support for native/downloadable fonts #23865

dulmandakh commented Mar 12, 2019 •

edited

Loading

dulmandakh commented Mar 12, 2019

hey99xx Mar 15, 2019

hey99xx Mar 15, 2019

hey99xx Mar 15, 2019 •

edited

Loading

dulmandakh Mar 16, 2019

hey99xx commented Mar 15, 2019

dulmandakh commented Mar 15, 2019

dulmandakh commented Mar 15, 2019

hey99xx commented Mar 15, 2019

facebook-github-bot left a comment

facebook-github-bot left a comment

react-native-bot commented Apr 10, 2019

fkgozali commented Apr 25, 2019

fkgozali Apr 25, 2019

fkgozali Apr 25, 2019

dulmandakh commented Apr 25, 2019

fkgozali commented Apr 25, 2019

dulmandakh commented Apr 25, 2019

fkgozali commented Apr 25, 2019

fkgozali commented Apr 25, 2019

dulmandakh commented Apr 25, 2019

dulmandakh commented Apr 25, 2019 •

edited

Loading

fkgozali commented Apr 25, 2019

betoharres commented Jul 24, 2019

ScreamZ commented Sep 9, 2019

chrisbobbe commented Feb 7, 2020

add support for native/downloadable fonts #23865

add support for native/downloadable fonts #23865

Conversation

dulmandakh commented Mar 12, 2019 • edited Loading

Summary

Changelog

Test Plan

dulmandakh commented Mar 12, 2019

hey99xx Mar 15, 2019

Choose a reason for hiding this comment

hey99xx Mar 15, 2019

Choose a reason for hiding this comment

hey99xx Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

dulmandakh Mar 16, 2019

Choose a reason for hiding this comment

hey99xx commented Mar 15, 2019

dulmandakh commented Mar 15, 2019

dulmandakh commented Mar 15, 2019

hey99xx commented Mar 15, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Apr 10, 2019

fkgozali commented Apr 25, 2019

fkgozali Apr 25, 2019

Choose a reason for hiding this comment

fkgozali Apr 25, 2019

Choose a reason for hiding this comment

dulmandakh commented Apr 25, 2019

fkgozali commented Apr 25, 2019

dulmandakh commented Apr 25, 2019

fkgozali commented Apr 25, 2019

fkgozali commented Apr 25, 2019

dulmandakh commented Apr 25, 2019

dulmandakh commented Apr 25, 2019 • edited Loading

fkgozali commented Apr 25, 2019

betoharres commented Jul 24, 2019

ScreamZ commented Sep 9, 2019

chrisbobbe commented Feb 7, 2020

dulmandakh commented Mar 12, 2019 •

edited

Loading

hey99xx Mar 15, 2019 •

edited

Loading

dulmandakh commented Apr 25, 2019 •

edited

Loading