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 editor.insertText never gets called inside plugins on Android #4753

Merged
merged 3 commits into from
Jan 4, 2022

Conversation

alessiogaldy
Copy link
Contributor

@alessiogaldy alessiogaldy commented Dec 30, 2021

Description
Call editor.insertText instead of Transforms.insertText to allow overriding behavior in plugins.

Issue
Fixes: #4709
I was able to verify that this now works correctly on my Android device, but I am not sure how to add a test for this.

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Dec 30, 2021

🦋 Changeset detected

Latest commit: 5880553

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

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

Thanks @alessiogaldy , I will land this sometime in the next few days unless there are objections.

@TheSpyder
Copy link
Collaborator

This code was added in #4734 and appears to very specifically not call Editor.insertText in order to adjust the at position. I don't have an Android device so I don't really understand the scenario where this might happen. We probably need @YasinChan's input here.

@YasinChan
Copy link
Contributor

Hi @TheSpyder~
Code in android-editable.tsx is mine, but it came from Code in android-input-manager.ts. So I didn't pay much attention to the difference between Editor.insertText and Transforms.insertText at that time.

@TheSpyder
Copy link
Collaborator

Ah, my apologies.

It looks like that code originally came from #4257:
a93c555

@clauderic, are you able to provide any insights here?

@alessiogaldy
Copy link
Contributor Author

I’ll test this again later today but I suspect that this caused a regression and now the text is inserted at the wrong position

@alessiogaldy
Copy link
Contributor Author

I checked, and it looks like setting at is needed because this function adjusts the location before inserting the text:

export function normalizeTextInsertionRange(

I was able to obtain the same behavior by replacing the Transforms.insertText call with:

Transforms.setSelection(editor, at)
Editor.insertText(editor, text)

I am not sure if this is a good idea, but it seems to work.
I think a cleaner solution would be to allow for an optional to parameter to be passed to Editor.insertText so that it can be passed down to Transforms.insertText. Unfortunately that would require all existing plugins to pass the new argument down the chain.
It might make sense to change every function of the Editor interface to accept two arguments, the editor and an object with all other parameters. This would make adding arguments a non-breaking change in most cases because each plugin will either override the behavior or pass the object down the chain.

When testing on my phone I also noticed that when keyboard inputs are delayed, which seems to be needed to deal with edge cases on Android, the cursor sometimes jumps back and forth.
This is easier to reproduce by typing special chars like @ $.
I can reproduce this issue on the examples page so it's not a regression caused by this change (AFAIK it's not live yet).

I am happy to open a PR to add the Transforms.setSelection call and to disable the input delayed logic.

@alessiogaldy
Copy link
Contributor Author

Reading the ProseMirror source code that is referenced in android-input-manager.ts it looks like the delay is only used for Internet Explorer <= 11 so it should be safe to remove it.

DougReeder pushed a commit to DougReeder/slate that referenced this pull request Apr 3, 2022
…nstormtaylor#4753)

* Call Editor.insertText instead of Transforms.insertText to allow overriding by plugins

* Use Editor.insertText in android-input-manager

* changeset
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.

editor.insertText never gets called inside plugins on android
4 participants