Browse Source

Merge pull request #1687 from ckeditor/t/ckeditor5/1578b

Fix: Filter out fake selection container before comparing DOM view root children in view renderer. Closes ckeditor/ckeditor5#1578.
Maciej 6 years ago
parent
commit
d7de7d67bd

+ 18 - 0
packages/ckeditor5-engine/src/view/renderer.js

@@ -581,6 +581,8 @@ export default class Renderer {
 	 * @returns {Array.<String>} 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;
+}

+ 260 - 2
packages/ckeditor5-engine/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( '<container:p><attribute:i>A</attribute:i>1</container:p>' ) );
 
@@ -3126,6 +3126,264 @@ 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( '<container:p>1</container:p>' ) );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+				cleanObserver( observer );
+
+				viewRoot._insertChild( 0, parse( '<container:p>2</container:p>' ) );
+
+				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( '<container:p>1</container:p>' ) );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+				cleanObserver( observer );
+
+				viewRoot._appendChild( parse( '<container:p>2</container:p>' ) );
+
+				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( '<container:p>1</container:p><container:p>2</container:p>' ) );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+				cleanObserver( observer );
+
+				viewRoot._insertChild( 1, parse( '<container:p>3</container:p>' ) );
+
+				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( '<container:p>1</container:p><container:p>2</container:p>' ) );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+				cleanObserver( observer );
+
+				viewRoot._insertChild( 1, parse( '<container:p>3</container:p>' ) );
+				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( '<container:p>1</container:p><container:p>2</container:p>' ) );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+				cleanObserver( observer );
+
+				viewRoot._insertChild( 1, parse( '<container:h1>3</container:h1>' ) );
+				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( '<container:p>1</container:p><container:p>2</container:p>' ) );
+
+				// 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'
+				] );
+			} );
+
+			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( '<container:p>x</container:p>' ) );
+
+					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( '<container:p>x</container:p>' ) );
+
+					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( '<container:p>x</container:p>' ) );
+
+					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( '<container:p>x</container:p>' ) );
+					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( '<container:h1>x</container:h1>' ) );
+					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 }`;
+				} );
+			}
+
+			function cleanObserver( observer ) {
+				observer.takeRecords();
+			}
+
+			function makeContainers( howMany ) {
+				const containers = [];
+
+				for ( let i = 1; i <= howMany; i++ ) {
+					containers.push( `<container:p>${ i }</container:p>` );
+				}
+
+				return containers.join( '' );
+			}
+		} );
+
 		// #1560
 		describe( 'attributes manipulation on replaced element', () => {
 			it( 'should rerender element if it was removed after having its attributes removed (attribute)', () => {