Sfoglia il codice sorgente

Merge branch 'master' into t/1289

Piotrek Koszuliński 8 anni fa
parent
commit
7a528f4b7d

+ 0 - 1
packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

@@ -476,7 +476,6 @@ export default class DowncastDispatcher {
 	 * @param {Object} data Additional information about the change.
 	 * @param {module:engine/model/item~Item} data.item Inserted item.
 	 * @param {module:engine/model/range~Range} data.range Range spanning over inserted item.
-	 * @param {module:engine/conversion/modelconsumable~ModelConsumable} consumable Values to consume.
 	 * @param {Object} conversionApi Conversion interface to be used by callback, passed in `DowncastDispatcher` constructor.
 	 */
 

+ 64 - 47
packages/ckeditor5-engine/src/model/differ.js

@@ -229,13 +229,6 @@ export default class Differ {
 
 		// Check all changed elements.
 		for ( const element of this._changesInElement.keys() ) {
-			// Each item in `this._changesInElement` describes changes of the _children_ of that element.
-			// If the element itself has been inserted we should skip all the changes in it because the element will be reconverted.
-			// If the element itself has been removed we should skip all the changes in it because they would be incorrect.
-			if ( this._isInsertedOrRemoved( element ) ) {
-				continue;
-			}
-
 			// Get changes for this element and sort them.
 			const changes = this._changesInElement.get( element ).sort( ( a, b ) => {
 				if ( a.offset === b.offset ) {
@@ -394,46 +387,6 @@ export default class Differ {
 		this._cachedChanges = null;
 	}
 
-	/**
-	 * Checks whether a given element is inserted or removed or one of its ancestors is inserted or removed. Used to
-	 * filter out sub-changes in elements that are changed.
-	 *
-	 * @private
-	 * @param {module:engine/model/element~Element} element An element to check.
-	 * @returns {Boolean}
-	 */
-	_isInsertedOrRemoved( element ) {
-		let parent = element.parent;
-
-		// Check all ancestors of given element.
-		while ( parent ) {
-			// Get the checked element's offset.
-			const offset = element.startOffset;
-
-			if ( this._changesInElement.has( parent ) ) {
-				const changes = this._changesInElement.get( parent );
-
-				// If there were changes in that element's ancestor, check all of them.
-				for ( const change of changes ) {
-					// Skip attribute changes. We are interested only if the element was inserted or removed.
-					if ( change.type == 'attribute' ) {
-						continue;
-					}
-
-					if ( change.offset <= offset && change.offset + change.howMany > offset ) {
-						return true;
-					}
-				}
-			}
-
-			// Move up.
-			parent = parent.parent;
-			element = element.parent;
-		}
-
-		return false;
-	}
-
 	/**
 	 * Saves and handles an insert change.
 	 *
@@ -443,6 +396,10 @@ export default class Differ {
 	 * @param {Number} howMany
 	 */
 	_markInsert( parent, offset, howMany ) {
+		if ( this._isInInsertedElement( parent ) ) {
+			return;
+		}
+
 		const changeItem = { type: 'insert', offset, howMany, count: this._changeCount++ };
 
 		this._markChange( parent, changeItem );
@@ -457,9 +414,15 @@ export default class Differ {
 	 * @param {Number} howMany
 	 */
 	_markRemove( parent, offset, howMany ) {
+		if ( this._isInInsertedElement( parent ) ) {
+			return;
+		}
+
 		const changeItem = { type: 'remove', offset, howMany, count: this._changeCount++ };
 
 		this._markChange( parent, changeItem );
+
+		this._removeAllNestedChanges( parent, offset, howMany );
 	}
 
 	/**
@@ -469,6 +432,10 @@ export default class Differ {
 	 * @param {module:engine/model/item~Item} item
 	 */
 	_markAttribute( item ) {
+		if ( this._isInInsertedElement( item.parent ) ) {
+			return;
+		}
+
 		const changeItem = { type: 'attribute', offset: item.startOffset, howMany: item.offsetSize, count: this._changeCount++ };
 
 		this._markChange( item.parent, changeItem );
@@ -831,6 +798,56 @@ export default class Differ {
 
 		return diffs;
 	}
+
+	/**
+	 * Checks whether given element or any of its parents is an element that is buffered as an inserted element.
+	 *
+	 * @private
+	 * @param {module:engine/model/element~Element} element Element to check.
+	 * @returns {Boolean}
+	 */
+	_isInInsertedElement( element ) {
+		const parent = element.parent;
+
+		if ( !parent ) {
+			return false;
+		}
+
+		const changes = this._changesInElement.get( parent );
+		const offset = element.startOffset;
+
+		if ( changes ) {
+			for ( const change of changes ) {
+				if ( change.type == 'insert' && offset >= change.offset && offset < change.offset + change.howMany ) {
+					return true;
+				}
+			}
+		}
+
+		return this._isInInsertedElement( parent );
+	}
+
+	/**
+	 * Removes deeply all buffered changes that are registered in elements from range specified by `parent`, `offset`
+	 * and `howMany`.
+	 *
+	 * @private
+	 * @param {module:engine/model/element~Element} parent
+	 * @param {Number} offset
+	 * @param {Number} howMany
+	 */
+	_removeAllNestedChanges( parent, offset, howMany ) {
+		const range = Range.createFromParentsAndOffsets( parent, offset, parent, offset + howMany );
+
+		for ( const item of range.getItems( { shallow: true } ) ) {
+			if ( item.is( 'element' ) ) {
+				this._elementSnapshots.delete( item );
+				this._changesInElement.delete( item );
+
+				this._removeAllNestedChanges( item, 0, item.maxOffset );
+			}
+		}
+	}
 }
 
 // Returns an array that is a copy of passed child list with the exception that text nodes are split to one or more

+ 46 - 0
packages/ckeditor5-engine/src/view/document.js

@@ -65,6 +65,14 @@ export default class Document {
 		 * @member {Boolean} module:engine/view/document~Document#isFocused
 		 */
 		this.set( 'isFocused', false );
+
+		/**
+		 * Post-fixer callbacks registered to the view document.
+		 *
+		 * @private
+		 * @member {Set}
+		 */
+		this._postFixers = new Set();
 	}
 
 	/**
@@ -78,6 +86,44 @@ export default class Document {
 	getRoot( name = 'main' ) {
 		return this.roots.get( name );
 	}
+
+	/**
+	 * Used to register a post-fixer callback. A post-fixers mechanism allows to update view tree just before rendering
+	 * to the DOM.
+	 *
+	 * Post-fixers are fired just after all changes from the outermost change block were applied but
+	 * before the {@link module:engine/view/view~View#event:render render event} is fired. If a post-fixer callback made
+	 * a change, it should return `true`. When this happens, all post-fixers are fired again to check if something else should
+	 * not be fixed in the new document tree state.
+	 *
+	 * As a parameter, a post-fixer callback receives a {@link module:engine/view/writer~Writer writer} instance connected with the
+	 * executed changes block.
+	 *
+	 * @param {Function} postFixer
+	 */
+	registerPostFixer( postFixer ) {
+		this._postFixers.add( postFixer );
+	}
+
+	/**
+	 * Performs post-fixer loops. Executes post-fixer callbacks as long as none of them has done any changes to the model.
+	 *
+	 * @protected
+	 * @param {module:engine/view/writer~Writer} writer
+	 */
+	_callPostFixers( writer ) {
+		let wasFixed = false;
+
+		do {
+			for ( const callback of this._postFixers ) {
+				wasFixed = callback( writer );
+
+				if ( wasFixed ) {
+					break;
+				}
+			}
+		} while ( wasFixed );
+	}
 }
 
 mix( Document, ObservableMixin );

+ 48 - 40
packages/ckeditor5-engine/src/view/placeholder.js

@@ -7,13 +7,8 @@
  * @module engine/view/placeholder
  */
 
-import extend from '@ckeditor/ckeditor5-utils/src/lib/lodash/extend';
-import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
 import '../../theme/placeholder.css';
 
-const listener = {};
-extend( listener, EmitterMixin );
-
 // Each document stores information about its placeholder elements and check functions.
 const documentPlaceholders = new WeakMap();
 
@@ -21,6 +16,7 @@ const documentPlaceholders = new WeakMap();
  * Attaches placeholder to provided element and updates it's visibility. To change placeholder simply call this method
  * once again with new parameters.
  *
+ * @param {module:engine/view/view~View} view View controller.
  * @param {module:engine/view/element~Element} element Element to attach placeholder to.
  * @param {String} placeholderText Placeholder text to use.
  * @param {Function} [checkFunction] If provided it will be called before checking if placeholder should be displayed.
@@ -29,28 +25,19 @@ const documentPlaceholders = new WeakMap();
 export function attachPlaceholder( view, element, placeholderText, checkFunction ) {
 	const document = view.document;
 
-	// Detach placeholder if was used before.
-	detachPlaceholder( view, element );
-
 	// Single listener per document.
 	if ( !documentPlaceholders.has( document ) ) {
 		documentPlaceholders.set( document, new Map() );
 
-		// Attach listener just before rendering and update placeholders.
-		listener.listenTo( view, 'render', () => updateAllPlaceholders( view ) );
+		// Create view post fixer that will add placeholder where needed.
+		document.registerPostFixer( writer => updateAllPlaceholders( document, writer ) );
 	}
 
-	// Store text in element's data attribute.
-	// This data attribute is used in CSS class to show the placeholder.
-	view.change( writer => {
-		writer.setAttribute( 'data-placeholder', placeholderText, element );
-	} );
-
-	// Store information about placeholder.
-	documentPlaceholders.get( document ).set( element, checkFunction );
+	// Store information about element with placeholder.
+	documentPlaceholders.get( document ).set( element, { placeholderText, checkFunction } );
 
-	// Update right away too.
-	updateSinglePlaceholder( view, element, checkFunction );
+	// Update view right away.
+	view.render();
 }
 
 /**
@@ -75,39 +62,55 @@ export function detachPlaceholder( view, element ) {
 // Updates all placeholders of given document.
 //
 // @private
-// @param {module:engine/view/view~View} view
-function updateAllPlaceholders( view ) {
-	const placeholders = documentPlaceholders.get( view.document );
-
-	for ( const [ element, checkFunction ] of placeholders ) {
-		updateSinglePlaceholder( view, element, checkFunction );
+// @param {module:engine/view/document~Document} view
+// @param {module:engine/view/writer~Writer} writer
+function updateAllPlaceholders( document, writer ) {
+	const placeholders = documentPlaceholders.get( document );
+	let changed = false;
+
+	for ( const [ element, info ] of placeholders ) {
+		if ( updateSinglePlaceholder( writer, element, info ) ) {
+			changed = true;
+		}
 	}
+
+	return changed;
 }
 
 // Updates placeholder class of given element.
 //
 // @private
-// @param {module:engine/view/view~View} view
+// @param {module:engine/view/writer~Writer} writer
 // @param {module:engine/view/element~Element} element
-// @param {Function} checkFunction
-function updateSinglePlaceholder( view, element, checkFunction ) {
+// @param {Object} info
+function updateSinglePlaceholder( writer, element, info ) {
 	const document = element.document;
+	const text = info.placeholderText;
+	let changed = false;
 
 	// Element was removed from document.
 	if ( !document ) {
-		return;
+		return false;
+	}
+
+	// Update data attribute if needed.
+	if ( element.getAttribute( 'data-placeholder' ) !== text ) {
+		writer.setAttribute( 'data-placeholder', text, element );
+		changed = true;
 	}
 
 	const viewSelection = document.selection;
 	const anchor = viewSelection.anchor;
+	const checkFunction = info.checkFunction;
 
 	// If checkFunction is provided and returns false - remove placeholder.
 	if ( checkFunction && !checkFunction() ) {
-		view.change( writer => {
+		if ( element.hasClass( 'ck-placeholder' ) ) {
 			writer.removeClass( 'ck-placeholder', element );
-		} );
+			changed = true;
+		}
 
-		return;
+		return changed;
 	}
 
 	// Element is empty for placeholder purposes when it has no children or only ui elements.
@@ -116,21 +119,26 @@ function updateSinglePlaceholder( view, element, checkFunction ) {
 
 	// If element is empty and editor is blurred.
 	if ( !document.isFocused && isEmptyish ) {
-		view.change( writer => {
+		if ( !element.hasClass( 'ck-placeholder' ) ) {
 			writer.addClass( 'ck-placeholder', element );
-		} );
+			changed = true;
+		}
 
-		return;
+		return changed;
 	}
 
 	// It there are no child elements and selection is not placed inside element.
 	if ( isEmptyish && anchor && anchor.parent !== element ) {
-		view.change( writer => {
+		if ( !element.hasClass( 'ck-placeholder' ) ) {
 			writer.addClass( 'ck-placeholder', element );
-		} );
+			changed = true;
+		}
 	} else {
-		view.change( writer => {
+		if ( element.hasClass( 'ck-placeholder' ) ) {
 			writer.removeClass( 'ck-placeholder', element );
-		} );
+			changed = true;
+		}
 	}
+
+	return changed;
 }

+ 52 - 60
packages/ckeditor5-engine/src/view/view.js

@@ -109,15 +109,20 @@ export default class View {
 		this._ongoingChange = false;
 
 		/**
-		 * Is set to `true` when rendering view to DOM was started.
-		 * This is used to check whether view document can accept changes in current state.
-		 * From the moment when rendering to DOM is stared view tree is locked to prevent changes that will not be
-		 * reflected in the DOM.
+		 * Used to prevent calling {@link #render} and {@link #change) during rendering view to the DOM.
 		 *
 		 * @private
-		 * @member {Boolean} module:engine/view/view~View#_renderingStarted
+		 * @member {Boolean} module:engine/view/view~View#_renderingInProgress
 		 */
-		this._renderingStarted = false;
+		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
+		 */
+		this._postFixersInProgress = false;
 
 		/**
 		 * Writer instance used in {@link #change change method) callbacks.
@@ -138,10 +143,10 @@ export default class View {
 		injectQuirksHandling( this );
 		injectUiElementHandling( this );
 
-		// Use 'low` priority so that all listeners on 'normal` priority will be executed before.
+		// Use 'normal' priority so that rendering is performed as first when using that priority.
 		this.on( 'render', () => {
 			this._render();
-		}, { priority: 'low' } );
+		} );
 	}
 
 	/**
@@ -312,21 +317,41 @@ export default class View {
 	 * @param {Function} callback Callback function which may modify the view.
 	 */
 	change( callback ) {
-		// Check if change is performed in correct moment.
-		this._assertRenderingInProgress();
+		if ( this._renderingInProgress || this._postFixersInProgress ) {
+			/**
+			 * Thrown when there is an attempt to make changes to the view tree when it is in incorrect state. This may
+			 * cause some unexpected behaviour and inconsistency between the DOM and the view.
+			 * This may be caused by:
+			 *   * calling {@link #change} or {@link #render} during rendering process,
+			 *   * calling {@link #change} or {@link #render} inside of
+			 *   {@link module:engine/view/document~Document#registerPostFixer post fixer function}.
+			 */
+			throw new CKEditorError(
+				'cannot-change-view-tree: ' +
+				'Attempting to make changes to the view when it is in incorrect state: rendering or post fixers are in progress. ' +
+				'This may cause some unexpected behaviour and inconsistency between the DOM and the view.'
+			);
+		}
 
-		// If other changes are in progress wait with rendering until every ongoing change is over.
+		// Recursive call to view.change() method - execute listener immediately.
 		if ( this._ongoingChange ) {
 			callback( this._writer );
-		} else {
-			this._ongoingChange = true;
-
-			callback( this._writer );
-			this.fire( 'render' );
 
-			this._ongoingChange = false;
-			this._renderingStarted = false;
+			return;
 		}
+
+		// This lock will assure that all recursive calls to view.change() will end up in same block - one "render"
+		// event for all nested calls.
+		this._ongoingChange = true;
+		callback( this._writer );
+		this._ongoingChange = false;
+
+		// Execute all document post fixers after the change.
+		this._postFixersInProgress = true;
+		this.document._callPostFixers( this._writer );
+		this._postFixersInProgress = false;
+
+		this.fire( 'render' );
 	}
 
 	/**
@@ -337,15 +362,7 @@ export default class View {
 	 * trying to re-render when rendering to DOM has already started.
 	 */
 	render() {
-		// Check if rendering is performed in correct moment.
-		this._assertRenderingInProgress();
-
-		// Render only if no ongoing changes are in progress. If there are some, view document will be rendered after all
-		// changes are done. This way view document will not be rendered in the middle of some changes.
-		if ( !this._ongoingChange ) {
-			this.fire( 'render' );
-			this._renderingStarted = false;
-		}
+		this.change( () => {} );
 	}
 
 	/**
@@ -366,47 +383,22 @@ export default class View {
 	 * @private
 	 */
 	_render() {
-		this._renderingStarted = true;
-
+		this._renderingInProgress = true;
 		this.disableObservers();
 		this._renderer.render();
 		this.enableObservers();
+		this._renderingInProgress = false;
 	}
 
 	/**
-	 * Throws `applying-view-changes-on-rendering` error when trying to modify or re-render view tree when rendering is
-	 * already started
+	 * Fired after a topmost {@link module:engine/view/view~View#change change block} and all
+	 * {@link module:engine/view/document~Document#registerPostFixer post fixers} are executed.
 	 *
-	 * @private
-	 */
-	_assertRenderingInProgress() {
-		if ( this._renderingStarted ) {
-			/**
-			 * There is an attempt to make changes in the view tree after the rendering process
-			 * has started. This may cause unexpected behaviour and inconsistency between the DOM and the view.
-			 * This may be caused by:
-			 *   * calling `view.change()` or `view.render()` methods during rendering process,
-			 *   * calling `view.change()` or `view.render()` methods in callbacks to
-			 *   {module:engine/view/document~Document#event:change view document change event) on `low` priority, after
-			 *   rendering is over for current `change` block.
-			 *
-			 * @error applying-view-changes-on-rendering
-			 */
-			throw new CKEditorError(
-				'applying-view-changes-on-rendering: ' +
-				'Attempting to make changes in the view during rendering process. ' +
-				'This may cause some unexpected behaviour and inconsistency between the DOM and the view.'
-			);
-		}
-	}
-
-	/**
-	 * Fired after a topmost {@link module:engine/view/view~View#change change block} is finished and the DOM rendering has
-	 * been executed.
+	 * Actual rendering is performed as a first listener on 'normal' priority.
 	 *
-	 * Actual rendering is performed on 'low' priority. This means that all listeners on 'normal' and above priorities
-	 * will be executed after changes made to view tree but before rendering to the DOM. Use `low` priority for callbacks that
-	 * should be executed after rendering to the DOM.
+	 *		view.on( 'render', () => {
+	 *			// Rendering to the DOM is complete.
+	 *		} );
 	 *
 	 * @event module:engine/view/view~View#event:render
 	 */

+ 57 - 0
packages/ckeditor5-engine/tests/model/differ.js

@@ -1193,6 +1193,63 @@ describe( 'Differ', () => {
 				] );
 			} );
 		} );
+
+		// ckeditor5#733.
+		it( 'proper filtering of changes in removed elements', () => {
+			// Before fix there was a buggy scenario described in ckeditor5#733.
+			// There was this structure: `<paragraph>foo[</paragraph><image /><blockQuote><p>te]xt</p></blockQuote>`
+			// On delete of above selection `image` and `paragraph` inside `blockQuote` are removed (it gets merged).
+			// However, since `image` was removed first, when checking if `paragraph` is in a removed element,
+			// it appeared that `blockQuote` looks like it is removed because it had the same path as the already removed `<image>`.
+			// In a result, removing `paragraph` was discarded.
+			// The mistake was that the checking for removing was done at incorrect moment.
+			root.removeChildren( 0, root.childCount );
+			root.appendChildren( [
+				new Element( 'paragraph', null, new Text( 'foo' ) ),
+				new Element( 'image' ),
+				new Element( 'blockQuote', null, [
+					new Element( 'paragraph', null, new Text( 'text' ) )
+				] )
+			] );
+
+			model.change( () => {
+				// Remove `"te"`.
+				remove( new Position( root, [ 2, 0, 0 ] ), 2 );
+				// Remove `image` before `blockQuote`.
+				remove( new Position( root, [ 1 ] ), 1 );
+				// Move `"xt"` to first `paragraph` (merging).
+				move( new Position( root, [ 1, 0, 0 ] ), 2, new Position( root, [ 0, 3 ] ) );
+				// Remove `paragraph` inside `blockQuote`.
+				remove( new Position( root, [ 1, 0 ] ), 1 );
+
+				expectChanges( [
+					{ type: 'insert', name: '$text', length: 2, position: new Position( root, [ 0, 3 ] ) },
+					{ type: 'remove', name: 'image', length: 1, position: new Position( root, [ 1 ] ) },
+					{ type: 'remove', name: 'paragraph', length: 1, position: new Position( root, [ 1, 0 ] ) }
+				] );
+			} );
+		} );
+
+		it( 'proper filtering of changes in inserted elements', () => {
+			root.removeChildren( 0, root.childCount );
+			root.appendChildren( new Element( 'image' ) );
+
+			const blockQuote = new Element( 'blockQuote', null, new Element( 'paragraph' ) );
+
+			model.change( () => {
+				// Insert `blockQuote` with `paragraph` after `image`.
+				insert( blockQuote, new Position( root, [ 1 ] ) );
+				// Remove `image` from before `blockQuote`.
+				remove( new Position( root, [ 0 ] ), 1 );
+				// Insert text into `paragraph` in `blockQuote`.
+				insert( new Text( 'foo' ), new Position( root, [ 0, 0, 0 ] ) );
+
+				expectChanges( [
+					{ type: 'remove', name: 'image', length: 1, position: new Position( root, [ 0 ] ) },
+					{ type: 'insert', name: 'blockQuote', length: 1, position: new Position( root, [ 0 ] ) }
+				] );
+			} );
+		} );
 	} );
 
 	describe( 'getChanges()', () => {

+ 33 - 0
packages/ckeditor5-engine/tests/view/document.js

@@ -58,4 +58,37 @@ describe( 'Document', () => {
 			expect( viewDocument.getRoot( 'not-existing' ) ).to.null;
 		} );
 	} );
+
+	describe( 'post fixers', () => {
+		it( 'should add a callback that is called on _callPostFixers', () => {
+			const spy1 = sinon.spy();
+			const spy2 = sinon.spy();
+			const writerMock = {};
+
+			viewDocument.registerPostFixer( spy1 );
+			viewDocument.registerPostFixer( spy2 );
+
+			sinon.assert.notCalled( spy1 );
+			sinon.assert.notCalled( spy2 );
+			viewDocument._callPostFixers( writerMock );
+			sinon.assert.calledOnce( spy1 );
+			sinon.assert.calledOnce( spy2 );
+			sinon.assert.calledWithExactly( spy1, writerMock );
+			sinon.assert.calledWithExactly( spy2, writerMock );
+		} );
+
+		it( 'should call post fixer until all returns false', () => {
+			let calls = 0;
+
+			const spy1 = sinon.spy( () => calls++ < 2 );
+			const spy2 = sinon.spy( () => calls++ < 2 );
+
+			viewDocument.registerPostFixer( spy1 );
+			viewDocument.registerPostFixer( spy2 );
+
+			viewDocument._callPostFixers();
+
+			expect( calls ).to.equal( 4 );
+		} );
+	} );
 } );

+ 4 - 4
packages/ckeditor5-engine/tests/view/observer/focusobserver.js

@@ -170,12 +170,12 @@ describe( 'FocusObserver', () => {
 			view.render();
 
 			viewDocument.on( 'selectionChange', selectionChangeSpy );
-			view.on( 'render', renderSpy, { priority: 'low' } );
+			view.on( 'render', renderSpy );
 
 			view.on( 'render', () => {
 				sinon.assert.callOrder( selectionChangeSpy, renderSpy );
 				done();
-			}, { priority: 'low' } );
+			} );
 
 			// Mock selectionchange event after focus event. Render called by focus observer should be fired after
 			// async selection change.
@@ -192,14 +192,14 @@ describe( 'FocusObserver', () => {
 			const domEditable = domRoot.childNodes[ 0 ];
 
 			viewDocument.on( 'selectionChange', selectionChangeSpy );
-			view.on( 'render', renderSpy, { priority: 'low' } );
+			view.on( 'render', renderSpy );
 
 			view.on( 'render', () => {
 				sinon.assert.notCalled( selectionChangeSpy );
 				sinon.assert.called( renderSpy );
 
 				done();
-			}, { priority: 'low' } );
+			} );
 
 			observer.onDomEvent( { type: 'focus', target: domEditable } );
 		} );

+ 16 - 1
packages/ckeditor5-engine/tests/view/placeholder.js

@@ -74,12 +74,17 @@ describe( 'placeholder', () => {
 		it( 'use check function if one is provided', () => {
 			setData( view, '<div></div><div>{another div}</div>' );
 			const element = viewRoot.getChild( 0 );
-			const spy = sinon.spy( () => false );
+			let result = true;
+			const spy = sinon.spy( () => result );
 
 			attachPlaceholder( view, element, 'foo bar baz', spy );
 
 			sinon.assert.called( spy );
 			expect( element.getAttribute( 'data-placeholder' ) ).to.equal( 'foo bar baz' );
+			expect( element.hasClass( 'ck-placeholder' ) ).to.be.true;
+
+			result = false;
+			view.render();
 			expect( element.hasClass( 'ck-placeholder' ) ).to.be.false;
 		} );
 
@@ -190,5 +195,15 @@ describe( 'placeholder', () => {
 			expect( element.hasAttribute( 'data-placeholder' ) ).to.be.false;
 			expect( element.hasClass( 'ck-placeholder' ) ).to.be.false;
 		} );
+
+		it( 'should not blow up when called on element without placeholder', () => {
+			setData( view, '<div></div><div>{another div}</div>' );
+			const element = viewRoot.getChild( 0 );
+
+			detachPlaceholder( view, element );
+
+			expect( element.hasAttribute( 'data-placeholder' ) ).to.be.false;
+			expect( element.hasClass( 'ck-placeholder' ) ).to.be.false;
+		} );
 	} );
 } );

+ 105 - 103
packages/ckeditor5-engine/tests/view/view/view.js

@@ -442,145 +442,147 @@ describe( 'view', () => {
 			view.destroy();
 			domRoot.remove();
 		} );
+	} );
 
-		describe( 'change()', () => {
-			it( 'should fire render event and it should trigger rendering on low priority', () => {
-				const renderSpy = sinon.spy( view._renderer, 'render' );
-				const beforeSpy = sinon.spy();
-				const afterSpy = sinon.spy();
+	describe( 'change()', () => {
+		it( 'should throw when someone tries to change view during rendering', () => {
+			const domDiv = document.createElement( 'div' );
+			const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
+			let renderingCalled = false;
+			view.attachDomRoot( domDiv );
 
-				view.on( 'render', beforeSpy );
-				view.on( 'render', afterSpy, { priority: 'low' } );
+			view.change( writer => {
+				const p = writer.createContainerElement( 'p' );
+				const ui = writer.createUIElement( 'span', null, function( domDocument ) {
+					const element = this.toDomElement( domDocument );
 
-				view.change( () => {} );
+					expect( () => view.change( () => {} ) ).to.throw( CKEditorError, /^cannot-change-view-tree/ );
+					renderingCalled = true;
 
-				sinon.assert.callOrder( beforeSpy, renderSpy, afterSpy );
+					return element;
+				} );
+				writer.insert( ViewPosition.createAt( p ), ui );
+				writer.insert( ViewPosition.createAt( viewRoot ), p );
 			} );
 
-			it( 'should fire render event once for nested change blocks', () => {
-				const renderSpy = sinon.spy( view._renderer, 'render' );
-				const beforeSpy = sinon.spy();
-				const afterSpy = sinon.spy();
+			expect( renderingCalled ).to.be.true;
+			domDiv.remove();
+		} );
 
-				view.on( 'render', beforeSpy );
-				view.on( 'render', afterSpy, { priority: 'low' } );
+		it( 'should throw when someone tries to use change() method in post fixer', () => {
+			const domDiv = document.createElement( 'div' );
+			createViewRoot( viewDocument, 'div', 'main' );
+			view.attachDomRoot( domDiv );
 
-				view.change( () => {
+			viewDocument.registerPostFixer( () => {
+				expect( () => {
 					view.change( () => {} );
-					view.change( () => {
-						view.change( () => {} );
-						view.change( () => {} );
-					} );
-					view.change( () => {} );
-				} );
-
-				sinon.assert.calledOnce( beforeSpy );
-				sinon.assert.calledOnce( renderSpy );
-				sinon.assert.calledOnce( afterSpy );
-				sinon.assert.callOrder( beforeSpy, renderSpy, afterSpy );
+				} ).to.throw( CKEditorError, /^cannot-change-view-tree/ );
 			} );
 
-			it( 'should fire render event once even if render is called during the change', () => {
-				const renderSpy = sinon.spy( view._renderer, 'render' );
-				const beforeSpy = sinon.spy();
-				const afterSpy = sinon.spy();
+			view.render();
+			domDiv.remove();
+		} );
+
+		it( 'should fire render event and it should trigger rendering before listeners on normal priority', () => {
+			const renderSpy = sinon.spy( view._renderer, 'render' );
+			const eventSpy = sinon.spy();
+
+			view.on( 'render', eventSpy );
+
+			view.change( () => {} );
+
+			sinon.assert.callOrder( renderSpy, eventSpy );
+		} );
+
+		it( 'should fire render event once for nested change blocks', () => {
+			const renderSpy = sinon.spy( view._renderer, 'render' );
+			const eventSpy = sinon.spy();
 
-				view.on( 'render', beforeSpy );
-				view.on( 'render', afterSpy, { priority: 'low' } );
+			view.on( 'render', eventSpy );
 
+			view.change( () => {
+				view.change( () => {} );
 				view.change( () => {
-					view.render();
-					view.change( () => {
-						view.render();
-					} );
-					view.render();
+					view.change( () => {} );
+					view.change( () => {} );
 				} );
-
-				sinon.assert.calledOnce( beforeSpy );
-				sinon.assert.calledOnce( renderSpy );
-				sinon.assert.calledOnce( afterSpy );
-				sinon.assert.callOrder( beforeSpy, renderSpy, afterSpy );
+				view.change( () => {} );
 			} );
 
-			it( 'should throw when someone tries to change view during rendering', () => {
-				const domDiv = document.createElement( 'div' );
-				const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
-				let renderingCalled = false;
-				view.attachDomRoot( domDiv );
+			sinon.assert.calledOnce( renderSpy );
+			sinon.assert.calledOnce( eventSpy );
+			sinon.assert.callOrder( renderSpy, eventSpy );
+		} );
 
-				view.change( writer => {
-					const p = writer.createContainerElement( 'p' );
-					const ui = writer.createUIElement( 'span', null, function( domDocument ) {
-						const element = this.toDomElement( domDocument );
+		it( 'should fire render event once even if render is called during the change', () => {
+			const renderSpy = sinon.spy( view._renderer, 'render' );
+			const eventSpy = sinon.spy();
 
-						expect( () => view.change( () => {} ) ).to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
-						renderingCalled = true;
+			view.on( 'render', eventSpy );
 
-						return element;
-					} );
-					writer.insert( ViewPosition.createAt( p ), ui );
-					writer.insert( ViewPosition.createAt( viewRoot ), p );
+			view.change( () => {
+				view.render();
+				view.change( () => {
+					view.render();
 				} );
-
-				expect( renderingCalled ).to.be.true;
-				domDiv.remove();
+				view.render();
 			} );
 
-			it( 'should throw when someone tries to call render() during rendering', () => {
-				const domDiv = document.createElement( 'div' );
-				const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
-				let renderingCalled = false;
-				view.attachDomRoot( domDiv );
+			sinon.assert.calledOnce( renderSpy );
+			sinon.assert.calledOnce( eventSpy );
+			sinon.assert.callOrder( renderSpy, eventSpy );
+		} );
 
-				view.change( writer => {
-					const p = writer.createContainerElement( 'p' );
-					const ui = writer.createUIElement( 'span', null, function( domDocument ) {
-						const element = this.toDomElement( domDocument );
+		it( 'should call post fixers after change but before rendering', () => {
+			const postFixer1 = sinon.spy( () => false );
+			const postFixer2 = sinon.spy( () => false );
+			const changeSpy = sinon.spy();
+			const eventSpy = sinon.spy();
 
-						expect( () => view.render() ).to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
-						renderingCalled = true;
+			viewDocument.registerPostFixer( postFixer1 );
+			viewDocument.registerPostFixer( postFixer2 );
+			view.on( 'render', eventSpy );
 
-						return element;
-					} );
-					writer.insert( ViewPosition.createAt( p ), ui );
-					writer.insert( ViewPosition.createAt( viewRoot ), p );
-				} );
+			view.change( changeSpy );
 
-				expect( renderingCalled ).to.be.true;
-				domDiv.remove();
-			} );
+			sinon.assert.calledOnce( postFixer1 );
+			sinon.assert.calledOnce( postFixer2 );
+			sinon.assert.calledOnce( changeSpy );
+			sinon.assert.calledOnce( eventSpy );
 
-			it( 'should throw when someone tries to call change() after rendering is finished but still in change block', () => {
-				view.on( 'render', () => {
-					expect( () => view.change( () => {} ) ).to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
-				}, { priority: 'low' } );
+			sinon.assert.callOrder( changeSpy, postFixer1, postFixer2, eventSpy );
+		} );
 
-				view.change( () => {} );
-			} );
+		it( 'should call post fixers until all are done', () => {
+			let called = false;
+			const postFixer1 = sinon.spy();
+			const postFixer2 = sinon.spy();
+			const changeSpy = sinon.spy();
+			const eventSpy = sinon.spy();
 
-			it( 'should throw when someone tries to call render() after rendering is finished but still in change block', () => {
-				view.on( 'render', () => {
-					expect( () => view.render() ).to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
-				}, { priority: 'low' } );
+			viewDocument.registerPostFixer( () => {
+				if ( !called ) {
+					called = true;
+					postFixer1();
 
-				view.change( () => {} );
-			} );
+					return true;
+				}
 
-			it( 'should NOT throw when someone tries to call change() before rendering', () => {
-				view.on( 'render', () => {
-					expect( () => view.change( () => {} ) ).not.to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
-				}, { priority: 'normal' } );
+				postFixer2();
 
-				view.change( () => {} );
+				return false;
 			} );
+			view.on( 'render', eventSpy );
 
-			it( 'should NOT throw when someone tries to call render() before rendering', () => {
-				view.on( 'render', () => {
-					expect( () => view.render() ).not.to.throw( CKEditorError, /^applying-view-changes-on-rendering/ );
-				}, { priority: 'normal' } );
+			view.change( changeSpy );
 
-				view.change( () => {} );
-			} );
+			sinon.assert.calledOnce( postFixer1 );
+			sinon.assert.calledOnce( postFixer2 );
+			sinon.assert.calledOnce( changeSpy );
+			sinon.assert.calledOnce( eventSpy );
+
+			sinon.assert.callOrder( changeSpy, postFixer1, postFixer2, eventSpy );
 		} );
 	} );