Adjust iOS line height calculation

[adrianha]

Summary:

This diff will adjust line height calculation for Text and TextInput on iOS which is needed to fix cropped text in some fonts that have higher accent, e.g. Vietnam.

Line height calculation will use fontSize as the anchor to define the text offset.

On Android, it seems fine, text not cropped, attached on the Test Plan section.

Changelog:

Test Plan:

It was tested using Be Vietnam Pro font to render CHỖ text. Before the adjustment, the text will be cropped as it needs higher line height.

Before

After

Android

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	19,707,581	-656,940
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	22,912,790	-656,841
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 8b53d41
Branch: main

[adrianha]

hello @cipolleschi , could you help to review the adjustment?

[cipolleschi]

Could you explain why we were using 2.0 before and now we are using 1.5?

[adrianha]

Honestly, its quite a "magic" number that can give big enough space to cater text with accent 🤞
This might be impacting normal text to have bigger white space than before, depends on the font's pointSize and lineHeight comparison

[cipolleschi]

The code looks good. However, it is always a bit problematic to work with these changes as text is a delicate beast and we could introduce regressions and test failures. I'll try to import it, but to set expectations, we might not be able to merge this.

[facebook-github-bot]

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

[cipolleschi]

Hi! A colleague pointed out that this PR only address the Old Architecture.
Could you double check:

1. You have the same issue in the New Architecture?
2. If yes, can you update the PR to provide a fix for the New Architecture as well?

Thank you so much!

[adrianha]

hi @cipolleschi! I haven't tried it on New Arch yet, mostly working with Old Arch. I will check if this issue also happens on the New Arch.

[adrianha]

hi @cipolleschi, just checked that it also happens in the New Arch, already updated the changes to handle Text on New Arch

[facebook-github-bot]

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

[floydkim]

@adrianha Hi, thank you for fixing this issue 🙏
Could you please check if the issue still occurs when emojis are used instead of Vietnamese text Ỗ?

I'm also using a custom Korean font Pretendard in react-native 0.73.8 (old architecture), I am experiencing an issue where the baseline of the text rises when emojis are included in the text.

[cipolleschi]

I was discussing this fix with @NickGerleman internally and we realized that this fix breaks several tests internally and it breaks vertical centering of Text. I really understand having some fonts cut off is an issue, but we have to look for alternative solutions, different from this one.

Unfortunately I don't have good suggestions for where to look, at the moment.

[adrianha]

@adrianha Hi, thank you for fixing this issue 🙏

Could you please check if the issue still occurs when emojis are used instead of Vietnamese text Ỗ?

I'm also using a custom Korean font Pretendard in react-native 0.73.8 (old architecture), I am experiencing an issue where the baseline of the text rises when emojis are included in the text.

hi @floydkim, I'm not able to check it atm. Anyway, you can try to apply the changes directly on node_modules and rebuild the iOS app, it should pick up the update. Let me know if this works for your case

[adrianha]

I was discussing this fix with @NickGerleman internally and we realized that this fix breaks several tests internally and it breaks vertical centering of Text. I really understand having some fonts cut off is an issue, but we have to look for alternative solutions, different from this one.

Unfortunately I don't have good suggestions for where to look, at the moment.

hi @cipolleschi, totally understandable, this issue is quite tricky. Could you add more details regarding vertical centering of Text issue? Do you think we also need to adjust that mechanism as well?

[floydkim]

@adrianha

hi @floydkim, I'm not able to check it atm. Anyway, you can try to apply the changes directly on node_modules and rebuild the iOS app, it should pick up the update. Let me know if this works for your case

I tried it. This diff lowers the baseline of all text regardless of whether a custom font is applied.
Additionally, for small font size like in my case, the expected effect is barely noticeable.
😢

• [ lightblue : plain text / orange : text with Emoji / lime : text with Ỗ ]
• fonts: Pretendard, NotoSansKR, System font

Before	After