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 ime double input with mark #4158

Merged
merged 5 commits into from
Apr 13, 2021

Conversation

githoniel
Copy link
Contributor

Description

this pr will force String element refresh it self to avoid ime double input

Issue

No

Example

xx

Context

image

while we should get

image

this is becasue we memoizedLeaf to avoid refresh. now we will refresh all sibling leaf when a leaf changes

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 Apr 1, 2021

🦋 Changeset detected

Latest commit: 9208676

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

@ianstormtaylor
Copy link
Owner

Thanks @githoniel!

@ianstormtaylor ianstormtaylor merged commit ea6dc08 into ianstormtaylor:main Apr 13, 2021
@github-actions github-actions bot mentioned this pull request Apr 13, 2021
beorn pushed a commit to beorn/slate that referenced this pull request Apr 13, 2021
* fix ime double when apply mark

* add changeset

* refactor to use auto-incrementing key and add comments for it

* Update lovely-walls-knock.md

* Update lovely-walls-knock.md

Co-authored-by: Ian Storm Taylor <ian@ianstormtaylor.com>
ulion pushed a commit to xliulian/slate that referenced this pull request Jul 1, 2021
* fix ime double when apply mark

* add changeset

* refactor to use auto-incrementing key and add comments for it

* Update lovely-walls-knock.md

* Update lovely-walls-knock.md

Co-authored-by: Ian Storm Taylor <ian@ianstormtaylor.com>
@rockettomatooo
Copy link
Contributor

@githoniel I'm running into issues in my editor because of this change. In my case, I'm listening to click events in the editor. Now a click event only fires, when mousedown and mouseup fire on the same node or have a common ancestor. Since the <String /> gets force-remounted, react seems to remove the DOM-Node the mousedown occured on from the DOM so when the mouseup occurs, it doesn't have a common ancestor thus the click never occurs.

Now I'm not really familiar with IME so is there a way to conditionalize the key increment?

@githoniel
Copy link
Contributor Author

@rockettomatooo you may try to compare slate nodes to see if they are the same instead of DOM nodes.

IME input is a hard problem to solve, strictly speaking it's a React Reconciliation's bug. I have not found a better solution for this issue

@rockettomatooo
Copy link
Contributor

rockettomatooo commented Jul 27, 2021

@githoniel The problem I have is, that the browser does this comparison. What I described is the browsers logic to fire a click event. I can try to detect a click on my own but I want to avoid that if possible somehow. I prepared a small jsfiddle to demonstrate here.

Can you maybe explain to me, how the reconciliation bug occurs specifically?
My guess would be, that the IME directly modifies the HTML and slate lets it do it because it's a composition and it's not done yet. Then, when the composition is done, slate inserts the composition as bold, but whats already in the HTML doesn't get updated because of the bug.

@githoniel
Copy link
Contributor Author

githoniel commented Jul 27, 2021

@rockettomatooo Chrome is able to remove composition text correctly when the final input text is on the side of the composition text.

we will create a new node with marks and insert text in it so the the final input text is not on the side of the composition text.

maybe we could move selection and insert new node in advance when onCompositionStart trigger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants