react-native
react-native copied to clipboard
facebook
fix: Improve text line height calculation to properly align text on iOS
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 |
|---|---|
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!
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@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
Hi @cipolleschi, the issue has been fixed, and the build runs successfully locally. Thanks!
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
@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?
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?
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
JS tests and the linter are failing. :( Could you have a look at those failures, please?
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Sorry if it is taking a bit. I'm trying to get an internal review on this.
Awesome if we could get this PR merged (if it works and passes tests ofc) @cipolleschi do you think it's possible next week?
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
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.