fixed4811: input Chinese at the end of link element, focus lose

[sennpang]

Description
input Chinese at the end of link element, focus lose (maybe others IME also has this bug)

Issue
Fixes: #4811

Example
before:
https://user-images.githubusercontent.com/16861647/151319338-747db06d-6fd7-4f67-8e00-a61241456bfe.mp4
after:
https://user-images.githubusercontent.com/16861647/159625215-4ff51bba-ed5b-43a7-8ec1-1a1158a1004f.mov

Context
i wonder known why we should change selection when input end of inline element on compositionStart event, as i comment this lines, everything is okay.

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]

⚠️ No Changeset found

Latest commit: 72a8692

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[dylans]

I’m afraid this change, while it may fix this issue, is going to break other behavior. We are working to add a test suite for slate-react as this change isn’t currently covered by our tests.

[sennpang]

I’m afraid this change, while it may fix this issue, is going to break other behavior. We are working to add a test suite for slate-react as this change isn’t currently covered by our tests.

okay

[walkdoer]

I’m afraid this change, while it may fix this issue, is going to break other behavior. We are working to add a test suite for slate-react as this change isn’t currently covered by our tests.

Hi @dylans , I have the same issue here, May I ask what is the purpose of this code in onCompositionStart, if remove these lines ,what behaviors will break ?

const inline = Editor.above(editor, {
  match: n => Editor.isInline(editor, n),
  mode: 'highest',
})
if (inline) {
  const [, inlinePath] = inline
  if (Editor.isEnd(editor, selection.anchor, inlinePath)) {
    const point = Editor.after(editor, inlinePath)!
    Transforms.setSelection(editor, {
      anchor: point,
      focus: point,
    })
  }
}

[dylans]

I’m afraid this change, while it may fix this issue, is going to break other behavior. We are working to add a test suite for slate-react as this change isn’t currently covered by our tests.

Hi @dylans , I have the same issue here, May I ask what is the purpose of this code in onCompositionStart, if remove these lines ,what behaviors will break ?

const inline = Editor.above(editor, {
  match: n => Editor.isInline(editor, n),
  mode: 'highest',
})
if (inline) {
  const [, inlinePath] = inline
  if (Editor.isEnd(editor, selection.anchor, inlinePath)) {
    const point = Editor.after(editor, inlinePath)!
    Transforms.setSelection(editor, {
      anchor: point,
      focus: point,
    })
  }
}

It was added as part of #4451 which fixed #4158 and #3888 . I think the change of removing these lines of code breaks IME in other ways.

[sennpang]

I’m afraid this change, while it may fix this issue, is going to break other behavior. We are working to add a test suite for slate-react as this change isn’t currently covered by our tests.

Hi @dylans , I have the same issue here, May I ask what is the purpose of this code in onCompositionStart, if remove these lines ,what behaviors will break ?

const inline = Editor.above(editor, {
  match: n => Editor.isInline(editor, n),
  mode: 'highest',
})
if (inline) {
  const [, inlinePath] = inline
  if (Editor.isEnd(editor, selection.anchor, inlinePath)) {
    const point = Editor.after(editor, inlinePath)!
    Transforms.setSelection(editor, {
      anchor: point,
      focus: point,
    })
  }
}

It was added as part of #4451 which fixed #4158 and #3888 . I think the change of removing these lines of code breaks IME in other ways.

can't reproduction #4158 on my branch, i comment these lines

const inline = Editor.above(editor, {
  match: n => Editor.isInline(editor, n),
  mode: 'highest',
})
if (inline) {
  const [, inlinePath] = inline
  if (Editor.isEnd(editor, selection.anchor, inlinePath)) {
    const point = Editor.after(editor, inlinePath)!
    Transforms.setSelection(editor, {
      anchor: point,
      focus: point,
    })
  }
}

QQ20220328-172629-HD.mp4

[dylans]

@githoniel we'd appreciate your feedback on this one if you have time, as it's related to your PR from last year. Thanks!

[githoniel]

I will check this later

[ssshiiin]

@githoniel
I would like you to confirm this one.
I need it for my project.

[ssshiiin]

@sennpang
Did you find another solution?

[sennpang]

No, i just comment these lines, i don't known why we should change selection when the focus at the end of inline element.

[12joan]

I've just noticed this PR now. It looks like my PR #5541 is a duplicate of this.

For what it's worth, I'm pretty sure that the offending code was left in unintentionally. Here's my analysis from issue #5540:

This behaviour was added in PR #4451. The main behaviour of that PR (everything below the "insert new node in advance..." comment) seems to have been removed at some point, but the Git history doesn't show when*. Regardless, the set_selection behaviour (those lines above the comment) remains in the code to this day. It may have been left in unintentionally even when no longer needed.

*Both git blame --reverse and git bisect point to commit f2736479 as the first time the lines were absent from the code, but this commit is clearly unrelated. Perhaps the Git history became corrupted at some point?

[12joan]

@dylans Can we try merging either this or my PR, on the understanding that we'll revert it immediately if it causes any IME issues more serious than the one it fixes? Currently, Plate's mentions are unusable for languages that use IMEs, and there's no simple workaround other than patching slate-react.