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: fix onCompositionEnd update error #5576

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

qirong77
Copy link
Contributor

@qirong77 qirong77 commented Dec 2, 2023

Description
When user is compositing, The code-block updating is not work correctly.
Dec-02-2023 21-23-41

Context
The onEompositionEnd Event will trigger two react update,one is setIsComposing(false) and anthor is Editor.insertText(editor, event.data)Editor.insertText(editor, event.data) should update first or it will cause update error(selection is not update) that in slate/packages/slate-react/src/components/element.tsx MemoizedElement。
So I use Promise to delay setIsComposing(false) and exec Editor.insertText(editor, event.data) first to make it resolve selection correctly when onEompositionEnd is triggered.

const MemoizedElement = React.memo(Element, (prev, next) => {
  return (
    prev.element === next.element &&
    prev.renderElement === next.renderElement &&
    prev.renderLeaf === next.renderLeaf &&
    prev.renderPlaceholder === next.renderPlaceholder &&
    isElementDecorationsEqual(prev.decorations, next.decorations) &&
// The selection is not update correctly when onEompositionEnd is triggered
    (prev.selection === next.selection ||
      (!!prev.selection &&
        !!next.selection &&
        Range.equals(prev.selection, next.selection)))
  )
})

The Right

Here is the right update when user composition end
Dec-02-2023 21-46-52

Copy link

changeset-bot bot commented Dec 2, 2023

🦋 Changeset detected

Latest commit: 44f6d60

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 for the PR.

This needs a changeset to support our contributor workflow.

Ideally, if reasonable, please add a test for this change (we're slowly trying to add tests for slate-react).

packages/slate-react/src/components/editable.tsx Outdated Show resolved Hide resolved
@qirong77
Copy link
Contributor Author

qirong77 commented Dec 5, 2023

Thanks for the PR.

This needs a changeset to support our contributor workflow.

Ideally, if reasonable, please add a test for this change (we're slowly trying to add tests for slate-react).

Thanks for review. I have add a changeset now.
I would like to do complete a test for this change but I'm sorry that I am not familiar with writing test , this is beyond my ability😂.

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.

Ok, we'll accept and release this, but if it does raise regressions then we'll roll it back.

@dylans dylans merged commit 8ce52fd into ianstormtaylor:main Dec 6, 2023
8 of 9 checks passed
@github-actions github-actions bot mentioned this pull request Dec 6, 2023
@daddybh
Copy link

daddybh commented Dec 11, 2023

2023-12-11.10.43.10.mov

this MR will rising another issue, which the caret will jump to the start.

@eagowang
Copy link
Contributor

https://github.com/ianstormtaylor/slate/assets/33490722/dee594c8-4514-4e89-803b-7594b5fb4988
t

2023-12-11.10.43.10.mov
this MR will rising another issue, which the caret will jump to the start.

Same here,the caret will jump to last position

2023-12-16.11.34.42.mov

@sennpang
Copy link
Contributor

image
packages\slate-react\src\components\editable.tsx
onCompositionEnd add this solved the problem

@yunwuxin
Copy link
Contributor

@dylans Please review this PR again.

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.

6 participants