Fix `adjustsFontSizeToFit` for strings with a single character #47082

j-piasecki · 2024-10-17T08:50:34Z

Summary:

On Android adjustsFontSizeToFit relies on two metrics:

Text with line breaks results in more lines than maximumNumberOfLines
The overall height of the text is larger than the available height

None of these two was fulfilled when a single-character string had a higher width than the available one (a single character will not be broken into multiple lines). This PR adds exactly that as a third option to trigger the scaling algorithm - a single-character string that has a higher width than the available one.

On iOS adjustsFontSizeToFit relies on truncatedGlyphRangeInLineFragmentForGlyphAtIndex which seems to be returning NSNotFound when a single-character sting gets truncated. Similarly to Android, this PR adds an additional check to make sure that single-character strings actually fit inside the container.

Changelog:

[GENERAL] [FIXED] - Fixed adjustsFontSizeToFit not working for text with a single character

Test Plan:

Tested on the code from the issue:

Android (old arch)	Android (new arch)	iOS (old arch)	iOS (new arch)

cortinico · 2024-10-17T16:06:59Z

Let's make sure the CI is green before we import this 👍

itsramiel · 2024-10-19T07:09:05Z

Fixes the bug

facebook-github-bot · 2024-10-20T12:30:58Z

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

cortinico · 2024-11-06T20:50:43Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

@@ -600,6 +601,7 @@ private static Layout createLayout(
            text.getSpanFlags(span));
        text.removeSpan(span);
      }
+      boring = BoringLayout.isBoring(text, paint);


Quoting @javache:

This is quite of an expensive call

Also we call this just above at the beginning of the adjustSpannableFontToFit function. Why do you need call this again?

It updates the font metrics after applying the smaller font size. In this exact case, the layout height is larger than the available height and without the update to boring it ends up shrinking the text to the smallest allowed value:

The other two numbers end up as a StaticLayout so they aren't affected.

We could probably guard this with a check to only recalculate BoringLayout metics if existing text was boring (adjusting font size won't change boringness, but will change the layout result). But I think needing to recreate it makes sense.

Added a check in fb3f1f0

cortinico · 2024-11-06T20:51:29Z

...act-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java

@@ -75,6 +75,7 @@ public long measure(
                  "Spannable element has not been prepared in onBeforeLayout");

          Layout layout = measureSpannedText(text, width, widthMode);
+          TextPaint paint = new TextPaint(sTextPaintInstance);


Can you amend all the changes to the paper implementation. This is quite intrusive and we'd rather just have the fix for Fabric only.

cc @NickGerleman

Done - ec2182b

cortinico · 2024-11-06T20:51:54Z

(Apologise for the late review @j-piasecki)

facebook-github-bot · 2024-11-07T15:05:28Z

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

NickGerleman · 2024-12-05T00:50:47Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

@@ -630,7 +630,8 @@ private static Layout createLayout(
                && maximumNumberOfLines != 0
                && layout.getLineCount() > maximumNumberOfLines)
            || (heightYogaMeasureMode != YogaMeasureMode.UNDEFINED
-                && layout.getHeight() > height))) {
+                && layout.getHeight() > height)
+            || (text.length() == 1 && layout.getPaint().measureText(text.toString()) > width))) {


This throws away metric effecting formatting on the text, like the font, whether its italic, how its justified, etc. It also throws out initial font size, which we only set on the paint after this.

If none of the existing line accessors in layout can tell that we've overflowed, and we really needed to do a new layout (we should avoid if possible), we would want to layout the full text, either using existing BoringLayout (simple not wrapping layout) which can give full width, or recreating a new layout without width constraint to get natural width. Though doing another layout here is really not ideal if there is any accessor on the existing layout that will give us this measurement.

Existing layout has fillCharacterBounds() accessor to get glyph rects in the existing layout. At a glance, if none of the line accessors show where the single glyph lies, this would I think let us avoid relayout.

I changed it to getLineWidth in fb3f1f0. It seems like this value isn't clipped to the width of the layout.

NickGerleman · 2024-12-05T00:51:29Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

@@ -600,6 +601,7 @@ private static Layout createLayout(
            text.getSpanFlags(span));
        text.removeSpan(span);
      }
+      boring = BoringLayout.isBoring(text, paint);


We could probably guard this with a check to only recalculate BoringLayout metics if existing text was boring (adjusting font size won't change boringness, but will change the layout result). But I think needing to recreate it makes sense.

facebook-github-bot · 2024-12-06T13:43:13Z

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

facebook-github-bot · 2024-12-09T18:16:39Z

@NickGerleman merged this pull request in 47822e9.

react-native-bot · 2024-12-09T18:16:43Z

This pull request was successfully merged by @j-piasecki in 47822e9

^{When will my fix make it into a release? | How to file a pick request?}

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 17, 2024

cortinico requested changes Nov 6, 2024

View reviewed changes

j-piasecki force-pushed the @jpiasecki/fix-font-size-adjustment-single-char branch from 3234b8e to ec2182b Compare November 7, 2024 11:04

NickGerleman requested changes Dec 5, 2024

View reviewed changes

j-piasecki added 2 commits December 5, 2024 09:24

Fix adjustsFontSizeToFit for strings with a single character

a2b3743

Use getLineWidth instead of measureText

fb3f1f0

j-piasecki force-pushed the @jpiasecki/fix-font-size-adjustment-single-char branch from ec2182b to fb3f1f0 Compare December 5, 2024 09:17

NickGerleman approved these changes Dec 6, 2024

View reviewed changes

facebook-github-bot closed this in 47822e9 Dec 9, 2024

facebook-github-bot added the Merged This PR has been merged. label Dec 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `adjustsFontSizeToFit` for strings with a single character #47082

Fix `adjustsFontSizeToFit` for strings with a single character #47082

j-piasecki commented Oct 17, 2024

cortinico commented Oct 17, 2024

itsramiel commented Oct 19, 2024

facebook-github-bot commented Oct 20, 2024

cortinico Nov 6, 2024

j-piasecki Nov 7, 2024

NickGerleman Dec 5, 2024 •

edited

Loading

j-piasecki Dec 5, 2024

cortinico Nov 6, 2024

j-piasecki Nov 7, 2024

cortinico commented Nov 6, 2024

facebook-github-bot commented Nov 7, 2024

NickGerleman Dec 5, 2024

j-piasecki Dec 5, 2024

NickGerleman Dec 5, 2024 •

edited

Loading

facebook-github-bot commented Dec 6, 2024

facebook-github-bot commented Dec 9, 2024

react-native-bot commented Dec 9, 2024

Fix adjustsFontSizeToFit for strings with a single character #47082

Fix adjustsFontSizeToFit for strings with a single character #47082

Conversation

j-piasecki commented Oct 17, 2024

Summary:

Changelog:

Test Plan:

cortinico commented Oct 17, 2024

itsramiel commented Oct 19, 2024

facebook-github-bot commented Oct 20, 2024

cortinico Nov 6, 2024

Choose a reason for hiding this comment

j-piasecki Nov 7, 2024

Choose a reason for hiding this comment

NickGerleman Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

j-piasecki Dec 5, 2024

Choose a reason for hiding this comment

cortinico Nov 6, 2024

Choose a reason for hiding this comment

j-piasecki Nov 7, 2024

Choose a reason for hiding this comment

cortinico commented Nov 6, 2024

facebook-github-bot commented Nov 7, 2024

NickGerleman Dec 5, 2024

Choose a reason for hiding this comment

j-piasecki Dec 5, 2024

Choose a reason for hiding this comment

NickGerleman Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Dec 6, 2024

facebook-github-bot commented Dec 9, 2024

react-native-bot commented Dec 9, 2024

Fix `adjustsFontSizeToFit` for strings with a single character #47082

Fix `adjustsFontSizeToFit` for strings with a single character #47082

NickGerleman Dec 5, 2024 •

edited

Loading

NickGerleman Dec 5, 2024 •

edited

Loading