Merge pull request #1657 from ckeditor/t/1653

Fix: Stopped invoking `view.render()` by `EditingController` when the model document isn't changed. Closes #1653.

Author: Piotr Jasiun

src/controller/editingcontroller.js

@@ -78,9 +78,11 @@ export default class EditingController {
 			this.view._renderingDisabled = true;
 		}, { priority: 'highest' } );
 
-		this.listenTo( this.model, '_afterChanges', () => {
+		this.listenTo( this.model, '_afterChanges', ( evt, { hasModelDocumentChanged } ) => {
 			this.view._renderingDisabled = false;
-			this.view.render();
+			if ( hasModelDocumentChanged ) {
+				this.view.render();
+			}
 		}, { priority: 'lowest' } );
 
 		// Whenever model document is changed, convert those changes to the view (using model.Document#differ).

src/model/document.js

@@ -46,7 +46,7 @@ export default class Document {
 		 * The {@link module:engine/model/model~Model model} that the document is a part of.
 		 *
 		 * @readonly
-		 * @member {module:engine/model/model~Model}
+		 * @type {module:engine/model/model~Model}
 		 */
 		this.model = model;
 
@@ -58,23 +58,23 @@ export default class Document {
 		 * a {@link module:utils/ckeditorerror~CKEditorError model-document-applyOperation-wrong-version} error is thrown.
 		 *
 		 * @readonly
-		 * @member {Number}
+		 * @type {Number}
 		 */
 		this.version = 0;
 
 		/**
 		 * The document's history.
 		 *
 		 * @readonly
-		 * @member {module:engine/model/history~History}
+		 * @type {module:engine/model/history~History}
 		 */
 		this.history = new History( this );
 
 		/**
 		 * The selection in this document.
 		 *
 		 * @readonly
-		 * @member {module:engine/model/documentselection~DocumentSelection}
+		 * @type {module:engine/model/documentselection~DocumentSelection}
 		 */
 		this.selection = new DocumentSelection( this );
 
@@ -83,26 +83,34 @@ export default class Document {
 		 * {@link #getRoot} to manipulate it.
 		 *
 		 * @readonly
-		 * @member {module:utils/collection~Collection}
+		 * @type {module:utils/collection~Collection}
 		 */
 		this.roots = new Collection( { idProperty: 'rootName' } );
 
 		/**
 		 * The model differ object. Its role is to buffer changes done on the model document and then calculate a diff of those changes.
 		 *
 		 * @readonly
-		 * @member {module:engine/model/differ~Differ}
+		 * @type {module:engine/model/differ~Differ}
 		 */
 		this.differ = new Differ( model.markers );
 
 		/**
 		 * Post-fixer callbacks registered to the model document.
 		 *
 		 * @private
-		 * @member {Set}
+		 * @type {Set.<Function>}
 		 */
 		this._postFixers = new Set();
 
+		/**
+		 * A boolean indicates whether the selection has changed until
+		 *
+		 * @private
+		 * @type {Boolean}
+		 */
+		this._hasSelectionChangedFromTheLastChangeBlock = false;
+
 		// Graveyard tree root. Document always have a graveyard root, which stores removed nodes.
 		this.createRoot( '$root', graveyardName );
 
@@ -144,29 +152,8 @@ export default class Document {
 		}, { priority: 'low' } );
 
 		// Listen to selection changes. If selection changed, mark it.
-		let hasSelectionChanged = false;
-
 		this.listenTo( this.selection, 'change', () => {
-			hasSelectionChanged = true;
-		} );
-
-		// Wait for `_change` event from model, which signalizes that outermost change block has finished.
-		// When this happens, check if there were any changes done on document, and if so, call post-fixers,
-		// fire `change` event for features and conversion and then reset the differ.
-		// Fire `change:data` event when at least one operation or buffered marker changes the data.
-		this.listenTo( model, '_change', ( evt, writer ) => {
-			if ( !this.differ.isEmpty || hasSelectionChanged ) {
-				this._callPostFixers( writer );
-
-				if ( this.differ.hasDataChanges() ) {
-					this.fire( 'change:data', writer.batch );
-				} else {
-					this.fire( 'change', writer.batch );
-				}
-
-				this.differ.reset();
-				hasSelectionChanged = false;
-			}
+			this._hasSelectionChangedFromTheLastChangeBlock = true;
 		} );
 
 		// Buffer marker changes.
@@ -306,6 +293,44 @@ export default class Document {
 		return json;
 	}
 
+	/**
+	 * Check if there were any changes done on document, and if so, call post-fixers,
+	 * fire `change` event for features and conversion and then reset the differ.
+	 * Fire `change:data` event when at least one operation or buffered marker changes the data.
+	 *
+	 * @protected
+	 * @fires change
+	 * @fires change:data
+	 * @param {module:engine/model/writer~Writer writer} writer The writer on which post-fixers will be called.
+	 */
+	_handleChangeBlock( writer ) {
+		if ( this._hasDocumentChangedFromTheLastChangeBlock() ) {
+			this._callPostFixers( writer );
+
+			if ( this.differ.hasDataChanges() ) {
+				this.fire( 'change:data', writer.batch );
+			} else {
+				this.fire( 'change', writer.batch );
+			}
+
+			this.differ.reset();
+		}
+
+		this._hasSelectionChangedFromTheLastChangeBlock = false;
+	}
+
+	/**
+	 * Returns whether there is a buffered change or if the selection has changed from the last
+	 * {@link module:engine/model/model~Model#enqueueChange `enqueueChange()` block}
+	 * or {@link module:engine/model/model~Model#change `change()` block}.
+	 *
+	 * @protected
+	 * @returns {Boolean} Returns `true` if document has changed from the last `change()` or `enqueueChange()` block.
+	 */
+	_hasDocumentChangedFromTheLastChangeBlock() {
+		return !this.differ.isEmpty || this._hasSelectionChangedFromTheLastChangeBlock;
+	}
+
 	/**
 	 * Returns the default root for this document which is either the first root that was added to the document using
 	 * {@link #createRoot} or the {@link #graveyard graveyard root} if no other roots were created.
@@ -359,6 +384,7 @@ export default class Document {
 	 * Performs post-fixer loops. Executes post-fixer callbacks as long as none of them has done any changes to the model.
 	 *
 	 * @private
+	 * @param {module:engine/model/writer~Writer writer} writer The writer on which post-fixer callbacks will be called.
 	 */
 	_callPostFixers( writer ) {
 		let wasFixed = false;

src/model/model.js

@@ -683,6 +683,7 @@ export default class Model {
 	 */
 	_runPendingChanges() {
 		const ret = [];
+		let hasModelDocumentChanged = false;
 
 		this.fire( '_beforeChanges' );
 
@@ -695,14 +696,19 @@ export default class Model {
 			const callbackReturnValue = this._pendingChanges[ 0 ].callback( this._currentWriter );
 			ret.push( callbackReturnValue );
 
-			// Fire internal `_change` event.
+			// Collect an information whether the model document has changed during from the last pending change.
+			hasModelDocumentChanged = hasModelDocumentChanged || this.document._hasDocumentChangedFromTheLastChangeBlock();
+
+			// Fire '_change' event before resetting differ.
 			this.fire( '_change', this._currentWriter );
 
+			this.document._handleChangeBlock( this._currentWriter );
+
 			this._pendingChanges.shift();
 			this._currentWriter = null;
 		}
 
-		this.fire( '_afterChanges' );
+		this.fire( '_afterChanges', { hasModelDocumentChanged } );
 
 		return ret;
 	}
@@ -713,6 +719,7 @@ export default class Model {
 	 *
 	 * **Note:** This is an internal event! Use {@link module:engine/model/document~Document#event:change} instead.
 	 *
+	 * @deprecated
 	 * @protected
 	 * @event _change
 	 * @param {module:engine/model/writer~Writer} writer `Writer` instance that has been used in the change block.
@@ -732,6 +739,9 @@ export default class Model {
 	 *
 	 * @protected
 	 * @event _afterChanges
+	 * @param {Object} options
+	 * @param {Boolean} options.hasModelDocumentChanged `true` if the model document has changed during the
+	 * {@link module:engine/model/model~Model#change} or {@link module:engine/model/model~Model#enqueueChange} blocks.
 	 */
 
 	/**

tests/tickets/1653.js

@@ -0,0 +1,34 @@
+/**
+ * @license Copyright (c) 2003-2019, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals document */
+
+import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+
+describe( 'Bug ckeditor5-engine#1653', () => {
+	it( '`DataController.parse()` should not invoke `editing.view.render()`', () => {
+		let editor;
+
+		const element = document.createElement( 'div' );
+		document.body.appendChild( element );
+
+		return ClassicTestEditor
+			.create( element, { plugins: [ Paragraph ] } )
+			.then( newEditor => {
+				editor = newEditor;
+
+				const spy = sinon.spy( editor.editing.view, 'render' );
+				editor.data.parse( '<p></p>' );
+
+				sinon.assert.notCalled( spy );
+			} )
+			.then( () => {
+				element.remove();
+
+				return editor.destroy();
+			} );
+	} );
+} );