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

Fix insertion point and placeholder placement for TextInput #1322

Merged

Conversation

christophpurrer
Copy link

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

This fixes issues where the insertion point (caret) would not initially render in the correct location for TextInput with padding. Also, for single-line inputs, the placeholder would also be in the wrong location.

Changelog

[macOS] [Fixed] - Fix insertion point and placeholder placement for TextInput

Test Plan

Set a padding of 10

    padding: 10,

Before

You must click to put the caret/insertionPointer in the correct location

before.mov

After

after.mov

@chiuam
Copy link

chiuam commented Aug 9, 2022

I think this change broke the auto-focus test case:

Untitled-1.mov

*** Assertion failure in -RCTTouchHandler _recordNewTouches:, /Users/amchiu/react-native-macos/React/Base/RCTTouchHandler.m:110
2022-08-08 21:00:41.054210-0400 RNTester-macOS[81288:6998379] [General] Touch is already recorded. This is a critical bug.

Maybe the logic in didMoveToWindow needs a second look?

@christophpurrer
Copy link
Author

Thanks for pointing this out. That's indeed an issue. We don't have atm as we also changed RTCTouchHandler.
I think I need to wait on this change and see if I can upstream some of our changes to RTCTouchHandler

@christophpurrer christophpurrer changed the title Fix insertion point and placeholder placement for TextInput Fix insertion point and placeholder placement for TextInput [WIP] Aug 9, 2022
@christophpurrer christophpurrer changed the title Fix insertion point and placeholder placement for TextInput [WIP] [WIP] Fix insertion point and placeholder placement for TextInput Aug 10, 2022
@ghost ghost removed the Needs: Author Feedback label Aug 10, 2022
@ghost ghost added the no-recent-activity label Aug 19, 2022
@ghost
Copy link

ghost commented Aug 19, 2022

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@Saadnajmi
Copy link
Collaborator

Don't close

@ghost ghost removed the no-recent-activity label Aug 19, 2022
@ghost ghost added the no-recent-activity label Aug 26, 2022
@ghost
Copy link

ghost commented Aug 26, 2022

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@Saadnajmi
Copy link
Collaborator

Gonna stop marking these where they get stale...

@ghost ghost removed the no-recent-activity label Aug 26, 2022
@ghost ghost removed the Needs: Author Feedback label Aug 26, 2022
@christophpurrer christophpurrer changed the title [WIP] Fix insertion point and placeholder placement for TextInput Fix insertion point and placeholder placement for TextInput Aug 26, 2022
@christophpurrer
Copy link
Author

The latest iteration fixes the problem for the RCTTouchHandler by applying another band-aid solution.
However in the long term we need to find a better approach to correctly handle mouseUp events.
Maybe the

_shouldSendMouseUpOnSystemBehalf

BOOL approach is not the best one.

We have a large number RCTTouchHandler fixes - but also those are not yet in a state to upstream them ...

mouseUp1080.mov

This fixes issues where the insertion point (caret) would not initially render in the correct location for `TextInput` with padding.
Also, for single-line inputs, the placeholder would also be in the wrong location.
@Saadnajmi
Copy link
Collaborator

@chiuam could you take a second look?

@chiuam chiuam merged commit 1d0e105 into microsoft:main Sep 15, 2022
chiuam pushed a commit to chiuam/react-native-macos that referenced this pull request Sep 27, 2022
chiuam added a commit that referenced this pull request Sep 27, 2022
…1439)

Co-authored-by: Christoph Purrer <christophpurrer@outlook.com>
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants