custom fonts support The Android Way #24595

dulmandakh · 2019-04-25T05:06:03Z

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.

Changelog

[Android] [Changed] - Require custom fonts to list in fonts array. Fixes performance regression.

Test Plan

CI is green and RNTester app works as expected.

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

dulmandakh · 2019-04-25T17:15:04Z

This is only one for an instance, and it's singleton. IMHO, asking developers to write a custom provider code just to have a custom font is a bad DX.

fkgozali · 2019-04-25T17:17:41Z

This is only one for an instance, and it's singleton.

It's still gonna be in the RN startup path, so it would still affect startup perf.

IMHO, asking developers to write a custom provider code just to have a custom font is a
bad DX.

You can always provide a default provider, and have the ability for apps to change or disable the default provider.

cpojer · 2019-04-25T17:19:09Z

@dulmandakh to clarify, calling this method in RN's startup path means that it will literally add 10s of ms to the startup. FB scale is crazy.

dulmandakh · 2019-04-26T00:16:02Z

ok, i'll figure out a solution for it, thank you.

dulmandakh · 2019-04-26T06:53:13Z

@fkgozali changed it to look into meta-data in AndroidManifest.xml, thus not lookup resources by name. It requires some effort from developers, but it's much easier and clean way to add custom fonts on Android.

hey99xx · 2019-04-26T09:13:51Z

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

+          for (int i = 0; i < fonts.length(); i++) {
+            int fontId = fonts.getResourceId(i, 0);
+            if (fontId != 0) {
+              Typeface font = ResourcesCompat.getFont(context, fontId);


How expensive is loading Typeface instances for every single font of the app into the cache and calling resources.getResourceEntryName at startup?

If it is also taking too long (for FB scale), although it's verbose you might do something like:

<array name="fonts"> <item>srisakdi</item> <item>@font/srisakdi</item> <item>anotherfont</item> <item>@font/anotherfont</item> </array>

This way you can iterate the array in increments of 2, and get away with only caching fontFamily->fontId mapping at startup, and only inflate fontId->Typeface still lazily.

Also this should allow font file names to not strictly match font family names, ex:

<item>Open Sans</item> <item>@font/open_sans</item>

Native devs will be more familiar underscored naming R.font.open_sans whereas web developers will be more familiar with human readable naming fontFamily: "Open Sans".

This will allow sharing cross platform styling code with iOS, where font family name is already retrieved from font file content, not from file names.

I tried to mimic iOS feature, where we only list font files, but also make typo-proof. Also I couldn't find anything about performance impact of getResourceEntryName, and hope it won't have impact on performance.

iOS works a bit differently, it reads font family name from the file content, Android in your implementation reads from the file name.
https://developer.apple.com/documentation/uikit/text_display_and_fonts/adding_a_custom_font_to_your_app
https://stackoverflow.com/questions/16788330/how-do-i-get-the-font-name-from-an-otf-or-ttf-file

IMO this will force developers to write strange JS code like:
{fontFamily: Platform.OS == 'ios' ? 'Open Sans' : 'open_sans'}
I dont know why typo-proof is such an important thing, you only need to match with what you use in JS code, you could make a mistake in font file name too.

Just looking at getResourceEntryName implementation in AOSP repo, it's implemented in C++ so you will at least jump between JNI bridge for each call.

Yep, I found that Android doesn't allow to use font family name, but resource filename without extension. So difference is inevitable.

With this change we can use various font weights like medium easily. Currently, you have to write something like which makes text style sharing difficult.

{ fontFamily: Platform.OS == 'ios' ? 'Open Sans' : 'open_sans_medium' fontWeight: Platform.OS == 'ios' ? '500' : 'normal' }

once PR lands you will write

{ fontFamily: Platform.OS == 'ios' ? 'Open Sans' : 'open_sans' fontWeight: '500' }

You can currently name your file "Open Sans.ttf", "Open Sans_bold.ttf"... etc though, so the code can already be cross platform fontFamily: "Open Sans". Can you leave a space in these font resource file names and reuse that resource in Java? IMO it's not good practice to enforce android resource naming conventions into JS land.

hey99xx · 2019-04-26T09:17:52Z

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

+      try {
+        ApplicationInfo app = context.getPackageManager().getApplicationInfo(context.getPackageName(), PackageManager.GET_META_DATA);
+        Bundle bundle = app.metaData;
+        int fontsId = bundle.getInt("rn_fonts", 0);


Can this be namespaced like com.facebook.react.font.resources instead of rn_fonts?

fkgozali · 2019-04-26T17:27:27Z

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

+        Bundle bundle = app.metaData;
+        int fontsId = bundle.getInt("rn_fonts", 0);
+        if (fontsId != 0) {
+          Resources resources = context.getResources();


Actually, I'm not sure if accessing anything dynamically from Resources is fast, because it needs to crack them open from the APK.

Can we make this not do any resource lookup by default and only perform this lookup if the hosting app configures it? You don't really need to change it to use AndroidManifest, using font resource is fine, but I'd like:

no lookup by default

do custom lookup with any impl for now only if the hosting app enables it explicitly

I think @dulmandakh is trying to find a way where non-native developers don't have to touch java code to customize these things, that's why he switched to using AndroidManifest that's supposed to be a cheaper alternative to resource lookups.

If an app like FB does not provide "rn_fonts" in AndroidManifest.xml, there will never be any look up (besides looking to AndroidManifest itself), isn't that sufficient?

got it. I'll figure out another solution :D

dulmandakh · 2019-04-27T04:08:02Z

@fkgozali another implementation which addresses your concerns, but also requires less work from developers.

fkgozali

Mostly nits

fkgozali · 2019-04-29T20:02:04Z

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

+import androidx.annotation.Nullable;
+import androidx.core.content.res.ResourcesCompat;
+
+


remove extra newline

fkgozali · 2019-04-29T20:03:51Z

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

@@ -37,9 +40,12 @@
  private static ReactFontManager sReactFontManagerInstance;

  private Map<String, FontFamily> mFontCache;
+  private Map<String, Typeface> mTypeCache;
+  private boolean mTypeCacheLoaded = false;


I think a better set of names is:

mCustomFontCache

mHasCustomFont

or:

mCustomTypefaceCache

mHasCustomTypeface

fkgozali · 2019-04-29T20:05:05Z

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

@@ -71,6 +83,15 @@ public static ReactFontManager getInstance() {
    return typeface;
  }

+  public void loadFont(Context context, int fontId) {


should rename to addCustomFont() to make it clearer

Can you also add comment for how to call this method? E.g. what you did for RNTester. Basically document how one can install a custom font support in their apps.

fkgozali · 2019-04-29T20:08:23Z

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

+  public void loadFont(Context context, int fontId) {
+    Typeface font = ResourcesCompat.getFont(context, fontId);
+    if (font != null) {
+      String fontFamily = context.getResources().getResourceEntryName(fontId);


This won't affect FB with this PR, but I wonder if this is efficient in practice. You don't have to address it in this PR, but I think allowing apps to provide a custom "typeface resolution" function can allow them to address slow lookup. For instance, if one day FB wants to use custom fonts, I'd actually implement the resolver via a switch statement, not via asking getResourceEntryName(). Most apps probably don't need such optimization, but having an option to do optimized thing is good.

Most apps probably don't need such optimization, but having an option to do optimized thing is good.

An example pseudo solution:

public interface ReactTypefaceResolver { public String resolveTypefaceName(int fontId); }

Then introduce a method in ReactFontManager:

public void setCustomTypefaceResolver(ReactTypefaceResolver resolver) { mCustomTypefaceResolver = resolver; }

Then you can just use mCustomTypefaceResolver != null as the flag, no need to have the boolean flag you had above. Then you can just call it inside loadFont().

dulmandakh · 2019-04-30T00:33:34Z

Thank you for the detailed review. I'll make the changes you suggested.

dulmandakh · 2019-04-30T03:27:59Z

@fkgozali renamed variables and removed resource lookup code, therefore resolved feature parity with iOS because we can assign any font family name to a font. Thank you

hey99xx · 2019-04-30T03:59:16Z

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

+        style
+      );
+    }
+
    Typeface typeface = fontFamily.getTypeface(style);
    if (typeface == null) {
      typeface = createTypeface(fontFamilyName, style, assetManager);


Is there value caching invidual styled Typeface (normal/italic/bold/boldItalic) instances here like it's done for fonts in assets? Besides that all LGTM.

to use styles or weights you have to define fonts using XML.

fkgozali · 2019-05-02T17:17:25Z

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

@@ -60,6 +65,13 @@ public static ReactFontManager getInstance() {
      mFontCache.put(fontFamilyName, fontFamily);
    }

+    if(mHasCustomTypeface && mCustomTypefaceCache.containsKey(fontFamilyName)) {


nit: add space after if

fkgozali · 2019-05-02T17:18:22Z

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

@@ -37,9 +40,12 @@
  private static ReactFontManager sReactFontManagerInstance;

  private Map<String, FontFamily> mFontCache;
+  private Map<String, Typeface> mCustomTypefaceCache;
+  private boolean mHasCustomTypeface = false;


Technically you could just make mCustomTypefaceCache @Nullable, then default to null. If someone adds a custom typeface, you initialize it as a new HashMap, then you don't need mHasCustomTypeface boolean. You can just check if mCustomTypefaceCache != null

Yes, my first version was like that but changed it to use booleans just before the commit because this seemed cleaner.

from @mdvacca:

let's add final to these variables

facebook-github-bot

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

dulmandakh · 2019-05-06T06:23:37Z

@hramos it's not merged yet. Could you please investigate it. Thank you

fkgozali · 2019-05-06T16:19:19Z

it's not merged yet. Could you please investigate it. Thank you

No one kicked off the merging. I still need to run the simulated perf test to verify there's no regression.

fkgozali · 2019-05-06T16:20:08Z

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

@@ -37,9 +40,12 @@
  private static ReactFontManager sReactFontManagerInstance;

  private Map<String, FontFamily> mFontCache;
+  private Map<String, Typeface> mCustomTypefaceCache;
+  private boolean mHasCustomTypeface = false;


from @mdvacca:

let's add final to these variables

fkgozali · 2019-05-06T16:20:38Z

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

+   *
+   * ReactFontManager.getInstance().addCustomFont(this, "Srisakdi", R.font.srisakdi);
+   */
+  public void addCustomFont(Context context, @NonNull String fontFamily, int fontId) {


from @mdvacca:

should we add @NonNull to context?

fkgozali · 2019-05-06T16:21:19Z

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

+    if (font != null) {
+      mCustomTypefaceCache.put(fontFamily, font);
+    }
+    mHasCustomTypeface = true;


From @mdvacca:

doesn't this need to be inside the if (font != null) {
also, why not to just remove this variable and create a private method:

private boolean hasCustomTypeface() { return mCustomTypefaceCache.isEmpty(); }

or maybe we don't even need this variable, it is just preventing a call to Map.containsKey?

we pass resource id to this method, but that ID might not be a font, but anything. So font != null is making sure that we have a font with provided id.

fkgozali · 2019-05-06T20:37:39Z

I still need to run the simulated perf test to verify there's no regression.

seems like we can land this for now, just need to address the comments here before we land

facebook-github-bot

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

react-native-bot · 2019-05-08T01:45:06Z

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

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

fix performance regression of custom fonts

f5d4aaf

dulmandakh added Platform: Android Android applications. Component: Text labels Apr 25, 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. labels Apr 25, 2019

dulmandakh requested review from fkgozali, mdvacca, cpojer and gengjiawen April 25, 2019 05:06

react-native-bot added Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Includes Changelog labels Apr 25, 2019

fkgozali requested changes Apr 25, 2019

View reviewed changes

ReactAndroid/src/main/java/com/facebook/react/views/text/ReactFontManager.java Outdated Show resolved Hide resolved

dulmandakh added 3 commits April 26, 2019 14:03

merge master

f9586f5

add custom font support

8f2ea3c

require meta-data in AndroidManifest.xml

ccaa2b3

dulmandakh changed the title ~~fix performance regression of custom fonts~~ custom fonts support The Android Way Apr 26, 2019

use androidx annotations

2b02c1c

hey99xx reviewed Apr 26, 2019

View reviewed changes

dulmandakh requested a review from fkgozali April 26, 2019 12:02

fkgozali reviewed Apr 26, 2019

View reviewed changes

dulmandakh added 2 commits April 27, 2019 11:50

revert some changes

e7dddce

ReactFontManager.loadFonts

eb108ff

loadFont: load fonts one by one

9bd2591

dulmandakh added 2 commits April 27, 2019 12:20

load fonts one by one

b2ba223

remove unused imports

0f6df0a

fkgozali requested changes Apr 29, 2019

View reviewed changes

dulmandakh added 2 commits April 30, 2019 10:59

Merge branch 'master' into fix-android-fonts

08cabaf

support custom font family name

fe3dd73

hey99xx reviewed Apr 30, 2019

View reviewed changes

dulmandakh requested a review from fkgozali April 30, 2019 06:23

fkgozali reviewed May 2, 2019

View reviewed changes

facebook-github-bot reviewed May 2, 2019

View reviewed changes

fkgozali requested changes May 6, 2019

View reviewed changes

hramos removed the Includes Changelog label May 6, 2019

fix suggestions

f9c1050

dulmandakh requested a review from fkgozali May 7, 2019 08:16

mdvacca approved these changes May 7, 2019

View reviewed changes

facebook-github-bot reviewed May 7, 2019

View reviewed changes

facebook-github-bot closed this in fd6386a May 8, 2019

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

dulmandakh deleted the fix-android-fonts branch October 15, 2019 05:14

This was referenced Feb 7, 2020

Supporting custom fonts the Android Way #19736

Closed

add support for native/downloadable fonts #23865

Closed

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

Open

		import androidx.annotation.Nullable;
		import androidx.core.content.res.ResourcesCompat;

custom fonts support The Android Way #24595

custom fonts support The Android Way #24595

Conversation

dulmandakh commented Apr 25, 2019

Summary

Changelog

Test Plan

dulmandakh commented Apr 25, 2019 via email

fkgozali commented Apr 25, 2019

cpojer commented Apr 25, 2019

dulmandakh commented Apr 26, 2019

dulmandakh commented Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hey99xx Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

hey99xx Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dulmandakh commented Apr 27, 2019

fkgozali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dulmandakh commented Apr 30, 2019 via email

dulmandakh commented Apr 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dulmandakh May 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

dulmandakh commented May 6, 2019

fkgozali commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fkgozali commented May 6, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented May 8, 2019

dulmandakh commented Apr 26, 2019 •

edited

Loading

hey99xx Apr 26, 2019 •

edited

Loading

hey99xx Apr 26, 2019 •

edited

Loading

dulmandakh May 2, 2019 •

edited

Loading