-
Notifications
You must be signed in to change notification settings - Fork 25.2k
fix: Improve text line height calculation to properly align text on iOS #46884
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
base: main
Are you sure you want to change the base?
Changes from 27 commits
f998a9a
8168cf0
164bf0c
f79a614
b044af9
4844731
76faa56
9e5ed70
09118be
193d804
a41f8c8
2ec676c
848e4b2
045ced9
c46a96f
d4e44de
b950279
3610212
ffc5503
7fd5214
4e35916
81e560b
a52fb88
3a13ddf
42a65c1
44d47fe
9293ecd
f935bef
eb3e471
074e047
ec11dc6
696c599
a763b38
0e2dbb6
59c85f1
16226ba
672ef86
17ee8c8
a8b5557
7bed2d9
b911e26
a822789
582e1be
ffaa3ae
93e547c
b8e9f96
cbb8efe
b361acb
8ec3967
a17189e
10fe311
be99178
54bd5d7
328ea06
3e94c38
c7407af
856cfaa
8a0de4a
be95918
8ddb782
adc2645
c86d767
d52dcc4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
|
|
||
| #import <React/RCTUtils.h> | ||
| #import <React/UIView+React.h> | ||
| #import <react/featureflags/ReactNativeFeatureFlags.h> | ||
|
|
||
| #import <React/RCTTextShadowView.h> | ||
|
|
||
|
|
@@ -98,6 +99,42 @@ - (void)setTextStorage:(NSTextStorage *)textStorage | |
| [self setNeedsDisplay]; | ||
| } | ||
|
|
||
| - (CGPoint)calculateDrawingPointWithTextStorage:(NSTextStorage *)textStorage | ||
| contentFrame:(CGRect)contentFrame { | ||
| if ([textStorage length] == 0) { | ||
| return contentFrame.origin; | ||
| } | ||
|
|
||
| UIFont *font = [textStorage attribute:NSFontAttributeName atIndex:0 effectiveRange:NULL]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if (!font) { | ||
| font = [UIFont systemFontOfSize:14]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Q] Maybe we can use https://developer.apple.com/documentation/uikit/uifont/1623395-systemfontsize?language=objc
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
||
| NSParagraphStyle *paragraphStyle = [textStorage attribute:NSParagraphStyleAttributeName atIndex:0 effectiveRange:NULL]; | ||
|
|
||
| CGFloat lineHeight = font.lineHeight; | ||
| if (paragraphStyle && paragraphStyle.minimumLineHeight > 0) { | ||
| lineHeight = paragraphStyle.minimumLineHeight; | ||
| } | ||
|
|
||
| CGFloat ascent = font.ascender; | ||
| CGFloat descent = fabs(font.descender); | ||
| CGFloat textHeight = ascent + descent; | ||
|
|
||
| CGFloat verticalOffset = 0; | ||
| // Adjust vertical offset to ensure text is vertically centered relative to the line height. | ||
| // Positive offset when text height exceeds line height, negative when line height exceeds text height. | ||
| if (textHeight > lineHeight) { | ||
| CGFloat difference = textHeight - lineHeight; | ||
| verticalOffset = difference / 2.0; | ||
| } else if (textHeight < lineHeight) { | ||
| CGFloat difference = lineHeight - textHeight; | ||
| verticalOffset = -(difference / 2.0); | ||
| } | ||
|
ArekChr marked this conversation as resolved.
Outdated
|
||
|
|
||
| return CGPointMake(contentFrame.origin.x, contentFrame.origin.y + verticalOffset); | ||
| } | ||
|
|
||
| - (void)drawRect:(CGRect)rect | ||
| { | ||
| [super drawRect:rect]; | ||
|
|
@@ -118,8 +155,15 @@ - (void)drawRect:(CGRect)rect | |
| #endif | ||
|
|
||
| NSRange glyphRange = [layoutManager glyphRangeForTextContainer:textContainer]; | ||
| [layoutManager drawBackgroundForGlyphRange:glyphRange atPoint:_contentFrame.origin]; | ||
| [layoutManager drawGlyphsForGlyphRange:glyphRange atPoint:_contentFrame.origin]; | ||
|
|
||
| if (facebook::react::ReactNativeFeatureFlags::enableLineHeightCenteringOnIOS()) { | ||
| CGPoint drawingPoint = [self calculateDrawingPointWithTextStorage:_textStorage contentFrame:_contentFrame]; | ||
| [layoutManager drawBackgroundForGlyphRange:glyphRange atPoint:drawingPoint]; | ||
| [layoutManager drawGlyphsForGlyphRange:glyphRange atPoint:drawingPoint]; | ||
| } else { | ||
| [layoutManager drawBackgroundForGlyphRange:glyphRange atPoint:_contentFrame.origin]; | ||
| [layoutManager drawGlyphsForGlyphRange:glyphRange atPoint:_contentFrame.origin]; | ||
| } | ||
|
|
||
| __block UIBezierPath *highlightPath = nil; | ||
| NSRange characterRange = [layoutManager characterRangeForGlyphRange:glyphRange actualGlyphRange:NULL]; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| #import "RCTParagraphComponentAccessibilityProvider.h" | ||
|
|
||
| #import <MobileCoreServices/UTCoreTypes.h> | ||
| #import <react/featureflags/ReactNativeFeatureFlags.h> | ||
| #import <react/renderer/components/text/ParagraphComponentDescriptor.h> | ||
| #import <react/renderer/components/text/ParagraphProps.h> | ||
| #import <react/renderer/components/text/ParagraphState.h> | ||
|
|
@@ -326,6 +327,40 @@ @implementation RCTParagraphTextView { | |
| CAShapeLayer *_highlightLayer; | ||
| } | ||
|
|
||
| - (CGRect)calculateCenteredFrameWithAttributedText:(NSAttributedString *)attributedText | ||
| frame:(CGRect)frame { | ||
| UIFont *font = [attributedText attribute:NSFontAttributeName atIndex:0 effectiveRange:NULL]; | ||
|
ArekChr marked this conversation as resolved.
|
||
| if (!font) { | ||
| font = [UIFont systemFontOfSize:14]; | ||
| } | ||
|
|
||
| NSParagraphStyle *paragraphStyle = [attributedText attribute:NSParagraphStyleAttributeName atIndex:0 effectiveRange:NULL]; | ||
| CGFloat lineHeight = font.lineHeight; | ||
|
|
||
| if (paragraphStyle && paragraphStyle.minimumLineHeight > 0) { | ||
| lineHeight = paragraphStyle.minimumLineHeight; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’ve inverted the condition to make it clearer and ensure we exit early when minimumLineHeight isn’t set. |
||
| } | ||
|
|
||
| CGFloat ascent = font.ascender; | ||
| CGFloat descent = fabs(font.descender); | ||
| CGFloat textHeight = ascent + descent; | ||
|
|
||
| CGFloat verticalOffset = 0; | ||
| // Adjust vertical offset to ensure text is vertically centered relative to the line height. | ||
| // Positive offset when text height exceeds line height, negative when line height exceeds text height. | ||
| if (textHeight > lineHeight) { | ||
| CGFloat difference = textHeight - lineHeight; | ||
| verticalOffset = difference / 2.0; | ||
| } else if (textHeight < lineHeight) { | ||
| CGFloat difference = lineHeight - textHeight; | ||
| verticalOffset = -(difference / 2.0); | ||
| } | ||
|
ArekChr marked this conversation as resolved.
Outdated
|
||
|
|
||
| frame.origin.y += verticalOffset; | ||
|
|
||
| return frame; | ||
| } | ||
|
|
||
| - (void)drawRect:(CGRect)rect | ||
| { | ||
| if (!_state) { | ||
|
|
@@ -343,6 +378,11 @@ - (void)drawRect:(CGRect)rect | |
|
|
||
| CGRect frame = RCTCGRectFromRect(_layoutMetrics.getContentFrame()); | ||
|
|
||
| if (ReactNativeFeatureFlags::enableLineHeightCenteringOnIOS()) { | ||
| NSAttributedString *attributedText = RCTNSAttributedStringFromAttributedString(_state->getData().attributedString); | ||
| frame = [self calculateCenteredFrameWithAttributedText:attributedText frame:frame]; | ||
| } | ||
|
|
||
| [nativeTextLayoutManager drawAttributedString:_state->getData().attributedString | ||
| paragraphAttributes:_paragraphAttributes | ||
| frame:frame | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NickGerleman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you point me to the specific place in the Fabric component where I should move this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.