feat: [iOS] add caretHeight and caretYoffset to TextInput component #37147

OlimpiaZurek · 2023-04-28T11:57:56Z

Summary:

This PR adds support for the caret height and caret position properties in iOS multi-line TextInput
PR for the docs update: facebook/react-native-website#3709

Changelog:

[IOS] [ADDED] - Add caretHeight and caretYOffset props to TextInput component

Test Plan:

Default caret:

default_caret.mov

Caret with caretHeight and caretYOffsetadjustment:

adjusting_caret.mov

facebook-github-bot · 2023-04-28T11:58:02Z

Hi @OlimpiaZurek!

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 [email protected]. Thanks!

analysis-bot · 2023-04-28T12:22:36Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	18,000,725	+118,595
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	21,359,082	+118,511
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 12f2c3c
Branch: main

facebook-github-bot · 2023-04-28T13:08:55Z

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

fabioh8010

Hey @OlimpiaZurek ! I reviewed your code and left some comments, minor things I just noticed. Besides that, I have two other points:

Please include a Test Plan in your PR. For example, you could attach a video showing you how your props works in the text input for iOS.
Also create a PR to include your props in the react-native-website repo. You can link each issue to the other to keep track of the overall progress.

packages/react-native/Libraries/Components/TextInput/TextInput.d.ts

packages/react-native/Libraries/Components/TextInput/TextInput.flow.js

packages/react-native/Libraries/Components/TextInput/TextInput.js

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

...nderer/components/textinput/iostextinput/react/renderer/components/iostextinput/primitives.h

OlimpiaZurek · 2023-05-12T09:36:39Z

All comments have already been resolved. Can someone take another look and review? Thanks!

koko57 · 2023-06-16T12:10:46Z

Conflicts resolved, ready for review

sakluger · 2024-01-16T21:09:19Z

Hey @fabioh8010 - would it be possible to add this PR to your list of priorities? It's one of our oldest outstanding bugs and we'd love to get it resolved. Thanks!

fabioh8010 · 2024-01-17T09:53:08Z

Hi @sakluger , I don't work at Meta, I just reviewed because I'm from the same company of the PR author.

cpojer · 2024-01-30T10:21:52Z

@sammy-SC @yungsters could you merge this?

sammy-SC · 2024-02-04T19:58:06Z

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

+@implementation CustomTextSelectionRect
+
+- (CGRect)rect {
+    return __rect;


why are there two underscores before member variables?

I used double underscores to avoid name collisions. Changed implementation a bit and removed them.

sammy-SC · 2024-02-04T19:59:32Z

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

+
+- (NSArray *)selectionRectsForRange:(UITextRange *)range {
+  NSArray *superRects = [super selectionRectsForRange:range];
+  if(_caretYOffset != 0 && _caretHeight != 0) {


what if only _caretYOffset is set? Shouldn't the below block be executed?

The block should only be executed if both properties are set.

sammy-SC · 2024-02-04T20:01:49Z

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

+  return originalRect;
+}
+
+- (NSArray *)selectionRectsForRange:(UITextRange *)range {


why is this override needed? I don't understand what it is trying to achieve on top of handling _caretYOffset and _caretHeight in the method caretRectForPosition above.

The goal is to avoid the cursor touching the the symbols of the previous line in the second and subsequent lines:

This is the default behavior derived from caretRectForPosition. To fix this, I added two new properties _caretYOffset and _caretHeight to calculate the caret position and caret height, thus preventing the cursor from overlapping the previous line of text.

To keep the same effect on selection selectionRectsForRange needs to be override as well.

Can you add a screenshot of what this looks like with a multi-line text selection?

Sure, here is a screenshot for multi-line text selection:

OlimpiaZurek · 2024-03-27T08:43:32Z

@sammy-SC All comments have been resolved. Can you take a look and review ? Thanks!
cc: @NickGerleman

javache · 2024-04-02T19:15:10Z

LGTM, few comments.

javache · 2024-04-02T19:10:40Z

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

+  return originalRect;
+}
+
+- (NSArray *)selectionRectsForRange:(UITextRange *)range {


Suggested change

- (NSArray *)selectionRectsForRange:(UITextRange *)range {

- (NSArray<UITextSelectionRect *> *)selectionRectsForRange:(UITextRange *)range {

javache · 2024-04-02T19:12:55Z

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

@@ -13,6 +13,33 @@
 #import <React/RCTBackedTextInputDelegateAdapter.h>
 #import <React/RCTTextAttributes.h>

+//the UITextSelectionRect subclass needs to be created because the original version is not writable
+@interface CustomTextSelectionRect : UITextSelectionRect


UITextSelectionRect is explicitly documented as being an abstract base class so this makes sense. Maybe mention that instead of it not being writable?

Use RCT as prefix

Suggested change

@interface CustomTextSelectionRect : UITextSelectionRect

@interface RCTTextSelectionRect : UITextSelectionRect

good point, changed

javache · 2024-04-02T19:12:59Z

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

+  return originalRect;
+}
+
+- (NSArray *)selectionRectsForRange:(UITextRange *)range {


Can you add a screenshot of what this looks like with a multi-line text selection?

javache · 2024-04-02T19:13:46Z

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

+
+- (NSArray *)selectionRectsForRange:(UITextRange *)range {
+    NSArray *superRects = [super selectionRectsForRange:range];
+    if(_caretYOffset != 0 && _caretHeight != 0) {


Should this be ||, so this works if you specify either one of the properties?

The block should only be executed if both properties are set.

javache · 2024-04-02T19:14:00Z

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

+        for (UITextSelectionRect *rect in superRects) {
+            CustomTextSelectionRect *customTextRect = [[CustomTextSelectionRect alloc] init];
+
+            customTextRect.rect = CGRectMake(rect.rect.origin.x, rect.rect.origin.y + _caretYOffset, rect.rect.size.width, _caretHeight);


This then needs to handle _caretHeight being potentially 0.

The block should only be executed if both properties are set.

react-native-bot · 2024-10-03T05:01:09Z

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

react-native-bot · 2024-10-10T05:01:16Z

This PR was closed because it has been stalled for 7 days with no activity.

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 Apr 28, 2023

fabioh8010 reviewed Apr 28, 2023

View reviewed changes

OlimpiaZurek mentioned this pull request May 4, 2023

feat: add caretYOffset and caretHeight props to textinput.md facebook/react-native-website#3709

Open

OlimpiaZurek changed the title ~~feat: add caretHeight and caretYoffset to TextInput component~~ feat: [iOS] add caretHeight and caretYoffset to TextInput component May 4, 2023

OlimpiaZurek force-pushed the feature/text-input-ios-support-for-adjusting-caret branch from e265c13 to 2cbd5a3 Compare May 4, 2023 09:55

OlimpiaZurek mentioned this pull request May 23, 2023

[iOS] add caretHeight and caretYoffset to TextInput component #37147 Expensify/react-native#58

Open

OlimpiaZurek force-pushed the feature/text-input-ios-support-for-adjusting-caret branch from caefbb8 to 5e48d26 Compare May 29, 2023 10:23

sammy-SC requested changes Feb 4, 2024

View reviewed changes

OlimpiaZurek added 4 commits February 23, 2024 13:39

feat: add caretHeight and caretYoffset to TextInput component

e8e577e

minor changes after code review

148d162

feat: add caretHeight and caretYoffset to TextInput component

1f1d08b

minor changes after code review

f55a9ff

OlimpiaZurek force-pushed the feature/text-input-ios-support-for-adjusting-caret branch from bd5c769 to f55a9ff Compare February 23, 2024 12:44

remove double double underscores and fix error

f91c45f

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Apr 2, 2024

javache reviewed Apr 2, 2024

View reviewed changes

changes after cr

1f3fd22

OlimpiaZurek requested review from javache and sammy-SC April 5, 2024 11:09

react-native-bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Oct 3, 2024

react-native-bot closed this Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [iOS] add caretHeight and caretYoffset to TextInput component #37147

feat: [iOS] add caretHeight and caretYoffset to TextInput component #37147

OlimpiaZurek commented Apr 28, 2023 •

edited

Loading

facebook-github-bot commented Apr 28, 2023

analysis-bot commented Apr 28, 2023 •

edited

Loading

facebook-github-bot commented Apr 28, 2023

fabioh8010 left a comment

OlimpiaZurek commented May 12, 2023

koko57 commented Jun 16, 2023

sakluger commented Jan 16, 2024

fabioh8010 commented Jan 17, 2024

cpojer commented Jan 30, 2024

sammy-SC Feb 4, 2024

OlimpiaZurek Feb 26, 2024

sammy-SC Feb 4, 2024

OlimpiaZurek Feb 26, 2024

sammy-SC Feb 4, 2024

OlimpiaZurek Feb 26, 2024

javache Apr 2, 2024

OlimpiaZurek Apr 5, 2024

OlimpiaZurek commented Mar 27, 2024

javache commented Apr 2, 2024

javache Apr 2, 2024

OlimpiaZurek Apr 5, 2024

javache Apr 2, 2024

OlimpiaZurek Apr 5, 2024

javache Apr 2, 2024

javache Apr 2, 2024

OlimpiaZurek Apr 5, 2024

javache Apr 2, 2024

OlimpiaZurek Apr 5, 2024

react-native-bot commented Oct 3, 2024

react-native-bot commented Oct 10, 2024

	- (NSArray )selectionRectsForRange:(UITextRange )range {
	- (NSArray<UITextSelectionRect > )selectionRectsForRange:(UITextRange *)range {

	@interface CustomTextSelectionRect : UITextSelectionRect
	@interface RCTTextSelectionRect : UITextSelectionRect

feat: [iOS] add caretHeight and caretYoffset to TextInput component #37147

feat: [iOS] add caretHeight and caretYoffset to TextInput component #37147

Conversation

OlimpiaZurek commented Apr 28, 2023 • edited Loading

Summary:

Changelog:

Test Plan:

facebook-github-bot commented Apr 28, 2023

Action Required

Process

analysis-bot commented Apr 28, 2023 • edited Loading

facebook-github-bot commented Apr 28, 2023

fabioh8010 left a comment

Choose a reason for hiding this comment

OlimpiaZurek commented May 12, 2023

koko57 commented Jun 16, 2023

sakluger commented Jan 16, 2024

fabioh8010 commented Jan 17, 2024

cpojer commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlimpiaZurek commented Mar 27, 2024

javache commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

react-native-bot commented Oct 3, 2024

react-native-bot commented Oct 10, 2024

OlimpiaZurek commented Apr 28, 2023 •

edited

Loading

analysis-bot commented Apr 28, 2023 •

edited

Loading