Skip to content
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

[HOLD facebook/react-native#37465] Fix TextInput vertical alignment issue when using lineHeight prop on iOS (Paper - old arch) #57

Conversation

fabOnReact
Copy link

@fabOnReact fabOnReact commented May 23, 2023

Upstream PR Link

Upstream PR facebook#37465
fixes Expensify/App#17767

Summary:

Adding paragraphStyle.maximumLineHeight to a iOS UITextField displays the text under the UITextField (ios-screenshot-1, ios-screenshot-2, ios-screenshot-3). The issue reproduces on Storyboard App (iOS) using UITextField and paragraphStyle.maximumLineHeight. It is not caused by react-native.

The issue is caused by a private class _UITextLayoutFragmentView (a CALayer children of UITextField), which assumes the wrong position when setting the lineHeight. _UITextLayoutFragmentView frame's y coordinates are set to 30, instead of 0 (react-native-screenshot-1, react-native-screenshot-2)

  • The _UITextLayoutFragmentView layer does not correctly position
  • The issue is caused by adding paragraphStyle.maximumLineHeight to UITextField.attributedText
  • The parent UITextField bounds do not correctly position child _UITextLayoutFragmentView.
    The issue causes the below text Sdfsd to display under the TextInput.

I was able to fix the issue and correctly align the private layout view _UITextLayoutFragmentView using the public API RCTUITextField textRectForBound, which allows modifying the UITextField frame and inset.
The solution consists in the following steps, applied only to UITextField with lineHeight prop:

  1. set _UITextLayoutFragmentView to vertically align to the top. Required to correctly align _UITextLayoutFragmentView.
  2. apply custom bounds with RCTUITextField textRectForBound to align _UITextLayoutFragmentView with the correct y coordinates and height
  3. Adjust text baseline to be center aligned

fixes facebook#28012

Changelog:

[IOS] [FIXED] - Fix TextInput vertical alignment issue when using lineHeight prop on iOS (Paper - old arch)

Test Plan:

Extensive tests included in the PR comments facebook#37465 (comment) and below

@fabOnReact
Copy link
Author

fabOnReact commented May 23, 2023

After the fix - Testing RNTester in Expensify/react-native

2023-05-23.09-25-28.mp4

@fabOnReact
Copy link
Author

fabOnReact commented May 23, 2023

1st Test - Verify that TextInput vertical alignment is fixed and text is not cut

Before After

2nd Test - No regression detected

Before After
Before.Applying.the.fix.mp4
After.applying.the.fix.mp4

Copy link

@cead22 cead22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, but I'd love to get a review from someone that's more familiar with this code and how it works

paragraphStyle.minimumLineHeight = lineHeight;
paragraphStyle.maximumLineHeight = lineHeight;
isParagraphStyleUsed = YES;
// text with lineHeight lower then font.lineHeight does not correctly vertically align
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you capitalize all comments and add a line break before them (except for the one after the if which is in a new level of indentation)?

Suggested change
// text with lineHeight lower then font.lineHeight does not correctly vertically align
// Text with lineHeight lower then font.lineHeight does not correctly vertically align

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cead22. I made the changes with commit 12b51dd. I remain available for further improvements.

Comment on lines 170 to 176
if (self.fragmentViewContainerBounds.size.height > 0) {
// Apply custom bounds to fix iOS UITextField issue with lineHeight.
// Sets the correct y coordinates for _UITextLayoutFragmentView.
return UIEdgeInsetsInsetRect([super textRectForBounds:self.fragmentViewContainerBounds], borderAndPaddingInsets);
} else {
return UIEdgeInsetsInsetRect([super textRectForBounds:bounds], borderAndPaddingInsets);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (self.fragmentViewContainerBounds.size.height > 0) {
// Apply custom bounds to fix iOS UITextField issue with lineHeight.
// Sets the correct y coordinates for _UITextLayoutFragmentView.
return UIEdgeInsetsInsetRect([super textRectForBounds:self.fragmentViewContainerBounds], borderAndPaddingInsets);
} else {
return UIEdgeInsetsInsetRect([super textRectForBounds:bounds], borderAndPaddingInsets);
}
// Apply custom bounds to fix iOS UITextField issue with lineHeight.
// Sets the correct y coordinates for _UITextLayoutFragmentView.
return UIEdgeInsetsInsetRect([super textRectForBounds:self.fragmentViewContainerBounds.size.height > 0 ? self.fragmentViewContainerBounds : bounds], borderAndPaddingInsets);

Can’t be simplified ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, @fedirjh. I applied the changes with commits 613bf0a, a5ac3c3 and tested the functionality again

CLICK TO OPEN TESTS RESULTS

CLICK TO OPEN TESTS RESULTS

@flodnv flodnv mentioned this pull request May 30, 2023
j-piasecki
j-piasecki previously approved these changes May 31, 2023
Copy link

@j-piasecki j-piasecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cead22
cead22 previously approved these changes May 31, 2023
Copy link

@cead22 cead22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @fedirjh can you re-review when you get a chance please?

Comment on lines 169 to 170
UIEdgeInsets borderAndPaddingInsets = UIEdgeInsetsMake(_textContainerInset.top, leftPadding, _textContainerInset.bottom, rightPadding);
// The fragmentViewContainerBounds set the correct y coordinates for
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, so not a blocker

Suggested change
UIEdgeInsets borderAndPaddingInsets = UIEdgeInsetsMake(_textContainerInset.top, leftPadding, _textContainerInset.bottom, rightPadding);
// The fragmentViewContainerBounds set the correct y coordinates for
UIEdgeInsets borderAndPaddingInsets = UIEdgeInsetsMake(_textContainerInset.top, leftPadding, _textContainerInset.bottom, rightPadding);
// The fragmentViewContainerBounds set the correct y coordinates for

Copy link
Author

@fabOnReact fabOnReact Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cead22.

Adding a line-break before the comment

I believe you refer to

Can you capitalize all comments and add a line break before them (except for the one after the if which is in a new level of indentation)?

I added a line break before the comment with commit 3d12d11.

Github Website Issue

There was an issue with the GitHub (website), which would not update the PR Diff and commits. My remote branch fabriziobertoglio1987/text-input-vertical-alignment-expensify-2 included changes not available on Github Website PR.

CLICK TO OPEN VIDEO

2023-06-01.14-27-51.mp4

For this reason, I had to temporarily change the base branch.

Related Discussions: https://github.com/orgs/community/discussions/16351 isaacs/github#750 (comment)

I tested and reviewed the branch diff locally (git checkout text-input-vertical-alignment-expensify-2 && g diff ExpensifyRC1-0.72.0-alpha.0..), and the branch has no issues.

CLICK TO OPEN TESTS RESULTS

Screenshot 2023-06-01 at 2 47 34 PM

Sorry. I remain available. Thanks

fedirjh
fedirjh previously approved these changes Jun 1, 2023
Copy link

@fedirjh fedirjh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tests well

@cead22 which branch should we target ?

@fabOnReact fabOnReact marked this pull request as draft June 1, 2023 06:09
@fabOnReact fabOnReact changed the base branch from ExpensifyRC1-0.72.0-alpha.0 to main June 1, 2023 06:09
@fabOnReact fabOnReact dismissed stale reviews from fedirjh, cead22, and j-piasecki June 1, 2023 06:09

The base branch was changed.

@fabOnReact fabOnReact changed the base branch from main to ExpensifyRC1-0.72.0-alpha.0 June 1, 2023 06:09
@github-actions
Copy link

github-actions bot commented Jun 1, 2023

Fails
🚫

❗ Base Branch - The base branch for this PR is something other than main or a -stable branch. Are you sure you want to target something other than the main branch?

Generated by 🚫 dangerJS against 3d12d11

@fabOnReact fabOnReact marked this pull request as ready for review June 1, 2023 06:19
@fabOnReact fabOnReact marked this pull request as draft June 1, 2023 06:19
@fabOnReact fabOnReact marked this pull request as ready for review June 1, 2023 07:01
@cead22
Copy link

cead22 commented Jun 1, 2023

Out of curiosity, why not merge into main?

@fedirjh
Copy link

fedirjh commented Jun 1, 2023

It looks like ExpensifyRC1-0.72.0-alpha.0 will be released. We have other PRs are targeting the same branch, check this comment #58 (comment)

@fabOnReact
Copy link
Author

fabOnReact commented Jun 2, 2023

The main branch facebook/react-native changes every day. The facebook/react-native main branch sometimes has issues, for this reason, it is easier to use the release branch.
The release branches are tested from the community (for example see reactwg/react-native-releases#54 for 0.72).
The Expensify team is working to upgrade react-native to RC1-0.72.0-alpha.0 see discussion Expensify/App#18507

@cead22 cead22 changed the title Fix TextInput vertical alignment issue when using lineHeight prop on iOS (Paper - old arch) [HOLD facebok/react-native#37465] Fix TextInput vertical alignment issue when using lineHeight prop on iOS (Paper - old arch) Jun 5, 2023
@cead22
Copy link

cead22 commented Jun 5, 2023

Holding until the FB PR is approved and merged

@cead22
Copy link

cead22 commented Jun 20, 2023

@fabriziobertoglio1987 any idea if/when the upstream PR will be merged?

@fabOnReact
Copy link
Author

fabOnReact commented Jun 20, 2023

Thanks @cead22. The PR will be merged. I reached out via Discord to receive feedback on the PR.
I will work until the PR is merged. I will keep you updated about the progress.

@fabOnReact
Copy link
Author

Thanks @cead22

This is the last update from Samuel Susla facebook#37465 (comment)

Sorry for the slow reply, I was away for +2 weeks on vacation. I will just fix the linter issues, they are only related to code formatting.

The was an issue with the internal facebook code linter (not open source). The PR will be merged soon. I remain available.

@cead22
Copy link

cead22 commented Jun 21, 2023

Awesome, thanks for the update!

@fedirjh
Copy link

fedirjh commented Jul 5, 2023

cc @cead22 @fabriziobertoglio1987 The upstream PR was merged today 🎉 🎉

@cead22
Copy link

cead22 commented Jul 7, 2023

@fedirjh @j-piasecki can you re-review this please? I think we're ready to remove the hold, but to confirm, is this PR still targeting the right branch?

@fabOnReact
Copy link
Author

fabOnReact commented Jul 7, 2023

This are the information I found:

  • Expensify/react-native: Expensify is updating react-native to 0.72 Update App to React Native 0.72 version App#18507 (comment)
  • Expensify/react-native: Expensify is merging a list of selected PRs in their Expensify/react-native fork to be included in the 0.72 expensify/react-native release (see this discussion). We may ask to cherry pick this PR in that discussion.
  • facebook/react-native: Cherry pick request for this commit could be done in the next release 0.73 Road to 0.73.0 reactwg/react-native-releases#64. Cherry picks in 0.72.2 are for regressions introduced with 0.72.
  • Expensify/react-native: React Native and Expensify did not yet prepare 0.73 branch. We can not target main, as Expensify/react-native is a fork of facebook/react-native.

@j-piasecki
Copy link

@cead22 It looks like all commits since the last review are cosmetic or done simply to trigger CI, so should be good 🚀

@fabOnReact
Copy link
Author

Update 07/07

@fabOnReact
Copy link
Author

Update 08/07

@fabOnReact
Copy link
Author

fabOnReact commented Jul 11, 2023

Update 11/07

  • Further reviewed the solutions from PRs #38258 and 37465
  • Testing different edge cases
  • Investigating the root cause of the regression and evaluating an alternative implementation.

@fabOnReact
Copy link
Author

fabOnReact commented Jul 12, 2023

Update 12/07

There is a difference in the baseline computation between RCTTextShadowView postprocessAttributedText and RCTTextAttributes effectiveParagraphStyle.

Given the following example:

<Text style={{lineHeight: 500, fontSize: 15, backgroundColor: 'red'}}>
  font size 15
  <Text style={{fontSize: 50, lineHeight: 500}}>font size 50</Text>
</Text>
  • The RCTTextShadowView implementation calculates the same baseline for the entire Text based on the highest lineHeight/fontSize
  • The RCTTextAttributes implementation calculates different baseline for each Text span, based on the Text span lineHeight and fontSize

@cead22 cead22 changed the title [HOLD facebok/react-native#37465] Fix TextInput vertical alignment issue when using lineHeight prop on iOS (Paper - old arch) [HOLD facebook/react-native#37465] Fix TextInput vertical alignment issue when using lineHeight prop on iOS (Paper - old arch) May 3, 2024
@cead22
Copy link

cead22 commented Jun 3, 2024

@fabOnReact any update on this one?

@fedirjh
Copy link

fedirjh commented Jun 3, 2024

@cead22 I think we should close this PR. This fork is not used anymore in newDot.

@fabOnReact
Copy link
Author

The upstream PR was merged. I'm closing this PR. Thanks.

@fabOnReact fabOnReact closed this Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants