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 Text: More robust logic for handling inherited text props #22917

Closed
wants to merge 3 commits into from

Conversation

rigdern
Copy link
Contributor

@rigdern rigdern commented Jan 9, 2019

Purpose

This commit fixes a bug and prepares us for adding support for the maxContentSizeMultiplier prop (it's currently only supported on iOS).

Details

Today we don't explicitly track inheritance of text props. Instead we rely on SpannableStringBuilder to handle this for us. Consider this example:

<Text style={{fontSize: 10}}>
  <Text style={{letterSpacing: 5}}>
    ...
  </Text>
</Text>

In today's implementation, the inner text doesn't know about fontSize (i.e. its mFontSize instance variable is Float.NaN). But everything works properly because the outer Text told SpannableStringBuilder to apply the font size across the entire string of text.

However, today's approach breaks down when computing the value to apply to the SpannableStringBuilder depends on multiple props. Suppose that RN Android supported the maxContentSizeMultiplier prop. Then calculating the font size to apply to the SpannableStringBuilder would involve both the fontSize prop and the maxContentSizeMultiplier prop. If fontSize was set on an outer Text and maxContentSizeMultiplier was set on an inner Text then the inner Text wouldn't be able to calculate the font size to apply to the SpannableStringBuilder because the outer Text's fontSize prop isn't available to it.

The TextAttributes class solves this problem. Every Text has a TextAttributes instance which stores its text props. During rendering, a child's TextAttributes is combined with its parent's and handed down the tree. In this way, during rendering a Text has access to the relevant text props from itself and its ancestors.

This design is inspired by the RCTTextAttributes class from RN iOS.

Bug Fix

This refactoring happens to fix a bug. Today, when setting fontSize on nested Text, allowFontScaling is always treated as though it is true regardless of the value on the root Text. For example, the following snippets should render "hello" identically, Instead, the bottom snippet renders "hello" as though allowFontScaling is true.

<Text allowFontScaling={false} style={{fontSize: 50}}>hello</Text>
<Text allowFontScaling={false}><Text style={{fontSize: 50}}>hello</Text></Text>

(The repro assumes you've increased your device's font setting so that the font size multiplier is not 1.0.)

Introducing the TextAttributes class fixed this. It forced us to think about how inheritance should work for allowFontScaling. In the new implementation, Text components use the value of allowFontScaling from the outermost Text component. This matches the behavior on iOS (the allowFontScaling prop gets ignored on virtual text nodes because it doesn't appear in this list.)

Test Plan:

Verified that fontSize, lineHeight, and letterSpacing (the props that moved to TextAttributes) work properly when inherited, overridden, and not specified (so the default value is used). Tested that these are properly affected by allowFontScaling. Verified that these props also work with TextInput. Verified that lineHeight and inline images behave the same way that they did before my change.

Changelog:

[Android] [Fixed] - Obey allowFontScaling when setting fontSize on nested Text

Adam Comella
Microsoft Corp.

Adam Comella added 2 commits January 8, 2019 16:57
Purpose
----------

This commit fixes a bug and prepares us for adding support for the `maxContentSizeMultiplier` prop (it's currently only supported on iOS).

Details
----------

Today we don't explicitly track inheritance of text props. Instead we rely on `SpannableStringBuilder` to handle this for us. Consider this example:

```
<Text style={{fontSize: 10}}>
  <Text style={{letterSpacing: 5}}>
    ...
  </Text>
</Text>
```

In today's implementation, the inner text doesn't know about `fontSize` (i.e. its `mFontSize` instance variable is `Float.NaN`). But everything works properly because the outer `Text` told `SpannableStringBuilder` to apply the font size across the entire string of text.

However, today's approach breaks down when computing the value to apply to the `SpannableStringBuilder` depends on multiple props. Suppose that RN Android supported the `maxContentSizeMultiplier` prop. Then calculating the font size to apply to the `SpannableStringBuilder` would involve both the `fontSize` prop and the `maxContentSizeMultiplier` prop. If `fontSize` was set on an outer `Text` and `maxContentSizeMultiplier` was set on an inner `Text` then the inner `Text` wouldn't be able to calculate the font size to apply to the `SpannableStringBuilder` because the outer `Text's` `fontSize` prop isn't available to it.

The `TextAttributes` class solves this problem. Every `Text` has a `TextAttributes` instance which stores its text props. During rendering, a child's `TextAttributes` is combined with its parent's and handed down the tree. In this way, during rendering a `Text` has access to the relevant text props from itself and its ancestors.

This design is inspired by the [`RCTTextAttributes`](https://github.com/facebook/react-native/blob/7197aa026b6d262faa8f4dc6bb3e591f860cdc95/Libraries/Text/RCTTextAttributes.m) class from RN iOS.

Bug Fix
----------

This refactoring happens to fix a bug. Today, when setting `fontSize` on nested Text, `allowFontScaling` is always treated as though it is true regardless of the value on the root `Text`. For example, the following snippets should render "hello" identically, Instead, the bottom snippet renders "hello" as though `allowFontScaling` is true.

```
<Text allowFontScaling={false} style={{fontSize: 50}}>hello</Text>
<Text allowFontScaling={false}><Text style={{fontSize: 50}}>hello</Text></Text>
```

(The repro assumes you've increased your device's font setting so that the font size multiplier is not 1.0.)

Introducing the `TextAttributes` class fixed this. It forced us to think about how inheritance should work for `allowFontScaling`. In the new implementation, `Text` components use the value of `allowFontScaling` from the outermost `Text` component. This matches the behavior on iOS (the `allowFontScaling` prop gets ignored on virtual text nodes because it doesn't appear [in this list](https://github.com/facebook/react-native/blob/3749da13127cb7455d533cb2bc5f2cf37470c0c7/Libraries/Text/Text.js#L266-L269).)

Test Plan:
----------

Verified that `fontSize`, `lineHeight`, and `letterSpacing` (the props that moved to `TextAttributes`) work properly when inherited, overridden, and not specified (so the default value is used). Tested that these are properly affected by `allowFontScaling`. Verified that these props also work with `TextInput`. Verified that `lineHeight` and inline images behave the same way that they did before my change.

Changelog:
----------

[Android] [Fixed] - Obey `allowFontScaling` when setting `fontSize` on nested `Text`

Adam Comella
Microsoft Corp.
@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 Jan 9, 2019
We used to use `COMPLEX_UNIT_PX` with `setTextSize` but I accidentally removed it in the last commit.

The symptom was that the placeholder text of a `TextInput` would be rendered at the wrong size.
Copy link
Contributor

@shergin shergin left a comment

Choose a reason for hiding this comment

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

Looks good to me!
I will import and see how this pass internal tests.

int start) {

TextAttributes textAttributes;
if (parentTextAttributes != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we can simplify logic if parentTextAttributes always be non-null. So we can implement TextAttributes::defaultTextAttributes() (with default values, obviously) and pass into the function initially.

I am not sure about this though. I am also curious how I did this in Fabric.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I had a defaultTextAttributes(), I could get rid of the parentTextAttributes == null checks.

Some tricky things to watch out for:

  • What if the data type doesn't have a good value to represent "not set" (everything is float now and float has Float.NaN)?
  • We have to ensure that the root text shadow node always adds fontSize to the span even if it matches the default value (ViewDefaults.FONT_SIZE_SP).

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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@shergin shergin left a comment

Choose a reason for hiding this comment

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

(This is copy paste from David's internal review.)

Can we add an example of this new feature in: react-native-github/RNTester/js/TextExample.android.js that would illustrate the changes?

* LICENSE file in the root directory of this source tree.
*/

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to the top of the java class?

@react-native-bot
Copy link
Collaborator

@rigdern merged commit 1bdb250 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Jan 15, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 15, 2019
@rigdern rigdern deleted the rigdern/text-attributes1 branch January 15, 2019 02:25
matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019
…cebook#22917)

Summary:
Purpose
----------

This commit fixes a bug and prepares us for adding support for the `maxContentSizeMultiplier` prop (it's currently only supported on iOS).

Details
----------

Today we don't explicitly track inheritance of text props. Instead we rely on `SpannableStringBuilder` to handle this for us. Consider this example:

```
<Text style={{fontSize: 10}}>
  <Text style={{letterSpacing: 5}}>
    ...
  </Text>
</Text>
```

In today's implementation, the inner text doesn't know about `fontSize` (i.e. its `mFontSize` instance variable is `Float.NaN`). But everything works properly because the outer `Text` told `SpannableStringBuilder` to apply the font size across the entire string of text.

However, today's approach breaks down when computing the value to apply to the `SpannableStringBuilder` depends on multiple props. Suppose that RN Android supported the `maxContentSizeMultiplier` prop. Then calculating the font size to apply to the `SpannableStringBuilder` would involve both the `fontSize` prop and the `maxContentSizeMultiplier` prop. If `fontSize` was set on an outer `Text` and `maxContentSizeMultiplier` was set on an inner `Text` then the inner `Text` wouldn't be able to calculate the font size to apply to the `SpannableStringBuilder` because the outer `Text's` `fontSize` prop isn't available to it.

The `TextAttributes` class solves this problem. Every `Text` has a `TextAttributes` instance which stores its text props. During rendering, a child's `TextAttributes` is combined with its parent's and handed down the tree. In this way, during rendering a `Text` has access to the relevant text props from itself and its ancestors.

This design is inspired by the [`RCTTextAttributes`](https://github.com/facebook/react-native/blob/7197aa026b6d262faa8f4dc6bb3e591f860cdc95/Libraries/Text/RCTTextAttributes.m) class from RN iOS.

Bug Fix
----------

This refactoring happens to fix a bug. Today, when setting `fontSize` on nested Text, `allowFontScaling` is always treated as though it is true regardless of the value on the root `Text`. For example, the following snippets should render "hello" identically, Instead, the bottom snippet renders "hello" as though `allowFontScaling` is true.

```
<Text allowFontScaling={false} style={{fontSize: 50}}>hello</Text>
<Text allowFontScaling={false}><Text style={{fontSize: 50}}>hello</Text></Text>
```

(The repro assumes you've increased your device's font setting so that the font size multiplier is not 1.0.)

Introducing the `TextAttributes` class fixed this. It forced us to think about how inheritance should work for `allowFontScaling`. In the new implementation, `Text` components use the value of `allowFontScaling` from the outermost `Text` component. This matches the behavior on iOS (the `allowFontScaling` prop gets ignored on virtual text nodes because it doesn't appear [in this list](https://github.com/facebook/react-native/blob/3749da13127cb7455d533cb2bc5f2cf37470c0c7/Libraries/Text/Text.js#L266-L269).)
Pull Request resolved: facebook#22917

Reviewed By: mdvacca

Differential Revision: D13630235

Pulled By: shergin

fbshipit-source-id: e58f458de4fc3cdcbec49c8e0509da51966ef93c
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.

4 participants