ソースを参照

Added docs and tests to engine view controller.

Szymon Kupś 7 年 前
コミット
6e38219aba

+ 46 - 3
packages/ckeditor5-engine/src/view/view.js

@@ -106,7 +106,20 @@ export default class View {
 		injectQuirksHandling( this );
 		injectUiElementHandling( this );
 
+		/**
+		 * Is set to `true` when {@link #change view changes} are currently in progress.
+		 *
+		 * @private
+		 * @member {Boolean} module:engine/view/view~View#_ongoingChange
+		 */
 		this._ongoingChange = false;
+
+		/**
+		 * Is set to `true` when rendering view to DOM is currently in progress.
+		 *
+		 * @private
+		 * @member {Boolean} module:engine/view/view~View#_renderingInProgress
+		 */
 		this._renderingInProgress = false;
 	}
 
@@ -228,8 +241,8 @@ export default class View {
 	}
 
 	/**
-	 * Focuses document. It will focus {@link module:engine/view/editableelement~EditableElement EditableElement} that is currently having
-	 * selection inside.
+	 * It will focus DOM element representing {@link module:engine/view/editableelement~EditableElement EditableElement}
+	 * that is currently having selection inside.
 	 */
 	focus() {
 		if ( !this.document.isFocused ) {
@@ -251,6 +264,29 @@ export default class View {
 		}
 	}
 
+	/**
+	 * Change method is the primary way of changing the view. You should use it to modify any node in the view tree.
+	 * It makes sure that after all changes are made view is rendered to DOM. It prevents situations when DOM is updated
+	 * when view state is not yet correct. It allows to nest calls one inside another and still perform single rendering
+	 * after all changes are applied.
+	 *
+	 *		view.change( writer => {
+	 *			writer.insert( position1, writer.createText( 'foo' );
+	 *
+	 *			view.change( writer => {
+	 *				writer.insert( position2, writer.createText( 'bar' );
+	 *			} );
+	 *
+	 * 			writer.remove( range );
+	 *		} );
+	 *
+	 * Change block is executed immediately.
+	 *
+	 * When the outermost block is done and rendering to DOM is over it fires {@link module:engine/view/document~Document#change }
+	 * event.
+	 *
+	 * @param {Function} callback Callback function which may modify the view.
+	 */
 	change( callback ) {
 		if ( this._renderingInProgress ) {
 			/**
@@ -260,7 +296,7 @@ export default class View {
 			 */
 			log.warn(
 				'applying-view-changes-on-rendering: ' +
-				'Attempting to make changes in the view during rendering process.' +
+				'Attempting to make changes in the view during rendering process. ' +
 				'This may cause some unexpected behaviour and inconsistency between the DOM and the view.'
 			);
 		}
@@ -281,6 +317,10 @@ export default class View {
 		}
 	}
 
+	/**
+	 * Renders {@link module:engine/view/document~Document view document} to DOM. If any view changes are
+	 * currently in progress, rendering will start after all {@link #change change blocks} are processed.
+	 */
 	render() {
 		// Render only if no ongoing changes in progress. If there are some, view document will be rendered after all
 		// changes are done. This way view document will not be rendered in the middle of some changes.
@@ -289,6 +329,9 @@ export default class View {
 		}
 	}
 
+	/**
+	 * Destroys this instance. Makes sure that all observers are destroyed and listeners removed.
+	 */
 	destroy() {
 		for ( const observer of this._observers.values() ) {
 			observer.destroy();

+ 90 - 0
packages/ckeditor5-engine/tests/view/view/view.js

@@ -15,6 +15,7 @@ import global from '@ckeditor/ckeditor5-utils/src/dom/global';
 import ViewRange from '../../../src/view/range';
 import RootEditableElement from '../../../src/view/rooteditableelement';
 import ViewElement from '../../../src/view/element';
+import ViewPosition from '../../../src/view/position';
 import { isBlockFiller, BR_FILLER } from '../../../src/view/filler';
 import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement';
 
@@ -91,6 +92,7 @@ describe( 'view', () => {
 			expect( view.domConverter.mapViewToDom( viewRoot ) ).to.equal( domDiv );
 
 			expect( view.renderer.markedChildren.has( viewRoot ) ).to.be.true;
+			domDiv.remove();
 		} );
 
 		it( 'should attach DOM element to custom view element', () => {
@@ -394,6 +396,7 @@ describe( 'view', () => {
 			expect( isBlockFiller( domDiv.childNodes[ 0 ], BR_FILLER ) ).to.be.true;
 
 			view.destroy();
+			domDiv.remove();
 		} );
 
 		it( 'should render changes in the Document', () => {
@@ -436,6 +439,93 @@ describe( 'view', () => {
 			expect( domRoot.childNodes[ 0 ].getAttribute( 'class' ) ).to.equal( 'bar' );
 
 			view.destroy();
+			domRoot.remove();
+		} );
+
+		describe( 'change()', () => {
+			it( 'should call render and fire event after the change', () => {
+				const renderSpy = sinon.spy();
+				const changeSpy = sinon.spy();
+				view.renderer.on( 'render', renderSpy );
+				view.document.on( 'change', changeSpy );
+
+				view.change( () => {} );
+
+				sinon.assert.callOrder( renderSpy, changeSpy );
+			} );
+
+			it( 'should render and fire change event once for nested change blocks', () => {
+				const renderSpy = sinon.spy();
+				const changeSpy = sinon.spy();
+				view.renderer.on( 'render', renderSpy );
+				view.document.on( 'change', changeSpy );
+
+				view.change( () => {
+					view.change( () => {} );
+					view.change( () => {
+						view.change( () => {} );
+						view.change( () => {} );
+					} );
+					view.change( () => {} );
+				} );
+
+				sinon.assert.calledOnce( renderSpy );
+				sinon.assert.calledOnce( changeSpy );
+				sinon.assert.callOrder( renderSpy, changeSpy );
+			} );
+
+			it( 'should render and fire change event once even if render is called during the change', () => {
+				const renderSpy = sinon.spy();
+				const changeSpy = sinon.spy();
+				view.renderer.on( 'render', renderSpy );
+				view.document.on( 'change', changeSpy );
+
+				view.change( () => {
+					view.render();
+					view.change( () => {
+						view.render();
+					} );
+					view.render();
+				} );
+
+				sinon.assert.calledOnce( renderSpy );
+				sinon.assert.calledOnce( changeSpy );
+				sinon.assert.callOrder( renderSpy, changeSpy );
+			} );
+
+			it( 'should log warning when someone tries to change view during rendering', () => {
+				const domDiv = document.createElement( 'div' );
+				const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
+				sinon.stub( log, 'warn' );
+				view.attachDomRoot( domDiv );
+
+				view.change( writer => {
+					const p = writer.createContainerElement( 'p' );
+					const ui = writer.createUIElement( 'span' );
+
+					// This UIElement will try to modify view tree during rendering.
+					ui.render = function( domDocument ) {
+						const element = this.toDomElement( domDocument );
+
+						view.change( () => {} );
+
+						return element;
+					};
+
+					writer.insert( ViewPosition.createAt( p ), ui );
+					writer.insert( ViewPosition.createAt( viewRoot ), p );
+				} );
+
+				sinon.assert.calledOnce( log.warn );
+				sinon.assert.calledWithExactly( log.warn,
+					'applying-view-changes-on-rendering: ' +
+					'Attempting to make changes in the view during rendering process. ' +
+					'This may cause some unexpected behaviour and inconsistency between the DOM and the view.'
+				);
+
+				domDiv.remove();
+				log.warn.restore();
+			} );
 		} );
 	} );