-
Notifications
You must be signed in to change notification settings - Fork 40
Nodes are removed from their old parent when they are appended to new parent #1141
Conversation
…ed to new parent.
Needed to fix a test in typing ckeditor/ckeditor5-typing#121 |
Tests: Test fix connected with changes in the engine (ckeditor/ckeditor5-engine#1141).
src/dev-utils/view.js
Outdated
@@ -852,7 +852,7 @@ function _convertViewElements( rootNode ) { | |||
const convertedElement = rootNode.is( 'documentFragment' ) ? new ViewDocumentFragment() : _convertElement( rootNode ); | |||
|
|||
// Convert all child nodes. | |||
for ( const child of rootNode.getChildren() ) { | |||
for ( const child of [ ...rootNode.getChildren() ] ) { |
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.
COMMMWENTR~!!#@
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.
Why so serious?
// Convert to array because it is correct to do so.
:)
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.
Fixed
@@ -235,6 +235,11 @@ export default class DocumentFragment { | |||
nodes = normalize( nodes ); | |||
|
|||
for ( const node of nodes ) { | |||
// If node that is being added to this element is already inside another element, first remove it from the old parent. | |||
if ( node.parent !== null ) { |
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'm hesitating whether I'd rather like if ( node.parent )
or !== null
;| Dunno
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.
!== null
is easier to find when searching codebase
What's up with this PR? |
W8ing for me having time to check it. |
Suggested merge commit message (convention)
Fix:
view
andmodel
nodes will now be removed from their old parent when they are added to a new parent to prevent having same node on multiple elements' children lists. Closes ckeditor/ckeditor5#4182.BREAKING CHANGE: If you will append
view
ormodel
node that already has a parent, that node will be first removed from it's parent. This is important 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.