-
Notifications
You must be signed in to change notification settings - Fork 24.4k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Android Text: More robust logic for handling inherited text props (#2…
…2917) 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: #22917 Reviewed By: mdvacca Differential Revision: D13630235 Pulled By: shergin fbshipit-source-id: e58f458de4fc3cdcbec49c8e0509da51966ef93c
- Loading branch information
1 parent
e6f7d69
commit 1bdb250
Showing
6 changed files
with
209 additions
and
93 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
143 changes: 143 additions & 0 deletions
143
ReactAndroid/src/main/java/com/facebook/react/views/text/TextAttributes.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| /** | ||
| * Copyright (c) Facebook, Inc. and its affiliates. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| /* | ||
| * Currently, TextAttributes consists of a subset of text props that need to be passed from parent | ||
| * to child so inheritance can be implemented correctly. An example complexity that causes a prop | ||
| * to end up in TextAttributes is when multiple props need to be considered together to determine | ||
| * the rendered aka effective value. For example, to figure out the rendered/effective font size, | ||
| * you need to take into account the fontSize and allowFontScaling props. | ||
| */ | ||
|
|
||
| package com.facebook.react.views.text; | ||
|
|
||
| import com.facebook.react.uimanager.PixelUtil; | ||
| import com.facebook.react.uimanager.ViewDefaults; | ||
|
|
||
| public class TextAttributes { | ||
| private boolean mAllowFontScaling = true; | ||
| private float mFontSize = Float.NaN; | ||
| private float mLineHeight = Float.NaN; | ||
| private float mLetterSpacing = Float.NaN; | ||
| private float mHeightOfTallestInlineImage = Float.NaN; | ||
|
|
||
| public TextAttributes() { | ||
| } | ||
|
|
||
| public TextAttributes applyChild(TextAttributes child) { | ||
| TextAttributes result = new TextAttributes(); | ||
|
|
||
| // allowFontScaling is always determined by the root Text | ||
| // component so don't allow the child to overwrite it. | ||
| result.mAllowFontScaling = mAllowFontScaling; | ||
|
|
||
| result.mFontSize = !Float.isNaN(child.mFontSize) ? child.mFontSize : mFontSize; | ||
| result.mLineHeight = !Float.isNaN(child.mLineHeight) ? child.mLineHeight : mLineHeight; | ||
| result.mLetterSpacing = !Float.isNaN(child.mLetterSpacing) ? child.mLetterSpacing : mLetterSpacing; | ||
| result.mHeightOfTallestInlineImage = !Float.isNaN(child.mHeightOfTallestInlineImage) ? child.mHeightOfTallestInlineImage : mHeightOfTallestInlineImage; | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| // Getters and setters | ||
| // | ||
|
|
||
| public boolean getAllowFontScaling() { | ||
| return mAllowFontScaling; | ||
| } | ||
|
|
||
| public void setAllowFontScaling(boolean value) { | ||
| mAllowFontScaling = value; | ||
| } | ||
|
|
||
| public float getFontSize() { | ||
| return mFontSize; | ||
| } | ||
|
|
||
| public void setFontSize(float value) { | ||
| mFontSize = value; | ||
| } | ||
|
|
||
| public float getLineHeight() { | ||
| return mLineHeight; | ||
| } | ||
|
|
||
| public void setLineHeight(float value) { | ||
| mLineHeight = value; | ||
| } | ||
|
|
||
| public float getLetterSpacing() { | ||
| return mLetterSpacing; | ||
| } | ||
|
|
||
| public void setLetterSpacing(float value) { | ||
| mLetterSpacing = value; | ||
| } | ||
|
|
||
| public float getHeightOfTallestInlineImage() { | ||
| return mHeightOfTallestInlineImage; | ||
| } | ||
|
|
||
| public void setHeightOfTallestInlineImage(float value) { | ||
| mHeightOfTallestInlineImage = value; | ||
| } | ||
|
|
||
| // Getters for effective values | ||
| // | ||
| // In general, these return `Float.NaN` if the property doesn't have a value. | ||
| // | ||
|
|
||
| // Always returns a value because uses a hardcoded default as a fallback. | ||
| public int getEffectiveFontSize() { | ||
| float fontSize = !Float.isNaN(mFontSize) ? mFontSize : ViewDefaults.FONT_SIZE_SP; | ||
| return mAllowFontScaling | ||
| ? (int) Math.ceil(PixelUtil.toPixelFromSP(fontSize)) | ||
| : (int) Math.ceil(PixelUtil.toPixelFromDIP(fontSize)); | ||
| } | ||
|
|
||
| public float getEffectiveLineHeight() { | ||
| if (Float.isNaN(mLineHeight)) { | ||
| return Float.NaN; | ||
| } | ||
|
|
||
| float lineHeight = mAllowFontScaling | ||
| ? PixelUtil.toPixelFromSP(mLineHeight) | ||
| : PixelUtil.toPixelFromDIP(mLineHeight); | ||
|
|
||
| // Take into account the requested line height | ||
| // and the height of the inline images. | ||
| boolean useInlineViewHeight = | ||
| !Float.isNaN(mHeightOfTallestInlineImage) | ||
| && mHeightOfTallestInlineImage > lineHeight; | ||
| return useInlineViewHeight ? mHeightOfTallestInlineImage : lineHeight; | ||
| } | ||
|
|
||
| public float getEffectiveLetterSpacing() { | ||
| if (Float.isNaN(mLetterSpacing)) { | ||
| return Float.NaN; | ||
| } | ||
|
|
||
| return mAllowFontScaling | ||
| ? PixelUtil.toPixelFromSP(mLetterSpacing) | ||
| : PixelUtil.toPixelFromDIP(mLetterSpacing); | ||
| } | ||
|
|
||
| public String toString() { | ||
| return ( | ||
| "TextAttributes {" | ||
| + "\n getAllowFontScaling(): " + getAllowFontScaling() | ||
| + "\n getFontSize(): " + getFontSize() | ||
| + "\n getEffectiveFontSize(): " + getEffectiveFontSize() | ||
| + "\n getHeightOfTallestInlineImage(): " + getHeightOfTallestInlineImage() | ||
| + "\n getLetterSpacing(): " + getLetterSpacing() | ||
| + "\n getEffectiveLetterSpacing(): " + getEffectiveLetterSpacing() | ||
| + "\n getLineHeight(): " + getLineHeight() | ||
| + "\n getEffectiveLineHeight(): " + getEffectiveLineHeight() | ||
| + "\n}" | ||
| ); | ||
| } | ||
| } |
Oops, something went wrong.