Przeglądaj źródła

Merge pull request #1535 from ckeditor/t/1528

Internal: Prevent rendering when in the `model.change()` or `model.enqueueChange()` block. Closes #1528.
Piotrek Koszuliński 7 lat temu
rodzic
commit
a1e1f93083

+ 14 - 0
packages/ckeditor5-engine/src/controller/editingcontroller.js

@@ -70,6 +70,20 @@ export default class EditingController {
 		const selection = doc.selection;
 		const markers = this.model.markers;
 
+		// When plugins listen on model changes (on selection change, post fixers, etc) and change the view as a result of
+		// model's change, they might trigger view rendering before the conversion is completed (e.g. before the selection
+		// is converted). We disable rendering for the length of the outermost model change() block to prevent that.
+		//
+		// See  https://github.com/ckeditor/ckeditor5-engine/issues/1528
+		this.listenTo( this.model, '_beforeChanges', () => {
+			this.view._renderingDisabled = true;
+		}, { priority: 'highest' } );
+
+		this.listenTo( this.model, '_afterChanges', () => {
+			this.view._renderingDisabled = false;
+			this.view.render();
+		}, { priority: 'lowest' } );
+
 		// Whenever model document is changed, convert those changes to the view (using model.Document#differ).
 		// Do it on 'low' priority, so changes are converted after other listeners did their job.
 		// Also convert model selection.

+ 20 - 0
packages/ckeditor5-engine/src/model/model.js

@@ -457,6 +457,8 @@ export default class Model {
 	_runPendingChanges() {
 		const ret = [];
 
+		this.fire( '_beforeChanges' );
+
 		while ( this._pendingChanges.length ) {
 			// Create a new writer using batch instance created for this chain of changes.
 			const currentBatch = this._pendingChanges[ 0 ].batch;
@@ -473,6 +475,8 @@ export default class Model {
 			this._currentWriter = null;
 		}
 
+		this.fire( '_afterChanges' );
+
 		return ret;
 	}
 
@@ -488,6 +492,22 @@ export default class Model {
 	 */
 
 	/**
+	 * Fired when entering the outermost {@link module:engine/model/model~Model#enqueueChange} or
+	 * {@link module:engine/model/model~Model#change} block.
+	 *
+	 * @protected
+	 * @event _beforeChanges
+	 */
+
+	/**
+	 * Fired when leaving the outermost {@link module:engine/model/model~Model#enqueueChange} or
+	 * {@link module:engine/model/model~Model#change} block.
+	 *
+	 * @protected
+	 * @event _afterChanges
+	 */
+
+	/**
 	 * Fired every time any {@link module:engine/model/operation/operation~Operation operation} is applied on the model
 	 * using {@link #applyOperation}.
 	 *

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

@@ -127,6 +127,14 @@ export default class View {
 		this._postFixersInProgress = false;
 
 		/**
+		 * Internal flag to temporary disable rendering. See usage in the editing controller.
+		 *
+		 * @protected
+		 * @member {Boolean} module:engine/view/view~View#_renderingDisabled
+		 */
+		this._renderingDisabled = false;
+
+		/**
 		 * DowncastWriter instance used in {@link #change change method) callbacks.
 		 *
 		 * @private
@@ -358,7 +366,9 @@ export default class View {
 		this.document._callPostFixers( this._writer );
 		this._postFixersInProgress = false;
 
-		this.fire( 'render' );
+		if ( !this._renderingDisabled ) {
+			this.fire( 'render' );
+		}
 	}
 
 	/**

+ 106 - 0
packages/ckeditor5-engine/tests/controller/editingcontroller.js

@@ -339,6 +339,112 @@ describe( 'EditingController', () => {
 			expect( getViewData( editing.view, { withoutSelection: true } ) )
 				.to.equal( '<p></p><p>f<span>oo</span></p><p>bar</p>' );
 		} );
+
+		describe( 'preventing rendering while in the model.change() block', () => {
+			let renderSpy;
+
+			beforeEach( () => {
+				renderSpy = sinon.spy();
+
+				editing.view.on( 'render', renderSpy );
+			} );
+
+			it( 'should not call render in the model.change() block', () => {
+				model.change( writer => {
+					executeSomeModelChange( writer );
+
+					expect( renderSpy.called ).to.be.false;
+				} );
+
+				expect( renderSpy.called ).to.be.true;
+			} );
+
+			it( 'should not call render in the model.change() block even if view.change() was called', () => {
+				model.change( writer => {
+					executeSomeModelChange( writer );
+
+					editing.view.change( writer => executeSomeViewChange( writer ) );
+
+					expect( renderSpy.called ).to.be.false;
+				} );
+
+				expect( renderSpy.called ).to.be.true;
+			} );
+
+			it( 'should not call render in enqueued changes', () => {
+				model.enqueueChange( writer => {
+					executeSomeModelChange( writer );
+
+					expect( renderSpy.called ).to.be.false;
+
+					model.enqueueChange( writer => {
+						executeSomeOtherModelChange( writer );
+
+						expect( renderSpy.called ).to.be.false;
+					} );
+
+					expect( renderSpy.called ).to.be.false;
+				} );
+
+				expect( renderSpy.called ).to.be.true;
+			} );
+
+			it( 'should not call render if some model changes were executed in the post fixer', () => {
+				const postfixerSpy = sinon.spy();
+
+				model.document.registerPostFixer( () => {
+					model.change( writer => executeSomeOtherModelChange( writer ) );
+
+					expect( renderSpy.called ).to.be.false;
+
+					postfixerSpy();
+				} );
+
+				model.change( writer => {
+					executeSomeModelChange( writer );
+
+					expect( renderSpy.called ).to.be.false;
+				} );
+
+				expect( renderSpy.called ).to.be.true;
+				expect( postfixerSpy.calledOnce ).to.be.true;
+			} );
+
+			it( 'should not call render if some view changes were executed in the change listener', () => {
+				const changeListenerSpy = sinon.spy();
+
+				model.document.on( 'change', () => {
+					editing.view.change( writer => executeSomeViewChange( writer ) );
+
+					expect( renderSpy.called ).to.be.false;
+
+					changeListenerSpy();
+				} );
+
+				model.change( writer => {
+					executeSomeModelChange( writer );
+
+					expect( renderSpy.called ).to.be.false;
+				} );
+
+				expect( renderSpy.called ).to.be.true;
+				expect( changeListenerSpy.calledOnce ).to.be.true;
+			} );
+
+			function executeSomeModelChange( writer ) {
+				const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) );
+				writer.addMarker( 'marker1', { range, usingOperation: true } );
+			}
+
+			function executeSomeOtherModelChange( writer ) {
+				const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) );
+				writer.addMarker( 'marker2', { range, usingOperation: true } );
+			}
+
+			function executeSomeViewChange( writer ) {
+				writer.addClass( 'foo', editing.view.document.getRoot() );
+			}
+		} );
 	} );
 
 	describe( 'destroy()', () => {