From c457a1bf6be92647ee18ef1950664b943384ee57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 28 May 2018 12:28:41 +0200 Subject: [PATCH 01/21] Avoid unnecessary structure updates in renderer with smarter diffing. --- src/view/renderer.js | 258 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 220 insertions(+), 38 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index ecbc5c153..243b27fa5 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -19,6 +19,7 @@ import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import isText from '@ckeditor/ckeditor5-utils/src/dom/istext'; import fastDiff from '@ckeditor/ckeditor5-utils/src/fastdiff'; +import isNode from '@ckeditor/ckeditor5-utils/src/dom/isnode'; /** * Renderer updates DOM structure and selection, to make them a reflection of the view structure and selection. @@ -180,6 +181,16 @@ export default class Renderer { render() { let inlineFillerPosition; + if ( this.markedChildren.size > 1 ) { + // Sort `this.markedChildren` by the nesting level. + this.markedChildren = this._sortElementsByNestingLevel( this.markedChildren ); + } + + // Refresh mappings. + for ( const element of this.markedChildren ) { + this._updateChildrenMappings( element ); + } + // There was inline filler rendered in the DOM but it's not // at the selection position any more, so we can remove it // (cause even if it's needed, it must be placed in another location). @@ -243,6 +254,85 @@ export default class Renderer { this.markedChildren.clear(); } + /** + * Sorts elements set based on their nesting. The outermost elements are placed first. + * It check only for 3 element types: `containerElement`, `attributeElement` and `text`. Those elements + * are sorted in such order (from `containerElement` to `text`). Any other type (e.g. `rootElement` or + * `documentFragment`) have higher priority and is placed higher in the sorted set. + * Additionally if elements are of the same type, they are both checked if one is another parent so proper + * order can be established (parent first). + * + * @private + * @param {Set.} elements Elements to be sorted. + * @returns {Set.} Sorted elements. + */ + _sortElementsByNestingLevel( elements ) { + function getPriority( node ) { + let priority = 3; + if ( node.is( 'containerElement' ) ) { + priority = 2; + } else if ( 'attributeElement' ) { + priority = 1; + } else if ( 'text' ) { + priority = 0; + } + return priority; + } + + const elementsArray = Array.from( elements ); + elementsArray.sort( ( node1, node2 ) => { + let priority = getPriority( node2 ) - getPriority( node1 ); + if ( priority === 0 && !node1.is( 'text' ) ) { + if ( node1.parent === node2 ) { + priority = 1; + } else if ( node2.parent === node1 ) { + priority = -1; + } + } + return priority; + } ); + + return new Set( elementsArray ); + } + + /** + * Updates element children mappings. Children which were replaced in the view structure by the similar + * element (same tag name) are treated as 'replaced'. Their mappings are rebind to the corresponding, + * existing DOM element so they will not be replaced by new DOM element during rerendering. + * + * @private + * @param {module:engine/view/node~Node} viewElement View element which children mappings will be updated. + */ + _updateChildrenMappings( viewElement ) { + const diff = this._diffElementChildren( viewElement ); + + if ( diff ) { + const actions = this._findReplaceActions( diff.actions, diff.actualDomChildren, diff.expectedDomChildren ); + + if ( actions.indexOf( 'replace' ) !== -1 ) { + const counter = { equal: 0, insert: 0, delete: 0 }; + for ( const action of actions ) { + if ( action === 'replace' ) { + const insertIndex = counter.equal + counter.insert; + const deleteIndex = counter.equal + counter.delete; + const viewChild = viewElement.getChild( insertIndex ); + if ( viewChild ) { + this.domConverter.unbindDomElement( diff.actualDomChildren[ deleteIndex ] ); + this.domConverter.bindElements( diff.actualDomChildren[ deleteIndex ], viewChild ); + + // View element may have children which needs to be updated but are not marked, mark them to update. + this.markedChildren.add( viewChild ); + } + remove( diff.expectedDomChildren[ insertIndex ] ); + counter.equal++; + } else { + counter[ action ]++; + } + } + } + } + } + /** * Adds inline filler at given position. * @@ -473,19 +563,91 @@ export default class Renderer { * filler should be rendered. */ _updateChildren( viewElement, options ) { + const diff = this._diffElementChildren( viewElement, { inlineFillerPosition: options.inlineFillerPosition, bind: true } ); + + if ( diff ) { + const actions = diff.actions; + const domElement = diff.domElement; + const actualDomChildren = diff.actualDomChildren; + const expectedDomChildren = diff.expectedDomChildren; + + let i = 0; + const nodesToUnbind = new Set(); + for ( const action of actions ) { + if ( action === 'insert' ) { + insertAt( domElement, i, expectedDomChildren[ i ] ); + i++; + } else if ( action === 'delete' ) { + nodesToUnbind.add( actualDomChildren[ i ] ); + remove( actualDomChildren[ i ] ); + } else { // 'equal' + // Force updating text nodes inside elements which did not change and do not need to be re-rendered (#1125). + this._markDescendantTextToSync( this.domConverter.domToView( expectedDomChildren[ i ] ) ); + i++; + } + } + + // Unbind removed nodes. When node does not have a parent it means that it was removed from DOM tree during + // comparision with the expected DOM. We don't need to check child nodes, because if child node was reinserted, + // it was moved to DOM tree out of the removed node. + for ( const node of nodesToUnbind ) { + if ( !node.parentNode ) { + this.domConverter.unbindDomElement( node ); + } + } + } + } + + /** + * Compares element actual and expected children and finds list of actions which can be used to transform + * actual children to expected ones. + * + * @private + * @param viewElement + * @param {Object} options + * @param {module:engine/view/position~Position} options.inlineFillerPosition The position on which the inline + * filler should be rendered. + * @param {Boolean} options.bind If new view elements should be bind to their corresponding DOM elements. + * @returns {Object|null} result + * @returns {Array} result.actions List of actions based on {@link module:utils/diff~diff} function. + * @returns {Node} result.domElement ViewElement corresponding DOM element. + * @returns {Array} result.actualDomChildren Current viewElement DOM children. + * @returns {Array} result.expectedDomChildren Expected viewElement DOM children. + * + */ + _diffElementChildren( viewElement, options = {} ) { const domConverter = this.domConverter; const domElement = domConverter.mapViewToDom( viewElement ); if ( !domElement ) { // If there is no `domElement` it means that it was already removed from DOM. // There is no need to update it. It will be updated when re-inserted. - return; + return null; + } + + function sameNodes( actualDomChild, expectedDomChild ) { + // Elements. + if ( actualDomChild === expectedDomChild ) { + return true; + } + // Texts. + else if ( isText( actualDomChild ) && isText( expectedDomChild ) ) { + return actualDomChild.data === expectedDomChild.data; + } + // Block fillers. + else if ( isBlockFiller( actualDomChild, domConverter.blockFiller ) && + isBlockFiller( expectedDomChild, domConverter.blockFiller ) ) { + return true; + } + + // Not matching types. + return false; } const domDocument = domElement.ownerDocument; const filler = options.inlineFillerPosition; const actualDomChildren = domElement.childNodes; - const expectedDomChildren = Array.from( domConverter.viewChildrenToDom( viewElement, domDocument, { bind: true } ) ); + const expectedDomChildren = Array.from( domConverter.viewChildrenToDom( viewElement, domDocument, { bind: options.bind } ) ); // Inline filler element has to be created during children update because we need it to diff actual dom // elements with expected dom elements. We need inline filler in expected dom elements so we won't re-render @@ -494,52 +656,72 @@ export default class Renderer { this._addInlineFiller( domDocument, expectedDomChildren, filler.offset ); } - const actions = diff( actualDomChildren, expectedDomChildren, sameNodes ); + return { + actions: diff( actualDomChildren, expectedDomChildren, sameNodes ), + domElement, + actualDomChildren, + expectedDomChildren + }; + } + + /** + * Finds DOM nodes which were replaced with the similar nodes (same tag name) in the `insert`/`delete` + * action groups (based on actual and expected DOM). For example: + * + * Actual DOM:

FooBarBazBax

+ * Expected DOM:

Bar123Baz456

+ * Input actions: [ insert, insert, delete, delete, equal, insert, delete ] + * Output actions: [ insert, replace, delete, equal, replace ] + * + * @private + * @param {Array} actions Actions array which is result of {@link module:utils/diff~diff} function. + * @param {Array} actualDom Actual DOM children + * @param {Array} expectedDom Expected DOM children. + * @returns {Array} Actions array modified with `replace` actions. + */ + _findReplaceActions( actions, actualDom, expectedDom ) { + // If there is no both 'insert' and 'delete' actions, no need to check for replaced elements. + if ( actions.indexOf( 'insert' ) === -1 || actions.indexOf( 'delete' ) === -1 ) { + return actions; + } + + function areSimilar( domNode1, domNode2 ) { + return isNode( domNode1 ) && isNode( domNode2 ) && + !isText( domNode1 ) && !isText( domNode2 ) && + domNode1.tagName.toLowerCase() === domNode2.tagName.toLowerCase(); + } + + function calculateReplaceActions( actual, expected ) { + return diff( actual, expected, areSimilar ).map( x => x === 'equal' ? 'replace' : x ); + } - let i = 0; - const nodesToUnbind = new Set(); + let newActions = []; + let actualSlice = []; + let expectedSlice = []; + const counter = { equal: 0, insert: 0, delete: 0 }; for ( const action of actions ) { if ( action === 'insert' ) { - insertAt( domElement, i, expectedDomChildren[ i ] ); - i++; + expectedSlice.push( expectedDom[ counter.equal + counter.insert ] ); } else if ( action === 'delete' ) { - nodesToUnbind.add( actualDomChildren[ i ] ); - remove( actualDomChildren[ i ] ); - } else { // 'equal' - // Force updating text nodes inside elements which did not change and do not need to be re-rendered (#1125). - this._markDescendantTextToSync( domConverter.domToView( expectedDomChildren[ i ] ) ); - i++; + actualSlice.push( actualDom[ counter.equal + counter.delete ] ); + } else { // equal + if ( expectedSlice.length && actualSlice.length ) { + newActions = newActions.concat( calculateReplaceActions( actualSlice, expectedSlice ) ); + } + newActions.push( 'equal' ); + // Reset stored elements on 'equal'. + actualSlice = []; + expectedSlice = []; } + counter[ action ]++; } - // Unbind removed nodes. When node does not have a parent it means that it was removed from DOM tree during - // comparision with the expected DOM. We don't need to check child nodes, because if child node was reinserted, - // it was moved to DOM tree out of the removed node. - for ( const node of nodesToUnbind ) { - if ( !node.parentNode ) { - this.domConverter.unbindDomElement( node ); - } + if ( expectedSlice.length && actualSlice.length ) { + newActions = newActions.concat( calculateReplaceActions( actualSlice, expectedSlice ) ); } - function sameNodes( actualDomChild, expectedDomChild ) { - // Elements. - if ( actualDomChild === expectedDomChild ) { - return true; - } - // Texts. - else if ( isText( actualDomChild ) && isText( expectedDomChild ) ) { - return actualDomChild.data === expectedDomChild.data; - } - // Block fillers. - else if ( isBlockFiller( actualDomChild, domConverter.blockFiller ) && - isBlockFiller( expectedDomChild, domConverter.blockFiller ) ) { - return true; - } - - // Not matching types. - return false; - } + return newActions; } /** From 37f745289e570faba72ece0f0369186e0c74c7ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 28 May 2018 14:14:53 +0200 Subject: [PATCH 02/21] Rendering flow adjustments. --- src/view/renderer.js | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index 243b27fa5..547d202d7 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -256,8 +256,8 @@ export default class Renderer { /** * Sorts elements set based on their nesting. The outermost elements are placed first. - * It check only for 3 element types: `containerElement`, `attributeElement` and `text`. Those elements - * are sorted in such order (from `containerElement` to `text`). Any other type (e.g. `rootElement` or + * It check only for 2 element types: `containerElement` and `attributeElement`. Those elements + * are sorted in such order (from `containerElement` to `attributeElement`). Any other type (e.g. `rootElement` or * `documentFragment`) have higher priority and is placed higher in the sorted set. * Additionally if elements are of the same type, they are both checked if one is another parent so proper * order can be established (parent first). @@ -268,12 +268,10 @@ export default class Renderer { */ _sortElementsByNestingLevel( elements ) { function getPriority( node ) { - let priority = 3; + let priority = 2; if ( node.is( 'containerElement' ) ) { - priority = 2; - } else if ( 'attributeElement' ) { priority = 1; - } else if ( 'text' ) { + } else if ( node.is( 'attributeElement' ) ) { priority = 0; } return priority; @@ -282,7 +280,7 @@ export default class Renderer { const elementsArray = Array.from( elements ); elementsArray.sort( ( node1, node2 ) => { let priority = getPriority( node2 ) - getPriority( node1 ); - if ( priority === 0 && !node1.is( 'text' ) ) { + if ( priority === 0 ) { if ( node1.parent === node2 ) { priority = 1; } else if ( node2.parent === node1 ) { @@ -304,7 +302,8 @@ export default class Renderer { * @param {module:engine/view/node~Node} viewElement View element which children mappings will be updated. */ _updateChildrenMappings( viewElement ) { - const diff = this._diffElementChildren( viewElement ); + // We do not perform any operations on DOM here so there is no need to bind view element or convert its' children. + const diff = this._diffElementChildren( viewElement, { bind: false, withChildren: false } ); if ( diff ) { const actions = this._findReplaceActions( diff.actions, diff.actualDomChildren, diff.expectedDomChildren ); @@ -316,13 +315,12 @@ export default class Renderer { const insertIndex = counter.equal + counter.insert; const deleteIndex = counter.equal + counter.delete; const viewChild = viewElement.getChild( insertIndex ); - if ( viewChild ) { - this.domConverter.unbindDomElement( diff.actualDomChildren[ deleteIndex ] ); - this.domConverter.bindElements( diff.actualDomChildren[ deleteIndex ], viewChild ); - // View element may have children which needs to be updated but are not marked, mark them to update. - this.markedChildren.add( viewChild ); - } + this.domConverter.unbindDomElement( diff.actualDomChildren[ deleteIndex ] ); + this.domConverter.bindElements( diff.actualDomChildren[ deleteIndex ], viewChild ); + // View element may have children which needs to be updated but are not marked, mark them to update. + this.markedChildren.add( viewChild ); + remove( diff.expectedDomChildren[ insertIndex ] ); counter.equal++; } else { @@ -563,7 +561,8 @@ export default class Renderer { * filler should be rendered. */ _updateChildren( viewElement, options ) { - const diff = this._diffElementChildren( viewElement, { inlineFillerPosition: options.inlineFillerPosition, bind: true } ); + const diff = this._diffElementChildren( viewElement, + { inlineFillerPosition: options.inlineFillerPosition, bind: true, withChildren: true } ); if ( diff ) { const actions = diff.actions; @@ -608,6 +607,7 @@ export default class Renderer { * @param {module:engine/view/position~Position} options.inlineFillerPosition The position on which the inline * filler should be rendered. * @param {Boolean} options.bind If new view elements should be bind to their corresponding DOM elements. + * @param {Boolean} options.withChildren If children of newly bound view elements should also be converted. * @returns {Object|null} result * @returns {Array} result.actions List of actions based on {@link module:utils/diff~diff} function. * @returns {Node} result.domElement ViewElement corresponding DOM element. @@ -615,7 +615,7 @@ export default class Renderer { * @returns {Array} result.expectedDomChildren Expected viewElement DOM children. * */ - _diffElementChildren( viewElement, options = {} ) { + _diffElementChildren( viewElement, options ) { const domConverter = this.domConverter; const domElement = domConverter.mapViewToDom( viewElement ); @@ -647,7 +647,7 @@ export default class Renderer { const domDocument = domElement.ownerDocument; const filler = options.inlineFillerPosition; const actualDomChildren = domElement.childNodes; - const expectedDomChildren = Array.from( domConverter.viewChildrenToDom( viewElement, domDocument, { bind: options.bind } ) ); + const expectedDomChildren = Array.from( domConverter.viewChildrenToDom( viewElement, domDocument, options ) ); // Inline filler element has to be created during children update because we need it to diff actual dom // elements with expected dom elements. We need inline filler in expected dom elements so we won't re-render From 450e1ee288ade49fc5f1904010d1cb8dda0b7488 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 28 May 2018 15:02:18 +0200 Subject: [PATCH 03/21] Tests: optimal rendering unit tests. --- tests/view/renderer.js | 422 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 422 insertions(+) diff --git a/tests/view/renderer.js b/tests/view/renderer.js index 49ea9db05..7d1ef6c11 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -2182,6 +2182,428 @@ describe( 'Renderer', () => { expect( selectionExtendSpy.notCalled ).to.true; } ); } ); + + // #1417 + describe( 'optimal rendering', () => { + it( 'should render inline element replacement (before text)', () => { + viewRoot._appendChild( parse( 'A1' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

A1

' ); + + const viewP = viewRoot.getChild( 0 ); + viewP._removeChildren( 0, 2 ); + viewP._insertChild( 0, parse( 'B2' ) ); + + const domI = domRoot.childNodes[ 0 ].childNodes[ 0 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewP ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

B2

' ); + expect( domI ).to.equal( domRoot.childNodes[ 0 ].childNodes[ 0 ] ); + } ); + + it( 'should render inline element replacement (after text)', () => { + viewRoot._appendChild( parse( '1A' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

1A

' ); + + const viewP = viewRoot.getChild( 0 ); + viewP._removeChildren( 0, 2 ); + viewP._insertChild( 0, parse( '2B' ) ); + + const domI = domRoot.childNodes[ 0 ].childNodes[ 1 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewP ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

2B

' ); + expect( domI ).to.equal( domRoot.childNodes[ 0 ].childNodes[ 1 ] ); + } ); + + it( 'should render inline element replacement (before text swapped order)', () => { + viewRoot._appendChild( parse( 'A1' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

A1

' ); + + const viewP = viewRoot.getChild( 0 ); + viewP._removeChildren( 0, 2 ); + viewP._insertChild( 0, parse( '2B' ) ); + + const domI = domRoot.childNodes[ 0 ].childNodes[ 0 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewP ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

2B

' ); + expect( domI ).to.equal( domRoot.childNodes[ 0 ].childNodes[ 1 ] ); + } ); + + it( 'should render inline element replacement (after text swapped order)', () => { + viewRoot._appendChild( parse( '1A' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

1A

' ); + + const viewP = viewRoot.getChild( 0 ); + viewP._removeChildren( 0, 2 ); + viewP._insertChild( 0, parse( 'B2' ) ); + + const domI = domRoot.childNodes[ 0 ].childNodes[ 1 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewP ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

B2

' ); + expect( domI ).to.equal( domRoot.childNodes[ 0 ].childNodes[ 0 ] ); + } ); + + it( 'should render single replacement in p group', () => { + const content = '' + + '1' + + '2' + + '3'; + + viewRoot._appendChild( parse( content ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

1

2

3

' ); + + viewRoot._removeChildren( 1 ); + viewRoot._insertChild( 1, parse( '4' ) ); + + const domP = domRoot.childNodes[ 1 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

1

4

3

' ); + expect( domP ).to.equal( domRoot.childNodes[ 1 ] ); + } ); + + it( 'should render replacement and insertion in p group', () => { + const content = '' + + '1A' + + '2B' + + '3C'; + + const replacement = '' + + 'D' + + '5E'; + + viewRoot._appendChild( parse( content ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

1A

2B

3C

' ); + + viewRoot._removeChildren( 1 ); + viewRoot._insertChild( 1, parse( replacement ) ); + + const domP2 = domRoot.childNodes[ 1 ]; + const domP3 = domRoot.childNodes[ 2 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

1A

D

5E

3C

' ); + expect( domP2 ).to.equal( domRoot.childNodes[ 1 ] ); + expect( domP3 ).to.equal( domRoot.childNodes[ 3 ] ); + } ); + + it( 'should render replacement and deletion in p group', () => { + const content = '' + + 'A1' + + 'B2' + + 'C3'; + + viewRoot._appendChild( parse( content ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

A1

B2

C3

' ); + + viewRoot._removeChildren( 0, 2 ); + viewRoot._insertChild( 0, parse( '4' ) ); + + const domP0 = domRoot.childNodes[ 0 ]; + const domP2 = domRoot.childNodes[ 2 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

4

C3

' ); + expect( domP0 ).to.equal( domRoot.childNodes[ 0 ] ); + expect( domP2 ).to.equal( domRoot.childNodes[ 1 ] ); + } ); + + it( 'should render multiple continuous replacement in p group', () => { + const content = '' + + '1' + + '2' + + '3' + + '4' + + '5'; + + viewRoot._appendChild( parse( content ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

1

2

3

4

5

' ); + + viewRoot._removeChildren( 0, 3 ); + viewRoot._insertChild( 0, parse( '6A7' ) ); + + const domP1 = domRoot.childNodes[ 0 ]; + const domP2 = domRoot.childNodes[ 1 ]; + const domP4 = domRoot.childNodes[ 3 ]; + const domP5 = domRoot.childNodes[ 4 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

6A

7

4

5

' ); + expect( domP1 ).to.equal( domRoot.childNodes[ 0 ] ); + expect( domP2 ).to.equal( domRoot.childNodes[ 1 ] ); + expect( domP4 ).to.equal( domRoot.childNodes[ 2 ] ); + expect( domP5 ).to.equal( domRoot.childNodes[ 3 ] ); + } ); + + it( 'should render multiple replacement in p group', () => { + const content = '' + + '1' + + '2' + + '3' + + '4' + + '5'; + + viewRoot._appendChild( parse( content ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

1

2

3

4

5

' ); + + viewRoot._removeChildren( 4 ); + viewRoot._removeChildren( 1, 2 ); + viewRoot._insertChild( 2, parse( '6' ) ); + viewRoot._insertChild( 1, parse( 'A7' ) ); + + const domP1 = domRoot.childNodes[ 0 ]; + const domP2 = domRoot.childNodes[ 1 ]; + const domP4 = domRoot.childNodes[ 3 ]; + const domP5 = domRoot.childNodes[ 4 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

1

A7

4

6

' ); + expect( domP1 ).to.equal( domRoot.childNodes[ 0 ] ); + expect( domP2 ).to.equal( domRoot.childNodes[ 1 ] ); + expect( domP4 ).to.equal( domRoot.childNodes[ 2 ] ); + expect( domP5 ).to.equal( domRoot.childNodes[ 3 ] ); + } ); + + it( 'should not rerender DOM when view replaced with the same structure', () => { + const content = '' + + 'He' + + 'ading 1' + + '' + + 'Ph ' + + 'Bold' + + '' + + 'Link' + + '' + + '' + + '' + + '' + + 'Quoted item 1' + + '' + + ''; + + viewRoot._appendChild( parse( content ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

Heading 1

Ph Bold' + + 'Link

  • ' + + 'Quoted item 1
' ); + + viewRoot._removeChildren( 0, viewRoot.childCount ); + viewRoot._appendChild( parse( content ) ); + + const viewH = viewRoot.getChild( 0 ); + const viewP = viewRoot.getChild( 1 ); + const viewQ = viewRoot.getChild( 2 ); + + const domH = domRoot.childNodes[ 0 ]; + const domHI = domH.childNodes[ 1 ]; + const domP = domRoot.childNodes[ 1 ]; + const domPT = domP.childNodes[ 0 ]; + const domPABI = domP.childNodes[ 2 ].childNodes[ 0 ].childNodes[ 1 ]; + const domQ = domRoot.childNodes[ 2 ]; + const domQULB = domQ.childNodes[ 0 ].childNodes[ 0 ].childNodes[ 1 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + // Assert content. + expect( domRoot.innerHTML ).to.equal( '

Heading 1

Ph Bold' + + 'Link

  • ' + + 'Quoted item 1
' ); + + // Assert if DOM elements did not change. + expect( domRoot.childNodes[ 0 ] ).to.equal( domH ); + expect( domH.childNodes[ 1 ] ).to.equal( domHI ); + expect( domRoot.childNodes[ 1 ] ).to.equal( domP ); + expect( domP.childNodes[ 0 ] ).to.equal( domPT ); + expect( domP.childNodes[ 2 ].childNodes[ 0 ].childNodes[ 1 ] ).to.equal( domPABI ); + expect( domRoot.childNodes[ 2 ] ).to.equal( domQ ); + expect( domQ.childNodes[ 0 ].childNodes[ 0 ].childNodes[ 1 ] ).to.equal( domQULB ); + + // Assert mappings. + const mappings = renderer.domConverter._domToViewMapping; + expect( mappings.get( domH ) ).to.equal( viewH ); + expect( mappings.get( domHI ) ).to.equal( viewH.getChild( 1 ) ); + expect( mappings.get( domP ) ).to.equal( viewP ); + expect( mappings.get( domPABI ) ).to.equal( viewP.getChild( 2 ).getChild( 0 ).getChild( 1 ) ); + expect( mappings.get( domQ ) ).to.equal( viewQ ); + expect( mappings.get( domQULB ) ).to.equal( viewQ.getChild( 0 ).getChild( 0 ).getChild( 1 ) ); + } ); + + it( 'should not rerender DOM when view replaced with the same structure without first node', () => { + const content = '' + + 'He' + + 'ading 1' + + '' + + 'Ph ' + + 'Bold' + + '' + + 'Link' + + '' + + '' + + '' + + '' + + 'Quoted item 1' + + '' + + ''; + + const content2 = '' + + 'Ph ' + + 'Bold' + + '' + + 'Link' + + '' + + '' + + '' + + '' + + 'Quoted item 1' + + '' + + ''; + + viewRoot._appendChild( parse( content ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

Heading 1

Ph Bold' + + 'Link

  • ' + + 'Quoted item 1
' ); + + viewRoot._removeChildren( 0, viewRoot.childCount ); + viewRoot._appendChild( parse( content2 ) ); + + const viewP = viewRoot.getChild( 0 ); + const viewQ = viewRoot.getChild( 1 ); + + const domP = domRoot.childNodes[ 1 ]; + const domPT = domP.childNodes[ 0 ]; + const domPABI = domP.childNodes[ 2 ].childNodes[ 0 ].childNodes[ 1 ]; + const domQ = domRoot.childNodes[ 2 ]; + const domQULB = domQ.childNodes[ 0 ].childNodes[ 0 ].childNodes[ 1 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + // Assert content. + expect( domRoot.innerHTML ).to.equal( '

Ph Bold' + + 'Link

  • ' + + 'Quoted item 1
' ); + + // Assert if DOM elements did not change. + expect( domRoot.childNodes[ 0 ] ).to.equal( domP ); + expect( domP.childNodes[ 0 ] ).to.equal( domPT ); + expect( domP.childNodes[ 2 ].childNodes[ 0 ].childNodes[ 1 ] ).to.equal( domPABI ); + expect( domRoot.childNodes[ 1 ] ).to.equal( domQ ); + expect( domQ.childNodes[ 0 ].childNodes[ 0 ].childNodes[ 1 ] ).to.equal( domQULB ); + + // Assert mappings. + const mappings = renderer.domConverter._domToViewMapping; + expect( mappings.get( domP ) ).to.equal( viewP ); + expect( mappings.get( domPABI ) ).to.equal( viewP.getChild( 2 ).getChild( 0 ).getChild( 1 ) ); + expect( mappings.get( domQ ) ).to.equal( viewQ ); + expect( mappings.get( domQULB ) ).to.equal( viewQ.getChild( 0 ).getChild( 0 ).getChild( 1 ) ); + } ); + + it( 'should not rerender DOM when typing inside empty inline element', () => { + const view = parse( 'Foo Bar' ); + + viewRoot._appendChild( view ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

Foo Bar

' ); + + const viewP = viewRoot.getChild( 0 ); + viewP._removeChildren( 1 ); + viewP._insertChild( 1, parse( 'a' ) ); + + const domP = domRoot.childNodes[ 0 ]; + const domText = domP.childNodes[ 0 ]; + const domB = domP.childNodes[ 1 ]; + + domB.innerHTML = 'a'; + + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewRoot.getChild( 0 ) ); + renderer.render(); + + // Assert content. + expect( domRoot.innerHTML ).to.equal( '

Foo Bara

' ); + + // Assert if DOM elements did not change. + expect( domRoot.childNodes[ 0 ] ).to.equal( domP ); + expect( domRoot.childNodes[ 0 ].childNodes[ 0 ] ).to.equal( domText ); + expect( domRoot.childNodes[ 0 ].childNodes[ 1 ] ).to.equal( domB ); + + // Assert mappings. + const mappings = renderer.domConverter._domToViewMapping; + expect( mappings.get( domP ) ).to.equal( viewP ); + expect( mappings.get( domB ) ).to.equal( viewP.getChild( 1 ) ); + } ); + } ); } ); describe( '#922', () => { From 7cbfc8a911b159d73c4b723e440a016ef8550dc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 29 May 2018 17:07:01 +0200 Subject: [PATCH 04/21] Tests: complex DOM modifications rendering unit tests added. --- tests/view/renderer.js | 154 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) diff --git a/tests/view/renderer.js b/tests/view/renderer.js index 7d1ef6c11..ae65db69c 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -2603,6 +2603,160 @@ describe( 'Renderer', () => { expect( mappings.get( domP ) ).to.equal( viewP ); expect( mappings.get( domB ) ).to.equal( viewP.getChild( 1 ) ); } ); + + it( 'should handle complex view duplication', () => { + const content = '' + + '' + + '' + + 'Quoted item 1' + + 'Item 2' + + '' + + 'Link' + + '' + + '' + + ''; + + const expected = '' + + '
' + + '
    ' + + '
  • Quoted item 1
    • ' + + '
  • Item 2
    • ' + + '
  • Link
    • ' + + '
' + + '
'; + + viewRoot._appendChild( parse( content ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( expected ); + + viewRoot._removeChildren( 0, viewRoot.childCount ); + viewRoot._appendChild( parse( content + content ) ); + + const domBQ = domRoot.childNodes[ 0 ]; + const domUL = domBQ.childNodes[ 0 ]; + const domLI1 = domUL.childNodes[ 0 ]; + const domLI2 = domUL.childNodes[ 1 ]; + const domLI3 = domUL.childNodes[ 2 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + // Assert content. + expect( domRoot.innerHTML ).to.equal( expected + expected ); + + // Assert if DOM elements did not change. + expect( domRoot.childNodes[ 0 ] ).to.equal( domBQ ); + expect( domBQ.childNodes[ 0 ] ).to.equal( domUL ); + expect( domUL.childNodes[ 0 ] ).to.equal( domLI1 ); + expect( domUL.childNodes[ 1 ] ).to.equal( domLI2 ); + expect( domUL.childNodes[ 2 ] ).to.equal( domLI3 ); + + // Assert mappings. + const domMappings = renderer.domConverter._domToViewMapping; + expect( domMappings.get( domBQ ) ).to.equal( viewRoot.getChild( 0 ) ); + expect( domMappings.get( domUL ) ).to.equal( viewRoot.getChild( 0 ).getChild( 0 ) ); + expect( domMappings.get( domLI1 ) ).to.equal( viewRoot.getChild( 0 ).getChild( 0 ).getChild( 0 ) ); + expect( domMappings.get( domLI2 ) ).to.equal( viewRoot.getChild( 0 ).getChild( 0 ).getChild( 1 ) ); + expect( domMappings.get( domLI3 ) ).to.equal( viewRoot.getChild( 0 ).getChild( 0 ).getChild( 2 ) ); + + // Assert if new view elements are bind to new DOM elements. + const viewMappings = renderer.domConverter._domToViewMapping; + expect( viewMappings.get( viewRoot.getChild( 1 ) ) ).not.equal( domBQ ); + expect( viewMappings.get( viewRoot.getChild( 1 ).getChild( 0 ) ) ).not.equal( domUL ); + expect( viewMappings.get( viewRoot.getChild( 1 ).getChild( 0 ).getChild( 0 ) ) ).not.equal( domLI1 ); + expect( viewMappings.get( viewRoot.getChild( 1 ).getChild( 0 ).getChild( 1 ) ) ).not.equal( domLI2 ); + expect( viewMappings.get( viewRoot.getChild( 1 ).getChild( 0 ).getChild( 2 ) ) ).not.equal( domLI3 ); + } ); + + it( 'should handle complex view replace', () => { + const content = '' + + 'He' + + 'ading 1' + + '' + + 'Ph ' + + 'Bold' + + '' + + 'Link' + + '' + + '' + + '' + + '' + + 'Quoted item 1' + + 'Item 2' + + '' + + 'Link' + + '' + + '' + + ''; + + const replacement = '' + + '' + + '1' + + 'A' + + '' + + '' + + '' + + 'Li' + + 'nk' + + '' + + '' + + '' + + 'Heading ' + + '1' + + '' + + '' + + 'Heading 2' + + '' + + '' + + 'Heading' + + ' 3' + + '' + + '' + + 'Foo Bar Baz' + + '' + + '' + + '' + + 'Item ' + + '1' + + '' + + '' + + 'Item' + + ' 2' + + '' + + ''; + + viewRoot._appendChild( parse( content ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '' + + '

Heading 1

' + + '

Ph BoldLink

' + + '
    ' + + '
  • Quoted item 1
    • ' + + '
  • Item 2
  • Link
    • ' + + '
' ); + + viewRoot._removeChildren( 0, viewRoot.childCount ); + viewRoot._appendChild( parse( replacement ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + // Here we just check if new DOM structure was properly rendered. + expect( domRoot.innerHTML ).to.equal( '' + + '

1A

' + + '

Link

' + + '

Heading 1

' + + '

Heading 2

' + + '

Heading 3

' + + '
Foo Bar Baz
' + + '' ); + } ); } ); } ); From 9e365304c89f394ef7f6e1addeea60fc9ed609d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Wed, 30 May 2018 16:22:41 +0200 Subject: [PATCH 05/21] Handle Safari 'br' edge case. --- src/view/renderer.js | 10 ++++++---- tests/view/renderer.js | 24 ++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index 547d202d7..c0c11e9e3 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -316,10 +316,12 @@ export default class Renderer { const deleteIndex = counter.equal + counter.delete; const viewChild = viewElement.getChild( insertIndex ); - this.domConverter.unbindDomElement( diff.actualDomChildren[ deleteIndex ] ); - this.domConverter.bindElements( diff.actualDomChildren[ deleteIndex ], viewChild ); - // View element may have children which needs to be updated but are not marked, mark them to update. - this.markedChildren.add( viewChild ); + if ( viewChild ) { + this.domConverter.unbindDomElement( diff.actualDomChildren[ deleteIndex ] ); + this.domConverter.bindElements( diff.actualDomChildren[ deleteIndex ], viewChild ); + // View element may have children which needs to be updated but are not marked, mark them to update. + this.markedChildren.add( viewChild ); + } remove( diff.expectedDomChildren[ insertIndex ] ); counter.equal++; diff --git a/tests/view/renderer.js b/tests/view/renderer.js index ae65db69c..8a51c1d66 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -2757,6 +2757,30 @@ describe( 'Renderer', () => { '
Foo Bar Baz
' + '' ); } ); + + it( 'should properly handle br elements while refreshing bindings', () => { + const expected = `

Foo Bar

${ BR_FILLER( document ).outerHTML }

`; // eslint-disable-line new-cap + + viewRoot._appendChild( parse( 'Foo Bar' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( expected ); + + // There is a case in Safari that during accent panel navigation on macOS our 'BR_FILLER' is replaced with + // just '
' element which breaks accent composition in an empty paragraph. It also throws an error while + // refreshing mappings in a renderer. Simulate such behaviour (#1354). + domRoot.childNodes[ 1 ].innerHTML = '
'; + + viewRoot._removeChildren( 1 ); + viewRoot._insertChild( 1, parse( '' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( expected ); + } ); } ); } ); From d0509012f63111c89fd3a48c4adeed1ea861c42a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Fri, 1 Jun 2018 13:32:17 +0200 Subject: [PATCH 06/21] Docs adjustments. --- src/view/renderer.js | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index c0c11e9e3..a13c5fad9 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -182,7 +182,7 @@ export default class Renderer { let inlineFillerPosition; if ( this.markedChildren.size > 1 ) { - // Sort `this.markedChildren` by the nesting level. + // Sort `this.markedChildren` by nesting level. this.markedChildren = this._sortElementsByNestingLevel( this.markedChildren ); } @@ -255,12 +255,12 @@ export default class Renderer { } /** - * Sorts elements set based on their nesting. The outermost elements are placed first. - * It check only for 2 element types: `containerElement` and `attributeElement`. Those elements - * are sorted in such order (from `containerElement` to `attributeElement`). Any other type (e.g. `rootElement` or - * `documentFragment`) have higher priority and is placed higher in the sorted set. + * Sorts elements set based on their nesting level. The outermost elements are placed first. + * It check only for 2 element types: `containerElement` and `attributeElement`. Elements of these types + * are sorted (from `containerElement` to `attributeElement`) and any other type (e.g. `rootElement` or + * `documentFragment`) have higher priority so is placed higher in the sorted set. * Additionally if elements are of the same type, they are both checked if one is another parent so proper - * order can be established (parent first). + * order can be established between them (parent first). * * @private * @param {Set.} elements Elements to be sorted. @@ -294,12 +294,12 @@ export default class Renderer { } /** - * Updates element children mappings. Children which were replaced in the view structure by the similar + * Updates viewElement children mappings. Children which were replaced in the view structure by the similar * element (same tag name) are treated as 'replaced'. Their mappings are rebind to the corresponding, - * existing DOM element so they will not be replaced by new DOM element during rerendering. + * existing DOM element so they will not be replaced by a new DOM element during rendering. * * @private - * @param {module:engine/view/node~Node} viewElement View element which children mappings will be updated. + * @param {module:engine/view/node~Node} viewElement The view element which children mappings will be updated. */ _updateChildrenMappings( viewElement ) { // We do not perform any operations on DOM here so there is no need to bind view element or convert its' children. @@ -319,7 +319,7 @@ export default class Renderer { if ( viewChild ) { this.domConverter.unbindDomElement( diff.actualDomChildren[ deleteIndex ] ); this.domConverter.bindElements( diff.actualDomChildren[ deleteIndex ], viewChild ); - // View element may have children which needs to be updated but are not marked, mark them to update. + // View element may have children which needs to be updated, but are not marked, mark them to update. this.markedChildren.add( viewChild ); } @@ -600,8 +600,8 @@ export default class Renderer { } /** - * Compares element actual and expected children and finds list of actions which can be used to transform - * actual children to expected ones. + * Compares viewElement actual and expected children and actions sequence which can be used to transform + * actual children into expected ones. * * @private * @param viewElement @@ -615,7 +615,6 @@ export default class Renderer { * @returns {Node} result.domElement ViewElement corresponding DOM element. * @returns {Array} result.actualDomChildren Current viewElement DOM children. * @returns {Array} result.expectedDomChildren Expected viewElement DOM children. - * */ _diffElementChildren( viewElement, options ) { const domConverter = this.domConverter; @@ -623,7 +622,7 @@ export default class Renderer { if ( !domElement ) { // If there is no `domElement` it means that it was already removed from DOM. - // There is no need to update it. It will be updated when re-inserted. + // There is no need to process it. It will be processed when re-inserted. return null; } @@ -667,8 +666,8 @@ export default class Renderer { } /** - * Finds DOM nodes which were replaced with the similar nodes (same tag name) in the `insert`/`delete` - * action groups (based on actual and expected DOM). For example: + * Finds DOM nodes which were replaced with the similar nodes (same tag name) in the view. All nodes are compared + * within one `insert`/`delete` action group, for example: * * Actual DOM:

FooBarBazBax

* Expected DOM:

Bar123Baz456

From ebfacb0fe8c42dc26cc8cc945181ec98545acb1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Thu, 7 Jun 2018 18:27:55 +0200 Subject: [PATCH 07/21] Fix for skipped actions in 'actions replace diff'. --- src/view/renderer.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/view/renderer.js b/src/view/renderer.js index a13c5fad9..32c922c27 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -697,21 +697,27 @@ export default class Renderer { } let newActions = []; + let skipActions = []; let actualSlice = []; let expectedSlice = []; const counter = { equal: 0, insert: 0, delete: 0 }; for ( const action of actions ) { if ( action === 'insert' ) { + skipActions.push( 'insert' ); expectedSlice.push( expectedDom[ counter.equal + counter.insert ] ); } else if ( action === 'delete' ) { + skipActions.push( 'delete' ); actualSlice.push( actualDom[ counter.equal + counter.delete ] ); } else { // equal if ( expectedSlice.length && actualSlice.length ) { newActions = newActions.concat( calculateReplaceActions( actualSlice, expectedSlice ) ); + } else if ( expectedSlice.length || actualSlice.length ) { + newActions = newActions.concat( skipActions ); } newActions.push( 'equal' ); // Reset stored elements on 'equal'. + skipActions = []; actualSlice = []; expectedSlice = []; } From 0cee0a12720edf94d3e5d1d7adbc9d3e95b43f60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Fri, 8 Jun 2018 11:20:52 +0200 Subject: [PATCH 08/21] Tests: skipped actions unit test added. --- tests/view/renderer.js | 51 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/tests/view/renderer.js b/tests/view/renderer.js index 8a51c1d66..15193a6eb 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -2758,7 +2758,7 @@ describe( 'Renderer', () => { '' ); } ); - it( 'should properly handle br elements while refreshing bindings', () => { + it( 'should handle br elements while refreshing bindings', () => { const expected = `

Foo Bar

${ BR_FILLER( document ).outerHTML }

`; // eslint-disable-line new-cap viewRoot._appendChild( parse( 'Foo Bar' ) ); @@ -2781,6 +2781,55 @@ describe( 'Renderer', () => { expect( domRoot.innerHTML ).to.equal( expected ); } ); + + it( 'should handle list to paragraph conversion', () => { + const view = '' + + '' + + 'Item 1' + + '' + + 'Item 2' + + '' + + '' + + '' + + 'Paragraph' + + '' + + 'Item 3' + + '' + + 'Item 4' + + '' + + '' + + ''; + + viewRoot._appendChild( parse( view ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( + '
  1. Item 1
    1. Item 2

Paragraph

  1. Item 3
    1. Item 4
' ); + + const viewOL1 = viewRoot.getChild( 0 ); + viewOL1.getChild( 0 )._removeChildren( 1 ); + viewRoot._removeChildren( 2 ); + viewRoot._insertChild( 1, parse( 'Item 2' ) ); + viewRoot._insertChild( 3, parse( 'Item 3' ) ); + viewRoot._insertChild( 4, parse( 'Item 4' ) ); + + const domOL1 = domRoot.childNodes[ 0 ]; + const domOL2 = domRoot.childNodes[ 2 ]; + const domP = domRoot.childNodes[ 1 ]; + + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewOL1.getChild( 0 ) ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( + '
  1. Item 1

Item 2

Paragraph

Item 3

  1. Item 4
' ); + + expect( domRoot.childNodes[ 0 ] ).to.equal( domOL1 ); + expect( domRoot.childNodes[ 2 ] ).to.equal( domP ); + expect( domRoot.childNodes[ 4 ] ).to.equal( domOL2 ); + } ); } ); } ); From 8147685ff8a0746cb09b31b3edcac950713acc12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 11 Jun 2018 12:26:04 +0200 Subject: [PATCH 09/21] Fixed attributes rendering for replaced elements. --- src/view/renderer.js | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index 32c922c27..ff7a3f55f 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -302,7 +302,7 @@ export default class Renderer { * @param {module:engine/view/node~Node} viewElement The view element which children mappings will be updated. */ _updateChildrenMappings( viewElement ) { - // We do not perform any operations on DOM here so there is no need to bind view element or convert its' children. + // We do not perform any operations on DOM here so there is no need to bind view element or convert its children. const diff = this._diffElementChildren( viewElement, { bind: false, withChildren: false } ); if ( diff ) { @@ -317,8 +317,23 @@ export default class Renderer { const viewChild = viewElement.getChild( insertIndex ); if ( viewChild ) { + // Because we replace previous view element mapping with the new one, the corresponding DOM element + // will not be rerendered. The new view element may have different attributes than the old one. + // Its corresponding DOM element will not be rerendered so new attributes will not be present in a DOM. + // Such situation may take place if only previous view element was added to `this.markedAttributes` + // or none of elements were added (relying on 'this._updateChildren()' which by rerendering the element + // also rerenders its attributes). See #1427 for more detailed case study. + // It may also happen that 'oldViewElement' mapping is not present since its parent mapping + // was removed ('domConverter.unbindDomElement' also unbinds children mappings). + const oldViewChild = this.domConverter.mapDomToView( diff.actualDomChildren[ deleteIndex ] ); + if ( !oldViewChild || oldViewChild && !oldViewChild.isSimilar( viewChild ) ) { + this.markedAttributes.add( viewChild ); + } + + // Remap 'DomConverter' bindings. this.domConverter.unbindDomElement( diff.actualDomChildren[ deleteIndex ] ); this.domConverter.bindElements( diff.actualDomChildren[ deleteIndex ], viewChild ); + // View element may have children which needs to be updated, but are not marked, mark them to update. this.markedChildren.add( viewChild ); } @@ -537,6 +552,15 @@ export default class Renderer { */ _updateAttrs( viewElement ) { const domElement = this.domConverter.mapViewToDom( viewElement ); + + if ( !domElement ) { + // If there is no `domElement` it means that 'viewElement' is outdated as its mapping was updated + // in 'this._updateChildrenMappings()'. There is no need to process it as new view element which + // replaced old 'viewElement' mapping was also added to 'this.markedAttributes' + // in 'this._updateChildrenMappings()' so it will be processed separately. + return; + } + const domAttrKeys = Array.from( domElement.attributes ).map( attr => attr.name ); const viewAttrKeys = viewElement.getAttributeKeys(); From 37017e8afea14c5f5bfe6e3e3f57947f59375c16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 11 Jun 2018 12:29:21 +0200 Subject: [PATCH 10/21] Tests: attributes rendering unit tests. --- tests/view/renderer.js | 118 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/tests/view/renderer.js b/tests/view/renderer.js index 15193a6eb..23c2d80f5 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -2830,6 +2830,124 @@ describe( 'Renderer', () => { expect( domRoot.childNodes[ 2 ] ).to.equal( domP ); expect( domRoot.childNodes[ 4 ] ).to.equal( domOL2 ); } ); + + it( 'should handle attributes change in replaced elements', () => { + const view = '' + + '' + + 'Item 1' + + '' + + 'Paragraph ' + + 'Link' + + '' + + 'BarBaz'; + + viewRoot._appendChild( parse( view ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml( + '
  1. Item 1
' + + '

Paragraph Link

BarBaz

' ) ); + + const viewOL = viewRoot.getChild( 0 ); + viewOL._removeChildren( 0 ); + viewOL._insertChild( 0, parse( 'Item 1' ) ); + + const viewP1 = viewRoot.getChild( 1 ); + viewP1._removeChildren( 1 ); + viewP1._insertChild( 1, parse( 'Foo' ) ); + + viewRoot._removeChildren( 2 ); + viewRoot._insertChild( 2, parse( 'Bar' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewOL ); + renderer.markToSync( 'children', viewP1 ); + renderer.render(); + + expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml( + '
  1. Item 1
' + + '

Paragraph Foo

Bar

' ) ); + } ); + + it( 'should handle classes change in replaced elements', () => { + const view = '' + + '' + + 'Item 1' + + '' + + 'BarBaz'; + + viewRoot._appendChild( parse( view ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml( + '
  1. Item 1

BarBaz

' ) ); + + const viewOL = viewRoot.getChild( 0 ); + const oldViewLI = viewOL.getChild( 0 ); + viewOL._removeChildren( 0 ); + viewOL._insertChild( 0, parse( 'Item 1' ) ); + + const oldViewP = viewRoot.getChild( 1 ); + viewRoot._removeChildren( 1 ); + viewRoot._insertChild( 1, parse( 'Foo' ) ); + + renderer.markToSync( 'attributes', oldViewLI ); + renderer.markToSync( 'attributes', oldViewP ); + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewOL ); + renderer.render(); + + expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml( + '
  1. Item 1

Foo

' ) ); + } ); + + it( 'should handle styles change in replaced elements', () => { + const view = '' + + '' + + 'Foo' + + 'Bar ' + + '' + + 'Baz' + + ' Bax' + + '' + + ''; + + viewRoot._appendChild( parse( view ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml( + '
  1. Foo
    2. ' + + '
  2. Bar Baz Bax
' ) ); + + const viewOL = viewRoot.getChild( 0 ); + const viewLI1 = viewOL.getChild( 0 ); + const viewLI2 = viewOL.getChild( 1 ); + + viewLI1._removeStyle( 'font-weight' ); + viewLI1._setStyle( { color: '#FFF' } ); + viewLI2._setStyle( { 'font-weight': 'bold' } ); + + viewLI2._removeChildren( 0, viewLI2.childCount ); + viewLI2._insertChild( 0, parse( 'Ba1 Ba3 ' + + 'Ba2' ) ); + + renderer.markToSync( 'attributes', viewLI1 ); + renderer.markToSync( 'attributes', viewLI2 ); + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewLI2 ); + renderer.render(); + + expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml( + '
  1. Foo
    2. ' + + '
  2. Ba1 Ba3 ' + + 'Ba2
' ) ); + } ); } ); } ); From 86423cbb77beb930f93b61939821f10f7508c897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 11 Jun 2018 12:36:40 +0200 Subject: [PATCH 11/21] Removed marked elements sorting as it is no longer needed. --- src/view/renderer.js | 44 -------------------------------------------- 1 file changed, 44 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index ff7a3f55f..e636d1e07 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -181,11 +181,6 @@ export default class Renderer { render() { let inlineFillerPosition; - if ( this.markedChildren.size > 1 ) { - // Sort `this.markedChildren` by nesting level. - this.markedChildren = this._sortElementsByNestingLevel( this.markedChildren ); - } - // Refresh mappings. for ( const element of this.markedChildren ) { this._updateChildrenMappings( element ); @@ -254,45 +249,6 @@ export default class Renderer { this.markedChildren.clear(); } - /** - * Sorts elements set based on their nesting level. The outermost elements are placed first. - * It check only for 2 element types: `containerElement` and `attributeElement`. Elements of these types - * are sorted (from `containerElement` to `attributeElement`) and any other type (e.g. `rootElement` or - * `documentFragment`) have higher priority so is placed higher in the sorted set. - * Additionally if elements are of the same type, they are both checked if one is another parent so proper - * order can be established between them (parent first). - * - * @private - * @param {Set.} elements Elements to be sorted. - * @returns {Set.} Sorted elements. - */ - _sortElementsByNestingLevel( elements ) { - function getPriority( node ) { - let priority = 2; - if ( node.is( 'containerElement' ) ) { - priority = 1; - } else if ( node.is( 'attributeElement' ) ) { - priority = 0; - } - return priority; - } - - const elementsArray = Array.from( elements ); - elementsArray.sort( ( node1, node2 ) => { - let priority = getPriority( node2 ) - getPriority( node1 ); - if ( priority === 0 ) { - if ( node1.parent === node2 ) { - priority = 1; - } else if ( node2.parent === node1 ) { - priority = -1; - } - } - return priority; - } ); - - return new Set( elementsArray ); - } - /** * Updates viewElement children mappings. Children which were replaced in the view structure by the similar * element (same tag name) are treated as 'replaced'. Their mappings are rebind to the corresponding, From c974fbbb8f57bf347c7e7add060c299a7b47ce8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 11 Jun 2018 15:04:35 +0200 Subject: [PATCH 12/21] Fix for 'uiElements' rendering. --- src/view/renderer.js | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index e636d1e07..2cc502732 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -272,17 +272,23 @@ export default class Renderer { const deleteIndex = counter.equal + counter.delete; const viewChild = viewElement.getChild( insertIndex ); - if ( viewChild ) { - // Because we replace previous view element mapping with the new one, the corresponding DOM element - // will not be rerendered. The new view element may have different attributes than the old one. - // Its corresponding DOM element will not be rerendered so new attributes will not be present in a DOM. - // Such situation may take place if only previous view element was added to `this.markedAttributes` - // or none of elements were added (relying on 'this._updateChildren()' which by rerendering the element + // The 'uiElement' is a special one and its children are not stored in a view (#799), + // so we cannot use it with replacing flow (since it uses view children during rendering + // which will always result in rendering empty element). + if ( viewChild && !viewChild.is( 'uiElement' ) ) { + // Because we replace new view element mapping with the existing one, the corresponding DOM element + // will not be rerendered. The new view element may have different attributes than the previous one. + // Since its corresponding DOM element will not be rerendered, new attributes will not be added + // to the DOM, so we need to mark it here to make sure its attributes gets updated. + // Such situations may happen if only new view element was added to `this.markedAttributes` + // or none of the elements were added (relying on 'this._updateChildren()' which by rerendering the element // also rerenders its attributes). See #1427 for more detailed case study. - // It may also happen that 'oldViewElement' mapping is not present since its parent mapping - // was removed ('domConverter.unbindDomElement' also unbinds children mappings). - const oldViewChild = this.domConverter.mapDomToView( diff.actualDomChildren[ deleteIndex ] ); - if ( !oldViewChild || oldViewChild && !oldViewChild.isSimilar( viewChild ) ) { + const newViewChild = this.domConverter.mapDomToView( diff.actualDomChildren[ deleteIndex ] ); + + // It may also happen that 'newViewChild' mapping is not present since its parent mapping + // was already removed (the 'domConverter.unbindDomElement()' method also unbinds children + // mappings) so we also check for '!newViewChild'. + if ( !newViewChild || newViewChild && !newViewChild.isSimilar( viewChild ) ) { this.markedAttributes.add( viewChild ); } From 3a4ffd401845051ae223f55756278f8bfdc1c185 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 11 Jun 2018 15:33:05 +0200 Subject: [PATCH 13/21] Tests: 'uiElement' rerendering unit test added. --- tests/view/renderer.js | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/view/renderer.js b/tests/view/renderer.js index 23c2d80f5..b6e5ca091 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -12,6 +12,7 @@ import ViewAttributeElement from '../../src/view/attributeelement'; import ViewText from '../../src/view/text'; import ViewRange from '../../src/view/range'; import ViewPosition from '../../src/view/position'; +import UIElement from '../../src/view/uielement'; import DocumentSelection from '../../src/view/documentselection'; import DomConverter from '../../src/view/domconverter'; import Renderer from '../../src/view/renderer'; @@ -2948,6 +2949,40 @@ describe( 'Renderer', () => { '
  • Ba1 Ba3 ' + 'Ba2
    • ' ) ); } ); + + it( 'should handle uiElement rendering', () => { + function createUIElement( id, text ) { + const element = new UIElement( 'span' ); + element.render = function( domDocument ) { + const domElement = this.toDomElement( domDocument ); + domElement.innerText = `${ text }`; + return domElement; + }; + + return element; + } + + const ui1 = createUIElement( 'id1', 'UI1' ); + const ui2 = createUIElement( 'id2', 'UI2' ); + const viewP = new ViewContainerElement( 'p', null, [ new ViewText( 'Foo ' ), ui1, new ViewText( 'Bar' ) ] ); + viewRoot._appendChild( viewP ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml( + '

    Foo UI1Bar

    ' ) ); + + viewP._removeChildren( 0, viewP.childCount ); + viewP._insertChild( 0, [ new ViewText( 'Foo' ), ui2, new ViewText( ' Bar' ) ] ); + + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewP ); + renderer.render(); + + expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml( + '

    FooUI2 Bar

    ' ) ); + } ); } ); } ); From 4d6a100f7b81e19c57a5308091cef74637d5e576 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 12 Jun 2018 11:38:16 +0200 Subject: [PATCH 14/21] Improved API docs and code style. --- src/view/renderer.js | 61 +++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 37 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index 2cc502732..762ccffd5 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -18,21 +18,21 @@ import remove from '@ckeditor/ckeditor5-utils/src/dom/remove'; import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import isText from '@ckeditor/ckeditor5-utils/src/dom/istext'; -import fastDiff from '@ckeditor/ckeditor5-utils/src/fastdiff'; import isNode from '@ckeditor/ckeditor5-utils/src/dom/isnode'; +import fastDiff from '@ckeditor/ckeditor5-utils/src/fastdiff'; /** - * Renderer updates DOM structure and selection, to make them a reflection of the view structure and selection. - * - * View nodes which may need to be rendered needs to be {@link module:engine/view/renderer~Renderer#markToSync marked}. - * Then, on {@link module:engine/view/renderer~Renderer#render render}, renderer compares view nodes with DOM nodes - * in order to check which ones really need to be refreshed. Finally, it creates DOM nodes from these view nodes, - * {@link module:engine/view/domconverter~DomConverter#bindElements binds} them and inserts into the DOM tree. + * Renderer is responsible for updating the DOM structure and the DOM selection based on + * the {@link module:engine/view/renderer~Renderer#markToSync information about updated view nodes}. + * In other words, it renders the view to the DOM. * - * Every time {@link module:engine/view/renderer~Renderer#render render} is called, renderer additionally checks if - * {@link module:engine/view/renderer~Renderer#selection selection} needs update and updates it if so. + * Its main responsibility is to make only the necessary, minimal changes to the DOM. However, unlike in many + * virtual DOM implementations, the primary reason for doing minimal changes is not the performance but ensuring + * that native editing features such as text composition, autocompletion, spell checking, selection's x-index are + * affected as little as possible. * - * Renderer uses {@link module:engine/view/domconverter~DomConverter} to transform and bind nodes. + * Renderer uses {@link module:engine/view/domconverter~DomConverter} to transform view nodes and positions + * to and from the DOM. */ export default class Renderer { /** @@ -90,14 +90,6 @@ export default class Renderer { */ this.selection = selection; - /** - * The text node in which the inline filler was rendered. - * - * @private - * @member {Text} - */ - this._inlineFiller = null; - /** * Indicates if the view document is focused and selection can be rendered. Selection will not be rendered if * this is set to `false`. @@ -106,6 +98,14 @@ export default class Renderer { */ this.isFocused = false; + /** + * The text node in which the inline filler was rendered. + * + * @private + * @member {Text} + */ + this._inlineFiller = null; + /** * DOM element containing fake selection. * @@ -116,7 +116,7 @@ export default class Renderer { } /** - * Mark node to be synchronized. + * Marks node to be synchronized with the DOM. * * Note that only view nodes which parents have corresponding DOM elements need to be marked to be synchronized. * @@ -155,27 +155,14 @@ export default class Renderer { } /** - * Render method checks {@link #markedAttributes}, - * {@link #markedChildren} and {@link #markedTexts} and updates all - * nodes which need to be updated. Then it clears all three sets. Also, every time render is called it compares and - * if needed updates the selection. + * Renders all buffered changes ({@link #markedAttributes}, {@link #markedChildren} and {@link #markedTexts}) and + * the current view selection (if needed) to the DOM by applying a minimal set of changes to it. * - * Renderer tries not to break text composition (e.g. IME) and x-index of the selection, + * Renderer tries not to break the text composition (e.g. IME) and x-index of the selection, * so it does as little as it is needed to update the DOM. * - * For attributes it adds new attributes to DOM elements, updates values and removes - * attributes which do not exist in the view element. - * - * For text nodes it updates the text string if it is different. Note that if parent element is marked as an element - * which changed child list, text node update will not be done, because it may not be possible to - * {@link module:engine/view/domconverter~DomConverter#findCorrespondingDomText find a corresponding DOM text}. - * The change will be handled in the parent element. - * - * For elements, which child lists have changed, it calculates a {@link module:utils/diff~diff} and adds or removes children which have - * changed. - * - * Rendering also handles {@link module:engine/view/filler fillers}. Especially, it checks if the inline filler is needed - * at selection position and adds or removes it. To prevent breaking text composition inline filler will not be + * Renderer also handles {@link module:engine/view/filler fillers}. Especially, it checks if the inline filler is needed + * at the selection position and adds or removes it. To prevent breaking text composition inline filler will not be * removed as long selection is in the text node which needed it at first. */ render() { From dd11e72faf331eb7a8cc818ca0a9668b7a4df5c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 12 Jun 2018 13:50:15 +0200 Subject: [PATCH 15/21] Wording. --- src/view/renderer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index 762ccffd5..84a11294c 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -116,7 +116,7 @@ export default class Renderer { } /** - * Marks node to be synchronized with the DOM. + * Marks node to be updated in the DOM by {@link #render `render()`}. * * Note that only view nodes which parents have corresponding DOM elements need to be marked to be synchronized. * From 3a8085fac79ed6fb71e6c80f4b36dab2ecdbb4e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 12 Jun 2018 13:55:01 +0200 Subject: [PATCH 16/21] Wording. --- src/view/renderer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index 84a11294c..eb6ff28fd 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -116,7 +116,7 @@ export default class Renderer { } /** - * Marks node to be updated in the DOM by {@link #render `render()`}. + * Marks a view node to be updated in the DOM by {@link #render `render()`}. * * Note that only view nodes which parents have corresponding DOM elements need to be marked to be synchronized. * From e0972462c8567c95f9a3696aaac5b8e35dda483d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 12 Jun 2018 17:09:10 +0200 Subject: [PATCH 17/21] Wording. --- src/view/renderer.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index eb6ff28fd..b4d775676 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -237,15 +237,17 @@ export default class Renderer { } /** - * Updates viewElement children mappings. Children which were replaced in the view structure by the similar - * element (same tag name) are treated as 'replaced'. Their mappings are rebind to the corresponding, - * existing DOM element so they will not be replaced by a new DOM element during rendering. + * Updates mapping of `viewElement`'s children. + * + * Children which were replaced in the view structure by similar elements (same tag name) are treated as 'replaced'. + * This means that we can update their mappings so the new view elements are mapped to the existing DOM elements. + * Thanks to that we won't need to re-render these elements completely. * * @private * @param {module:engine/view/node~Node} viewElement The view element which children mappings will be updated. */ _updateChildrenMappings( viewElement ) { - // We do not perform any operations on DOM here so there is no need to bind view element or convert its children. + // We're diffing only so we don't have to bind anything. const diff = this._diffElementChildren( viewElement, { bind: false, withChildren: false } ); if ( diff ) { From 8d3a0d1a6940f7e65ad1bd45065968376fcbef2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Wed, 13 Jun 2018 16:02:02 +0200 Subject: [PATCH 18/21] Renderer refactoring. --- src/view/renderer.js | 257 +++++++++++++++++++++++-------------------- 1 file changed, 139 insertions(+), 118 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index b4d775676..262602634 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -237,7 +237,7 @@ export default class Renderer { } /** - * Updates mapping of `viewElement`'s children. + * Updates mappings of `viewElement`'s children. * * Children which were replaced in the view structure by similar elements (same tag name) are treated as 'replaced'. * This means that we can update their mappings so the new view elements are mapped to the existing DOM elements. @@ -248,57 +248,71 @@ export default class Renderer { */ _updateChildrenMappings( viewElement ) { // We're diffing only so we don't have to bind anything. - const diff = this._diffElementChildren( viewElement, { bind: false, withChildren: false } ); - - if ( diff ) { - const actions = this._findReplaceActions( diff.actions, diff.actualDomChildren, diff.expectedDomChildren ); - - if ( actions.indexOf( 'replace' ) !== -1 ) { - const counter = { equal: 0, insert: 0, delete: 0 }; - for ( const action of actions ) { - if ( action === 'replace' ) { - const insertIndex = counter.equal + counter.insert; - const deleteIndex = counter.equal + counter.delete; - const viewChild = viewElement.getChild( insertIndex ); - - // The 'uiElement' is a special one and its children are not stored in a view (#799), - // so we cannot use it with replacing flow (since it uses view children during rendering - // which will always result in rendering empty element). - if ( viewChild && !viewChild.is( 'uiElement' ) ) { - // Because we replace new view element mapping with the existing one, the corresponding DOM element - // will not be rerendered. The new view element may have different attributes than the previous one. - // Since its corresponding DOM element will not be rerendered, new attributes will not be added - // to the DOM, so we need to mark it here to make sure its attributes gets updated. - // Such situations may happen if only new view element was added to `this.markedAttributes` - // or none of the elements were added (relying on 'this._updateChildren()' which by rerendering the element - // also rerenders its attributes). See #1427 for more detailed case study. - const newViewChild = this.domConverter.mapDomToView( diff.actualDomChildren[ deleteIndex ] ); - - // It may also happen that 'newViewChild' mapping is not present since its parent mapping - // was already removed (the 'domConverter.unbindDomElement()' method also unbinds children - // mappings) so we also check for '!newViewChild'. - if ( !newViewChild || newViewChild && !newViewChild.isSimilar( viewChild ) ) { - this.markedAttributes.add( viewChild ); - } - - // Remap 'DomConverter' bindings. - this.domConverter.unbindDomElement( diff.actualDomChildren[ deleteIndex ] ); - this.domConverter.bindElements( diff.actualDomChildren[ deleteIndex ], viewChild ); - - // View element may have children which needs to be updated, but are not marked, mark them to update. - this.markedChildren.add( viewChild ); - } - - remove( diff.expectedDomChildren[ insertIndex ] ); - counter.equal++; - } else { - counter[ action ]++; + const diff = this._diffChildren( viewElement, { bind: false, withChildren: false } ); + + if ( !diff ) { + return; + } + + const actions = this._findReplaceActions( diff.actions, diff.actualDomChildren, diff.expectedDomChildren ); + + if ( actions.indexOf( 'replace' ) !== -1 ) { + const counter = { equal: 0, insert: 0, delete: 0 }; + + for ( const action of actions ) { + if ( action === 'replace' ) { + const insertIndex = counter.equal + counter.insert; + const deleteIndex = counter.equal + counter.delete; + const viewChild = viewElement.getChild( insertIndex ); + + // The 'uiElement' is a special one and its children are not stored in a view (#799), + // so we cannot use it with replacing flow (since it uses view children during rendering + // which will always result in rendering empty element). + if ( viewChild && !viewChild.is( 'uiElement' ) ) { + this._updateElementMappings( viewChild, diff.actualDomChildren[ deleteIndex ] ); } + + remove( diff.expectedDomChildren[ insertIndex ] ); + counter.equal++; + } else { + counter[ action ]++; } } } } + /** + * Updates mappings of a given view element. + * + * @private + * @param {module:engine/view/node~Node} viewElement The view element which mappings will be updated. + * @param {Node} domElement DOM representation of a given view element. + */ + _updateElementMappings( viewElement, domElement ) { + // Because we replace new view element mapping with the existing one, the corresponding DOM element + // will not be rerendered. The new view element may have different attributes than the previous one. + // Since its corresponding DOM element will not be rerendered, new attributes will not be added + // to the DOM, so we need to mark it here to make sure its attributes gets updated. + // Such situations may happen if only new view element was added to `this.markedAttributes` + // or none of the elements were added (relying on 'this._updateChildren()' which by rerendering the element + // also rerenders its attributes). See #1427 for more detailed case study. + const newViewChild = this.domConverter.mapDomToView( domElement ); + + // It may also happen that 'newViewChild' mapping is not present since its parent mapping + // was already removed (the 'domConverter.unbindDomElement()' method also unbinds children + // mappings) so we also check for '!newViewChild'. + if ( !newViewChild || newViewChild && !newViewChild.isSimilar( viewElement ) ) { + this.markedAttributes.add( viewElement ); + } + + // Remap 'DomConverter' bindings. + this.domConverter.unbindDomElement( domElement ); + this.domConverter.bindElements( domElement, viewElement ); + + // View element may have children which needs to be updated, but are not marked, mark them to update. + this.markedChildren.add( viewElement ); + } + /** * Adds inline filler at given position. * @@ -538,38 +552,41 @@ export default class Renderer { * filler should be rendered. */ _updateChildren( viewElement, options ) { - const diff = this._diffElementChildren( viewElement, + const diff = this._diffChildren( viewElement, { inlineFillerPosition: options.inlineFillerPosition, bind: true, withChildren: true } ); - if ( diff ) { - const actions = diff.actions; - const domElement = diff.domElement; - const actualDomChildren = diff.actualDomChildren; - const expectedDomChildren = diff.expectedDomChildren; + if ( !diff ) { + return; + } - let i = 0; - const nodesToUnbind = new Set(); - for ( const action of actions ) { - if ( action === 'insert' ) { - insertAt( domElement, i, expectedDomChildren[ i ] ); - i++; - } else if ( action === 'delete' ) { - nodesToUnbind.add( actualDomChildren[ i ] ); - remove( actualDomChildren[ i ] ); - } else { // 'equal' - // Force updating text nodes inside elements which did not change and do not need to be re-rendered (#1125). - this._markDescendantTextToSync( this.domConverter.domToView( expectedDomChildren[ i ] ) ); - i++; - } + const actions = diff.actions; + const domElement = diff.domElement; + const actualDomChildren = diff.actualDomChildren; + const expectedDomChildren = diff.expectedDomChildren; + + let i = 0; + const nodesToUnbind = new Set(); + + for ( const action of actions ) { + if ( action === 'insert' ) { + insertAt( domElement, i, expectedDomChildren[ i ] ); + i++; + } else if ( action === 'delete' ) { + nodesToUnbind.add( actualDomChildren[ i ] ); + remove( actualDomChildren[ i ] ); + } else { // 'equal' + // Force updating text nodes inside elements which did not change and do not need to be re-rendered (#1125). + this._markDescendantTextToSync( this.domConverter.domToView( expectedDomChildren[ i ] ) ); + i++; } + } - // Unbind removed nodes. When node does not have a parent it means that it was removed from DOM tree during - // comparision with the expected DOM. We don't need to check child nodes, because if child node was reinserted, - // it was moved to DOM tree out of the removed node. - for ( const node of nodesToUnbind ) { - if ( !node.parentNode ) { - this.domConverter.unbindDomElement( node ); - } + // Unbind removed nodes. When node does not have a parent it means that it was removed from DOM tree during + // comparision with the expected DOM. We don't need to check child nodes, because if child node was reinserted, + // it was moved to DOM tree out of the removed node. + for ( const node of nodesToUnbind ) { + if ( !node.parentNode ) { + this.domConverter.unbindDomElement( node ); } } } @@ -591,7 +608,7 @@ export default class Renderer { * @returns {Array} result.actualDomChildren Current viewElement DOM children. * @returns {Array} result.expectedDomChildren Expected viewElement DOM children. */ - _diffElementChildren( viewElement, options ) { + _diffChildren( viewElement, options ) { const domConverter = this.domConverter; const domElement = domConverter.mapViewToDom( viewElement ); @@ -601,25 +618,6 @@ export default class Renderer { return null; } - function sameNodes( actualDomChild, expectedDomChild ) { - // Elements. - if ( actualDomChild === expectedDomChild ) { - return true; - } - // Texts. - else if ( isText( actualDomChild ) && isText( expectedDomChild ) ) { - return actualDomChild.data === expectedDomChild.data; - } - // Block fillers. - else if ( isBlockFiller( actualDomChild, domConverter.blockFiller ) && - isBlockFiller( expectedDomChild, domConverter.blockFiller ) ) { - return true; - } - - // Not matching types. - return false; - } - const domDocument = domElement.ownerDocument; const filler = options.inlineFillerPosition; const actualDomChildren = domElement.childNodes; @@ -633,7 +631,7 @@ export default class Renderer { } return { - actions: diff( actualDomChildren, expectedDomChildren, sameNodes ), + actions: diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, domConverter.blockFiller ) ), domElement, actualDomChildren, expectedDomChildren @@ -661,49 +659,28 @@ export default class Renderer { return actions; } - function areSimilar( domNode1, domNode2 ) { - return isNode( domNode1 ) && isNode( domNode2 ) && - !isText( domNode1 ) && !isText( domNode2 ) && - domNode1.tagName.toLowerCase() === domNode2.tagName.toLowerCase(); - } - - function calculateReplaceActions( actual, expected ) { - return diff( actual, expected, areSimilar ).map( x => x === 'equal' ? 'replace' : x ); - } - let newActions = []; - let skipActions = []; let actualSlice = []; let expectedSlice = []; const counter = { equal: 0, insert: 0, delete: 0 }; + for ( const action of actions ) { if ( action === 'insert' ) { - skipActions.push( 'insert' ); expectedSlice.push( expectedDom[ counter.equal + counter.insert ] ); } else if ( action === 'delete' ) { - skipActions.push( 'delete' ); actualSlice.push( actualDom[ counter.equal + counter.delete ] ); } else { // equal - if ( expectedSlice.length && actualSlice.length ) { - newActions = newActions.concat( calculateReplaceActions( actualSlice, expectedSlice ) ); - } else if ( expectedSlice.length || actualSlice.length ) { - newActions = newActions.concat( skipActions ); - } + newActions = newActions.concat( diff( actualSlice, expectedSlice, areSimilar ).map( x => x === 'equal' ? 'replace' : x ) ); newActions.push( 'equal' ); // Reset stored elements on 'equal'. - skipActions = []; actualSlice = []; expectedSlice = []; } counter[ action ]++; } - if ( expectedSlice.length && actualSlice.length ) { - newActions = newActions.concat( calculateReplaceActions( actualSlice, expectedSlice ) ); - } - - return newActions; + return newActions.concat( diff( actualSlice, expectedSlice, areSimilar ).map( x => x === 'equal' ? 'replace' : x ) ); } /** @@ -928,3 +905,47 @@ function _isEditable( element ) { return !parent || parent.getAttribute( 'contenteditable' ) == 'true'; } + +// Whether two DOM nodes should be considered as similar. +// Nodes are considered similar if they have the same tag name. +// +// @private +// @param {Node} node1 +// @param {Node} node2 +// @returns {Boolean} +function areSimilar( node1, node2 ) { + return isNode( node1 ) && isNode( node2 ) && + !isText( node1 ) && !isText( node2 ) && + node1.tagName.toLowerCase() === node2.tagName.toLowerCase(); +} + +// Whether two dom nodes should be considered as the same. +// Two nodes which are considered the same are: +// +// * Text nodes with the same text. +// * Element nodes represented by the same object. +// * Two block filler elements. +// +// @private +// @param {Function} blockFiller Block filler creator function, see {@link module:engine/view/domconverter~DomConverter#blockFiller}. +// @param {Node} node1 +// @param {Node} node2 +// @returns {Boolean} +function sameNodes( blockFiller, actualDomChild, expectedDomChild ) { + // Elements. + if ( actualDomChild === expectedDomChild ) { + return true; + } + // Texts. + else if ( isText( actualDomChild ) && isText( expectedDomChild ) ) { + return actualDomChild.data === expectedDomChild.data; + } + // Block fillers. + else if ( isBlockFiller( actualDomChild, blockFiller ) && + isBlockFiller( expectedDomChild, blockFiller ) ) { + return true; + } + + // Not matching types. + return false; +} From 4beb2267141ddb5e72122f785af7492eca263a40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Wed, 13 Jun 2018 17:04:53 +0200 Subject: [PATCH 19/21] Make 'renderer._diffChildren()' method follow single responsibility principle. --- src/view/renderer.js | 79 ++++++++++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 33 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index 262602634..8f86b8a25 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -247,8 +247,7 @@ export default class Renderer { * @param {module:engine/view/node~Node} viewElement The view element which children mappings will be updated. */ _updateChildrenMappings( viewElement ) { - // We're diffing only so we don't have to bind anything. - const diff = this._diffChildren( viewElement, { bind: false, withChildren: false } ); + const diff = this._diffChildren( viewElement ); if ( !diff ) { return; @@ -282,11 +281,11 @@ export default class Renderer { } /** - * Updates mappings of a given view element. + * Updates mappings of a given `viewElement`. * * @private * @param {module:engine/view/node~Node} viewElement The view element which mappings will be updated. - * @param {Node} domElement DOM representation of a given view element. + * @param {Node} domElement The DOM element representing given view element. */ _updateElementMappings( viewElement, domElement ) { // Because we replace new view element mapping with the existing one, the corresponding DOM element @@ -513,7 +512,7 @@ export default class Renderer { * Checks if attributes list needs to be updated and possibly updates it. * * @private - * @param {module:engine/view/element~Element} viewElement View element to update. + * @param {module:engine/view/element~Element} viewElement The view element to update. */ _updateAttrs( viewElement ) { const domElement = this.domConverter.mapViewToDom( viewElement ); @@ -552,8 +551,8 @@ export default class Renderer { * filler should be rendered. */ _updateChildren( viewElement, options ) { - const diff = this._diffChildren( viewElement, - { inlineFillerPosition: options.inlineFillerPosition, bind: true, withChildren: true } ); + const inlineFillerPosition = options.inlineFillerPosition; + const diff = this._diffChildren( viewElement, inlineFillerPosition ); if ( !diff ) { return; @@ -562,7 +561,7 @@ export default class Renderer { const actions = diff.actions; const domElement = diff.domElement; const actualDomChildren = diff.actualDomChildren; - const expectedDomChildren = diff.expectedDomChildren; + const expectedDomChildren = this._getElementExpectedChildren( viewElement, domElement, { bind: true, inlineFillerPosition } ); let i = 0; const nodesToUnbind = new Set(); @@ -592,23 +591,20 @@ export default class Renderer { } /** - * Compares viewElement actual and expected children and actions sequence which can be used to transform + * Compares `viewElement`'s actual and expected children and returns actions sequence which can be used to transform * actual children into expected ones. * * @private - * @param viewElement - * @param {Object} options - * @param {module:engine/view/position~Position} options.inlineFillerPosition The position on which the inline + * @param {module:engine/view/node~Node} viewElement The view element which children will be compared. + * @param {module:engine/view/position~Position} [inlineFillerPosition=null] The position on which the inline * filler should be rendered. - * @param {Boolean} options.bind If new view elements should be bind to their corresponding DOM elements. - * @param {Boolean} options.withChildren If children of newly bound view elements should also be converted. * @returns {Object|null} result - * @returns {Array} result.actions List of actions based on {@link module:utils/diff~diff} function. - * @returns {Node} result.domElement ViewElement corresponding DOM element. - * @returns {Array} result.actualDomChildren Current viewElement DOM children. - * @returns {Array} result.expectedDomChildren Expected viewElement DOM children. + * @returns {Array.} result.actions List of actions based on {@link module:utils/diff~diff} function. + * @returns {Node} result.domElement The DOM element representing given view element. + * @returns {Array.} result.actualDomChildren Current `viewElement`'s DOM children. + * @returns {Array.} result.expectedDomChildren Expected `viewElement`'s DOM children. */ - _diffChildren( viewElement, options ) { + _diffChildren( viewElement, inlineFillerPosition = null ) { const domConverter = this.domConverter; const domElement = domConverter.mapViewToDom( viewElement ); @@ -618,17 +614,9 @@ export default class Renderer { return null; } - const domDocument = domElement.ownerDocument; - const filler = options.inlineFillerPosition; const actualDomChildren = domElement.childNodes; - const expectedDomChildren = Array.from( domConverter.viewChildrenToDom( viewElement, domDocument, options ) ); - - // Inline filler element has to be created during children update because we need it to diff actual dom - // elements with expected dom elements. We need inline filler in expected dom elements so we won't re-render - // text node if it is not necessary. - if ( filler && filler.parent == viewElement ) { - this._addInlineFiller( domDocument, expectedDomChildren, filler.offset ); - } + const expectedDomChildren = this._getElementExpectedChildren( viewElement, domElement, + { withChildren: false, inlineFillerPosition } ); return { actions: diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, domConverter.blockFiller ) ), @@ -638,6 +626,31 @@ export default class Renderer { }; } + /** + * Returns expected DOM children for a given `viewElement`. + * + * @private + * @param {module:engine/view/node~Node} viewElement View element which children will be returned. + * @param {Node} domElement DOM representation of a given view element. + * @param {Object} options See {@link module:engine/view/domconverter~DomConverter#viewToDom} options parameter. + * @param {module:engine/view/position~Position} [options.inlineFillerPosition=null] The position on which + * the inline filler should be rendered. + * @returns {Array.} The `viewElement`'s expected children. + */ + _getElementExpectedChildren( viewElement, domElement, options ) { + const expectedDomChildren = Array.from( this.domConverter.viewChildrenToDom( viewElement, domElement.ownerDocument, options ) ); + const filler = options.inlineFillerPosition; + + // Inline filler element has to be created as it is present in a DOM, but not in a view. It is required + // during diffing so text nodes could be compared correctly and also during rendering to maintain + // proper order and indexes while updating the DOM. + if ( filler && filler.parent === viewElement ) { + this._addInlineFiller( domElement.ownerDocument, expectedDomChildren, filler.offset ); + } + + return expectedDomChildren; + } + /** * Finds DOM nodes which were replaced with the similar nodes (same tag name) in the view. All nodes are compared * within one `insert`/`delete` action group, for example: @@ -648,10 +661,10 @@ export default class Renderer { * Output actions: [ insert, replace, delete, equal, replace ] * * @private - * @param {Array} actions Actions array which is result of {@link module:utils/diff~diff} function. - * @param {Array} actualDom Actual DOM children - * @param {Array} expectedDom Expected DOM children. - * @returns {Array} Actions array modified with `replace` actions. + * @param {Array.} actions Actions array which is result of {@link module:utils/diff~diff} function. + * @param {Array.} actualDom Actual DOM children + * @param {Array.} expectedDom Expected DOM children. + * @returns {Array.} Actions array modified with `replace` actions. */ _findReplaceActions( actions, actualDom, expectedDom ) { // If there is no both 'insert' and 'delete' actions, no need to check for replaced elements. From 34a1788070c512182b17e11db4533da2c54dcdd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 18 Jun 2018 11:48:30 +0200 Subject: [PATCH 20/21] Bind new DOM elements before diffing. --- src/view/renderer.js | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index 8f86b8a25..d1b3cc875 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -247,12 +247,14 @@ export default class Renderer { * @param {module:engine/view/node~Node} viewElement The view element which children mappings will be updated. */ _updateChildrenMappings( viewElement ) { - const diff = this._diffChildren( viewElement ); + const domElement = this.domConverter.mapViewToDom( viewElement ); - if ( !diff ) { + if ( !domElement ) { + // If there is no `domElement` it means that it was already removed from DOM and there is no need to process it. return; } + const diff = this._diffChildren( viewElement ); const actions = this._findReplaceActions( diff.actions, diff.actualDomChildren, diff.expectedDomChildren ); if ( actions.indexOf( 'replace' ) !== -1 ) { @@ -551,22 +553,24 @@ export default class Renderer { * filler should be rendered. */ _updateChildren( viewElement, options ) { - const inlineFillerPosition = options.inlineFillerPosition; - const diff = this._diffChildren( viewElement, inlineFillerPosition ); + const domElement = this.domConverter.mapViewToDom( viewElement ); - if ( !diff ) { + if ( !domElement ) { + // If there is no `domElement` it means that it was already removed from DOM. + // There is no need to process it. It will be processed when re-inserted. return; } - const actions = diff.actions; - const domElement = diff.domElement; - const actualDomChildren = diff.actualDomChildren; + const inlineFillerPosition = options.inlineFillerPosition; + // As binding may change actual DOM children we need to do this before diffing. const expectedDomChildren = this._getElementExpectedChildren( viewElement, domElement, { bind: true, inlineFillerPosition } ); + const diff = this._diffChildren( viewElement, inlineFillerPosition ); + const actualDomChildren = diff.actualDomChildren; let i = 0; const nodesToUnbind = new Set(); - for ( const action of actions ) { + for ( const action of diff.actions ) { if ( action === 'insert' ) { insertAt( domElement, i, expectedDomChildren[ i ] ); i++; @@ -600,27 +604,17 @@ export default class Renderer { * filler should be rendered. * @returns {Object|null} result * @returns {Array.} result.actions List of actions based on {@link module:utils/diff~diff} function. - * @returns {Node} result.domElement The DOM element representing given view element. * @returns {Array.} result.actualDomChildren Current `viewElement`'s DOM children. * @returns {Array.} result.expectedDomChildren Expected `viewElement`'s DOM children. */ _diffChildren( viewElement, inlineFillerPosition = null ) { - const domConverter = this.domConverter; - const domElement = domConverter.mapViewToDom( viewElement ); - - if ( !domElement ) { - // If there is no `domElement` it means that it was already removed from DOM. - // There is no need to process it. It will be processed when re-inserted. - return null; - } - + const domElement = this.domConverter.mapViewToDom( viewElement ); const actualDomChildren = domElement.childNodes; const expectedDomChildren = this._getElementExpectedChildren( viewElement, domElement, { withChildren: false, inlineFillerPosition } ); return { - actions: diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, domConverter.blockFiller ) ), - domElement, + actions: diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, this.domConverter.blockFiller ) ), actualDomChildren, expectedDomChildren }; From 7e1e9630892feaed1e027a6e30d652e5cae9a887 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 18 Jun 2018 12:35:37 +0200 Subject: [PATCH 21/21] Tests: unit test covering 'linking styled content' case. --- tests/view/renderer.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/view/renderer.js b/tests/view/renderer.js index b6e5ca091..7e6600138 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -2983,6 +2983,30 @@ describe( 'Renderer', () => { expect( normalizeHtml( domRoot.innerHTML ) ).to.equal( normalizeHtml( '

    FooUI2 Bar

    ' ) ); } ); + + it( 'should handle linking entire content', () => { + viewRoot._appendChild( parse( 'FooBar' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

    FooBar

    ' ); + + const viewP = viewRoot.getChild( 0 ); + // While linking, the existing DOM children are moved to a new `a` element during binding + // inside the `domConverter.viewToDom()` method. It happens because of a modified view structure + // where view elements were moved to a newly created link view element. + const viewA = new ViewAttributeElement( 'a', { href: '#href' }, [ new ViewText( 'Foo' ), viewP.getChild( 1 ) ] ); + + viewP._removeChildren( 0, viewP.childCount ); + viewP._insertChild( 0, viewA ); + + renderer.markToSync( 'children', viewRoot ); + renderer.markToSync( 'children', viewP ); + renderer.render(); + + expect( domRoot.innerHTML ).to.equal( '

    FooBar

    ' ); + } ); } ); } );