Przeglądaj źródła

Fixed: ViewCollection and DOM desynchronise in some cases.

Aleksander Nowodzinski 9 lat temu
rodzic
commit
63e4246846

+ 6 - 17
packages/ckeditor5-ui/src/viewcollection.js

@@ -34,14 +34,14 @@ export default class ViewCollection extends Collection {
 
 		// Handle {@link module:ui/view~View#element} in DOM when a new view is added to the collection.
 		this.on( 'add', ( evt, view, index ) => {
-			if ( this.ready && view.element && this._parentElement ) {
+			if ( view.element && this._parentElement ) {
 				this._parentElement.insertBefore( view.element, this._parentElement.children[ index ] );
 			}
 		} );
 
 		// Handle {@link module:ui/view~View#element} in DOM when a view is removed from the collection.
 		this.on( 'remove', ( evt, view ) => {
-			if ( this.ready && view.element && this._parentElement ) {
+			if ( view.element && this._parentElement ) {
 				view.element.remove();
 			}
 		} );
@@ -97,21 +97,10 @@ export default class ViewCollection extends Collection {
 			throw new CKEditorError( 'ui-viewcollection-init-reinit: This ViewCollection has already been initialized.' );
 		}
 
-		const promises = [];
-
-		for ( let view of this ) {
-			// Do not render unbound children. They're already in DOM by explicit declaration
-			// in Template definition.
-			if ( this._parentElement && view.element ) {
-				this._parentElement.appendChild( view.element );
-			}
-
-			promises.push( view.init() );
-		}
-
-		return Promise.all( promises ).then( () => {
-			this.ready = true;
-		} );
+		return Promise.all( this.map( v => v.init() ) )
+			.then( () => {
+				this.ready = true;
+			} );
 	}
 
 	/**

+ 122 - 24
packages/ckeditor5-ui/tests/viewcollection.js

@@ -3,7 +3,7 @@
  * For licensing, see LICENSE.md.
  */
 
-/* global document */
+/* global document, setTimeout */
 
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import Collection from '@ckeditor/ckeditor5-utils/src/collection';
@@ -35,39 +35,89 @@ describe( 'ViewCollection', () => {
 			expect( new ViewCollection( locale ).locale ).to.equal( locale );
 		} );
 
-		it( 'manages view#element of the children in DOM', () => {
-			const parentElement = document.createElement( 'p' );
-			collection.setParent( parentElement );
+		describe( 'child view management in DOM', () => {
+			it( 'manages view#element of the children in DOM', () => {
+				const parentElement = document.createElement( 'p' );
+				collection.setParent( parentElement );
 
-			const viewA = new View();
+				const viewA = new View();
 
-			expect( () => {
-				collection.add( viewA );
-				collection.remove( viewA );
-			} ).to.not.throw();
+				expect( () => {
+					collection.add( viewA );
+					collection.remove( viewA );
+				} ).to.not.throw();
 
-			expect( () => {
-				collection.ready = true;
+				expect( () => {
+					collection.ready = true;
+					collection.add( viewA );
+					collection.remove( viewA );
+				} ).to.not.throw();
+
+				viewA.element = document.createElement( 'b' );
 				collection.add( viewA );
+
+				expect( normalizeHtml( parentElement.outerHTML ) ).to.equal( '<p><b></b></p>' );
+
+				const viewB = new View();
+				viewB.element = document.createElement( 'i' );
+
+				collection.add( viewB, 0 );
+				expect( normalizeHtml( parentElement.outerHTML ) ).to.equal( '<p><i></i><b></b></p>' );
+
 				collection.remove( viewA );
-			} ).to.not.throw();
+				expect( normalizeHtml( parentElement.outerHTML ) ).to.equal( '<p><i></i></p>' );
 
-			viewA.element = document.createElement( 'b' );
-			collection.add( viewA );
+				collection.remove( viewB );
+				expect( normalizeHtml( parentElement.outerHTML ) ).to.equal( '<p></p>' );
+			} );
 
-			expect( normalizeHtml( parentElement.outerHTML ) ).to.equal( '<p><b></b></p>' );
+			it( 'always keeps the collection synchronized with DOM', () => {
+				const view = new View();
+				const viewA = getView( 'A' );
+				const viewB = getView( 'B' );
+
+				function getView( text ) {
+					const view = new View();
+
+					view.template = new Template( {
+						tag: 'li',
+						children: [
+							{
+								text: text
+							}
+						]
+					} );
 
-			const viewB = new View();
-			viewB.element = document.createElement( 'i' );
+					return view;
+				}
 
-			collection.add( viewB, 0 );
-			expect( normalizeHtml( parentElement.outerHTML ) ).to.equal( '<p><i></i><b></b></p>' );
+				// Fill the collection with children.
+				collection.add( viewA );
+				collection.add( viewB );
+
+				// Put the collection in the template.
+				view.template = new Template( {
+					tag: 'ul',
+					children: collection
+				} );
 
-			collection.remove( viewA );
-			expect( normalizeHtml( parentElement.outerHTML ) ).to.equal( '<p><i></i></p>' );
+				// Render view#template along with collection of children.
+				view.element;
 
-			collection.remove( viewB );
-			expect( normalizeHtml( parentElement.outerHTML ) ).to.equal( '<p></p>' );
+				const viewC = getView( 'C' );
+
+				// Modify the collection, while the view#element has already
+				// been rendered but the view has **not** been inited yet.
+				collection.add( viewC );
+				collection.remove( 1 );
+
+				// Finally init the view. Check what's in there.
+				return view.init().then( () => {
+					expect( view.element.childNodes ).to.have.length( 2 );
+					expect( view.element.childNodes[ 0 ] ).to.equal( viewA.element );
+					expect( view.element.childNodes[ 1 ] ).to.equal( viewC.element );
+				} );
+			} );
 		} );
 	} );
 
@@ -76,6 +126,54 @@ describe( 'ViewCollection', () => {
 			expect( collection.init() ).to.be.instanceof( Promise );
 		} );
 
+		it( 'should return a composition of child init() promises', () => {
+			const spyA = sinon.spy().named( 'A' );
+			const spyB = sinon.spy().named( 'B' );
+			const spyC = sinon.spy().named( 'C' );
+
+			const childA = {
+				init: () => {
+					return new Promise( ( resolve ) => {
+						setTimeout( () => {
+							spyA();
+							resolve();
+						}, 20 );
+					} );
+				}
+			};
+
+			const childB = {
+				init: () => {
+					return new Promise( ( resolve ) => {
+						setTimeout( () => {
+							spyB();
+							resolve();
+						}, 10 );
+					} );
+				}
+			};
+
+			const childC = {
+				init: () => {
+					return new Promise( ( resolve ) => {
+						setTimeout( () => {
+							spyC();
+							resolve();
+						}, 0 );
+					} );
+				}
+			};
+
+			collection.add( childA );
+			collection.add( childB );
+			collection.add( childC );
+
+			return collection.init()
+				.then( () => {
+					sinon.assert.callOrder( spyC, spyB, spyA );
+				} );
+		} );
+
 		it( 'should throw if already initialized', () => {
 			return collection.init()
 				.then( () => {
@@ -182,7 +280,7 @@ describe( 'ViewCollection', () => {
 		} );
 	} );
 
-	describe( 'setParent', () => {
+	describe( 'setParent()', () => {
 		it( 'sets #_parentElement', () => {
 			const el = {};
 			expect( collection._parentElement ).to.be.null;