Skip to content

Conversation

@rozele
Copy link
Contributor

@rozele rozele commented Mar 5, 2021

As stated in issue #7283, the cursor position changes seem to be somewhat different on each platform. Since we're already adding code in react-native-windows to change the Windows default cursor positioning behavior, we might as well make it a bit more consistent with other platforms. The change here matches the behavior on iOS, macOS and Web (so it's only different from Android, which seems to keep cursor position relative to the TextInput value length constant).

Fixes #7283

Microsoft Reviewers: Open in CodeFlow

As stated in issue microsoft#7283, the cursor position changes seem to be
somewhat different on each platform. Since we're already adding code in
react-native-windows to change the Windows default cursor positioning
behavior, we might as well make it a bit more consistent with other
platforms. The change here matches the behavior on iOS, macOS and Web
(so it's only different from Android, which seems to keep cursor
position relative to the TextInput value length constant).

Fixes microsoft#7283
@rozele rozele requested a review from a team as a code owner March 5, 2021 01:46
@asklar
Copy link
Member

asklar commented Mar 5, 2021

+@rectified95 since he recently changed this code. There is a natural tension between matching the native platform, and being consistent across platforms. Do we know why Android behaves differently? Partial alignment seems potentially more confusing than each platform being consistent with the native controls (e.g. what if I don't happen to test this on Android, whereas doing what the native platform does would give me something that feels familiar to the user)

@rozele
Copy link
Contributor Author

rozele commented Mar 5, 2021

@asklar Windows seems to be the only React Native platform that specializes the text selection behavior when the text is changed. Android does do some simple checks to make sure the position isn't out of bounds (to avoid a crash) but everything else is the platform specific behavior.

I totally agree that the best option is just to let each platform decide how to handle this. Unfortunately, the XAML default seems to lead to a poor UX (resetting the cursor position back to 0) and would force the app developer to always provide custom handling for cursor position if they ever touch the value prop while the user is typing.

Another +1 for this algorithm comes from the HTML spec: https://html.spec.whatwg.org/multipage/input.html#dom-input-value-value:

If the element's value (after applying the value sanitization algorithm) is different from oldValue, and the element has a text entry cursor position, move the text entry cursor position to the end of the text control, unselecting any selected text and resetting the selection direction to "none".

@asklar
Copy link
Member

asklar commented Mar 5, 2021

@rozele good discussion points. Adding a few more folks from PM to get their take. One of the things we set out to do with RNW from our side is to use it as a forcing function to drive changes into the native platform so it would be good to capture these kinds of things as bugs against XAML/WinUI. If it's not too much trouble, would you be able to file / link to make the WinUI default "make sense"?
@stmoy @kikisaints @harinikmsft

@NickGerleman
Copy link
Contributor

NickGerleman commented Mar 5, 2021

I would prefer we make any behavior changes needed on our side.

Seems like this would be a good issue to raise to the WinUI team, but I don't expect we will be able to stop supporting system xaml users for the foreseeable future.

@rectified95
Copy link
Contributor

rectified95 commented Mar 5, 2021

I think change is ok to go in. When I introduced the current mechanism, I mostly wanted to fix the scenario of the cursor resetting to the beginning during dynamic re-write (eg. auto-capitalize), while thinking what would make sense for other cases. I agree the case mentioned of rewriting 'aa' -> 'bbb' is better handled by this algorithm.

Also, the scaling done in Android seems a little strange to me - if I understand it correctly, it only approximately moves the position if the string length increases. The HTML spec is a good argument to go for this, imo. We're trying elsewhere to align with it, eg. with keyboard/mouse events.

@rozele
Copy link
Contributor Author

rozele commented Mar 6, 2021

@NickGerleman
Copy link
Contributor

NickGerleman commented Mar 7, 2021

Going to give this till end of this week for feedback, but merge if we don't hear anything by then.

@NickGerleman
Copy link
Contributor

@msftbot merge in 120 hours

@ghost ghost added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Mar 7, 2021
@ghost
Copy link

ghost commented Mar 7, 2021

Hello @NickGerleman!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Fri, 12 Mar 2021 20:20:00 GMT, which is in 5 days

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit 7e20245 into microsoft:master Mar 12, 2021
rectified95 pushed a commit to rectified95/react-native-windows that referenced this pull request Apr 23, 2021
* Change TextInput cursor behavior

As stated in issue microsoft#7283, the cursor position changes seem to be
somewhat different on each platform. Since we're already adding code in
react-native-windows to change the Windows default cursor positioning
behavior, we might as well make it a bit more consistent with other
platforms. The change here matches the behavior on iOS, macOS and Web
(so it's only different from Android, which seems to keep cursor
position relative to the TextInput value length constant).

Fixes microsoft#7283

* Change files
ghost pushed a commit that referenced this pull request Apr 23, 2021
* Change TextInput cursor behavior (#7285)

* Change TextInput cursor behavior

As stated in issue #7283, the cursor position changes seem to be
somewhat different on each platform. Since we're already adding code in
react-native-windows to change the Windows default cursor positioning
behavior, we might as well make it a bit more consistent with other
platforms. The change here matches the behavior on iOS, macOS and Web
(so it's only different from Android, which seems to keep cursor
position relative to the TextInput value length constant).

Fixes #7283

* Change files

* Change files

Co-authored-by: Eric Rozell <erozell@outlook.com>
Co-authored-by: Igor Klemenski <igklemen@microsoft.com>
Co-authored-by: Nick Gerleman <ngerlem@microsoft.com>
rozele added a commit to rozele/react-native-windows that referenced this pull request Oct 6, 2021
microsoft#7285)

Summary:
* Change TextInput cursor behavior

As stated in issue microsoft#7283, the cursor position changes seem to be
somewhat different on each platform. Since we're already adding code in
react-native-windows to change the Windows default cursor positioning
behavior, we might as well make it a bit more consistent with other
platforms. The change here matches the behavior on iOS, macOS and Web
(so it's only different from Android, which seems to keep cursor
position relative to the TextInput value length constant).

Fixes microsoft#7283

* Change files

Test Plan:
|Before|After|
|{F621787254}|{F621786620}|

Reviewers: skyle, lyahdav, mylando

Reviewed By: skyle

Subscribers: eliwhite

Differential Revision: https://phabricator.intern.facebook.com/D28900859

Tasks: T90751153

Signature: 28900859:1622824234:7fad03df1097de112c8673664f0c7aa44b435df4
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should TextInput cursor movement better match other platforms?

5 participants