Explorar el Código

Get rid of view.document#change event and rednerer#render events. Created render event on view controller. Updated tests.

Szymon Kupś hace 8 años
padre
commit
4e689fd676

+ 1 - 1
packages/ckeditor5-engine/src/view/observer/mutationobserver.js

@@ -59,7 +59,7 @@ export default class MutationObserver extends Observer {
 		 *
 		 * @member {module:engine/view/renderer~Renderer}
 		 */
-		this.renderer = view.renderer;
+		this.renderer = view._renderer;
 
 		/**
 		 * Observed DOM elements.

+ 1 - 1
packages/ckeditor5-engine/src/view/placeholder.js

@@ -37,7 +37,7 @@ export function attachPlaceholder( view, element, placeholderText, checkFunction
 		documentPlaceholders.set( document, new Map() );
 
 		// Attach listener just before rendering and update placeholders.
-		listener.listenTo( view.renderer, 'render', () => updateAllPlaceholders( view ), { priority: 'highest' } );
+		listener.listenTo( view, 'render', () => updateAllPlaceholders( view ) );
 	}
 
 	// Store text in element's data attribute.

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

@@ -110,7 +110,6 @@ export default class Renderer {
 		 * @type {null|HTMLElement}
 		 */
 		this._fakeSelectionContainer = null;
-		this.decorate( 'render' );
 	}
 
 	/**
@@ -709,13 +708,6 @@ export default class Renderer {
 			}
 		}
 	}
-
-	/**
-	 * Fired when {@link #render render} method is called. Actual rendering is executed as a listener to
-	 * this event with default priority. This way other listeners can be used to run code before or after rendering.
-	 *
-	 * @event render
-	 */
 }
 
 mix( Renderer, ObservableMixin );

+ 71 - 37
packages/ckeditor5-engine/src/view/view.js

@@ -22,9 +22,9 @@ import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
 import log from '@ckeditor/ckeditor5-utils/src/log';
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
 import { scrollViewportToShowTarget } from '@ckeditor/ckeditor5-utils/src/dom/scroll';
-import priorities from '@ckeditor/ckeditor5-utils/src/priorities';
 import { injectUiElementHandling } from './uielement';
 import { injectQuirksHandling } from './filler';
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 /**
  * Editor's view controller class.
@@ -71,11 +71,11 @@ export default class View {
 		/**
 		 * Instance of the {@link module:engine/view/renderer~Renderer renderer}.
 		 *
-		 * @readonly
+		 * @protected
 		 * @member {module:engine/view/renderer~Renderer} module:engine/view/view~View#renderer
 		 */
-		this.renderer = new Renderer( this.domConverter, this.document.selection );
-		this.renderer.bind( 'isFocused' ).to( this.document );
+		this._renderer = new Renderer( this.domConverter, this.document.selection );
+		this._renderer.bind( 'isFocused' ).to( this.document );
 
 		/**
 		 * Roots of the DOM tree. Map on the `HTMLElement`s with roots names as keys.
@@ -102,12 +102,15 @@ export default class View {
 		this._ongoingChange = false;
 
 		/**
-		 * Is set to `true` when rendering view to DOM is currently in progress.
+		 * Is set to `true` when rendering view to DOM was started.
+		 * This is used to check whether view document can accept changes in current state.
+		 * From the moment when rendering to DOM is stared view tree is locked to prevent changes that will not be
+		 * reflected in the DOM.
 		 *
 		 * @private
-		 * @member {Boolean} module:engine/view/view~View#_renderingInProgress
+		 * @member {Boolean} module:engine/view/view~View#_renderingStarted
 		 */
-		this._renderingInProgress = false;
+		this._renderingStarted = false;
 
 		/**
 		 * Writer instance used in {@link #change change method) callbacks.
@@ -127,6 +130,11 @@ export default class View {
 		// Inject quirks handlers.
 		injectQuirksHandling( this );
 		injectUiElementHandling( this );
+
+		// Use 'low` priority so that all listeners on 'normal` priority will be executed before.
+		this.on( 'render', () => {
+			this._render();
+		}, { priority: 'low' } );
 	}
 
 	/**
@@ -148,12 +156,12 @@ export default class View {
 
 		this.domRoots.set( name, domRoot );
 		this.domConverter.bindElements( domRoot, viewRoot );
-		this.renderer.markToSync( 'children', viewRoot );
-		this.renderer.domDocuments.add( domRoot.ownerDocument );
+		this._renderer.markToSync( 'children', viewRoot );
+		this._renderer.domDocuments.add( domRoot.ownerDocument );
 
-		viewRoot.on( 'change:children', ( evt, node ) => this.renderer.markToSync( 'children', node ) );
-		viewRoot.on( 'change:attributes', ( evt, node ) => this.renderer.markToSync( 'attributes', node ) );
-		viewRoot.on( 'change:text', ( evt, node ) => this.renderer.markToSync( 'text', node ) );
+		viewRoot.on( 'change:children', ( evt, node ) => this._renderer.markToSync( 'children', node ) );
+		viewRoot.on( 'change:attributes', ( evt, node ) => this._renderer.markToSync( 'attributes', node ) );
+		viewRoot.on( 'change:text', ( evt, node ) => this._renderer.markToSync( 'text', node ) );
 
 		for ( const observer of this._observers.values() ) {
 			observer.observe( domRoot, name );
@@ -291,22 +299,14 @@ export default class View {
 	 * When the outermost change block is done and rendering to DOM is over it fires
 	 * {@link module:engine/view/document~Document#event:change} event.
 	 *
+	 * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `applying-view-changes-on-rendering` when
+	 * change block is used after rendering to DOM has started.
+	 *
 	 * @param {Function} callback Callback function which may modify the view.
 	 */
 	change( callback ) {
-		if ( this._renderingInProgress ) {
-			/**
-			 * Warning displayed when there is an attempt to make changes in the view tree during the rendering process.
-			 * This may cause unexpected behaviour and inconsistency between the DOM and the view.
-			 *
-			 * @error applying-view-changes-on-rendering
-			 */
-			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.'
-			);
-		}
+		// Check if change is performed in correct moment.
+		this._assertRenderingInProgress();
 
 		// If other changes are in progress wait with rendering until every ongoing change is over.
 		if ( this._ongoingChange ) {
@@ -315,23 +315,29 @@ export default class View {
 			this._ongoingChange = true;
 
 			callback( this._writer );
-			this._render();
+			this.fire( 'render' );
 
 			this._ongoingChange = false;
-
-			this.document.fire( 'change' );
+			this._renderingStarted = false;
 		}
 	}
 
 	/**
 	 * 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.
+	 *
+	 * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `applying-view-changes-on-rendering` when
+	 * trying to re-render when rendering to DOM has already started.
 	 */
 	render() {
-		// Render only if no ongoing changes in progress. If there are some, view document will be rendered after all
+		// Check if rendering is performed in correct moment.
+		this._assertRenderingInProgress();
+
+		// Render only if no ongoing changes are 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.
 		if ( !this._ongoingChange ) {
-			this._render();
+			this.fire( 'render' );
+			this._renderingStarted = false;
 		}
 	}
 
@@ -353,21 +359,49 @@ export default class View {
 	 * @private
 	 */
 	_render() {
-		// Lock just before rendering and unlock just after.
-		// This way other parts of the code can listen to the `render` event and modify the view tree just before rendering.
-		this.renderer.once( 'render', () => ( this._renderingInProgress = true ), { priority: priorities.get( 'normal' ) + 1 } );
-		this.renderer.once( 'render', () => ( this._renderingInProgress = false ), { priority: priorities.get( 'normal' ) - 1 } );
+		this._renderingStarted = true;
 
 		this.disableObservers();
-		this.renderer.render();
+		this._renderer.render();
 		this.enableObservers();
 	}
 
 	/**
-	 * Fired after a topmost {@link module:engine/view/view~View#change change block} is finished and DOM rendering has
+	 * Throws `applying-view-changes-on-rendering` error when trying to modify or re-render view tree when rendering is
+	 * already started
+	 *
+	 * @private
+	 */
+	_assertRenderingInProgress() {
+		if ( this._renderingStarted ) {
+			/**
+			 * There is an attempt to make changes in the view tree after the rendering process
+			 * has started. This may cause unexpected behaviour and inconsistency between the DOM and the view.
+			 * This may be caused by:
+			 *   * calling `view.change()` or `view.render()` methods during rendering process,
+			 *   * calling `view.change()` or `view.render()` methods in callbacks to
+			 *   {module:engine/view/document~Document#event:change view document change event) on `low` priority, after
+			 *   rendering is over for current `change` block.
+			 *
+			 * @error applying-view-changes-on-rendering
+			 */
+			throw new CKEditorError(
+				'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.'
+			);
+		}
+	}
+
+	/**
+	 * Fired after a topmost {@link module:engine/view/view~View#change change block} is finished and the DOM rendering has
 	 * been executed.
 	 *
-	 * @event module:engine/view/document~Document#event:change
+	 * Actual rendering is performed on 'low' priority. This means that all listeners on 'normal' and above priorities
+	 * will be executed after changes made to view tree but before rendering to the DOM. Use `low` priority for callbacks that
+	 * should be executed after rendering to the DOM.
+	 *
+	 * @event module:engine/view/view~View#event:render
 	 */
 }
 

+ 4 - 4
packages/ckeditor5-engine/tests/view/observer/focusobserver.js

@@ -170,9 +170,9 @@ describe( 'FocusObserver', () => {
 			view.render();
 
 			viewDocument.on( 'selectionChange', selectionChangeSpy );
-			view.renderer.on( 'render', renderSpy, { priority: 'low' } );
+			view.on( 'render', renderSpy, { priority: 'low' } );
 
-			view.renderer.on( 'render', () => {
+			view.on( 'render', () => {
 				sinon.assert.callOrder( selectionChangeSpy, renderSpy );
 				done();
 			}, { priority: 'low' } );
@@ -192,9 +192,9 @@ describe( 'FocusObserver', () => {
 			const domEditable = domRoot.childNodes[ 0 ];
 
 			viewDocument.on( 'selectionChange', selectionChangeSpy );
-			view.renderer.on( 'render', renderSpy, { priority: 'low' } );
+			view.on( 'render', renderSpy, { priority: 'low' } );
 
-			view.renderer.on( 'render', () => {
+			view.on( 'render', () => {
 				sinon.assert.notCalled( selectionChangeSpy );
 				sinon.assert.called( renderSpy );
 

+ 0 - 10
packages/ckeditor5-engine/tests/view/renderer.js

@@ -140,16 +140,6 @@ describe( 'Renderer', () => {
 			domRoot.remove();
 		} );
 
-		it( 'should be decorated', () => {
-			const spy = sinon.spy();
-
-			renderer.on( 'render', spy );
-
-			renderer.render();
-
-			expect( spy.calledOnce ).to.be.true;
-		} );
-
 		it( 'should update attributes', () => {
 			viewRoot._setAttribute( 'class', 'foo' );
 

+ 99 - 44
packages/ckeditor5-engine/tests/view/view/view.js

@@ -18,6 +18,7 @@ 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';
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 describe( 'view', () => {
 	const DEFAULT_OBSERVERS_COUNT = 5;
@@ -91,7 +92,7 @@ describe( 'view', () => {
 			expect( view.getDomRoot() ).to.equal( domDiv );
 			expect( view.domConverter.mapViewToDom( viewRoot ) ).to.equal( domDiv );
 
-			expect( view.renderer.markedChildren.has( viewRoot ) ).to.be.true;
+			expect( view._renderer.markedChildren.has( viewRoot ) ).to.be.true;
 			domDiv.remove();
 		} );
 
@@ -106,7 +107,7 @@ describe( 'view', () => {
 			expect( count( view.domRoots ) ).to.equal( 1 );
 			expect( view.getDomRoot( 'header' ) ).to.equal( domH1 );
 			expect( view.domConverter.mapViewToDom( viewH1 ) ).to.equal( domH1 );
-			expect( view.renderer.markedChildren.has( viewH1 ) ).to.be.true;
+			expect( view._renderer.markedChildren.has( viewH1 ) ).to.be.true;
 		} );
 
 		it( 'should call observe on each observer', () => {
@@ -115,7 +116,7 @@ describe( 'view', () => {
 
 			view = new View();
 			viewDocument = view.document;
-			view.renderer.render = sinon.spy();
+			view._renderer.render = sinon.spy();
 
 			const domDiv1 = document.createElement( 'div' );
 			domDiv1.setAttribute( 'id', 'editor' );
@@ -141,7 +142,7 @@ describe( 'view', () => {
 
 			view = new View();
 			viewDocument = view.document;
-			view.renderer.render = sinon.spy();
+			view._renderer.render = sinon.spy();
 		} );
 
 		afterEach( () => {
@@ -197,7 +198,7 @@ describe( 'view', () => {
 			view.render();
 
 			sinon.assert.calledOnce( observerMock.disable );
-			sinon.assert.calledOnce( view.renderer.render );
+			sinon.assert.calledOnce( view._renderer.render );
 			sinon.assert.calledTwice( observerMock.enable );
 		} );
 
@@ -358,19 +359,19 @@ describe( 'view', () => {
 	describe( 'isFocused', () => {
 		it( 'should change renderer.isFocused too', () => {
 			expect( viewDocument.isFocused ).to.equal( false );
-			expect( view.renderer.isFocused ).to.equal( false );
+			expect( view._renderer.isFocused ).to.equal( false );
 
 			viewDocument.isFocused = true;
 
 			expect( viewDocument.isFocused ).to.equal( true );
-			expect( view.renderer.isFocused ).to.equal( true );
+			expect( view._renderer.isFocused ).to.equal( true );
 		} );
 	} );
 
 	describe( 'render()', () => {
 		it( 'disable observers, renders and enable observers', () => {
 			const observerMock = view.addObserver( ObserverMock );
-			const renderStub = sinon.stub( view.renderer, 'render' );
+			const renderStub = sinon.stub( view._renderer, 'render' );
 
 			view.render();
 
@@ -443,22 +444,26 @@ describe( 'view', () => {
 		} );
 
 		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 );
+			it( 'should fire render event and it should trigger rendering on low priority', () => {
+				const renderSpy = sinon.spy( view._renderer, 'render' );
+				const beforeSpy = sinon.spy();
+				const afterSpy = sinon.spy();
+
+				view.on( 'render', beforeSpy );
+				view.on( 'render', afterSpy, { priority: 'low' } );
 
 				view.change( () => {} );
 
-				sinon.assert.callOrder( renderSpy, changeSpy );
+				sinon.assert.callOrder( beforeSpy, renderSpy, afterSpy );
 			} );
 
-			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 );
+			it( 'should fire render event once for nested change blocks', () => {
+				const renderSpy = sinon.spy( view._renderer, 'render' );
+				const beforeSpy = sinon.spy();
+				const afterSpy = sinon.spy();
+
+				view.on( 'render', beforeSpy );
+				view.on( 'render', afterSpy, { priority: 'low' } );
 
 				view.change( () => {
 					view.change( () => {} );
@@ -469,16 +474,19 @@ describe( 'view', () => {
 					view.change( () => {} );
 				} );
 
+				sinon.assert.calledOnce( beforeSpy );
 				sinon.assert.calledOnce( renderSpy );
-				sinon.assert.calledOnce( changeSpy );
-				sinon.assert.callOrder( renderSpy, changeSpy );
+				sinon.assert.calledOnce( afterSpy );
+				sinon.assert.callOrder( beforeSpy, renderSpy, afterSpy );
 			} );
 
-			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 );
+			it( 'should fire render event once even if render is called during the change', () => {
+				const renderSpy = sinon.spy( view._renderer, 'render' );
+				const beforeSpy = sinon.spy();
+				const afterSpy = sinon.spy();
+
+				view.on( 'render', beforeSpy );
+				view.on( 'render', afterSpy, { priority: 'low' } );
 
 				view.change( () => {
 					view.render();
@@ -488,43 +496,90 @@ describe( 'view', () => {
 					view.render();
 				} );
 
+				sinon.assert.calledOnce( beforeSpy );
 				sinon.assert.calledOnce( renderSpy );
-				sinon.assert.calledOnce( changeSpy );
-				sinon.assert.callOrder( renderSpy, changeSpy );
+				sinon.assert.calledOnce( afterSpy );
+				sinon.assert.callOrder( beforeSpy, renderSpy, afterSpy );
 			} );
 
-			it( 'should log warning when someone tries to change view during rendering', () => {
+			it( 'should throw when someone tries to change view during rendering', () => {
 				const domDiv = document.createElement( 'div' );
 				const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
-				sinon.stub( log, 'warn' );
+				let renderingCalled = false;
 				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 ui = writer.createUIElement( 'span', null, function( domDocument ) {
 						const element = this.toDomElement( domDocument );
 
-						view.change( () => {} );
+						expect( () => view.change( () => {} ) ).to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
+						renderingCalled = true;
 
 						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.'
-				);
+				expect( renderingCalled ).to.be.true;
+				domDiv.remove();
+			} );
+
+			it( 'should throw when someone tries to call render() during rendering', () => {
+				const domDiv = document.createElement( 'div' );
+				const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
+				let renderingCalled = false;
+				view.attachDomRoot( domDiv );
+
+				view.change( writer => {
+					const p = writer.createContainerElement( 'p' );
+					const ui = writer.createUIElement( 'span', null, function( domDocument ) {
+						const element = this.toDomElement( domDocument );
+
+						expect( () => view.render() ).to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
+						renderingCalled = true;
+
+						return element;
+					} );
+					writer.insert( ViewPosition.createAt( p ), ui );
+					writer.insert( ViewPosition.createAt( viewRoot ), p );
+				} );
 
+				expect( renderingCalled ).to.be.true;
 				domDiv.remove();
-				log.warn.restore();
+			} );
+
+			it( 'should throw when someone tries to call change() after rendering is finished but still in change block', () => {
+				view.on( 'render', () => {
+					expect( () => view.change( () => {} ) ).to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
+				}, { priority: 'low' } );
+
+				view.change( () => {} );
+			} );
+
+			it( 'should throw when someone tries to call render() after rendering is finished but still in change block', () => {
+				view.on( 'render', () => {
+					expect( () => view.render() ).to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
+				}, { priority: 'low' } );
+
+				view.change( () => {} );
+			} );
+
+			it( 'should NOT throw when someone tries to call change() before rendering', () => {
+				view.on( 'render', () => {
+					expect( () => view.change( () => {} ) ).not.to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
+				}, { priority: 'normal' } );
+
+				view.change( () => {} );
+			} );
+
+			it( 'should NOT throw when someone tries to call render() before rendering', () => {
+				view.on( 'render', () => {
+					expect( () => view.render() ).not.to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
+				}, { priority: 'normal' } );
+
+				view.change( () => {} );
 			} );
 		} );
 	} );