-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Conversation
Hi @perunt! Thank you for your pull request and welcome to our community. Action RequiredIn 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. ProcessIn 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 If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Base commit: 28c26dc |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
…position property across both iOS and Android platforms
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.
Hey @perunt
Can you please take a look at the CI signal as the Android code is not compiling
Now it's compiling successfully, and all tests are passing well. Please let me know if you notice any other issues. |
...ctAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputSelectionEvent.java
Outdated
Show resolved
Hide resolved
Can you add a RNTester page or add to an exisiting one, a usage of the new API? |
I suspect this also needs some changes in fabric to support the extra properties? |
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. |
Got it, let me address all this stuff |
…text-input-cursor-position
Would it make sense for the caret position to be a pair of points / rect? What point does the current XY represent? |
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. |
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'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 |
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.
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:
- New event data, and new prop
- Android, iOS, and JS layer changes
- 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?
Thanks for the feedback! So, about that cursor position - It updates when the |
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? |
@NickGerleman all necessary updates made in that PRs
Current PR I'll leave for grouping this issue |
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. |
This PR was closed because it has been stalled for 7 days with no activity. |
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:
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: