Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Unit test covering edge case related to 'withChildren' parameter inside Renderer added #1455

Merged
merged 2 commits into from
Jul 4, 2018
Merged

Unit test covering edge case related to 'withChildren' parameter inside Renderer added #1455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
165c22a
Restored 'withChildren: false' param inside the 'renderer._updateChil…
f1ames Jul 2, 2018
fde5971
Tests: unit test for 'DomConverter' modyfing DOM structure.
f1ames Jul 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/view/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ export default class Renderer {

const actualDomChildren = this.domConverter.mapViewToDom( viewElement ).childNodes;
const expectedDomChildren = Array.from(
this.domConverter.viewChildrenToDom( viewElement, domElement.ownerDocument )
this.domConverter.viewChildrenToDom( viewElement, domElement.ownerDocument, { withChildren: false } )
);
const diff = this._diffNodeLists( actualDomChildren, expectedDomChildren );
const actions = this._findReplaceActions( diff, actualDomChildren, expectedDomChildren );
Expand Down
30 changes: 30 additions & 0 deletions tests/view/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1683,6 +1683,36 @@ describe( 'Renderer', () => {
);
} );

// #1451
it( 'should correctly render changed children even if direct parent is not marked to sync', () => {
Copy link
Member

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.

const inputView =
'<container:ul>' +
'<container:li>Foo</container:li>' +
'<container:li><attribute:b>Bar</attribute:b></container:li>' +
'</container:ul>';

const view = parse( inputView );

viewRoot._appendChild( view );

renderer.markToSync( 'children', viewRoot );
renderer.render();

expect( domRoot.innerHTML ).to.equal( '<ul><li>Foo</li><li><b>Bar</b></li></ul>' );

const viewLi = view.getChild( 0 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

viewLi of viewUl?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, view == <ul>. Sorry.

const viewLiIndented = view._removeChildren( 1, 1 ); // Array with one element.
Copy link
Member

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.

viewLiIndented[ 0 ]._appendChild( parse( '<attribute:i>Baz</attribute:i>' ) );
Copy link
Member

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

Copy link

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! ;)

const viewUl = new ViewContainerElement( 'ul', null, viewLiIndented );
viewLi._appendChild( viewUl );

renderer.markToSync( 'children', view );
renderer.markToSync( 'children', viewLi );
renderer.render();

expect( domRoot.innerHTML ).to.equal( '<ul><li>Foo<ul><li><b>Bar</b><i>Baz</i></li></ul></li></ul>' );
} );

describe( 'fake selection', () => {
beforeEach( () => {
const { view: viewP, selection: newSelection } = parse(
Expand Down