Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Prevent rendering when one is in the change block. #1535

Merged
merged 9 commits into from
Sep 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/controller/editingcontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
20 changes: 20 additions & 0 deletions src/model/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -473,6 +475,8 @@ export default class Model {
this._currentWriter = null;
}

this.fire( '_afterChanges' );

return ret;
}

Expand All @@ -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}.
Expand Down
12 changes: 11 additions & 1 deletion src/view/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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' );
}
}

/**
Expand Down
106 changes: 106 additions & 0 deletions tests/controller/editingcontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()', () => {
Expand Down