From a1607f34f387b7a5053741901f7ae82089242fc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Mon, 25 Feb 2019 23:32:02 +0100 Subject: [PATCH 1/2] Filter out FSC before comparing expected children with the actual one because FSC is never returned as part of the expected ones. --- src/view/renderer.js | 18 ++++++ tests/view/renderer.js | 141 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 157 insertions(+), 2 deletions(-) diff --git a/src/view/renderer.js b/src/view/renderer.js index e0b40be3b..b615c4233 100644 --- a/src/view/renderer.js +++ b/src/view/renderer.js @@ -581,6 +581,8 @@ export default class Renderer { * @returns {Array.} The list of actions based on the {@link module:utils/diff~diff} function. */ _diffNodeLists( actualDomChildren, expectedDomChildren ) { + actualDomChildren = filterOutFakeSelectionContainer( actualDomChildren, this._fakeSelectionContainer ); + return diff( actualDomChildren, expectedDomChildren, sameNodes.bind( null, this.domConverter.blockFiller ) ); } @@ -955,3 +957,19 @@ function fixGeckoSelectionAfterBr( focus, domSelection ) { domSelection.addRange( domSelection.getRangeAt( 0 ) ); } } + +function filterOutFakeSelectionContainer( domChildList, fakeSelectionContainer ) { + const childList = Array.from( domChildList ); + + if ( childList.length == 0 || !fakeSelectionContainer ) { + return childList; + } + + const last = childList[ childList.length - 1 ]; + + if ( last == fakeSelectionContainer ) { + childList.pop(); + } + + return childList; +} diff --git a/tests/view/renderer.js b/tests/view/renderer.js index c505bf206..4bac91d7f 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -/* globals document, window, NodeFilter */ +/* globals document, window, NodeFilter, MutationObserver */ import View from '../../src/view/view'; import ViewElement from '../../src/view/element'; @@ -2302,7 +2302,7 @@ describe( 'Renderer', () => { } ); // #1417 - describe( 'optimal rendering', () => { + describe( 'optimal rendering – reusing existing nodes', () => { it( 'should render inline element replacement (before text)', () => { viewRoot._appendChild( parse( 'A1' ) ); @@ -3126,6 +3126,143 @@ describe( 'Renderer', () => { } ); } ); + describe( 'optimal (minimal) rendering – minimal children changes', () => { + let observer; + + beforeEach( () => { + observer = new MutationObserver( () => {} ); + + observer.observe( domRoot, { + childList: true, + attributes: false, + subtree: false + } ); + } ); + + afterEach( () => { + observer.disconnect(); + } ); + + it( 'should add only one child (at the beginning)', () => { + viewRoot._appendChild( parse( '1' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._insertChild( 0, parse( '2' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 1, removed: 0' + ] ); + } ); + + it( 'should add only one child (at the end)', () => { + viewRoot._appendChild( parse( '1' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._appendChild( parse( '2' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 1, removed: 0' + ] ); + } ); + + it( 'should add only one child (in the middle)', () => { + viewRoot._appendChild( parse( '12' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._insertChild( 1, parse( '3' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 1, removed: 0' + ] ); + } ); + + it( 'should not touch elements at all (rendering texts is enough)', () => { + viewRoot._appendChild( parse( '12' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._insertChild( 1, parse( '3' ) ); + viewRoot._removeChildren( 0, 1 ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.be.empty; + } ); + + it( 'should add and remove one', () => { + viewRoot._appendChild( parse( '12' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._insertChild( 1, parse( '3' ) ); + viewRoot._removeChildren( 0, 1 ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 1, removed: 0', + 'added: 0, removed: 1' + ] ); + } ); + + it( 'should not touch the FSC when rendering children', () => { + viewRoot._appendChild( parse( '12' ) ); + + // Set fake selection on the second paragraph. + selection._setTo( viewRoot.getChild( 1 ), 'on', { fake: true } ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + // Remove the second paragraph. + viewRoot._removeChildren( 1, 1 ); + // And set the fake selection on the first one. + selection._setTo( viewRoot.getChild( 0 ), 'on', { fake: true } ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 0, removed: 1' + ] ); + } ); + + function getMutationStats( mutationList ) { + return mutationList.map( mutation => { + return `added: ${ mutation.addedNodes.length }, removed: ${ mutation.removedNodes.length }`; + } ); + } + + function cleanObserver( observer ) { + observer.takeRecords(); + } + } ); + // #1560 describe( 'attributes manipulation on replaced element', () => { it( 'should rerender element if it was removed after having its attributes removed (attribute)', () => { From 4227f1d3d264cb42f2a57765e31e0a124521eb2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 26 Feb 2019 13:21:29 +0100 Subject: [PATCH 2/2] Add test for fast diff in renderer. --- tests/view/renderer.js | 121 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/tests/view/renderer.js b/tests/view/renderer.js index 4bac91d7f..dd4772a57 100644 --- a/tests/view/renderer.js +++ b/tests/view/renderer.js @@ -3252,6 +3252,117 @@ describe( 'Renderer', () => { ] ); } ); + describe( 'using fastDiff() - significant number of nodes in the editor', () => { + it( 'should add only one child (at the beginning)', () => { + viewRoot._appendChild( parse( makeContainers( 151 ) ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._insertChild( 0, parse( 'x' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 1, removed: 0' + ] ); + } ); + + it( 'should add only one child (at the end)', () => { + viewRoot._appendChild( parse( makeContainers( 151 ) ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._appendChild( parse( 'x' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 1, removed: 0' + ] ); + } ); + + it( 'should add only one child (in the middle)', () => { + viewRoot._appendChild( parse( makeContainers( 151 ) ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._insertChild( 75, parse( 'x' ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 1, removed: 0' + ] ); + } ); + + it( 'should not touch elements at all (rendering texts is enough)', () => { + viewRoot._appendChild( parse( makeContainers( 151 ) ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._insertChild( 1, parse( 'x' ) ); + viewRoot._removeChildren( 0, 1 ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.be.empty; + } ); + + it( 'should add and remove one', () => { + viewRoot._appendChild( parse( makeContainers( 151 ) ) ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + viewRoot._insertChild( 1, parse( 'x' ) ); + viewRoot._removeChildren( 0, 1 ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 1, removed: 0', + 'added: 0, removed: 1' + ] ); + } ); + + it( 'should not touch the FSC when rendering children', () => { + viewRoot._appendChild( parse( makeContainers( 151 ) ) ); + + // Set fake selection on the second paragraph. + selection._setTo( viewRoot.getChild( 1 ), 'on', { fake: true } ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + cleanObserver( observer ); + + // Remove the second paragraph. + viewRoot._removeChildren( 1, 1 ); + // And set the fake selection on the first one. + selection._setTo( viewRoot.getChild( 0 ), 'on', { fake: true } ); + + renderer.markToSync( 'children', viewRoot ); + renderer.render(); + + expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [ + 'added: 0, removed: 1' + ] ); + } ); + } ); + function getMutationStats( mutationList ) { return mutationList.map( mutation => { return `added: ${ mutation.addedNodes.length }, removed: ${ mutation.removedNodes.length }`; @@ -3261,6 +3372,16 @@ describe( 'Renderer', () => { function cleanObserver( observer ) { observer.takeRecords(); } + + function makeContainers( howMany ) { + const containers = []; + + for ( let i = 1; i <= howMany; i++ ) { + containers.push( `${ i }` ); + } + + return containers.join( '' ); + } } ); // #1560