diff --git a/src/controller/editingcontroller.js b/src/controller/editingcontroller.js index 458753475..1ae8b14e9 100644 --- a/src/controller/editingcontroller.js +++ b/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. diff --git a/src/model/model.js b/src/model/model.js index 462dc3d01..0601132f1 100644 --- a/src/model/model.js +++ b/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; } @@ -487,6 +491,22 @@ export default class Model { * @param {module:engine/model/writer~Writer} writer `Writer` instance that has been used in the change block. */ + /** + * 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}. diff --git a/src/view/view.js b/src/view/view.js index 84f01e9e1..01dee363f 100644 --- a/src/view/view.js +++ b/src/view/view.js @@ -126,6 +126,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. * @@ -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' ); + } } /** diff --git a/tests/controller/editingcontroller.js b/tests/controller/editingcontroller.js index 2e6d06237..2736657bd 100644 --- a/tests/controller/editingcontroller.js +++ b/tests/controller/editingcontroller.js @@ -339,6 +339,112 @@ describe( 'EditingController', () => { expect( getViewData( editing.view, { withoutSelection: true } ) ) .to.equal( '

foo

bar

' ); } ); + + 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()', () => {