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

Adding cursor coordinates to TextInput #36979

Closed
wants to merge 22 commits into from

Conversation

perunt
Copy link

@perunt perunt commented Apr 19, 2023

Summary:

This pull request aims to address the lack of information about the cursor (caret) position in the TextInput component. Currently, only the number of symbols where the cursor is located can be obtained(start and end cursor position), but this PR introduces a new property to track the cursor position in the x-y system coordinates.
iOS - #38110
Android - #38123

Changelog:

[GENERAL] [ADDED] - To achieve this, a new property has been added to track the cursor position, and the initializer has been updated to accept it. The conversion method has also been modified to parse this property from JSON and pass it to the updated initializer. This allows the code to handle cursor position alongside text selection information.

With this PR, the onSelectionChange method will now return the cursor position alongside the start and end positions, as follows:

selection: {
    start: number,
    end: number,
    cursorPosition: {
        start: {x: number, y: number},
        end: {x: number, y: number}
    }
}

The cursorPosition object contains the coordinates of the top left corner (start) and the bottom right corner (end) of the cursor or selected text.

Test Plan:

@facebook-github-bot
Copy link
Contributor

Hi @perunt!

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
Copy link

analysis-bot commented Apr 19, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,757,981 -256,628
android hermes armeabi-v7a 8,070,554 -205,283
android hermes x86 9,250,582 -277,742
android hermes x86_64 9,099,727 -270,992
android jsc arm64-v8a 9,319,506 -256,451
android jsc armeabi-v7a 8,509,417 -205,083
android jsc x86 9,383,000 -277,535
android jsc x86_64 9,636,240 -270,796

Base commit: 28c26dc
Branch: main

@facebook-github-bot
Copy link
Contributor

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 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 20, 2023
@perunt perunt marked this pull request as ready for review April 21, 2023 08:37
@Pranav-yadav Pranav-yadav added the Component: TextInput Related to the TextInput component. label Apr 27, 2023
…position property across both iOS and Android platforms
Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Hey @perunt
Can you please take a look at the CI signal as the Android code is not compiling

@perunt
Copy link
Author

perunt commented Jun 9, 2023

Now it's compiling successfully, and all tests are passing well. Please let me know if you notice any other issues.

@cortinico cortinico requested a review from NickGerleman June 9, 2023 13:43
@acoates-ms
Copy link
Contributor

Can you add a RNTester page or add to an exisiting one, a usage of the new API?
I dont see the new properties added to the type information in the JS? I'd expect to see an update to TextInput.js's Selection type, and a matching change in TextInput.d.ts.

@acoates-ms
Copy link
Contributor

I suspect this also needs some changes in fabric to support the extra properties?

@NickGerleman
Copy link
Contributor

NickGerleman commented Jun 10, 2023

Yes, I think previously the RN team had decided we will not be taking new API surface with only a Paper implementation, since these will stop working the moment we switch to Fabric as the default.

Though Android does reuse view managers on Fabric, and it looks like on iOS this might be some shared view between them, so this might just work. But we should be explicitly testing that.

@perunt
Copy link
Author

perunt commented Jun 14, 2023

Got it, let me address all this stuff

@PPatBoyd
Copy link

Would it make sense for the caret position to be a pair of points / rect? What point does the current XY represent?

@perunt
Copy link
Author

perunt commented Jun 15, 2023

The XY is just pointing to the top-left start of the selection right now. I get what you're saying - it might be a good move to add the end point too.

@acoates-ms
Copy link
Contributor

The XY is just pointing to the top-left start of the selection right now. I get what you're saying - it might be a good move to add the end point too.

I'm not sure what you are doing with these coords, but many of the use cases I can think of would require having both topleft and bottom right coords. Want to show a menu at the cursor? - depending on if the menu is going to show above or below the cursor, you'd need both coords.

@perunt
Copy link
Author

perunt commented Jun 16, 2023

I'm not sure what you are doing with these coords, but many of the use cases I can think of would require having both topleft and bottom right coords. Want to show a menu at the cursor? - depending on if the menu is going to show above or below the cursor, you'd need both coords.

Yes, my goal is to use this for menu positioning. I've attempted using both coordinates as you've suggested, but encountered a bit of a hiccup when it came to selection. I've been considering whether it should just pinpoint to a single location instead.
I can do the two-coordinates thing. Do you reckon I should do the same with the selection? I mean, if there were a few lines of selection, it would show the top left and bottom right corner coordinates,

@perunt
Copy link
Author

perunt commented Jun 20, 2023

I've made some updates. Now, we receive two points that represent the cursor position or the selection position. In the case of a selection, the start point represents the top left coordinate, and the end point signifies the bottom right coordinate of the selection. For representing the cursor, we use the same concept. The difference between this point is cursor width and height
Also i tested it with Fabric and it works as well
@NickGerleman will appreciate any thoughts on it, thanks!

Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

It looks like this does not implement the change on iOS Fabric at least (did a quick check that RCTTextInputComponentView does not reuse the view edited).

This change seems fairly mechanical, but we usually ask folks to split these sorts of PRs, to enable finding the most appropriate reviewers for each part (and to make it easy to ship things as they become ready). Right now we have:

  1. New event data, and new prop
  2. Android, iOS, and JS layer changes
  3. Paper and Fabric support (these may end up being the same on Android)

If this change gets much larger, I think we will want to split this up a bit, and might actually recommend we do that for the next iteration. E.g. start with a PR for the event, and then a second one for the prop.

I have not looked closely at the implementation for this yet, but the other questions I had were around the new API to allow explicitly controlling cursor position. I don't see this mentioned in your description, but I am curious how explicit control of cursor position interacts with character index based selection. Are these independent?

@perunt
Copy link
Author

perunt commented Jun 22, 2023

I have not looked closely at the implementation for this yet, but the other questions I had were around the new API to allow explicitly controlling cursor position. I don't see this mentioned in your description, but I am curious how explicit control of cursor position interacts with character index based selection. Are these independent?

Thanks for the feedback! So, about that cursor position - It updates when the onSelectionChanged event fires and it doesn't refresh when we explicitly set the selection.
Now, about the Fabric support, yep, I dropped the ball there. I was testing on a fork from the main and completely spaced on checking if changing the flag really turned Fabric. Good catch! I'm on it - will set up new PRs for Android and iOS Fabric support asap.

@perunt
Copy link
Author

perunt commented Jul 4, 2023

iOS - In this pr (#38110), I've added Fabric support and fixed the coordinates update when controlling cursor position explicitly.

Android - In this pr (#38123), It works with Fabric support and has the same updates for coordinates.

I've separated these for iOS and Android for better code review. However, I'm unsure how to handle the JavaScript portion as it's part of the example. @NickGerleman should I create separate pull requests for iOS fabric and paper?

@perunt
Copy link
Author

perunt commented Aug 2, 2023

@NickGerleman all necessary updates made in that PRs

iOS - In this pr (#38110), I've added Fabric support and fixed the coordinates update when controlling cursor position explicitly.
Android - In this pr (#38123), It works with Fabric support and has the same updates for coordinates.

Current PR I'll leave for grouping this issue

@facebook-github-bot 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 Aug 30, 2023
@perunt perunt mentioned this pull request Jan 25, 2024
50 tasks
Copy link

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.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 27, 2024
Copy link

github-actions bot commented Mar 5, 2024

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

@github-actions github-actions bot closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: TextInput Related to the TextInput component. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants