[iOS] reduce cursor size for multiline(TextInput) #36484

soumyajit4419 · 2023-03-15T16:47:36Z

Summary

Currently in multiline input the cursor touches the previous line.
So this reduces its height sets its position so that I does not touch previous line.
This PR will also fix the issue #28012 (Problem with TextInput lineHeight on iOS)

Changelog

[IOS] [ADDED] - Fixed cursor height on multiline text input

Test Plan

UI Before the change

UI After the change

facebook-github-bot · 2023-03-15T16:47:42Z

Hi @soumyajit4419!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

analysis-bot · 2023-03-15T17:11:42Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,519,193	+0
android	hermes	armeabi-v7a	7,834,776	+0
android	hermes	x86	8,999,081	+0
android	hermes	x86_64	8,854,121	+0
android	jsc	arm64-v8a	9,141,838	+0
android	jsc	armeabi-v7a	8,333,472	+0
android	jsc	x86	9,196,418	+0
android	jsc	x86_64	9,454,245	+0

Base commit: 221aacd
Branch: main

facebook-github-bot · 2023-03-15T17:12:06Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

facebook-github-bot · 2023-03-15T17:12:11Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

facebook-github-bot · 2023-03-16T11:09:34Z

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

javache · 2023-03-16T11:12:44Z

The height manipulation and Y offset seem like arbitrary numbers. Is there any source for why these numbers are correct/the most appropriates solution here?

javache

Caret sizing should be based on the font metrics, 0.75x or offsetting by 4 is not safe for all fonts.

javache · 2023-03-16T11:44:55Z

Libraries/Text/TextInput/Multiline/RCTUITextView.m

-  return [super caretRectForPosition:position];
+   CGRect originalRect = [super caretRectForPosition:position];
+   originalRect.size.height *= 0.75;
+   return CGRectMake(originalRect.origin.x, originalRect.origin.y + 4 , originalRect.size.width, originalRect.size.height);


This assumes that originalRect's height < 4, so we can offset it by this much, which is not safe.

Okay .
Got it.

soumyajit4419 · 2023-03-19T18:04:50Z

@javache I have updated my PR based on the comments
Can you say if this approach is correct?

packages/react-native/Libraries/Text/TextInput/Multiline/RCTUITextView.m

…TextView.m Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

facebook-github-bot · 2023-03-21T14:41:40Z

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

situchan · 2023-03-22T10:51:00Z

This PR shouldn't be merged at all. It causes bigger issue.

It doesn't completely fix cursor issue on iOS multiline TextInput.
After reducing cursor height, it aligns to top, while text aligns to bottom:
Also, selected range cursor doesn't match.

Before:

before.mp4

After:

after.mp4

The ideal solution should be to align center both cursor and text, same as Text component.

facebook-github-bot · 2023-03-22T12:37:08Z

@javache merged this pull request in e188585.

situchan · 2023-03-22T12:53:47Z

@javache can you please check my concern here?

javache · 2023-03-22T15:37:25Z

@situchan Thanks for the report, can you share a code-sample for it?

I'll revert this change for now. @soumyajit4419: can you revisit this PR and include @situchan's test case?

facebook-github-bot · 2023-03-22T15:44:56Z

This pull request has been reverted by 8313f23.

situchan · 2023-03-22T15:47:00Z

@javache this code should be enough for test. Just return in root App.js

    <View style={{flex: 1, paddingTop: 200}}>
        <View style={{marginHorizontal: 50}}>
            <Text>Text</Text>
            <Text style={{
                height: 110, borderWidth: 1, lineHeight: 50, fontSize: 16,
            }}
            >
                sdj fdsjflj slafjal ksdj afklakjfkajdskfj; asjfjs;dfs klfjalsjf j;ksd fkjalkj sfjlks j
            </Text>
        </View>
        <View style={{marginHorizontal: 50, marginTop: 50}}>
            <Text>TextInput</Text>
            <TextInput
                multiline
                placeholder="sdj fdsjflj slafjal ksdj afklakjfkajdskfj; asjfjs;dfs klfjalsjf j;ksd fkjalkj sfjlks j"
                style={{
                    height: 110, borderWidth: 1, lineHeight: 50, fontSize: 16,
                }}
            />
        </View>
    </View>

soumyajit4419 · 2023-03-22T18:38:20Z

@javache
I have created a PR fixing the issue of caret position with different lineHeight.
PR

@situchan I have tested it for different line height and the caret remains in the center.

Summary: Currently in multiline input the cursor touches the previous line. So this reduces its height sets its position so that I does not touch previous line. This PR will also fix the issue #28012 (Problem with TextInput lineHeight on iOS) This RP will fix the issue caused in [PR](#36484) Changelog: [iOS][Added] - Fixed cursor height on multiline text input Pull Request resolved: #36586 Test Plan: Tested for different cursor height https://user-images.githubusercontent.com/46092576/227004355-3886a0b5-7cdb-4fdc-a16b-3c4abb729737.mov https://user-images.githubusercontent.com/46092576/227004361-48099f81-9f52-460d-8ae8-d0ddb09dc47d.mov Reviewed By: javache Differential Revision: D44307457 Pulled By: genkikondo fbshipit-source-id: afeea5605ed8557cdeec1e62324c85665ce367d6

situchan · 2023-03-24T03:42:14Z

@soumyajit4419 do you think the issue was fixed completely?

Still has issues:

suggestion highlight rectangle is still aligned to the top
selection rectangle still takes full height
text and caret should be center aligned like Text view, not bottom

bug.mp4

Summary: Currently in multiline input the cursor touches the previous line. So this reduces its height sets its position so that I does not touch previous line. This PR will also fix the issue facebook#28012 (Problem with TextInput lineHeight on iOS) ## Changelog [IOS] [ADDED] - Fixed cursor height on multiline text input  Pull Request resolved: facebook#36484 Test Plan: UI Before the change <img width="379" alt="Screenshot 2023-03-15 at 10 16 07 PM" src="https://user-images.githubusercontent.com/46092576/225380938-23b9b8a4-4b8a-45e1-bbf1-4af8ac8d9183.png"> UI After the change <img width="509" alt="Screenshot 2023-03-14 at 1 57 27 AM" src="https://user-images.githubusercontent.com/46092576/225380930-77ca853c-fae5-4bfa-82cf-03b2e22bf8da.png"> Reviewed By: dmytrorykun Differential Revision: D44130814 Pulled By: javache fbshipit-source-id: 09d53ecee6812e9a875dc5364bd91b76cc2bc1f5

Summary: Currently in multiline input the cursor touches the previous line. So this reduces its height sets its position so that I does not touch previous line. This PR will also fix the issue facebook#28012 (Problem with TextInput lineHeight on iOS) This RP will fix the issue caused in [PR](facebook#36484) Changelog: [iOS][Added] - Fixed cursor height on multiline text input Pull Request resolved: facebook#36586 Test Plan: Tested for different cursor height https://user-images.githubusercontent.com/46092576/227004355-3886a0b5-7cdb-4fdc-a16b-3c4abb729737.mov https://user-images.githubusercontent.com/46092576/227004361-48099f81-9f52-460d-8ae8-d0ddb09dc47d.mov Reviewed By: javache Differential Revision: D44307457 Pulled By: genkikondo fbshipit-source-id: afeea5605ed8557cdeec1e62324c85665ce367d6

Summary: Currently in multiline input the cursor touches the previous line. So this reduces its height sets its position so that I does not touch previous line. This PR will also fix the issue facebook#28012 (Problem with TextInput lineHeight on iOS) ## Changelog [IOS] [ADDED] - Fixed cursor height on multiline text input  Pull Request resolved: facebook#36484 Test Plan: UI Before the change <img width="379" alt="Screenshot 2023-03-15 at 10 16 07 PM" src="https://user-images.githubusercontent.com/46092576/225380938-23b9b8a4-4b8a-45e1-bbf1-4af8ac8d9183.png"> UI After the change <img width="509" alt="Screenshot 2023-03-14 at 1 57 27 AM" src="https://user-images.githubusercontent.com/46092576/225380930-77ca853c-fae5-4bfa-82cf-03b2e22bf8da.png"> Reviewed By: dmytrorykun Differential Revision: D44130814 Pulled By: javache fbshipit-source-id: 09d53ecee6812e9a875dc5364bd91b76cc2bc1f5

Summary: Currently in multiline input the cursor touches the previous line. So this reduces its height sets its position so that I does not touch previous line. This PR will also fix the issue facebook#28012 (Problem with TextInput lineHeight on iOS) This RP will fix the issue caused in [PR](facebook#36484) Changelog: [iOS][Added] - Fixed cursor height on multiline text input Pull Request resolved: facebook#36586 Test Plan: Tested for different cursor height https://user-images.githubusercontent.com/46092576/227004355-3886a0b5-7cdb-4fdc-a16b-3c4abb729737.mov https://user-images.githubusercontent.com/46092576/227004361-48099f81-9f52-460d-8ae8-d0ddb09dc47d.mov Reviewed By: javache Differential Revision: D44307457 Pulled By: genkikondo fbshipit-source-id: afeea5605ed8557cdeec1e62324c85665ce367d6

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 15, 2023

javache reviewed Mar 16, 2023

View reviewed changes

soumyajit4419 added 2 commits March 20, 2023 00:07

[iOS] reduce cursor size for multiline(TextInput)

571d71b

change in height based on the font

171ada4

javache reviewed Mar 20, 2023

View reviewed changes

packages/react-native/Libraries/Text/TextInput/Multiline/RCTUITextView.m Outdated Show resolved Hide resolved

soumyajit4419 and others added 4 commits March 20, 2023 20:40

modified rect origin position

8c5693b

Update packages/react-native/Libraries/Text/TextInput/Multiline/RCTUI…

030c45d

…TextView.m Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

Merge branch 'facebook:main' into ios-cursor-height

0ee95e1

fixed return

56976be

javache approved these changes Mar 21, 2023

View reviewed changes

soumyajit4419 closed this by deleting the head repository Mar 21, 2023

soumyajit4419 reopened this Mar 21, 2023

soumyajit4419 mentioned this pull request Mar 21, 2023

[iOS] fix: reduce cursor size for multiline(TextInput) Expensify/react-native#48

Closed

facebook-github-bot closed this in e188585 Mar 22, 2023

facebook-github-bot added the Merged This PR has been merged. label Mar 22, 2023

facebook-github-bot added the Reverted label Mar 22, 2023

soumyajit4419 mentioned this pull request Mar 22, 2023

fix: make cursor center for different line height #36586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS] reduce cursor size for multiline(TextInput) #36484

[iOS] reduce cursor size for multiline(TextInput) #36484

soumyajit4419 commented Mar 15, 2023

facebook-github-bot commented Mar 15, 2023

analysis-bot commented Mar 15, 2023 •

edited

Loading

facebook-github-bot commented Mar 15, 2023

facebook-github-bot commented Mar 15, 2023

facebook-github-bot commented Mar 16, 2023

javache commented Mar 16, 2023

javache left a comment

javache Mar 16, 2023

soumyajit4419 Mar 16, 2023

soumyajit4419 commented Mar 19, 2023 •

edited

Loading

facebook-github-bot commented Mar 21, 2023

situchan commented Mar 22, 2023

facebook-github-bot commented Mar 22, 2023

situchan commented Mar 22, 2023

javache commented Mar 22, 2023

facebook-github-bot commented Mar 22, 2023

situchan commented Mar 22, 2023

soumyajit4419 commented Mar 22, 2023 •

edited

Loading

situchan commented Mar 24, 2023 •

edited

Loading

[iOS] reduce cursor size for multiline(TextInput) #36484

[iOS] reduce cursor size for multiline(TextInput) #36484

Conversation

soumyajit4419 commented Mar 15, 2023

Summary

Changelog

Test Plan

facebook-github-bot commented Mar 15, 2023

Action Required

Process

analysis-bot commented Mar 15, 2023 • edited Loading

facebook-github-bot commented Mar 15, 2023

facebook-github-bot commented Mar 15, 2023

facebook-github-bot commented Mar 16, 2023

javache commented Mar 16, 2023

javache left a comment

Choose a reason for hiding this comment

javache Mar 16, 2023

Choose a reason for hiding this comment

soumyajit4419 Mar 16, 2023

Choose a reason for hiding this comment

soumyajit4419 commented Mar 19, 2023 • edited Loading

facebook-github-bot commented Mar 21, 2023

situchan commented Mar 22, 2023

facebook-github-bot commented Mar 22, 2023

situchan commented Mar 22, 2023

javache commented Mar 22, 2023

facebook-github-bot commented Mar 22, 2023

situchan commented Mar 22, 2023

soumyajit4419 commented Mar 22, 2023 • edited Loading

situchan commented Mar 24, 2023 • edited Loading

analysis-bot commented Mar 15, 2023 •

edited

Loading

soumyajit4419 commented Mar 19, 2023 •

edited

Loading

soumyajit4419 commented Mar 22, 2023 •

edited

Loading

situchan commented Mar 24, 2023 •

edited

Loading