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

Children nodes are inserted to new parent without first removing them from their old parent #4182

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

Comments

@scofalik
Copy link
Contributor

I spotted a bug that crashed editor during rendering. It appeared that Renderer wants to re-render a part of a tree that is detached from main tree. What was weird about that tree was that detached-root had a child but that child's parent property was set to null. Similarly, that child also had a child which also had parent set to null.

After debugging it appeared that in view.Writer some nodes were supposed to be moved from one node to another, however those nodes were not removed from their original parent before appending to new parent, which caused this bug.

@scofalik scofalik self-assigned this Sep 11, 2017
@scofalik
Copy link
Contributor Author

While I am at it, I'll check the whole code for appendChildren and insertChildren usage and will fix it where needed.

@Reinmar
Copy link
Member

Reinmar commented Sep 12, 2017

@scofalik
Copy link
Contributor Author

scofalik commented Sep 12, 2017

Yes, it is.

Alternatively, we can solve it by adding more logic in appendChildren/insertChildren.

I can change the solution.

@Reinmar
Copy link
Member

Reinmar commented Sep 12, 2017

I'm for more logic inside appendChildren() (or whichever is the lowest level method).

@Reinmar
Copy link
Member

Reinmar commented Sep 12, 2017

PJ is too:

I agree that appendChild should remove them from the previous parent. In the model, #4009 will (should) solver it. However, we should solve it in the view too.

Reinmar referenced this issue in ckeditor/ckeditor5-engine Sep 15, 2017
Fix: View and model nodes will now be removed from their old parents when they are added to a new parent to prevent having same node on multiple elements' children lists. Closes #1139.

BREAKING CHANGE: View and model nodes are now automatically removed from their old parents when they are inserted into new elements. This is important e.g. if you iterate through element's children and they are moved during that iteration. In that case, it's safest to cache the element's children in an array.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 12 milestone Oct 9, 2019
@mlewand mlewand added module:view 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