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 HistoryPlugin selection out of sync #4390

Merged
merged 4 commits into from Apr 26, 2023
Merged

Fix HistoryPlugin selection out of sync #4390

merged 4 commits into from Apr 26, 2023

Conversation

zurfyx
Copy link
Member

@zurfyx zurfyx commented Apr 22, 2023

Need a proper review on this one...

When the sharedHistory is a shared across parent/nested editors, you can reach a state where the HistoryEntry contains an EditorState and a undoSelection that do not point to the editor. This will always cause the editor to crash as it won't be able to find the node to select.

For #4389 in particular it seems like this can ocurr when the you previously had a selection on another editor and you then create the first change onto the new editor. The first change will carry the editor and editorEditor from the previous parent iteration and the selection will be previousSelection of the nestedEditor, regardless of what the selection of the nestedEditor is it will never be the selection of the (parent) editor

What I'm suggesting in this PR is to remove any chances that it can be out-of-sync, by relying on the selection in the same EditorState.

But I'm not sure why undoSelection was created in the first place so I might be missing something important.

Fixes #4389

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 22, 2023
@vercel
Copy link

vercel bot commented Apr 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview Apr 22, 2023 1:12am
lexical-playground ✅ Ready (Inspect) Visit Preview Apr 22, 2023 1:12am

@github-actions
Copy link

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 26.94 KB (0%) 539 ms (0%) 155 ms (-7.53% 🔽) 694 ms
packages/lexical-rich-text/dist/LexicalRichText.js 37.8 KB (0%) 757 ms (0%) 383 ms (+27.41% 🔺) 1.2 s
packages/lexical-plain-text/dist/LexicalPlainText.js 37.78 KB (0%) 756 ms (0%) 327 ms (+46.42% 🔺) 1.1 s

@fantactuka
Copy link
Contributor

See #966 summary recording for a case where undo selection was added. Although it seems to work without undoSelection now, probably addressed with a better calculation of history push vs history merge cases

@zurfyx
Copy link
Member Author

zurfyx commented Apr 24, 2023

See #966 summary recording for a case where undo selection was added. Although it seems to work without undoSelection now, probably addressed with a better calculation of history push vs history merge cases

@fantactuka - I can repro the same behavior as the test plan without it

image

Mind sharing more details on why we need undoSelection please?

@fantactuka
Copy link
Contributor

Mind sharing more details on why we need undoSelection please?

IIRC it was specifically added for cases when few operations coalesced including caret move, and during undo caret position supposed to be in (N-1). That's all I can remember :) I suppose it has been fixed with history merge algo adjustments for selection-change cases, so as long as it works as in test plan above I'm more than happe to remove undoSelection

@zurfyx
Copy link
Member Author

zurfyx commented Apr 25, 2023

Mind sharing more details on why we need undoSelection please?

IIRC it was specifically added for cases when few operations coalesced including caret move, and during undo caret position supposed to be in (N-1). That's all I can remember :) I suppose it has been fixed with history merge algo adjustments for selection-change cases, so as long as it works as in test plan above I'm more than happe to remove undoSelection

That's how it works now which behavior is identical to the previous version. While arguably there's undo nits that don't work like other browsers (line new line) the behavior is the same.

Screen.Recording.2023-04-25.at.10.14.03.AM.mov

@zurfyx zurfyx merged commit 28fec90 into main Apr 26, 2023
40 of 45 checks passed
This was referenced May 23, 2023
@fantactuka fantactuka deleted the undo-selection branch July 6, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undo crashes when Nested editor is empty upon initialization
3 participants