-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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 elements that have adjustsFontSizeToFit do not have their font size adjusted when the height of a parent element is changed. #30717
Comments
Would this be a good first issue? I a newcomer and would love to try working on it |
Thanks for the Snack! Confirmed the repro on my end. Downsizes on iOS but not on Android. And does nothing on Web, I assume it's just not implemented there. The PR to implement this on Android is from September 2019, so more than a year ago. I assume it worked then so this is likely a regression? @ydalmia I don't know if this is a good first issue but it does look like a legit bug and I'm sure it would be helpful for you to take a look. |
This comment has been minimized.
This comment has been minimized.
I don't think I tested changing the layout dynamically in the original PR. If someone can figure out a solution feel free to open a PR and I can have a look. |
Thanks Janic Duplessis, I summarize my findings here, I'll try my best to fix this issue. how is the text size decreased to fit height of the view?
POINT 4b. BoringLayout isBoring()
/**
* Returns null if not boring; the width, ascent, and descent in the
* provided Metrics object (or a new one if the provided one was null)
* if boring.
* @hide
*/
@UnsupportedAppUsage
public static Metrics isBoring(CharSequence text, TextPaint paint,
TextDirectionHeuristic textDir, Metrics metrics) {
final int textLength = text.length();
if (hasAnyInterestingChars(text, textLength)) {
return null; // There are some interesting characters. Not boring.
}
if (textDir != null && textDir.isRtl(text, 0, textLength)) {
return null; // The heuristic considers the whole text RTL. Not boring.
}
if (text instanceof Spanned) {
Spanned sp = (Spanned) text;
Object[] styles = sp.getSpans(0, textLength, ParagraphStyle.class);
if (styles.length > 0) {
return null; // There are some ParagraphStyle spans. Not boring.
}
}
Metrics fm = metrics;
if (fm == null) {
fm = new Metrics();
} else {
fm.reset();
}
TextLine line = TextLine.obtain();
line.set(paint, text, 0, textLength, Layout.DIR_LEFT_TO_RIGHT,
Layout.DIRS_ALL_LEFT_TO_RIGHT, false, null,
0 /* ellipsisStart, 0 since text has not been ellipsized at this point */,
0 /* ellipsisEnd, 0 since text has not been ellipsized at this point */);
fm.width = (int) Math.ceil(line.metrics(fm));
TextLine.recycle(line);
return fm;
} The first time we render the text
The second time we render the text after updating the height, |
how is layout.getHeight() calculated and why does it stop the iteration?
layout.getHeight() is the difference between spacing = metrics.bottom - metrics.top; First Time we render
Second Time we render
the value react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java Lines 84 to 85 in 7b09eb5
how do we compute the new size of the text?
we add to the TextView a Span with the new font size and then measure the TextView react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java Lines 92 to 103 in 7b09eb5
how does `BoringLayout` compute the height/width of the text?
The computation is done in init /* package */ void init(CharSequence source, TextPaint paint, Alignment align,
BoringLayout.Metrics metrics, boolean includePad, boolean trustWidth) {
int spacing;
if (source instanceof String && align == Layout.Alignment.ALIGN_NORMAL) {
mDirect = source.toString();
} else {
mDirect = null;
}
mPaint = paint;
if (includePad) {
spacing = metrics.bottom - metrics.top;
mDesc = metrics.bottom;
} else {
spacing = metrics.descent - metrics.ascent;
mDesc = metrics.descent;
}
mBottom = spacing;
if (trustWidth) {
mMax = metrics.width;
} else {
/*
* If we have ellipsized, we have to actually calculate the
* width because the width that was passed in was for the
* full text, not the ellipsized form.
*/
TextLine line = TextLine.obtain();
line.set(paint, source, 0, source.length(), Layout.DIR_LEFT_TO_RIGHT,
Layout.DIRS_ALL_LEFT_TO_RIGHT, false, null,
mEllipsizedStart, mEllipsizedStart + mEllipsizedCount);
mMax = (int) Math.ceil(line.metrics(null));
TextLine.recycle(line);
}
if (includePad) {
mTopPadding = metrics.top - metrics.ascent;
mBottomPadding = metrics.bottom - metrics.descent;
}
} |
This PR fixes facebook#30717, a bug in `<Text adjustsFontSizeToFit={true}>` implementation that prevents it from adjusting text size dynamically on Android. The way `adjustsFontSizeToFit` was implemented in facebook#26389 (and the design of ReactTextShadowNode) implies that Yoga will call `onMeasure` on every size change of a `<Text>` component, which is actually not the case (Yoga can cache the results of the measures, call the function multiple times or do not call at all inferring the size from the size constraints). The implementation of `adjustsFontSizeToFit` computes the adjusted string inside the measure function and then eventually passes that to the view layer where it's being rendered. The proper fix of this issue requires the full redesign of the measure and rendering pipelines and separating them, and that... would be too invasive. And, I believe, this issue is already fixed in Fabric where this part is already designed this way. Instead, this diff implements a small workaround: if `adjustsFontSizeToFit` is enabled, we manually dirty the Yoga node and mark the shadow node updated to force remeasuring.
…tely (#33135) Summary: Fixes the infinite loop explained in the issue #33129 by reverting commit 5902152. PR #31538. `onCollectExtraUpdates` is part of the node update cycle. By marking the node as updated `markUpdated()` in `onCollectExtraUpdates` we are restarting the update infinitely. Unfortunately, reverting this PR also reintroduces the original issue #30717 which IMO is minor compared to the infinite loop. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Android] [Fixed] - Text with adjustsFontSizeToFit changes the text layout infinitely Pull Request resolved: #33135 Test Plan: I added a console.log to the Text `onTextLayout` in [packages/rn-tester/js/examples/Text/TextAdjustsDynamicLayoutExample.js ](https://github.com/facebook/react-native/blob/main/packages/rn-tester/js/examples/Text/TextAdjustsDynamicLayoutExample.js) to see if the infinite loop is gone. ![image](https://user-images.githubusercontent.com/3791120/154523914-e6aa7cf5-7a1c-488f-a392-898f4c85a833.png) ![Screen Shot 2022-02-17 at 11 20 31 AM](https://user-images.githubusercontent.com/3791120/154524274-880c3bed-d2c6-456b-8947-42e75793c424.jpg) ``` Reviewed By: ShikaSD Differential Revision: D34310218 Pulled By: lunaleaps fbshipit-source-id: 0d40f49d15c562ec25983145897bd95dc182f897
…tely (#33135) Summary: Fixes the infinite loop explained in the issue #33129 by reverting commit 5902152. PR #31538. `onCollectExtraUpdates` is part of the node update cycle. By marking the node as updated `markUpdated()` in `onCollectExtraUpdates` we are restarting the update infinitely. Unfortunately, reverting this PR also reintroduces the original issue #30717 which IMO is minor compared to the infinite loop. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Android] [Fixed] - Text with adjustsFontSizeToFit changes the text layout infinitely Pull Request resolved: #33135 Test Plan: I added a console.log to the Text `onTextLayout` in [packages/rn-tester/js/examples/Text/TextAdjustsDynamicLayoutExample.js ](https://github.com/facebook/react-native/blob/main/packages/rn-tester/js/examples/Text/TextAdjustsDynamicLayoutExample.js) to see if the infinite loop is gone. ![image](https://user-images.githubusercontent.com/3791120/154523914-e6aa7cf5-7a1c-488f-a392-898f4c85a833.png) ![Screen Shot 2022-02-17 at 11 20 31 AM](https://user-images.githubusercontent.com/3791120/154524274-880c3bed-d2c6-456b-8947-42e75793c424.jpg) ``` Reviewed By: ShikaSD Differential Revision: D34310218 Pulled By: lunaleaps fbshipit-source-id: 0d40f49d15c562ec25983145897bd95dc182f897
…tely (#33135) Summary: Fixes the infinite loop explained in the issue #33129 by reverting commit 5902152. PR #31538. `onCollectExtraUpdates` is part of the node update cycle. By marking the node as updated `markUpdated()` in `onCollectExtraUpdates` we are restarting the update infinitely. Unfortunately, reverting this PR also reintroduces the original issue #30717 which IMO is minor compared to the infinite loop. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Android] [Fixed] - Text with adjustsFontSizeToFit changes the text layout infinitely Pull Request resolved: #33135 Test Plan: I added a console.log to the Text `onTextLayout` in [packages/rn-tester/js/examples/Text/TextAdjustsDynamicLayoutExample.js ](https://github.com/facebook/react-native/blob/main/packages/rn-tester/js/examples/Text/TextAdjustsDynamicLayoutExample.js) to see if the infinite loop is gone. ![image](https://user-images.githubusercontent.com/3791120/154523914-e6aa7cf5-7a1c-488f-a392-898f4c85a833.png) ![Screen Shot 2022-02-17 at 11 20 31 AM](https://user-images.githubusercontent.com/3791120/154524274-880c3bed-d2c6-456b-8947-42e75793c424.jpg) ``` Reviewed By: ShikaSD Differential Revision: D34310218 Pulled By: lunaleaps fbshipit-source-id: 0d40f49d15c562ec25983145897bd95dc182f897
Description
On Android,
Text
elements that haveadjustsFontSizeToFit={true}
do not have their font size adjusted when the height of a parent element is changed. This works as desired on iOS. This seems to be a defect in the excellent PR #26389 by @janicduplessis.React Native version:
Steps To Reproduce
Expected Results
The font size of the
Text
element should be changed when the height of a parent element is changed on Android, consistent with iOS.Snack, code example, screenshot, or link to a repository:
https://snack.expo.io/TAJyBmNdv
The text was updated successfully, but these errors were encountered: