Sfoglia il codice sorgente

New change block for changes made in render event listeners.

Szymon Kupś 7 anni fa
parent
commit
8e35580868

+ 20 - 12
packages/ckeditor5-engine/src/view/placeholder.js

@@ -103,9 +103,11 @@ function updateSinglePlaceholder( view, element, checkFunction ) {
 
 	// If checkFunction is provided and returns false - remove placeholder.
 	if ( checkFunction && !checkFunction() ) {
-		view.change( writer => {
-			writer.removeClass( 'ck-placeholder', element );
-		} );
+		if ( element.hasClass( 'ck-placeholder' ) ) {
+			view.change( writer => {
+				writer.removeClass( 'ck-placeholder', element );
+			} );
+		}
 
 		return;
 	}
@@ -116,21 +118,27 @@ function updateSinglePlaceholder( view, element, checkFunction ) {
 
 	// If element is empty and editor is blurred.
 	if ( !document.isFocused && isEmptyish ) {
-		view.change( writer => {
-			writer.addClass( 'ck-placeholder', element );
-		} );
+		if ( !element.hasClass( 'ck-placeholder' ) ) {
+			view.change( writer => {
+				writer.addClass( 'ck-placeholder', element );
+			} );
+		}
 
 		return;
 	}
 
 	// It there are no child elements and selection is not placed inside element.
 	if ( isEmptyish && anchor && anchor.parent !== element ) {
-		view.change( writer => {
-			writer.addClass( 'ck-placeholder', element );
-		} );
+		if ( !element.hasClass( 'ck-placeholder' ) ) {
+			view.change( writer => {
+				writer.addClass( 'ck-placeholder', element );
+			} );
+		}
 	} else {
-		view.change( writer => {
-			writer.removeClass( 'ck-placeholder', element );
-		} );
+		if ( element.hasClass( 'ck-placeholder' ) ) {
+			view.change( writer => {
+				writer.removeClass( 'ck-placeholder', element );
+			} );
+		}
 	}
 }

+ 33 - 64
packages/ckeditor5-engine/src/view/view.js

@@ -24,7 +24,6 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix';
 import { scrollViewportToShowTarget } from '@ckeditor/ckeditor5-utils/src/dom/scroll';
 import { injectUiElementHandling } from './uielement';
 import { injectQuirksHandling } from './filler';
-import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 /**
  * Editor's view controller class. Its main responsibility is DOM - View management for editing purposes, to provide
@@ -108,16 +107,8 @@ export default class View {
 		 */
 		this._ongoingChange = false;
 
-		/**
-		 * Is set to `true` when rendering view to DOM is in progress.
-		 * 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
-		 */
-		this._renderingInProgress = false;
+		this._renderingEventProcessing = false;
+		this._callbacksWaiting = [];
 
 		/**
 		 * Writer instance used in {@link #change change method) callbacks.
@@ -303,26 +294,47 @@ export default class View {
 	 *
 	 * Change block is executed immediately.
 	 *
-	 * When the outermost change block is done it fires
+	 * When the outermost change block is done and rendering to DOM is over it fires
 	 * {@link module:engine/view/view~View#event:render} event.
 	 *
 	 * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `applying-view-changes-on-rendering` when
-	 * change block is used when rendering to DOM is in progress.
+	 * change block is used after rendering to DOM has started.
 	 *
 	 * @param {Function} callback Callback function which may modify the view.
 	 */
 	change( callback ) {
-		// Check if change is performed in correct moment.
-		this._assertRenderingInProgress();
+		// When "render" event is processed all callbacks need to wait until processing of that event is complete.
+		// Those callbacks will be processed later and create separate "render" event.
+		if ( this._renderingEventProcessing ) {
+			this._callbacksWaiting.push( callback );
+			return;
+		}
 
-		// If other changes are in progress wait with rendering until every ongoing change is over.
+		// Recursive call to view.change() method - execute listener immediately.
 		if ( this._ongoingChange ) {
 			callback( this._writer );
 		} else {
+			// This lock will assure that all recursive calls to view.change() will end up in same block - one "render"
+			// event for all nested calls.
 			this._ongoingChange = true;
-
 			callback( this._writer );
+			this._ongoingChange = false;
+
+			// This lock will assure that all view.change() calls in listeners will wait until all callbacks are processed
+			// and will create separate "render" event.
+			this._renderingEventProcessing = true;
 			this.fire( 'render' );
+			this._renderingEventProcessing = false;
+
+			// Call waiting callbacks that were called during `render` event.
+			if ( this._callbacksWaiting.length ) {
+				const callbacks = this._callbacksWaiting;
+				this._callbacksWaiting = [];
+
+				while ( callbacks.length ) {
+					this.change( callbacks.shift() );
+				}
+			}
 		}
 	}
 
@@ -334,14 +346,7 @@ export default class View {
 	 * trying to re-render when rendering to DOM has already started.
 	 */
 	render() {
-		// 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.change( () => {} );
-		}
+		this.change( () => {} );
 	}
 
 	/**
@@ -362,55 +367,19 @@ export default class View {
 	 * @private
 	 */
 	_render() {
-		this._renderingInProgress = true;
-
 		this.disableObservers();
 		this._renderer.render();
 		this.enableObservers();
-
-		// Current ongoing change is finished after rendering is done.
-		// Further render() or change() calls will create new ongoing change.
-		this._ongoingChange = false;
-		this._renderingInProgress = false;
-	}
-
-	/**
-	 * 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._renderingInProgress ) {
-			/**
-			 * There is an attempt to make changes in the view tree when the rendering process is in progress.
-			 * 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.
-			 *
-			 * @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.
+	 * Fired after a topmost {@link module:engine/view/view~View#change change block} is finished and the DOM rendering has
+	 * been executed.
 	 *
-	 * Actual rendering is performed on 'low' priority. This means that all listeners on 'normal' and higher priorities
+	 * 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.
 	 *
-	 * When listener on `normal` (or higher) priority call {@link module:engine/view/view~View#change change()} or
-	 * {@link module:engine/view/view~View#render render()} it will be included in currently executed change block (no
-	 * more `render` events will be fired).
-	 *
-	 * When listener on `low` (or lower) priority calls {@link module:engine/view/view~View#change change()} or
-	 * {@link module:engine/view/view~View#render render()} it will create a new change block (new `render` event will be fired).
-	 *
 	 * @event module:engine/view/view~View#event:render
 	 */
 }

+ 49 - 27
packages/ckeditor5-engine/tests/view/view/view.js

@@ -18,7 +18,6 @@ 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;
@@ -502,19 +501,22 @@ describe( 'view', () => {
 				sinon.assert.callOrder( beforeSpy, renderSpy, afterSpy );
 			} );
 
-			it( 'should throw when someone tries to change view during rendering', () => {
+			it( 'should create separate change block if view.change() is called during "render" event', () => {
 				const domDiv = document.createElement( 'div' );
 				const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
-				let renderingCalled = false;
 				view.attachDomRoot( domDiv );
 
+				const renderingEndSpy = sinon.spy();
+				view.on( 'render', renderingEndSpy, { priority: 'lowest' } );
+
+				const nestedChange = sinon.spy();
+
 				view.change( writer => {
 					const p = writer.createContainerElement( 'p' );
 					const ui = writer.createUIElement( 'span', null, function( domDocument ) {
 						const element = this.toDomElement( domDocument );
 
-						expect( () => view.change( () => {} ) ).to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
-						renderingCalled = true;
+						view.change( nestedChange );
 
 						return element;
 					} );
@@ -522,23 +524,25 @@ describe( 'view', () => {
 					writer.insert( ViewPosition.createAt( viewRoot ), p );
 				} );
 
-				expect( renderingCalled ).to.be.true;
+				sinon.assert.calledTwice( renderingEndSpy );
+				sinon.assert.callOrder( renderingEndSpy, nestedChange );
 				domDiv.remove();
 			} );
 
-			it( 'should throw when someone tries to call render() during rendering', () => {
+			it( 'should create separate change block if view.render() is called during "render" event', () => {
 				const domDiv = document.createElement( 'div' );
 				const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
-				let renderingCalled = false;
 				view.attachDomRoot( domDiv );
 
+				const renderingEndSpy = sinon.spy();
+				view.on( 'render', renderingEndSpy, { priority: 'lowest' } );
+
 				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;
+						view.render();
 
 						return element;
 					} );
@@ -546,7 +550,7 @@ describe( 'view', () => {
 					writer.insert( ViewPosition.createAt( viewRoot ), p );
 				} );
 
-				expect( renderingCalled ).to.be.true;
+				sinon.assert.calledTwice( renderingEndSpy );
 				domDiv.remove();
 			} );
 
@@ -567,6 +571,23 @@ describe( 'view', () => {
 				sinon.assert.calledTwice( spy );
 			} );
 
+			it( 'should create separate render event when change() called on high priority', () => {
+				let called = false;
+
+				const spy = sinon.spy( () => {
+					// Prevent infinite loop.
+					if ( !called ) {
+						called = true;
+						view.change( () => {} );
+					}
+				} );
+
+				view.on( 'render', spy, { priority: 'high' } );
+
+				view.change( () => {} );
+				sinon.assert.calledTwice( spy );
+			} );
+
 			it( 'should create separate render event when render() called on low priority', () => {
 				let called = false;
 
@@ -584,6 +605,23 @@ describe( 'view', () => {
 				sinon.assert.calledTwice( spy );
 			} );
 
+			it( 'should create separate render event when render() called on high priority', () => {
+				let called = false;
+
+				const spy = sinon.spy( () => {
+					// Prevent infinite loop.
+					if ( !called ) {
+						called = true;
+						view.render();
+					}
+				} );
+
+				view.on( 'render', spy, { priority: 'high' } );
+
+				view.render();
+				sinon.assert.calledTwice( spy );
+			} );
+
 			it( 'should call second render after the first is done', () => {
 				let called = false;
 				const order = [];
@@ -613,22 +651,6 @@ describe( 'view', () => {
 
 				expect( order ).to.deep.equal( [ 'low1', 'low2', 'lowest', 'low1', 'low2', 'lowest' ] );
 			} );
-
-			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( () => {} );
-			} );
 		} );
 	} );