custom fontWeight numeric values for Text on Android

[dulmandakh]

Summary

I found that on Android we only support 2 fontWeight options, either normal or bold, even developer can set any numeric value. But iOS supports all possible numeric values. This PR tries to add support for all possible numeric values on Android, even if it's supported only on Android P(28) and above.

This change might break texts where fontWeight use improperly, because this PR removes conversion of values above 500 to BOLD and below 500 to normal.

FYI, also moved mCustomTypefaceCache usage up because it was working after unnecessary mFontCache usage.

Changelog

[Android] [Changed] - add custom font weight support to Text component on Android, only on P(API 28) and above versions.

Test Plan

RNTester app's Text examples will show Rubik Regular, Rubik Light, Rubik Bold, Rubik Medium and Rubik Medium Italic texts in corresponding font family, style and weights.

[mdvacca]

can you change -1 for UNSET constant?

[mdvacca]

Hey @dulmandakh, thanks for working on this. The code looks good to me, can you just make that small change of the constant? and I will import it

CC @fkgozali as you were involved in changes in the way we manage fonts in android

[dulmandakh]

@mdvacca thanks for your review and comment. I made requested changes, please feel free to review, maybe import the changes.

[facebook-github-bot]

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

[mdvacca]

Thanks @dulmandakh, I'm landing it.

[react-native-bot]

This pull request was successfully merged by @dulmandakh in 3915c0f.

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

[greg7gkb]

It seems like this change broke previous behavior that was relied upon (my team included): #25696
So I'm curious to understand why the decision to break existing behaviors that seem reasonable?

[dulmandakh]

So sorry that the change broke your code, but intention is described in the summary.

[greg7gkb]

The description does not describe why it was decided that the previous behavior should be broken. New behaviors for P+ might have been appropriate but could have been applied in a backwards-compatible way.
I'm sure this broke behavior for a lot of apps.

[dulmandakh]

It's my bad that I didn't expected that developers would depend on this behavior. Please feel free to create a PR and add backwards-compatibility, I would be happy to review and test it.

[aforty]

I found this PR as I was investigating an issue.

My issue:
I'm declaring a custom font with all weights (300, 400, 500, 600, 700, 800, 900). I then have a Text node with fontWeight: '700' but because of the logic in ReactBaseTextShadowNode.setFontWeight() it gets reset to Typeface.BOLD. So if I use '800' or '900' it works correctly but specifically '700' gets reset to bold.

Further adding to my confusion is the fact that this logic differs from other setFontWeight() methods where anything over 500 is reset to bold. Then all three methods claim to be duplicates of the code of one another.

My question:
What should the proper logic be. Why are fontWeights being reset to bold and why '700' specifically in the case of ReactBaseTextShadowNode?

// TextAttributeProps.java


/**
 * /* This code is duplicated in ReactTextInputManager /* TODO: Factor into a common place they
 * can both use
 */
public void setFontWeight(@Nullable String fontWeightString) {
  int fontWeightNumeric =
      fontWeightString != null ? parseNumericFontWeight(fontWeightString) : -1;
  int fontWeight = UNSET;
  if (fontWeightNumeric >= 500 || "bold".equals(fontWeightString)) {
    fontWeight = Typeface.BOLD;
  } else if ("normal".equals(fontWeightString)
      || (fontWeightNumeric != -1 && fontWeightNumeric < 500)) {
    fontWeight = Typeface.NORMAL;
  }
  if (fontWeight != mFontWeight) {
    mFontWeight = fontWeight;
  }
}

// ReacTextInputManager.java


/**
 * /* This code was taken from the method setFontWeight of the class ReactTextShadowNode /* TODO:
 * Factor into a common place they can both use
 */
@ReactProp(name = ViewProps.FONT_WEIGHT)
public void setFontWeight(ReactEditText view, @Nullable String fontWeightString) {
  int fontWeightNumeric =
      fontWeightString != null ? parseNumericFontWeight(fontWeightString) : -1;
  int fontWeight = UNSET;
  if (fontWeightNumeric >= 500 || "bold".equals(fontWeightString)) {
    fontWeight = Typeface.BOLD;
  } else if ("normal".equals(fontWeightString)
      || (fontWeightNumeric != -1 && fontWeightNumeric < 500)) {
    fontWeight = Typeface.NORMAL;
  }
  Typeface currentTypeface = view.getTypeface();
  if (currentTypeface == null) {
    currentTypeface = Typeface.DEFAULT;
  }
  if (fontWeight != currentTypeface.getStyle()) {
    view.setTypeface(currentTypeface, fontWeight);
  }
}

// ReactBaseTextShadowNode.java


/**
 * /* This code is duplicated in ReactTextInputManager /* TODO: Factor into a common place they
 * can both use
 */
@ReactProp(name = ViewProps.FONT_WEIGHT)
public void setFontWeight(@Nullable String fontWeightString) {
  int fontWeightNumeric =
      fontWeightString != null ? parseNumericFontWeight(fontWeightString) : UNSET;
  int fontWeight = fontWeightNumeric != UNSET ? fontWeightNumeric : Typeface.NORMAL;

  if (fontWeight == 700 || "bold".equals(fontWeightString)) fontWeight = Typeface.BOLD;
  else if (fontWeight == 400 || "normal".equals(fontWeightString)) fontWeight = Typeface.NORMAL;

  if (fontWeight != mFontWeight) {
    mFontWeight = fontWeight;
    markUpdated();
  }
}

[ou2s]

So sad that the old behavior is broken now, because the impact is quite big on our app...
I understand the motivation behind this PR but why remove the code that removes conversion of values above 500 to BOLD and below 500 to normal?
Why not leave it for backward compatibility?