Fix TextInput vertical alignment issue when using lineHeight prop on iOS without changing Text baseline (Fabric - new arch)

fabOnReact commented 1 month ago

Details

Sets the NSBaselineOffsetAttributeName of the attributedText based on the following formula:

CGFloat baseLineOffset = (maximumLineHeight - maximumFontLineHeight) / 2.0;

The text is vertically centered in the lineHeight of the TextInput (which corresponds to the height of the cursor). The solution was already added in facebook/react-native:

PR facebook/react-native#38359 fixed the issue on the Old architecture (Paper) in release 0.73.5.
commit https://github.com/facebook/react-native/commit/70cc27c901aeb447910e30ac3ceac85990d3c16d fixed the issue in the New architecture (Fabric).

Manual Tests

Expensify Composer: https://gist.github.com/fabOnReact/dfb141ef083f99acea4b7152294aaca9?permalink_comment_id=5061893#gistcomment-5061893
Expensify Composer (When moving to another line, the word and emoji are close to each other): pull/350#issuecomment-2119690594 and pull/350#issuecomment-2119701442
Markdown: https://gist.github.com/fabOnReact/dfb141ef083f99acea4b7152294aaca9?permalink_comment_id=5061895#gistcomment-5061895
Markdown - Arabic text getting cut: https://gist.github.com/fabOnReact/dfb141ef083f99acea4b7152294aaca9?permalink_comment_id=5061901#gistcomment-5061901
Previous tests: PR comments https://githuh.com/facebook/react-native/pull/37465#issuecomment-1551459879 and https://github.com/Expensify/App/issues/17767#issuecomment-1640032626

Linked PRs

PR https://github.com/facebook/react-native/pull/38359 landed. Fixed the issue on the Old Architecture (Paper).
PR https://github.com/facebook/react-native/pull/37465 was reverted.

github-actions[bot] commented 1 month ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

fabOnReact commented 1 month ago

I have read the CLA Document and I hereby sign the CLA

fabOnReact commented 1 month ago

Expensify Composer (When moving to another line, the word and emoji are close to each other):

The difference of baseline is 1.15 (barely noticeable), maximumLineHeight 20, maximumFontLineHeight 17.69

Before	After

Before	After

fabOnReact commented 1 month ago

Expensify Composer (increasing lineHeight to display more clearly differences in the screenshots):

The result is achieved after setting textInputCompose.lineHeight: 40.

baseLineOffset is 11.15
maximumLineHeight is 40
maximumFontLineHeight is 17.69

Before	After

Before	After

Before	After

fabOnReact commented 1 month ago

iOS - Chat - Cursor in the second and following lines touches the symbols of the previous line #15640

Fixes issue https://github.com/Expensify/App/issues/15640

Before	After

tomekzaw commented 1 month ago

Thanks @fabOnReact for this amazing PR!

tomekzaw commented 1 month ago

@fabOnReact We've reviewed the changes and we'd like to merge this PR but it's still marked as draft

fabOnReact commented 1 month ago

@tomekzaw Thanks a lot. I moved the PR to ready for review.

hungvu193 commented 1 month ago

@fabOnReact I think this PR introduced this regression. Do we have any plan to fix it or should we create an issue for that?

fabOnReact commented 1 month ago

@hungvu193 did you test it?

Remove that line of code from Expensify
You need to build Expensify and run it on Android
Does the bug disappear?
Because the bug is Android only, while this PR introduces changes only to iOS. My PR does not change Android code, so I don't believe it introduces issues on Android. Thanks a lot!

hungvu193 commented 1 month ago

@hungvu193 did you test it?

Remove that line of code from Expensify

You need to build Expensify and run it on Android

Does the bug disappear?

Because the bug is Android only, while this PR introduces changes only to iOS. Thanks a lot!

My apologize for this confusion. I think I mentioned wrong PR here :(

fabOnReact commented 1 month ago

@hungvu193 Thanks for the clarification. No problem about that. 🙏

Expensify / react-native-live-markdown