瀏覽代碼

Added postfixers to view.

Szymon Kupś 7 年之前
父節點
當前提交
24ece4e4f2

+ 67 - 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 model document.
+		 *
+		 * @private
+		 * @member {Set}
+		 */
+		this._postFixers = new Set();
 	}
 
 	/**
@@ -78,6 +86,65 @@ export default class Document {
 	getRoot( name = 'main' ) {
 		return this.roots.get( name );
 	}
+
+	/**
+	 * TODO: update docs
+	 * 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.
+	 *
+	 * 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
+	 * before the {@link module:engine/model/document~Document#event:change change 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/model/writer~Writer writer} instance connected with the
+	 * executed changes block. Thanks to that, all changes done by the callback will be added to the same
+	 * {@link module:engine/model/batch~Batch batch} (and undo step) as the original changes. This makes post-fixer changes transparent
+	 * for the user.
+	 *
+	 * An example of a post-fixer is a callback that checks if all the data were removed from the editor. If so, the
+	 * callback should add an empty paragraph so that the editor is never empty:
+	 *
+	 *		document.registerPostFixer( writer => {
+	 *			const changes = document.differ.getChanges();
+	 *
+	 *			// Check if the changes lead to an empty root in the editor.
+	 *			for ( const entry of changes ) {
+	 *				if ( entry.type == 'remove' && entry.position.root.isEmpty ) {
+	 *					writer.insertElement( 'paragraph', entry.position.root, 0 );
+	 *
+	 *					// It is fine to return early, even if multiple roots would need to be fixed.
+	 *					// All post-fixers will be fired again, so if there are more empty roots, those will be fixed, too.
+	 *					return true;
+	 *				}
+	 *			}
+	 *		} );
+	 *
+	 * @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
+	 */
+	_callPostFixers( writer ) {
+		let wasFixed = false;
+
+		do {
+			for ( const callback of this._postFixers ) {
+				wasFixed = callback( writer );
+
+				if ( wasFixed ) {
+					break;
+				}
+			}
+		} while ( wasFixed );
+	}
 }
 
 mix( Document, ObservableMixin );

+ 38 - 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();
 
@@ -29,28 +24,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();
 }
 
 /**
@@ -76,12 +62,17 @@ export function detachPlaceholder( view, element ) {
 //
 // @private
 // @param {module:engine/view/view~View} view
-function updateAllPlaceholders( view ) {
-	const placeholders = documentPlaceholders.get( view.document );
+function updateAllPlaceholders( document, writer ) {
+	const placeholders = documentPlaceholders.get( document );
+	let changed = false;
 
-	for ( const [ element, checkFunction ] of placeholders ) {
-		updateSinglePlaceholder( view, element, checkFunction );
+	for ( const [ element, info ] of placeholders ) {
+		if ( updateSinglePlaceholder( writer, element, info ) ) {
+			changed = true;
+		}
 	}
+
+	return changed;
 }
 
 // Updates placeholder class of given element.
@@ -90,26 +81,34 @@ function updateAllPlaceholders( view ) {
 // @param {module:engine/view/view~View} view
 // @param {module:engine/view/element~Element} element
 // @param {Function} checkFunction
-function updateSinglePlaceholder( view, element, checkFunction ) {
+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() ) {
 		if ( element.hasClass( 'ck-placeholder' ) ) {
-			view.change( writer => {
-				writer.removeClass( 'ck-placeholder', element );
-			} );
+			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.
@@ -119,26 +118,25 @@ function updateSinglePlaceholder( view, element, checkFunction ) {
 	// If element is empty and editor is blurred.
 	if ( !document.isFocused && isEmptyish ) {
 		if ( !element.hasClass( 'ck-placeholder' ) ) {
-			view.change( writer => {
-				writer.addClass( 'ck-placeholder', element );
-			} );
+			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 ) {
 		if ( !element.hasClass( 'ck-placeholder' ) ) {
-			view.change( writer => {
-				writer.addClass( 'ck-placeholder', element );
-			} );
+			writer.addClass( 'ck-placeholder', element );
+			changed = true;
 		}
 	} else {
 		if ( element.hasClass( 'ck-placeholder' ) ) {
-			view.change( writer => {
-				writer.removeClass( 'ck-placeholder', element );
-			} );
+			writer.removeClass( 'ck-placeholder', element );
+			changed = true;
 		}
 	}
+
+	return changed;
 }

+ 39 - 31
packages/ckeditor5-engine/src/view/view.js

@@ -24,6 +24,7 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix';
 import { scrollViewportToShowTarget } from '@ckeditor/ckeditor5-utils/src/dom/scroll';
 import { injectUiElementHandling } from './uielement';
 import { injectQuirksHandling } from './filler';
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 /**
  * Editor's view controller class. Its main responsibility is DOM - View management for editing purposes, to provide
@@ -107,8 +108,21 @@ export default class View {
 		 */
 		this._ongoingChange = false;
 
-		this._renderingEventProcessing = false;
-		this._callbacksWaiting = [];
+		/**
+		 * 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._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.
@@ -129,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' } );
+		} );
 	}
 
 	/**
@@ -303,39 +317,30 @@ export default class View {
 	 * @param {Function} callback Callback function which may modify the view.
 	 */
 	change( callback ) {
-		// When "render" event is processed all callbacks need to wait until processing of that event is complete.
-		// Those callbacks will be processed later and create separate "render" event.
-		if ( this._renderingEventProcessing ) {
-			this._callbacksWaiting.push( callback );
-			return;
+		if ( this._renderingInProgress || this._postFixersInProgress ) {
+			// TODO: better description
+			throw new CKEditorError( 'incorrect-view-change' );
 		}
 
 		// Recursive call to view.change() method - execute listener immediately.
 		if ( this._ongoingChange ) {
 			callback( this._writer );
-		} else {
-			// 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;
-
-			// This lock will assure that all view.change() calls in listeners will wait until all callbacks are processed
-			// and will create separate "render" event.
-			this._renderingEventProcessing = true;
-			this.fire( 'render' );
-			this._renderingEventProcessing = false;
-
-			// Call waiting callbacks that were called during `render` event.
-			if ( this._callbacksWaiting.length ) {
-				const callbacks = this._callbacksWaiting;
-				this._callbacksWaiting = [];
-
-				while ( callbacks.length ) {
-					this.change( callbacks.shift() );
-				}
-			}
+
+			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' );
 	}
 
 	/**
@@ -367,12 +372,15 @@ export default class View {
 	 * @private
 	 */
 	_render() {
+		this._renderingInProgress = true;
 		this.disableObservers();
 		this._renderer.render();
 		this.enableObservers();
+		this._renderingInProgress = false;
 	}
 
 	/**
+	 * TODO: fix description
 	 * Fired after a topmost {@link module:engine/view/view~View#change change block} is finished and the DOM rendering has
 	 * been executed.
 	 *

+ 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 } );
 		} );

+ 68 - 177
packages/ckeditor5-engine/tests/view/view/view.js

@@ -15,7 +15,6 @@ import global from '@ckeditor/ckeditor5-utils/src/dom/global';
 import ViewRange from '../../../src/view/range';
 import RootEditableElement from '../../../src/view/rooteditableelement';
 import ViewElement from '../../../src/view/element';
-import ViewPosition from '../../../src/view/position';
 import { isBlockFiller, BR_FILLER } from '../../../src/view/filler';
 import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement';
 
@@ -441,216 +440,108 @@ 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 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', beforeSpy );
-				view.on( 'render', afterSpy, { priority: 'low' } );
+			view.on( 'render', eventSpy );
 
-				view.change( () => {} );
+			view.change( () => {} );
 
-				sinon.assert.callOrder( beforeSpy, renderSpy, afterSpy );
-			} );
+			sinon.assert.callOrder( renderSpy, eventSpy );
+		} );
 
-			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();
+		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.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 );
+				view.change( () => {} );
 			} );
 
-			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();
+			sinon.assert.calledOnce( renderSpy );
+			sinon.assert.calledOnce( eventSpy );
+			sinon.assert.callOrder( renderSpy, eventSpy );
+		} );
 
-				view.on( 'render', beforeSpy );
-				view.on( 'render', afterSpy, { priority: 'low' } );
+		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();
 
+			view.on( 'render', eventSpy );
+
+			view.change( () => {
+				view.render();
 				view.change( () => {
 					view.render();
-					view.change( () => {
-						view.render();
-					} );
-					view.render();
 				} );
-
-				sinon.assert.calledOnce( beforeSpy );
-				sinon.assert.calledOnce( renderSpy );
-				sinon.assert.calledOnce( afterSpy );
-				sinon.assert.callOrder( beforeSpy, renderSpy, afterSpy );
-			} );
-
-			it( 'should create separate change block if view.change() is called during "render" event', () => {
-				const domDiv = document.createElement( 'div' );
-				const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
-				view.attachDomRoot( domDiv );
-
-				const renderingEndSpy = sinon.spy();
-				view.on( 'render', renderingEndSpy, { priority: 'lowest' } );
-
-				const nestedChange = sinon.spy();
-
-				view.change( writer => {
-					const p = writer.createContainerElement( 'p' );
-					const ui = writer.createUIElement( 'span', null, function( domDocument ) {
-						const element = this.toDomElement( domDocument );
-
-						view.change( nestedChange );
-
-						return element;
-					} );
-					writer.insert( ViewPosition.createAt( p ), ui );
-					writer.insert( ViewPosition.createAt( viewRoot ), p );
-				} );
-
-				sinon.assert.calledTwice( renderingEndSpy );
-				sinon.assert.callOrder( renderingEndSpy, nestedChange );
-				domDiv.remove();
-			} );
-
-			it( 'should create separate change block if view.render() is called during "render" event', () => {
-				const domDiv = document.createElement( 'div' );
-				const viewRoot = createViewRoot( viewDocument, 'div', 'main' );
-				view.attachDomRoot( domDiv );
-
-				const renderingEndSpy = sinon.spy();
-				view.on( 'render', renderingEndSpy, { priority: 'lowest' } );
-
-				view.change( writer => {
-					const p = writer.createContainerElement( 'p' );
-					const ui = writer.createUIElement( 'span', null, function( domDocument ) {
-						const element = this.toDomElement( domDocument );
-
-						view.render();
-
-						return element;
-					} );
-					writer.insert( ViewPosition.createAt( p ), ui );
-					writer.insert( ViewPosition.createAt( viewRoot ), p );
-				} );
-
-				sinon.assert.calledTwice( renderingEndSpy );
-				domDiv.remove();
-			} );
-
-			it( 'should create separate render event when change() called on low priority', () => {
-				let called = false;
-
-				const spy = sinon.spy( () => {
-					// Prevent infinite loop.
-					if ( !called ) {
-						called = true;
-						view.change( () => {} );
-					}
-				} );
-
-				view.on( 'render', spy, { priority: 'low' } );
-
-				view.change( () => {} );
-				sinon.assert.calledTwice( spy );
+				view.render();
 			} );
 
-			it( 'should create separate render event when change() called on high priority', () => {
-				let called = false;
+			sinon.assert.calledOnce( renderSpy );
+			sinon.assert.calledOnce( eventSpy );
+			sinon.assert.callOrder( renderSpy, eventSpy );
+		} );
 
-				const spy = sinon.spy( () => {
-					// Prevent infinite loop.
-					if ( !called ) {
-						called = true;
-						view.change( () => {} );
-					}
-				} );
+		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();
 
-				view.on( 'render', spy, { priority: 'high' } );
+			viewDocument.registerPostFixer( postFixer1 );
+			viewDocument.registerPostFixer( postFixer2 );
+			view.on( 'render', eventSpy );
 
-				view.change( () => {} );
-				sinon.assert.calledTwice( spy );
-			} );
-
-			it( 'should create separate render event when render() called on low priority', () => {
-				let called = false;
+			view.change( changeSpy );
 
-				const spy = sinon.spy( () => {
-					// Prevent infinite loop.
-					if ( !called ) {
-						called = true;
-						view.render();
-					}
-				} );
+			sinon.assert.calledOnce( postFixer1 );
+			sinon.assert.calledOnce( postFixer2 );
+			sinon.assert.calledOnce( changeSpy );
+			sinon.assert.calledOnce( eventSpy );
 
-				view.on( 'render', spy, { priority: 'low' } );
+			sinon.assert.callOrder( changeSpy, postFixer1, postFixer2, eventSpy );
+		} );
 
-				view.render();
-				sinon.assert.calledTwice( spy );
-			} );
+		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 create separate render event when render() called on high priority', () => {
-				let called = false;
+			viewDocument.registerPostFixer( () => {
+				if ( !called ) {
+					called = true;
+					postFixer1();
 
-				const spy = sinon.spy( () => {
-					// Prevent infinite loop.
-					if ( !called ) {
-						called = true;
-						view.render();
-					}
-				} );
+					return true;
+				}
 
-				view.on( 'render', spy, { priority: 'high' } );
+				postFixer2();
 
-				view.render();
-				sinon.assert.calledTwice( spy );
+				return false;
 			} );
+			view.on( 'render', eventSpy );
 
-			it( 'should call second render after the first is done', () => {
-				let called = false;
-				const order = [];
-
-				const lowSpy = sinon.spy( () => {
-					order.push( 'low1' );
-
-					// Prevent infinite loop.
-					if ( !called ) {
-						called = true;
-						view.render();
-					}
+			view.change( changeSpy );
 
-					order.push( 'low2' );
-				} );
-
-				const lowestSpy = sinon.spy( () => {
-					order.push( 'lowest' );
-				} );
-
-				view.on( 'render', lowSpy, { priority: 'low' } );
-				view.on( 'render', lowestSpy, { priority: 'lowest' } );
+			sinon.assert.calledOnce( postFixer1 );
+			sinon.assert.calledOnce( postFixer2 );
+			sinon.assert.calledOnce( changeSpy );
+			sinon.assert.calledOnce( eventSpy );
 
-				view.render();
-				sinon.assert.calledTwice( lowSpy );
-				sinon.assert.calledTwice( lowestSpy );
-
-				expect( order ).to.deep.equal( [ 'low1', 'low2', 'lowest', 'low1', 'low2', 'lowest' ] );
-			} );
+			sinon.assert.callOrder( changeSpy, postFixer1, postFixer2, eventSpy );
 		} );
 	} );