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: supplement of (fix: Prevent ReactEditor.toDOMRange crash in setDomSelection #5741) #5792

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zhi-zhi-zhi
Copy link

@zhi-zhi-zhi zhi-zhi-zhi commented Jan 13, 2025

Sorry, ue to the problem of English writing ability, the following English content is translated from Chinese by claude3.5

Description
任何会改变 selection 的 op 同步到 dom 之前,onDOMSelectionChange 都不应该执行
onDOMSelectionChange should not be executed before any selection-changing operations are synchronized to the DOM

Issue
所有出现”Cannot resolve a DOM point from Slate point“的 issue 可能都是,包括但不限于:
Maybe all issues where 'Cannot resolve a DOM point from Slate point' occurs include but are not limited to:

Fixes: (link to issue)
#5694

Example

在协同场景下很常见,例如内容 ‘1234567',光标在末尾
1. 当前用户光标 selection 在 path: [0,0], offset: 7
2. 收到一条 op ,删除 '7',在相关处理中(applyToDraft),selection 的 offset 会变成6
3. 所有状态渲染至 dom ,在这之前,Editable.tsx 306 line 的 useIsomorphicLayoutEffect 会把 editor.selection 转变为 domSelection

上面的步骤是理想步骤,但是问题就出在 2 和 3 之间,可能会执行 onDOMSelectionChange (throttle 导致其执行时机不确定)
onDOMSelectionChange 做了下面的事情:
Line 279: Transforms.select(editor, range)
在此之前,editor.selection 由 [0,0], 7 变更为 [0,0], 6,但尚未同步至 domSelection 
然而在 onDOMSelectionChange 这里,会读取同步前旧的 domSelection,将其同步给 editor.selection,将 [0,0],6 又变回了 [0,0], 7

然而在删除'7'这个操作渲染到 dom 后,useIsomorphicLayoutEffect 又会同步一遍 editor.selection & domSelection。
此时页面 dom 上 '7' 已经不存在,导致 Cannot resolve a Slate point from DOM point: {path: [0,0], offset: 7}

This is common in collaborative scenarios. For example, with content '1234567' and cursor at the end:

  1. Current user's selection is at path: [0,0], offset: 7
  2. Receives an operation that deletes '7'. During processing (applyToDraft), the selection's offset changes to 6
  3. All states are rendered to DOM. Before this, the useIsomorphicLayoutEffect at line 306 in Editable.tsx will convert editor.selection to domSelection

These are the ideal steps, but the issue occurs between steps 2 and 3, where onDOMSelectionChange might execute (timing is uncertain due to throttle).

onDOMSelectionChange performs the following:
Line 279: Transforms.select(editor, range)
Before this, editor.selection changed from [0,0], 7 to [0,0], 6, but hasn't been synchronized to domSelection yet.
However, onDOMSelectionChange reads the old domSelection before synchronization and syncs it back to editor.selection, changing [0,0], 6 back to [0,0], 7.

After the operation to delete '7' is rendered to DOM, useIsomorphicLayoutEffect will synchronize editor.selection & domSelection again.
At this point, '7' no longer exists in the page DOM, leading to "Cannot resolve a DOM point from Slate point: {path: [0,0], offset: 7}"

Context
thanks to
Commit e97a9f8

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.)

Copy link

changeset-bot bot commented Jan 13, 2025

⚠️ No Changeset found

Latest commit: ec36e21

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

@zhi-zhi-zhi
Copy link
Author

case 'split_node':
case 'insert_text':
case 'remove_text':
case 'set_selection': {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think set_selection should be in this list no?

Copy link
Author

Choose a reason for hiding this comment

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

For same reason mentioned:
onDOMSelectionChange should not be executed before any selection-changing operations are synchronized to the DOM.

Because onDOMSelectionChange( throttle) may be executed at any time(May be just right after editor.selection change and before editor.selection sync to domSelection)

set_selection op also changes editor.selection like others

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah okay got it sorry 👍 In that case I would suggest to rename the IS_NODE_MAP_DIRTY to something closer to it's real implications, like IS_DOM_EDITOR_DESYNCED for example. My 2cts though, and could be done in another PR.

I faced the issue too, and I think your change could fix it 🙏

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.

2 participants