-
Notifications
You must be signed in to change notification settings - Fork 40
Unit test covering edge case related to 'withChildren' parameter inside Renderer added #1455
Conversation
|
||
expect( domRoot.innerHTML ).to.equal( '<ul><li>Foo</li><li><b>Bar</b></li></ul>' ); | ||
|
||
const viewLi = view.getChild( 0 ); |
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.
viewLi
of viewUl
?
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.
Uh, view
== <ul>
. Sorry.
expect( domRoot.innerHTML ).to.equal( '<ul><li>Foo</li><li><b>Bar</b></li></ul>' ); | ||
|
||
const viewLi = view.getChild( 0 ); | ||
const viewLiIndented = view._removeChildren( 1, 1 ); // Array with one element. |
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 don't understand what this line does. Which <li>
does it remove? There's no indented <li>
at this stage.
|
||
const viewLi = view.getChild( 0 ); | ||
const viewLiIndented = view._removeChildren( 1, 1 ); // Array with one element. | ||
viewLiIndented[ 0 ]._appendChild( parse( '<attribute:i>Baz</attribute:i>' ) ); |
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.
Isn't the problem here that we use appendChild()
on a detached node? That breaks automatic markToSync()
calls, am I right?
I have a bad feeling about this – should renderer handle such cases? Shouldn't it be able to assume that markToSync
is called for all nodes which are finally in the tree? That seems much easier than having markToSync
used for half of the nodes in the tree.
cc @pjasiun
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.
Yes, but it is View
class who binds the view structure and the renderer. I think that render should be tested together with the view and even with the view writer. Integration tests FTW! ;)
@@ -1683,6 +1683,36 @@ describe( 'Renderer', () => { | |||
); | |||
} ); | |||
|
|||
// #1451 | |||
it( 'should correctly render changed children even if direct parent is not marked to sync', () => { |
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.
This test is a bit too hard to read. Better variable naming + a comment what's happening inside it would help.
9fb3384
to
fde5971
Compare
I extracted the issue I found to https://github.com/ckeditor/ckeditor5-engine/issues/1456. Let's fix CC here and move forward because what we found is that we need to redesign this part of the engine :D |
Suggested merge commit message (convention)
Internal: Added
withChildren: false
parameter missing after refactoring and unit test covering that case. Closes ckeditor/ckeditor5#4370. Closes #1452.Additional information
What happens depending on
withChildren
value is described in more details in https://github.com/ckeditor/ckeditor5-engine/issues/1451#issuecomment-401356437. Added unit test shows the situation in whichwithChildren: true
behaviour results in incorrect DOM structure after rendering.I have added
Closes ckeditor/ckeditor5#4353
as #1452 is a previous PR related to this topic, so if we decided to merge this one, #1452 should be closed.