Fix minimumFontScale with adjustsFontSizeToFit in the New Architecture - #58492
conner1reimers wants to merge 6 commits into
Conversation
…itecture Fabric's ParagraphAttributes carried `minimumFontSize` and `maximumFontSize`, which no `<Text>` prop can set, so RCTTextLayoutManager always fell back to a 4pt floor and ignored `minimumFontScale`. Replace those two fields with the existing `minimumFontScale` (default 0) everywhere it is parsed, diffed and serialized, and derive the floor in RCTTextLayoutManager the way the legacy renderer did: MAX(minimumFontScale * largest font size in the attributed string, 4.0). MapBuffer key 6 now carries the scale instead of an absolute size; key 7 is no longer written. Updates ParagraphAttributesTest for the removed fields and adds an RNTester example exercising `minimumFontScale`.
…w Architecture TextLayoutManager read MapBuffer key 6 as an absolute minimum font size in pixels and fell back to 4dp when it was NaN. Since nothing on the JS side ever set `minimumFontSize`, Android always used the 4dp floor and `minimumFontScale` was ignored. Key 6 now carries `minimumFontScale`, so adjustSpannableFontToFit() finds the largest ReactAbsoluteSizeSpan first and derives the floor as max(minimumFontScale * largestFontSize, 4dp), matching iOS and the formula the original Android implementation (react#26389) used. A NaN or non-positive scale keeps the bare 4dp floor. Both the measurement path and the view path go through this one function, so only its interpretation of the value changes. Renames PA_KEY_MINIMUM_FONT_SIZE to PA_KEY_MINIMUM_FONT_SCALE, drops the unused PA_KEY_MAXIMUM_FONT_SIZE, and renames ReactTextView.setMinimumFontSize() to setMinimumFontScale() (public API dump updated). Adds Robolectric coverage for the floor computation, an RNTester example, and drops the `@platform ios` annotation from the `minimumFontScale` prop docs.
ReactTextView.updateView() cleared the ellipsize location whenever adjustsFontSizeToFit was on. Now that minimumFontScale can stop the text from shrinking further, text that still does not fit at the floor was clipped instead of ellipsized. iOS applies the ellipsize mode regardless of adjustsFontSizeToFit, so do the same here.
javache
left a comment
There was a problem hiding this comment.
Thanks for the PR! A few comments.
I think it would be good to split this up in different PR's
- Removal of the undocumented / inconsistently supported maximumFontSize API
- Adding minimumFontScale to Android, and deprecating minimumFontSize (backwards compat would be great if feasible)
| applyTextAttributes(); | ||
| } | ||
|
|
||
| public void setMinimumFontSize(float minimumFontSize) { |
There was a problem hiding this comment.
This is a public API change - please avoid. Can you add back the old API as deprecated?
There was a problem hiding this comment.
setMinimumFontSize() was added back as deprecated, and setMinimumFontScale() was added alongside it. minimumFontSize still takes precedence when explicitly set, so existing callers keep the old behavior.
| mNumberOfLines == ViewDefaults.NUMBER_OF_LINES || mAdjustsFontSizeToFit | ||
| ? null | ||
| : mEllipsizeLocation; |
There was a problem hiding this comment.
Why this behavioural chagne?
There was a problem hiding this comment.
This is for the case where text reaches minimumFontScale and still doesn't fit. Android currently clears ellipsizeMode whenever adjustsFontSizeToFit is enabled, so that overflow gets clipped instead.
The original Android implementation did this to avoid some incorrect ellipsizing cases (#26389 (comment)), with overflow at the minimum scale being a known tradeoff. Since minimumFontScale is now actually respected, I kept this change so overflow at the floor uses the requested ellipsizeMode, which matches iOS.
It's kept as a separate commit, so I can split it out if you'd prefer.
| const val PA_KEY_TEXT_ALIGN_VERTICAL: Int = 8 | ||
| const val PA_KEY_TEXT_WIDTH_MODE: Int = 9 |
| if (paragraphAttributes.adjustsFontSizeToFit) { | ||
| CGFloat minimumFontSize = !isnan(paragraphAttributes.minimumFontSize) ? paragraphAttributes.minimumFontSize : 4.0; | ||
| CGFloat maximumFontSize = !isnan(paragraphAttributes.maximumFontSize) ? paragraphAttributes.maximumFontSize : 96.0; | ||
| CGFloat maximumFontSize = [self _maximumFontSizeInAttributedString:attributedString]; |
There was a problem hiding this comment.
This was moved to #58529. The behavior change from the 96pt fallback is covered in that PR's summary.
|
Thanks for the review! I've split this up as suggested:
I'll rebase this onto #58529 after it's merged, since both use the same largest-font-size helper on iOS. The RNTester screenshots in the description are still from the pre-split branch, but the behavior shown there is unchanged. |
Summary:
minimumFontScalehas no effect when used withadjustsFontSizeToFitunder the New Architecture on either platform (#50248). Text shrinks all the way down to the hard-coded floor regardless of the requested scale.The root cause is the same on both platforms. Fabric's
ParagraphAttributescarriesminimumFontSize, but no<Text>or<TextInput>prop ever sets it, so it is alwaysNaN:RCTTextLayoutManagerfalls back to a 4pt minimum wheneverminimumFontSizeisNaN, so the floor is always 4pt.minimumFontSizeinto MapBuffer key 6, andTextLayoutManager.adjustSpannableFontToFit()treats that value as an absolute minimum font size, falling back to 4dp when it isNaN. Same result: the floor is always 4dp.This PR serializes
minimumFontScale(the field already existed) into a new MapBuffer key (10). Each platform then derives the minimum font size from that scale.minimumFontSizeand MapBuffer key 6 are left as they are and deprecated in favor ofminimumFontScale; an explicitminimumFontSizestill takes precedence over the scale on both platforms, so existing callers keep working unchanged. The legacy renderers based the scale on the outermost text's font size; using the largest font size in the attributed string follows the approach from #43543.RCTTextLayoutManagerfinds the largest font size in the attributed string and computes the minimum asMAX(minimumFontScale * largestFontSize, 4.0).adjustSpannableFontToFit()finds the largestReactAbsoluteSizeSpanfirst and usesmax(minimumFontScale * largestFontSize, 4dp). This uses the same scale-based calculation as the original Android implementation in ImplementadjustsFontSizeToFiton Android #26389, although the legacy renderer based it on the outer text's effective font size rather than the largest span. ANaNor non-positive scale keeps the bare 4dp floor. Both the measurement path (createLayout()) and the view path (ReactTextViewManager->ReactTextView.onDraw()) go through this one function, so only its interpretation of the value changes.PA_KEY_MINIMUM_FONT_SCALEis added as key 10,ReactTextView.setMinimumFontSize()is kept and marked@Deprecated, andsetMinimumFontScale()is added alongside it (theReactAndroid.apidump was also updated).ReactTextView.updateView()no longer clears the ellipsize location whenadjustsFontSizeToFitis set. With the floor effectively pinned at 4dp that branch was dead, since text always fit. Now that a floor can stop the text from shrinking, text that still overflows at the floor gets the requestedellipsizeModeinstead of being clipped, matching iOS.The 4pt/4dp floor is kept on both platforms.
minimumFontScalestill has an@platform iosannotation inTextProps.js, so that annotation is removed since it's supported on Android as well.Depends on #58529, which removes the unused
maximumFontSizeattribute and introduces the largest-font-size helper this PR uses on iOS; I'll rebase this PR onto it once it lands.This supersedes #54072 (iOS only) and #43543.
Fixes #50248.
Changelog:
[GENERAL] [FIXED] - Fix
minimumFontScalewithadjustsFontSizeToFitin the New ArchitectureTest Plan:
Unit tests
ParagraphAttributesTest.cppto coverminimumFontScale.TextLayoutManagerMinimumFontScaleTest.ktcovering the scale calculation, the 4dp floor, the precedence of an explicitminimumFontSizeover the scale, and text that already fits.ReactTextViewTest.ktcase verifying thatadjustsFontSizeToFitpreserves the ellipsize location.yarn cxx-api-build) for the new MapBuffer key../gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests 'com.facebook.react.views.text.*'All 47
com.facebook.react.views.texttests pass.RNTester
Added a
minimumFontScale={0.5}line to theadjustsFontSizeToFitsection of the Text example on both platforms ("Can limit how small the text becomes with minimumFontScale"). Before this change the line shrinks past the requested floor; after it stops at 50% of the original font size and is ellipsized.Each screenshot below was captured on the same machine, on an iPhone 17 simulator and an Android API 35 emulator. "Before" is this branch's base commit with only the new RNTester example applied.