-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
fix: supplement of (fix: Prevent ReactEditor.toDOMRange crash in setDomSelection #5741) #5792
Conversation
🦋 Changeset detectedLatest commit: fe9110c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
case 'split_node': | ||
case 'insert_text': | ||
case 'remove_text': | ||
case 'set_selection': { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with landing this and renaming it in another PR. I'll add a FIXME.
case 'split_node': | ||
case 'insert_text': | ||
case 'remove_text': | ||
case 'set_selection': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with landing this and renaming it in another PR. I'll add a FIXME.
@zhi-zhi-zhi one of my edits probably broke prettier, if you have time to fix, I'll land it, if not, I'll look as soon as possible. Thanks. |
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
This is common in collaborative scenarios. For example, with content '1234567' and cursor at the end:
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
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)