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

Inserting node into some element should remove it from its parent #4125

Closed
Reinmar opened this issue Jul 21, 2017 · 5 comments
Closed

Inserting node into some element should remove it from its parent #4125

Reinmar opened this issue Jul 21, 2017 · 5 comments
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jul 21, 2017

I wonder how many times we do this:

someElement.appendChild( someTree.getChild( 0 ) );

Every time we move a node we may be doing exactly this. And this means that such a node ends up in being someTree's and someElement's child.

Fortunately, in the view we bubble events using the parent chain so the events bubble to the new tree. But it may lead to issues anyway, especially in the tests.

E.g. I've just seen a code which did this:

const el = somElement.getChild( 0 );

new ViewElement( 'foo', null, [ el ] );

Which has exactly the same meaning as the move operation above. I don't want to be the first person who will spend a day debugging an issue caused by a node being in two places so I think we should prevent it.

@Reinmar
Copy link
Member Author

Reinmar commented Jul 21, 2017

Actually, it'll be easy to check how often we do such operations by checking the parent of node passed to insertChildren() :D

@scofalik
Copy link
Contributor

I think it won't be as many as you think because those bugs often show up pretty fast. But I agree for 100% that this should be changed. I myself did this kind of error twice already.

@pjasiun
Copy link

pjasiun commented Jul 24, 2017

I agree that appendChild should remove them from the previous parent. In the model, https://github.com/ckeditor/ckeditor5-engine/issues/858 will (should) solver it. However, we should solve it in the view too.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 12, 2017

@Reinmar
Copy link
Member Author

Reinmar commented Sep 15, 2017

@Reinmar Reinmar closed this as completed Sep 15, 2017
@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:model 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

No branches or pull requests

4 participants