fix: Improve text line height calculation to properly align text on iOS #46884

ArekChr · 2024-10-08T10:08:50Z

Summary:

This pull request addresses an issue in React Native on iOS: Text gets truncated when the line height is too small, even if the line-height matches or exceeds the font size.

The existing logic did not correctly handle font metrics (ascent, descent, top, bottom) when the total height exceeded the line height. The implementation now ensures a more balanced vertical space distribution, reducing text clipping and misalignment.

The fix mirrors a similar solution made on Android, ensuring consistency across both platforms.

For more context, see the related issue: React Native issue #29507.

Changelog:

[IOS] [FIXED] - Fixed an issue where text clipping and misalignment occurred when the line-height was smaller than the total ascent and descent of the font.

Test Plan:

The result is an improvement over the current implementation, reducing the occurrence of text truncation. While minimal truncation still exists at single-line heights, particularly with special characters, the implementation better preserves the vertical alignment of the text compared to previous behavior. However, like the Android implementation, this does not fully resolve the issue of React Native truncating content that is out of bounds of the span.

Old Arch Before	Old Arch After

New Arch Before	New Arch After

facebook-github-bot · 2024-10-08T10:08:57Z

Hi @ArekChr!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2024-10-08T11:00:54Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

okwasniewski · 2024-10-08T12:47:16Z

packages/react-native/Libraries/Text/Text/RCTTextView.mm

+                                   contentFrame:(CGRect)contentFrame {
+  UIFont *font = [textStorage attribute:NSFontAttributeName atIndex:0 effectiveRange:NULL];
+  if (!font) {
+      font = [UIFont systemFontOfSize:14];


[Q] Maybe we can use systemFontSize here?

https://developer.apple.com/documentation/uikit/uifont/1623395-systemfontsize?language=objc

I couldn’t find systemFontSize used anywhere else in the repo, and the default font size of 14 has been hardcoded. Also, since systemFontSize is not available on tvOS, I’m not sure if it would be a suitable replacement. Let me know your thoughts on whether we should stick with the hardcoded value for consistency or if there’s a preferred approach.

@NickGerleman

This seems sketch. E.g. what if there is a font multiplier applied?

okwasniewski · 2024-10-08T12:48:54Z

packages/react-native/Libraries/Text/Text/RCTTextView.mm

+  CGFloat verticalOffset = 0;
+  if (textHeight > lineHeight) {
+      CGFloat difference = textHeight - lineHeight;
+      verticalOffset = difference / 2.0;
+  } else if (textHeight < lineHeight) {
+      CGFloat difference = lineHeight - textHeight;
+      verticalOffset = -(difference / 2.0);
+  }


I think comment explaining the calculations might be useful

Sure, added comments

mrousavy · 2024-10-08T14:52:26Z

nice!

…ributes in calculateDrawingPoint.

cipolleschi

Changes looks good. We discussed this internally and we can proceed importing it.

To set the expectation, this might take some time to merge, given that this might affect internal tests and product code and we would need some time to fix them. But thankfully, we have a feature flag to control the rollout! 😄

facebook-github-bot · 2024-10-09T12:35:08Z

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

cipolleschi · 2024-10-09T12:49:45Z

@ArekChr CI is currently red. The error is

[React-RCTText] Compiling RCTTextView.mm
Error: use of undeclared identifier 'ReactNativeFeatureFlags'; did you mean 'facebook::react::ReactNativeFeatureFlags'?
  if (ReactNativeFeatureFlags::enableLineHeightCentering()) {

Could you please verify why this is happening and fix it?
You should be able to build it locally running these commands in the react-native root folder:

yarn install
cd packages/rn-tester
bundle install
bundle exec pod install
open RNTesterPods.xcworkspace

and then build from Xcode.

If you need to run the old architecture, you can disable it by adding:

- (BOOL) newArchEnabled
{
  return NO;
}

in the AppDelegate.mm file

ArekChr · 2024-10-09T18:52:57Z

Hi @cipolleschi, the issue has been fixed, and the build runs successfully locally. Thanks!

cipolleschi · 2024-10-10T09:38:55Z

We were discussing internally that it would be better to have two separate feature flags, one for Android and one for iOS. Could I ask you to:

revert the changes you made on the preexisting feature flag, which was targeting only Android
add the iOS dual of that feature flag

The idea is to be able to control the two platforms separatedly.

Thank you so much for your help and patience

ArekChr · 2024-10-11T12:02:20Z

@cipolleschi I have an issue where the ReactNativeFeatureFlags.h header is not found when using dynamic frameworks. I’ve tried a few things but haven’t resolved the issue. Do you have any suggestions on how to fix or work around this?

ArekChr · 2024-10-22T08:17:25Z

Hi, @cipolleschi. The CI failed due to some issues unrelated to my changes. I’ve likely fixed the problem with dynamic frameworks, but two workflows seem missing. Could you please trigger those workflows again so we can proceed?

ArekChr · 2024-12-17T14:03:00Z

@cipolleschi Fixed

facebook-github-bot · 2024-12-17T14:23:09Z

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

facebook-github-bot · 2024-12-17T14:58:06Z

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

…calc-ios

facebook-github-bot · 2024-12-20T14:27:18Z

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

facebook-github-bot · 2024-12-30T14:46:44Z

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

LJNGDAHL · 2025-01-07T08:08:39Z

Any progress on this? 👀

facebook-github-bot · 2025-01-16T17:01:02Z

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

cipolleschi · 2025-01-16T17:03:29Z

Sorry if it is taking a bit. I'm trying to get an internal review on this.

ludvigbartholdsson · 2025-01-19T20:09:01Z

Awesome if we could get this PR merged (if it works and passes tests ofc) @cipolleschi do you think it's possible next week?

facebook-github-bot · 2025-01-20T10:36:48Z

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

ludvigbartholdsson · 2025-01-29T18:32:12Z

Hi @cipolleschi , do you have a new status on this?

cipolleschi · 2025-01-31T12:31:28Z

packages/react-native/Libraries/Text/Text/RCTTextView.mm

@@ -98,6 +99,42 @@ - (void)setTextStorage:(NSTextStorage *)textStorage
  [self setNeedsDisplay];
 }

+- (CGPoint)calculateDrawingPointWithTextStorage:(NSTextStorage *)textStorage


@NickGerleman

It would be best to keep the changes to the Fabric component instead of this one.

Could you point me to the specific place in the Fabric component where I should move this change?

Hi! I'm a developer following this thread. Does this suggestion mean that this fix will not be made for the old architecture? I've been following the original ticket for years, and such outcome would be quite disappointing 😕 My team is not considering upgrading to Fabric any time soon (due to many old arch dependencies, and also performance/bundle size/reliability concerns).

@ArekChr I believe that the suggestion here was to not touch the files in the old architecture. The PR already updates the New Architecture, so we are good.

@SimpleCreations Thanks for the feedback, I believe that in this case we can keep the fix here. As a general thought, though.. Yes, in the future the Old Architecture will not be maintained.

Right now, we already released 0.76 and 0.77 where the New Architecture is the default. We are already working on 0.78 and once that's out, 0.75 will go out of support. That is the last version where the Old Architecture is used by default.

Consider that new features are only developed for the New Architecture and many libraries will be only compatible with the New Architecture, moving forward (reanimated and react-native-vision-camera to mention two of them). So we strongly advise to start migrating to the New Architecture.

I'd be curious to know what is holding you back in more details. If you can replicate Performance and bundle issues in separate reproducers, we are more than happy to look into them as soon as possible.

@cipolleschi Thank you for reconsidering! I appreciate the decision to keep the fix very much.

I'll try to answer your question about what is holding my team back. The primary reason is we use quite a few SaaS solutions that provide their own React Native SDKs that are old arch only, and they have no timeline of adding new arch support. Also we have developed a few of our own React Native wrappers for other SaaS solutions, and they are shared between other apps in our company, which are also on old arch.

Performance and bundle size concerns come from me following this thread: #47490 where multiple people share negative experiences with new arch migration, but I'm not sure if this would affect us, because we haven't tried migrating yet (due to what I described above). So please disregard this part for now 🙂

I would like to also mention that the same fix for line height has been made for old arch on Android here: #46362, so it would make sense to include an old arch fix on iOS too.

cipolleschi · 2025-01-31T12:31:50Z

packages/react-native/Libraries/Text/Text/RCTTextView.mm

+    return contentFrame.origin;
+  }
+
+  UIFont *font = [textStorage attribute:NSFontAttributeName atIndex:0 effectiveRange:NULL];


@NickGerleman

Do you know if this is influenced by the full set of text attributes on the Text component? Like, if this is italicized, will it pick up that version of the font?

This code will return the correct font if the entire text is consistently styled (e.g., italic or bold). However, if the text contains different styles in different parts (e.g., a mix of italic and bold), this code will only reflect the style of the first character.

cipolleschi · 2025-01-31T12:32:08Z

packages/react-native/Libraries/Text/Text/RCTTextView.mm

+                                   contentFrame:(CGRect)contentFrame {
+  UIFont *font = [textStorage attribute:NSFontAttributeName atIndex:0 effectiveRange:NULL];
+  if (!font) {
+      font = [UIFont systemFontOfSize:14];


@NickGerleman

This seems sketch. E.g. what if there is a font multiplier applied?

packages/react-native/Libraries/Text/Text/RCTTextView.mm

cipolleschi · 2025-01-31T12:33:53Z

packages/react-native/React/Fabric/Mounting/ComponentViews/Text/RCTParagraphComponentView.mm

+  CGFloat lineHeight = font.lineHeight;
+
+  if (paragraphStyle && paragraphStyle.minimumLineHeight > 0) {
+      lineHeight = paragraphStyle.minimumLineHeight;


@NickGerleman

Why do we do this work when an explicit lineHeight is not set?

I’ve inverted the condition to make it clearer and ensure we exit early when minimumLineHeight isn’t set.

packages/react-native/React/Fabric/Mounting/ComponentViews/Text/RCTParagraphComponentView.mm

cipolleschi · 2025-01-31T12:35:17Z

I am sorry that this is taking so long, but this diff is very delicate to land as it touches the very core of text rendering and it can introduce a lot of regression. That's why it is take more time.
On top of that, there is further feedback that needs to be addressed and that i forwarded from internal colleagues.

cipolleschi · 2025-02-07T15:35:59Z

need a rebase due to the feature flags. 😢

ludvigbartholdsson · 2025-02-19T11:55:47Z

Hi, how is this going?

improve text line height calculation on ios

f998a9a

ArekChr mentioned this pull request Oct 8, 2024

Line height is distributed unevenly when lineHeight <= fontSize #29507

Open

ArekChr changed the title ~~fix: Improve text line height calculation to properly align text~~ fix: Improve text line height calculation to properly align text on iOS Oct 8, 2024

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 8, 2024

okwasniewski reviewed Oct 8, 2024

View reviewed changes

ArekChr force-pushed the improve-lineheight-calc-ios branch from 246c550 to f998a9a Compare October 9, 2024 07:48

ArekChr added 4 commits October 9, 2024 09:55

add comments

8168cf0

add safeguards to prevent out-of-bounds crash when accessing text att…

164bf0c

…ributes in calculateDrawingPoint.

Merge branch 'main' into improve-lineheight-calc-ios

f79a614

codegen feature flags

b044af9

cipolleschi approved these changes Oct 9, 2024

View reviewed changes

Fix namespace for ReactNativeFeatureFlags in RCTTextView

4844731

refactor: separate feature flat per platform

76faa56

fix: add React-featureflags pod for dynamic frameworks configuration

9e5ed70

ArekChr force-pushed the improve-lineheight-calc-ios branch from fa44b24 to 9e5ed70 Compare October 14, 2024 07:01

ArekChr and others added 5 commits October 14, 2024 09:16

Merge branch 'main' into improve-lineheight-calc-ios

09118be

Merge branch 'main' into improve-lineheight-calc-ios

193d804

Merge branch 'main' into improve-lineheight-calc-ios

a41f8c8

Merge branch 'main' into improve-lineheight-calc-ios

2ec676c

Merge branch 'main' into improve-lineheight-calc-ios

848e4b2

refactor: remove unused flag on android

81e560b

Merge remote-tracking branch 'upstream/main' into improve-lineheight-…

a52fb88

…calc-ios

ArekChr added 2 commits January 7, 2025 10:33

Merge branch 'main' into improve-lineheight-calc-ios

3a13ddf

Merge branch 'main' into improve-lineheight-calc-ios

42a65c1

ArekChr added 2 commits January 30, 2025 08:14

Merge branch 'main' into improve-lineheight-calc-ios

44d47fe

Merge branch 'main' into improve-lineheight-calc-ios

9293ecd

cipolleschi requested changes Jan 31, 2025

View reviewed changes

ArekChr added 4 commits February 3, 2025 09:31

Merge branch 'main' into improve-lineheight-calc-ios

f935bef

simplify offset calculation

eb3e471

Handle font multiplier for dynamic type in text storage fallback

074e047

Exit early if minimumLineHeight is not set

ec11dc6

ArekChr added 3 commits February 10, 2025 10:42

Merge branch 'main' into improve-lineheight-calc-ios

696c599

Merge branch 'main' into improve-lineheight-calc-ios

a763b38

Merge branch 'main' into improve-lineheight-calc-ios

0e2dbb6

ArekChr added 2 commits February 25, 2025 09:49

Merge branch 'main' into improve-lineheight-calc-ios

59c85f1

update feature flags

16226ba

fix: Improve text line height calculation to properly align text on iOS #46884

Are you sure you want to change the base?

fix: Improve text line height calculation to properly align text on iOS #46884

Conversation

ArekChr commented Oct 8, 2024 • edited Loading

Summary:

Changelog:

Test Plan:

facebook-github-bot commented Oct 8, 2024

Action Required

Process

facebook-github-bot commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrousavy commented Oct 8, 2024

cipolleschi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 9, 2024

cipolleschi commented Oct 9, 2024

ArekChr commented Oct 9, 2024

cipolleschi commented Oct 10, 2024

ArekChr commented Oct 11, 2024 • edited Loading

ArekChr commented Oct 22, 2024

ArekChr commented Dec 17, 2024

facebook-github-bot commented Dec 17, 2024

facebook-github-bot commented Dec 17, 2024

facebook-github-bot commented Dec 20, 2024

facebook-github-bot commented Dec 30, 2024

LJNGDAHL commented Jan 7, 2025

facebook-github-bot commented Jan 16, 2025

cipolleschi commented Jan 16, 2025

ludvigbartholdsson commented Jan 19, 2025

facebook-github-bot commented Jan 20, 2025

ludvigbartholdsson commented Jan 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

@ArekChr I believe that the suggestion here was to not touch the files in the old architecture. The PR already updates the New Architecture, so we are good.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipolleschi commented Jan 31, 2025

cipolleschi commented Feb 7, 2025

ludvigbartholdsson commented Feb 19, 2025

ArekChr commented Oct 8, 2024 •

edited

Loading

ArekChr commented Oct 11, 2024 •

edited

Loading