Merge pull request #1661 from ckeditor/t/1653-b

Fix: Prevented `View` from firing the `render` event if there were no changes since the last rendering. Closes #1653. Closes #1660.

BREAKING CHANGE: The `editing.view.render()` method was renamed to `editing.view.forceRender()`. It should be used with caution as it will re-render editing view and repaint the UI.

Author: Piotr Jasiun

src/controller/editingcontroller.js

@@ -75,14 +75,11 @@ export default class EditingController {
 		//
 		// See  https://github.com/ckeditor/ckeditor5-engine/issues/1528
 		this.listenTo( this.model, '_beforeChanges', () => {
-			this.view._renderingDisabled = true;
+			this.view._disableRendering( true );
 		}, { priority: 'highest' } );
 
-		this.listenTo( this.model, '_afterChanges', ( evt, { hasModelDocumentChanged } ) => {
-			this.view._renderingDisabled = false;
-			if ( hasModelDocumentChanged ) {
-				this.view.render();
-			}
+		this.listenTo( this.model, '_afterChanges', () => {
+			this.view._disableRendering( false );
 		}, { priority: 'lowest' } );
 
 		// Whenever model document is changed, convert those changes to the view (using model.Document#differ).

src/model/document.js

@@ -240,8 +240,8 @@ export default class Document {
 	}
 
 	/**
-	 * Used to register a post-fixer callback. A post-fixer mechanism guarantees that the features that listen to
-	 * the {@link module:engine/model/model~Model#event:_change model's change event} will operate on a correct model state.
+	 * Used to register a post-fixer callback. A post-fixer mechanism guarantees that the features
+	 * will operate on a correct model state.
 	 *
 	 * An execution of a feature may lead to an incorrect document tree state. The callbacks are used to fix the document tree after
 	 * it has changed. Post-fixers are fired just after all changes from the outermost change block were applied but

src/model/model.js

@@ -683,7 +683,6 @@ export default class Model {
 	 */
 	_runPendingChanges() {
 		const ret = [];
-		let hasModelDocumentChanged = false;
 
 		this.fire( '_beforeChanges' );
 
@@ -696,9 +695,6 @@ export default class Model {
 			const callbackReturnValue = this._pendingChanges[ 0 ].callback( this._currentWriter );
 			ret.push( callbackReturnValue );
 
-			// 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 );
 
@@ -708,7 +704,7 @@ export default class Model {
 			this._currentWriter = null;
 		}
 
-		this.fire( '_afterChanges', { hasModelDocumentChanged } );
+		this.fire( '_afterChanges' );
 
 		return ret;
 	}
@@ -739,8 +735,6 @@ 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.
 	 */
 

src/view/document.js

@@ -115,6 +115,9 @@ export default class Document {
 	 * As a parameter, a post-fixer callback receives a {@link module:engine/view/downcastwriter~DowncastWriter downcast writer}
 	 * instance connected with the executed changes block.
 	 *
+	 * Note that registering a post-fixer won't re-render the editor's view. If the view should change after registering the post-fixer then
+	 * it should be done manually calling `view.forceRender();`.
+	 *
 	 * @param {Function} postFixer
 	 */
 	registerPostFixer( postFixer ) {

src/view/observer/focusobserver.js

@@ -37,7 +37,7 @@ export default class FocusObserver extends DomEventObserver {
 			// overwrite new DOM selection with selection from the view.
 			// See https://github.com/ckeditor/ckeditor5-engine/issues/795 for more details.
 			// Long timeout is needed to solve #676 and https://github.com/ckeditor/ckeditor5-engine/issues/1157 issues.
-			this._renderTimeoutId = setTimeout( () => view.render(), 50 );
+			this._renderTimeoutId = setTimeout( () => view.forceRender(), 50 );
 		} );
 
 		document.on( 'blur', ( evt, data ) => {
@@ -47,7 +47,7 @@ export default class FocusObserver extends DomEventObserver {
 				document.isFocused = false;
 
 				// Re-render the document to update view elements.
-				view.render();
+				view.forceRender();
 			}
 		} );
 

src/view/observer/mutationobserver.js

@@ -248,7 +248,7 @@ export default class MutationObserver extends Observer {
 
 		// If nothing changes on `mutations` event, at this point we have "dirty DOM" (changed) and de-synched
 		// view (which has not been changed). In order to "reset DOM" we render the view again.
-		this.view.render();
+		this.view.forceRender();
 
 		function sameNodes( child1, child2 ) {
 			// First level of comparison (array of children vs array of children) – use the Lodash's default behavior.

src/view/observer/selectionobserver.js

@@ -166,7 +166,7 @@ export default class SelectionObserver extends Observer {
 		if ( this.selection.isSimilar( newViewSelection ) ) {
 			// If selection was equal and we are at this point of algorithm, it means that it was incorrect.
 			// Just re-render it, no need to fire any events, etc.
-			this.view.render();
+			this.view.forceRender();
 		} else {
 			const data = {
 				oldSelection: this.selection,

src/view/placeholder.js

@@ -39,8 +39,7 @@ export function attachPlaceholder( view, element, placeholderText, checkFunction
 		checkFunction
 	} );
 
-	// Update view right away.
-	view.render();
+	view.change( writer => updateAllPlaceholders( document, writer ) );
 }
 
 /**

src/view/view.js

@@ -66,7 +66,7 @@ export default class View {
 		 * Instance of the {@link module:engine/view/document~Document} associated with this view controller.
 		 *
 		 * @readonly
-		 * @member {module:engine/view/document~Document} module:engine/view/view~View#document
+		 * @type {module:engine/view/document~Document}
 		 */
 		this.document = new Document();
 
@@ -76,15 +76,15 @@ export default class View {
 		 * and {@link module:engine/view/observer/observer~Observer observers}.
 		 *
 		 * @readonly
-		 * @member {module:engine/view/domconverter~DomConverter} module:engine/view/view~View#domConverter
+		 * @type {module:engine/view/domconverter~DomConverter}
 		 */
 		this.domConverter = new DomConverter();
 
 		/**
 		 * Instance of the {@link module:engine/view/renderer~Renderer renderer}.
 		 *
 		 * @protected
-		 * @member {module:engine/view/renderer~Renderer} module:engine/view/view~View#renderer
+		 * @type {module:engine/view/renderer~Renderer}
 		 */
 		this._renderer = new Renderer( this.domConverter, this.document.selection );
 		this._renderer.bind( 'isFocused' ).to( this.document );
@@ -93,55 +93,64 @@ export default class View {
 		 * Roots of the DOM tree. Map on the `HTMLElement`s with roots names as keys.
 		 *
 		 * @readonly
-		 * @member {Map} module:engine/view/view~View#domRoots
+		 * @type {Map.<String, HTMLElement>}
 		 */
 		this.domRoots = new Map();
 
 		/**
 		 * Map of registered {@link module:engine/view/observer/observer~Observer observers}.
 		 *
 		 * @private
-		 * @member {Map.<Function, module:engine/view/observer/observer~Observer>} module:engine/view/view~View#_observers
+		 * @type {Map.<Function, module:engine/view/observer/observer~Observer>}
 		 */
 		this._observers = new Map();
 
 		/**
 		 * Is set to `true` when {@link #change view changes} are currently in progress.
 		 *
 		 * @private
-		 * @member {Boolean} module:engine/view/view~View#_ongoingChange
+		 * @type {Boolean}
 		 */
 		this._ongoingChange = false;
 
 		/**
 		 * Used to prevent calling {@link #render} and {@link #change} during rendering view to the DOM.
 		 *
 		 * @private
-		 * @member {Boolean} module:engine/view/view~View#_renderingInProgress
+		 * @type {Boolean}
 		 */
 		this._renderingInProgress = false;
 
 		/**
 		 * Used to prevent calling {@link #render} and {@link #change} during rendering view to the DOM.
 		 *
 		 * @private
-		 * @member {Boolean} module:engine/view/view~View#_renderingInProgress
+		 * @type {Boolean}
 		 */
 		this._postFixersInProgress = false;
 
 		/**
-		 * Internal flag to temporary disable rendering. See usage in the editing controller.
+		 * Internal flag to temporary disable rendering. See the usage in the {@link #_disableRendering}.
 		 *
-		 * @protected
-		 * @member {Boolean} module:engine/view/view~View#_renderingDisabled
+		 * @private
+		 * @type {Boolean}
 		 */
 		this._renderingDisabled = false;
 
 		/**
-		 * DowncastWriter instance used in {@link #change change method) callbacks.
+		 * Internal flag that disables rendering when there are no changes since the last rendering.
+		 * It stores information about changed selection and changed elements from attached document roots.
+		 *
+		 * @private
+		 * @type {Boolean}
+		 */
+		this._hasChangedSinceTheLastRendering = false;
+
+		/**
+		 * DowncastWriter instance used in {@link #change change method} callbacks.
 		 *
 		 * @private
-		 * @member {module:engine/view/downcastwriter~DowncastWriter} module:engine/view/view~View#_writer
+		 * @type {module:engine/view/downcastwriter~DowncastWriter}
 		 */
 		this._writer = new DowncastWriter( this.document );
 
@@ -163,6 +172,14 @@ export default class View {
 
 			// Informs that layout has changed after render.
 			this.document.fire( 'layoutChanged' );
+
+			// Reset the `_hasChangedSinceTheLastRendering` flag after rendering.
+			this._hasChangedSinceTheLastRendering = false;
+		} );
+
+		// Listen to the document selection changes directly.
+		this.listenTo( this.document.selection, 'change', () => {
+			this._hasChangedSinceTheLastRendering = true;
 		} );
 	}
 
@@ -192,6 +209,10 @@ export default class View {
 		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', () => {
+			this._hasChangedSinceTheLastRendering = true;
+		} );
+
 		for ( const observer of this._observers.values() ) {
 			observer.observe( domRoot, name );
 		}
@@ -293,7 +314,7 @@ export default class View {
 
 			if ( editable ) {
 				this.domConverter.focus( editable );
-				this.render();
+				this.forceRender();
 			} else {
 				/**
 				 * Before focusing view document, selection should be placed inside one of the view's editables.
@@ -309,9 +330,10 @@ export default class View {
 
 	/**
 	 * The `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 the view is rendered to the DOM. It prevents situations when the DOM is updated
-	 * when the view state is not yet correct. It allows to nest calls one inside another and still performs a single rendering
-	 * after all those changes are made. It also returns the return value of its callback.
+	 * It makes sure that after all changes are made the view is rendered to the DOM (assuming that the view will be changed
+	 * inside the callback). It prevents situations when the DOM is updated when the view state is not yet correct. It allows
+	 * to nest calls one inside another and still performs a single rendering after all those changes are made.
+	 * It also returns the return value of its callback.
 	 *
 	 *		const text = view.change( writer => {
 	 *			const newText = writer.createText( 'foo' );
@@ -368,7 +390,8 @@ export default class View {
 
 		// This lock is used by editing controller to render changes from outer most model.change() once. As plugins might call
 		// view.change() inside model.change() block - this will ensures that postfixers and rendering are called once after all changes.
-		if ( !this._renderingDisabled ) {
+		// Also, we don't need to render anything if there're no changes since last rendering.
+		if ( !this._renderingDisabled && this._hasChangedSinceTheLastRendering ) {
 			this._postFixersInProgress = true;
 			this.document._callPostFixers( this._writer );
 			this._postFixersInProgress = false;
@@ -380,13 +403,17 @@ export default class View {
 	}
 
 	/**
-	 * Renders {@link module:engine/view/document~Document view document} to DOM. If any view changes are
+	 * Forces rendering {@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.
 	 *
+	 * Note that this method is dedicated for special cases. All view changes should be wrapped in the {@link #change}
+	 * block and the view will automatically check whether it needs to render DOM or not.
+	 *
 	 * 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() {
+	forceRender() {
+		this._hasChangedSinceTheLastRendering = true;
 		this.change( () => {} );
 	}
 
@@ -542,6 +569,22 @@ export default class View {
 		return new Selection( selectable, placeOrOffset, options );
 	}
 
+	/**
+	 * Disables or enables rendering. If the flag is set to `true` then the rendering will be disabled.
+	 * If the flag is set to `false` and if there was some change in the meantime, then the rendering action will be performed.
+	 *
+	 * @protected
+	 * @param {Boolean} flag A flag indicates whether the rendering should be disabled.
+	 */
+	_disableRendering( flag ) {
+		this._renderingDisabled = flag;
+
+		if ( flag == false ) {
+			// Render when you stop blocking rendering.
+			this.change( () => {} );
+		}
+	}
+
 	/**
 	 * Renders all changes. In order to avoid triggering the observers (e.g. mutations) all observers are disabled
 	 * before rendering and re-enabled after that.

tests/controller/editingcontroller.js

@@ -186,7 +186,7 @@ describe( 'EditingController', () => {
 			} );
 
 			editing.view.document.isFocused = true;
-			editing.view.render();
+			editing.view.forceRender();
 
 			const domSelection = document.getSelection();
 			domSelection.removeAllRanges();