-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Fix TextInput vertical alignment issue when using lineHeight prop on iOS (Paper - old arch) #37465
Conversation
Related Expensify/App#17767 (comment) Expensify/App#17767 (comment) More info type | task -- | -- task | Debug with Layout Inspector and verify height of the container of the TextInput. task | Compare coordinates (anchor point, x, y) of the different views (UITextLayoutCanvasView, UITextLayoutFragmentView).Find parameters that causes view to shift down by 50% (change the parameter and verify regression fixed. SOLUTION: The _UITextLayoutFragmentView.frame has Y coordinates > 0. The Y coordinates are relative to the parent views (UITextLayoutCanvas and RCTUITextField). task | Modify UITextLayoutCanvasView, UITextLayoutFragmentView frame position (UITextLayoutFramePosition). task | Read about UITextLayoutFragmentView (search) task | Try to change more values in paragraphStyles. Remove maximumLineHeight and verify if it is causing the issue. The issue seems to be caused by paragraphStyle.maxLineHeight. feature | Copy implementation RCTBaseTextShadowView layoutSubviewsWitContext task | Read Text implementation of baseline (github comment, postprocessAttributedText, layoutSubviewsWithContext, search, enumerate_attribute) task | Implement logic in RCTBaseTextInputShadowView layoutSubviewsWithContext task | Read implementation of RCTLayout and RCTLayoutContext task | Adapt implementation and use textContainer, textContainerOrigin and other APIS to align UITextLayoutFragmentView to position 0,0 (TextKit) feature | Copy implementation RCTBaseTextShadowView uiManagerWillPeformMounting task | Review the implementation in the main branch. Verify RCTBaseTextShadowViewEmbeddedShadowViewAttributeName task | Log in the console the number descendants.count task | Read RCTBaseTextInputManager uiManagerWillPerformMounting implementation (source) review | Review yesterday comment on discord and pages document (gist). feature | Modify y coordinates of a Dummy implementation of UITextField using NSTextContainer on a HelloWorld App (twitter example, gist#comment). task | Add UITextField and test it task | Change the UITextField attributedText maximumLineHeight and debug with layout inspector task | Use NSLayoutManager and NSTextContainer to change _UITextLayoutFragmentView coordinates frame.y. SOLUTION: textField.contentVerticalAlignment = UIControlContentVerticalAlignmentTop; feature | Try to align the UITextField top and apply existing solution task | Verify textField contentVerticalAlignment fixes regression.Change the textRectForBounds bounds to position _UITextLayoutFragmentView task | Add previous solution to react-native type task task Debug with Layout Inspector and verify height of the container of the TextInput. task "Compare coordinates (anchor point, x, y) of the different views (UITextLayoutCanvasView, UITextLayoutFragmentView). Find parameters that causes view to shift down by 50% (change the parameter and verify regression fixed. SOLUTION: The _UITextLayoutFragmentView.frame has Y coordinates > 0. The Y coordinates are relative to the parent views (UITextLayoutCanvas and RCTUITextField)." task Modify UITextLayoutCanvasView, UITextLayoutFragmentView frame position ([UITextLayoutFramePosition](https://www.google.com/search?q=UITextLayoutFragment+frame+position)). task Read about UITextLayoutFragmentView ([search](https://www.google.com/search?q=UITextLayoutCanvasView+UITextLayoutFragmentView%C2%A0maximumLineHeight%C2%A0&sxsrf=APwXEdd1MPGNECEcJTqTeOmRIfyXE6o1vA:1684209236160&ei=VP5iZLGjCebYseMPnv6S-A8&ved=0ahUKEwixnf6p-Pj-AhVmbGwGHR6_BP8Q4dUDCA8&uact=5&oq=UITextLayoutCanvasView+UITextLayoutFragmentView%C2%A0maximumLineHeight%C2%A0&gs_lcp=Cgxnd3Mtd2l6LXNlcnAQAzIECCMQJzoKCAAQRxDWBBCwA0oECEEYAFD3mAFYjaUBYKyoAWgCcAF4AIAB6QWIAckJkgEJMC4xLjEuNi0xmAEAoAEBoAECyAEIwAEB&sclient=gws-wiz-serp)) task Try to change more values in paragraphStyles. Remove [maximumLineHeight](https://developer.apple.com/documentation/uikit/nsparagraphstyle/1533343-maximumlineheight?language=objc) and verify if it is causing the issue. The issue seems to be caused by paragraphStyle.maxLineHeight. feature Copy implementation RCTBaseTextShadowView layoutSubviewsWitContext task Read Text implementation of baseline ([github comment](Expensify/App#17767 (comment)), [postprocessAttributedText](https://github.com/fabriziobertoglio1987/react-native/blob/243a148da49e778cc916fa607ed5b3668acab06f/packages/react-native/Libraries/Text/Text/RCTTextShadowView.m#L163), [layoutSubviewsWithContext](https://github.com/fabriziobertoglio1987/react-native/blob/243a148da49e778cc916fa607ed5b3668acab06f/packages/react-native/Libraries/Text/Text/RCTTextShadowView.m#L263), [search](https://github.com/search?q=repo:facebook/react-native+layoutSubviewsWithContext+&type=code), [enumerate_attribute](https://developer.apple.com/documentation/foundation/nsattributedstring/1412461-enumerateattribute?language=objc)) task Implement logic in RCTBaseTextInputShadowView [layoutSubviewsWithContext](https://github.com/facebook/react-native/blob/2b932c3820cfabef1590f6fcf8b95f65d9c21eb8/packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputShadowView.m#L59) task Read implementation of RCTLayout and [RCTLayoutContext](https://github.com/facebook/react-native/blob/2b932c3820cfabef1590f6fcf8b95f65d9c21eb8/packages/react-native/React/Views/RCTLayout.h#L32-L36) task Adapt implementation and use textContainer, textContainerOrigin and other APIS to align UITextLayoutFragmentView to position 0,0 ([TextKit](https://developer.apple.com/documentation/uikit/textkit?language=objc)) feature Copy implementation RCTBaseTextShadowView uiManagerWillPeformMounting task Review the implementation in the main branch. Verify RCTBaseTextShadowViewEmbeddedShadowViewAttributeName task Log in the console the number descendants.count task Read RCTBaseTextInputManager uiManagerWillPerformMounting implementation ([source](https://github.com/facebook/react-native/blob/2b932c3820cfabef1590f6fcf8b95f65d9c21eb8/packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputViewManager.m#L152-L157)) review Review yesterday comment on [discord](https://discord.com/channels/514829729862516747/514832108678807553/1108011289462181968) and pages document ([gist](https://gist.github.com/fabriziobertoglio1987/ebbd6292dacf49927afc670b5b422c0f)). feature Modify y coordinates of a Dummy implementation of UITextField using NSTextContainer on a HelloWorld App ([twitter example](https://twitter.com/LinguaBrowse/status/1658591561784127488?s=20), [gist#comment](https://gist.github.com/fabriziobertoglio1987/ebbd6292dacf49927afc670b5b422c0f?permalink_comment_id=4570667#gistcomment-4570667)). task Add UITextField and test it task Change the UITextField attributedText [maximumLineHeight](https://github.com/facebook/react-native/blob/a3102175c5dcccce48406fde4443f0e54c3a1299/packages/react-native/Libraries/Text/RCTTextAttributes.m#L174) and debug with layout inspector task Use [NSLayoutManager](https://developer.apple.com/documentation/uikit/nslayoutmanager) and [NSTextContainer](https://developer.apple.com/documentation/uikit/nstextcontainer?language=objc) to change _UITextLayoutFragmentView coordinates frame.y. SOLUTION: textField.contentVerticalAlignment = UIControlContentVerticalAlignmentTop; feature Try to align the UITextField top and apply existing solution task "Verify textField contentVerticalAlignment fixes regression. Change the textRectForBounds bounds to position _UITextLayoutFragmentView" task Add previous solution to react-native
type | task -- | -- issue | Text cut in edit mode when using paddingTop. editingRectForBounds does not apply _textContainerInset. task | Verify textField contentVerticalAlignment fixes regression.Change the textRectForBounds bounds to position _UITextLayoutFragmentView task | Debug using layout inspector task | Reimplement logic in editingRectForBounds. Check cached diff.
before fix - increase/decrease padding top
to make the video more clear, we change the style paddingTop to both TextInput Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-05-11.at.11.25.24.mp4after fix - increase/decrease padding top
We change the style paddingTop only for the first TextInput Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-05-11.at.11.18.19.mp4before fix - increase/decrease border
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-05-11.at.10.53.38.mp4after fix - increase/decrease border
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-05-11.at.11.21.25.mp4 |
before the fix - text is not correctly aligned with different lineHeight, fontSize and height
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-05-12.at.11.56.58.mp4after fix - text is correctly aligned with different lineHeight, fontSize and height
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-05-12.at.11.53.20.mp4 |
Before - behaviour single line TextInput without lineHeight prop
2023-05-12.14-23-38.mp4Before - behaviour single line TextInput with lineHeight prop
2023-05-12.14-27-24.mp4Before - behaviour multi line TextInput without lineHeight prop
2023-05-12.14-30-27.mp4Before - behaviour multi line TextInput with a lineHeight prop
2023-05-12.14-32-44.mp4After - multi line TextInput with a lineHeight prop (first video)
2023-05-12.15-35-37.mp4After - multi line TextInput with a lineHeight prop (second video)
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-05-12.at.15.50.24.mp4After - multi line TextInput without a lineHeight prop
2023-05-12.14-59-35.mp4 |
Before/After - Testing multiline on Fabric New Architecture (not yet available on Fabric)
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-05-12.at.16.22.49.mp4Before/After - Testing singleline on Fabric New Architecture (not yet available on Fabric)
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-05-12.at.16.25.18.mp4 |
Base commit: 462c648 |
Thanks a lot 🙏 @NandoMB
|
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.
Sorry for the slow reply, I was away for +2 weeks on vacation. I will just just fix the linter issues, they are only related to code formatting. |
This pull request was successfully merged by @fabriziobertoglio1987 in 35a1648. When will my fix make it into a release? | Upcoming Releases |
Hello @fabriziobertoglio1987 we will have to revert this change and evaluate next steps. The change causes a slight difference on how text is rendered. I realise the change is small but it can lead to text's baselines being off, which we don't want. |
This pull request has been reverted by e075a3f. |
The diff would trigger linter error internal in Facebook. I compared the commit and the original diff and I was able to identify the linter issue. I fixed the linter issue to avoid again failures when importing it. Comment facebook#37465 (comment) Imported in Facebook facebook@35a1648#diff-08824b4f7ca9c31bd2a7aeb8b1180bd55b2b834d99667861e14e59082d06c137R179-R190 Original Diff facebook@c4996fc#diff-08824b4f7ca9c31bd2a7aeb8b1180bd55b2b834d99667861e14e59082d06c137R170-R177
Thanks, @sammy-SC, could you please share the source code for this example? |
Update 12/07There 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>
|
Thanks, @sammy-SC. I published PR #38359 to fix the regression from comment #37465 (comment). |
…iOS without changing Text baseline (Paper - old arch) (#38359) Summary: This PR fixes visual regression introduced with #37465 (comment) Adding paragraphStyle.maximumLineHeight to a iOS UITextField displays the text under the UITextField ([ios-screenshot-1][1], [ios-screenshot-2][2], [ios-screenshot-3][3]). The PR implements the logic from RCTTextShadowView [#postprocessAttributedText](https://github.com/facebook/react-native/blob/9ab27e8895d6934e72ebdc601d169578ab9628f1/packages/react-native/Libraries/Text/Text/RCTTextShadowView.m#L165-L167) in RCTBaseTextInpuShadowView [#uiManagerWillPerformMounting](https://github.com/facebook/react-native/blob/4c944540f732c6055d447ecaf37d5c8f3eec1bc4/packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputShadowView.m#L130-L192). [1]: https://user-images.githubusercontent.com/24992535/238834159-566f7eef-ea2d-4fd4-a519-099b0a12046c.png "ios-screenshot-1" [2]: https://user-images.githubusercontent.com/24992535/238834184-feb454a9-6504-4832-aec8-989f1d027861.png "ios-screenshot-2" [3]: https://user-images.githubusercontent.com/24992535/238834283-cf572f94-a641-4790-92bf-bbe43afb1443.png "ios-screenshot-3" [4]: https://github.com/Expensify/App/assets/24992535/06726b45-7e35-4003-9fcc-50c8d0dff0f6 [5]: https://github.com/Expensify/App/assets/24992535/d9745d29-8863-4170-bcc3-e78fa7e550d2 fixes #28012 fixes #33986 Related #35741 #31112 ## Changelog: [IOS] [FIXED] - Fix TextInput vertical alignment issue when using lineHeight prop on iOS without changing Text baseline (Paper - old arch) Pull Request resolved: #38359 Test Plan: Extensive test included in the PR comments #37465 (comment) and Expensify/App#17767 (comment) Reviewed By: cipolleschi Differential Revision: D52325261 Pulled By: dmytrorykun fbshipit-source-id: d072a598bfaafbbffc41005b1fda1795cf3d8ab9
…iOS without changing Text baseline (Paper - old arch) (#38359) Summary: This PR fixes visual regression introduced with #37465 (comment) Adding paragraphStyle.maximumLineHeight to a iOS UITextField displays the text under the UITextField ([ios-screenshot-1][1], [ios-screenshot-2][2], [ios-screenshot-3][3]). The PR implements the logic from RCTTextShadowView [#postprocessAttributedText](https://github.com/facebook/react-native/blob/9ab27e8895d6934e72ebdc601d169578ab9628f1/packages/react-native/Libraries/Text/Text/RCTTextShadowView.m#L165-L167) in RCTBaseTextInpuShadowView [#uiManagerWillPerformMounting](https://github.com/facebook/react-native/blob/4c944540f732c6055d447ecaf37d5c8f3eec1bc4/packages/react-native/Libraries/Text/TextInput/RCTBaseTextInputShadowView.m#L130-L192). [1]: https://user-images.githubusercontent.com/24992535/238834159-566f7eef-ea2d-4fd4-a519-099b0a12046c.png "ios-screenshot-1" [2]: https://user-images.githubusercontent.com/24992535/238834184-feb454a9-6504-4832-aec8-989f1d027861.png "ios-screenshot-2" [3]: https://user-images.githubusercontent.com/24992535/238834283-cf572f94-a641-4790-92bf-bbe43afb1443.png "ios-screenshot-3" [4]: https://github.com/Expensify/App/assets/24992535/06726b45-7e35-4003-9fcc-50c8d0dff0f6 [5]: https://github.com/Expensify/App/assets/24992535/d9745d29-8863-4170-bcc3-e78fa7e550d2 fixes #28012 fixes #33986 Related #35741 #31112 ## Changelog: [IOS] [FIXED] - Fix TextInput vertical alignment issue when using lineHeight prop on iOS without changing Text baseline (Paper - old arch) Pull Request resolved: #38359 Test Plan: Extensive test included in the PR comments #37465 (comment) and Expensify/App#17767 (comment) Reviewed By: cipolleschi Differential Revision: D52325261 Pulled By: dmytrorykun fbshipit-source-id: d072a598bfaafbbffc41005b1fda1795cf3d8ab9
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 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:
fixes #28012 fixes #33986
Related #35741 #31112
Changelog:
[IOS] [FIXED] - Fix TextInput vertical alignment issue when using lineHeight prop on iOS (Paper - old arch)
Test Plan:
Extensive test included in the PR comments #37465 (comment)