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

Reversed ReinsertOperation should remove nodes back to the same graveyard holder #4029

Closed
scofalik opened this issue Mar 27, 2017 · 0 comments · Fixed by ckeditor/ckeditor5-engine#892
Assignees
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@scofalik
Copy link
Contributor

Whenever new RemoveOperation is created, $graveyard root is checked for how many $graveyardHolder nodes it has and the target of RemoveOperation is set to the new $graveyardHolder that will be inserted during operation execution at the end of $graveyard.

Simple example of situation when editor throws. Consider a batch that removes a character and removes bold from it afterwards. It's based on real scenario where post fixer callback fixes model and adds it's changes to the same batch (original issue: https://github.com/ckeditor/ckeditor5-list/issues/51):

  1. REMOVE from $root [ 0, 1 ], 1 node, to $graveyard [ 0, 0 ].
  2. REMOVE ATTRIBUTE "bold" from $graveyard [ 0, 0 ] - [ 0, 1 ].

When it is undone, following batch is generated:

  1. SET ATTRIBUTE "bold"=true on $graveyard [ 0, 0 ] - [ 0, 1 ].
  2. REINSERT from $graveyard [ 0, 0 ], 1 node, to $root [ 0, 1 ].

Now redo:

  1. REMOVE from $root [ 0, 1 ], 1 node, to $graveyard [ 1, 0 ] (new remove operation, creates new graveyard holder).
  2. REMOVE ATTRIBUTE "bold" from $graveyard [ 0, 0 ] - [ 0, 1 ]. <-- this throws, as there are no nodes in [ 0, 0 ].

It makes sense that RemoveOperation created from ReinsertOperation reversing should put nodes back in their original place. The only scenario where ReinsertOperation is reversed is redo.

@scofalik scofalik self-assigned this Mar 27, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-engine Mar 28, 2017
Fix: Reversed `ReinsertOperation` targets back to same graveyard holder from which the nodes were re-inserted. Closes #891.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 9 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants