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

Ensure ghost text doesn't appear when performing undos #2105

Merged
merged 3 commits into from
Apr 3, 2024

Conversation

amgleitman
Copy link
Member

Summary:

Right now, the ghost text feature is quite incompatible with undo/redo support. Since we weren't filtering out ghost text from a text input's contents when registering undoable actions, we were susceptible to a bunch of weird side effects, such as ghost text becoming editable or even crashes.

The fix here is to make sure any undoable actions ignore ghost text. Ghost text is very transient and is designed to disappear as soon as you make any sort of change, so the easiest way to do this is to just remove it directly before doing anything.

A related issue pops up where a write to setAttributedText: also triggers a call to set the ghost text to nil. Since setAttributedText: already gets rid of the ghost text when we ask it to, the subsequent call to setGhostText: may not need any action. We solve this by being more lenient in this case, assuming that if the ghost text isn't in where our internal state expects it to be, we probably already removed it.

Test Plan:

Validated that ghost text behaves properly against the following scenarios:

  • Modify the value of the text input by typing manually.
  • Setting the value of the text input on the JS side.
  • Moving the cursor via a mouse click or arrow keys.
  • Placing ghost text either in the middle or at the end of the text.
  • Cases where the ghost text immediately precedes or follows "real" text with the same value (e.g., "This is ghost(ghost) text" or "This is (ghost)ghost text").

@amgleitman amgleitman requested a review from a team as a code owner April 3, 2024 17:36
@amgleitman amgleitman requested a review from nakambo April 3, 2024 17:36
@amgleitman amgleitman enabled auto-merge (squash) April 3, 2024 20:00
@amgleitman amgleitman merged commit fee391c into microsoft:main Apr 3, 2024
16 checks passed
@amgleitman amgleitman deleted the undo-ignores-ghost-text branch April 3, 2024 21:02
amgleitman added a commit to amgleitman/react-native-macos that referenced this pull request Apr 3, 2024
* Ensure ghost text doesn't appear when performing undos

* Clarify purpose of strict mode in `removingGhostTextFromString:strict:`

* Add clarifying comments about `removingGhostTextFromString:strict:` nil checks

---------

Co-authored-by: Adam Gleitman <adgleitm@microsoft.com>
amgleitman added a commit that referenced this pull request Apr 3, 2024
* Ensure ghost text doesn't appear when performing undos

* Clarify purpose of strict mode in `removingGhostTextFromString:strict:`

* Add clarifying comments about `removingGhostTextFromString:strict:` nil checks

---------

Co-authored-by: Adam Gleitman <adgleitm@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants