Browse Source

Introduced core.treeModel.Document#_getDefaultRoot, core.treeModel.Selection#_getDefaultRange. Made Selection always have a range.

Szymon Cofalik 9 years ago
parent
commit
8fecc60482

+ 2 - 2
packages/ckeditor5-engine/src/command/attributecommand.js

@@ -121,8 +121,8 @@ export default class AttributeCommand extends Command {
 			} else {
 				selection.removeAttribute( this.attributeKey );
 			}
-		} else if ( selection.hasAnyRange ) {
-			// If selection is not collapsed and has ranges, we change attribute on those ranges.
+		} else {
+			// If selection has non-collapsed ranges, we change attribute on nodes inside those ranges.
 			document.enqueueChanges( () => {
 				const ranges = this._getSchemaValidRanges( selection.getRanges() );
 

+ 18 - 0
packages/ckeditor5-engine/src/treemodel/document.js

@@ -242,6 +242,24 @@ export default class Document {
 		return {};
 	}
 
+	/**
+	 * Returns default root for this document which is either the first root that was added to the the document using
+	 * {@link core.treeModel.Document#createRoot} or the {@link core.treeModel.Document#graveyard graveyard root} if
+	 * no other roots were created.
+	 *
+	 * @protected
+	 * @returns {core.treeModel.RootElement} The default root for this document.
+	 */
+	_getDefaultRoot() {
+		for ( let root of this._roots.values() ) {
+			if ( root !== this.graveyard ) {
+				return root;
+			}
+		}
+
+		return this.graveyard;
+	}
+
 	/**
 	 * Fired when document changes by applying an operation.
 	 *

+ 114 - 134
packages/ckeditor5-engine/src/treemodel/selection.js

@@ -16,8 +16,11 @@ import utils from '../utils.js';
 const storePrefix = 'selection:';
 
 /**
- * Represents a selection that is made on nodes in {@link core.treeModel.Document}. Selection instance is
- * created by {@link core.treeModel.Document}. You should not need to create an instance of Selection.
+ * Represents a selection that is made on nodes in {@link core.treeModel.Document}. `Selection` instance is
+ * created by {@link core.treeModel.Document}. You should not need to create an instance of `Selection`.
+ *
+ * Keep in mind that selection always contains at least one range. If no ranges has been added to selection or all ranges
+ * got removed from selection, the selection will be reset to contain {@link core.treeModel.Selection#_getDefaultRange the default range}.
  *
  * @memberOf core.treeModel
  */
@@ -60,61 +63,38 @@ export default class Selection {
 	}
 
 	/**
-	 * Selection anchor. Anchor may be described as a position where the selection starts.
-	 * Together with {@link core.treeModel.Selection#focus} they define the direction of selection, which is important
-	 * when expanding/shrinking selection. When there are no ranges in selection anchor is null. Anchor is always
-	 * the start or end of the most recent added range. It may be a bit unintuitive when there are multiple ranges in selection.
+	 * Selection anchor. Anchor may be described as a position where the selection starts. Together with
+	 * {@link core.treeModel.Selection#focus} they define the direction of selection, which is important
+	 * when expanding/shrinking selection. Anchor is always the start or end of the most recent added range.
+	 * It may be a bit unintuitive when there are multiple ranges in selection.
 	 *
 	 * @see core.treeModel.Selection#focus
-	 * @type {core.treeModel.LivePosition|null}
+	 * @type {core.treeModel.LivePosition}
 	 */
 	get anchor() {
-		if ( this.hasAnyRange ) {
-			let range = this._ranges[ this._ranges.length - 1 ];
-
-			return this._lastRangeBackward ? range.end : range.start;
-		}
+		let range = this._ranges.length ? this._ranges[ this._ranges.length - 1 ] : this._getDefaultRange();
 
-		return null;
+		return this._lastRangeBackward ? range.end : range.start;
 	}
 
 	/**
-	 * Selection focus. Focus is a position where the selection ends. When there are no ranges in selection, focus is null.
+	 * Selection focus. Focus is a position where the selection ends.
 	 *
 	 * @see core.treeModel.Selection#anchor
-	 * @type {core.treeModel.LivePosition|null}
+	 * @type {core.treeModel.LivePosition}
 	 */
 	get focus() {
-		if ( this.hasAnyRange ) {
-			let range = this._ranges[ this._ranges.length - 1 ];
+		let range = this._ranges.length ? this._ranges[ this._ranges.length - 1 ] : this._getDefaultRange();
 
-			return this._lastRangeBackward ? range.start : range.end;
-		}
-
-		return null;
-	}
-
-	/**
-	 * Flag indicating whether the selection has any range in it.
-	 *
-	 * @readonly
-	 * @type {Boolean}
-	 */
-	get hasAnyRange() {
-		return this._ranges.length > 0;
+		return this._lastRangeBackward ? range.start : range.end;
 	}
 
 	/**
 	 * Returns whether the selection is collapsed. Selection is collapsed when all it's ranges are collapsed.
-	 * If selection has no ranges, returns null instead.
 	 *
 	 * @type {Boolean}
 	 */
 	get isCollapsed() {
-		if ( !this.hasAnyRange ) {
-			return null;
-		}
-
 		for ( let i = 0; i < this._ranges.length; i++ ) {
 			if ( !this._ranges[ i ].isCollapsed ) {
 				return false;
@@ -161,7 +141,7 @@ export default class Selection {
 	 * @returns {Array.<LiveRange>}
 	 */
 	getRanges() {
-		return this._ranges.slice();
+		return this._ranges.length ? this._ranges.slice() : [ this._getDefaultRange() ];
 	}
 
 	/**
@@ -169,9 +149,7 @@ export default class Selection {
 	 * {@link core.treeModel.Position#isBefore is before} start position of all other ranges (not to confuse with the first range
 	 * added to the selection).
 	 *
-	 * If there are no ranges in selection, retruns null instead.
-	 *
-	 * @returns {core.treeModel.Range|null}
+	 * @returns {core.treeModel.Range}
 	 */
 	getFirstRange() {
 		let first = null;
@@ -184,19 +162,17 @@ export default class Selection {
 			}
 		}
 
-		return first && Range.createFromRange( first );
+		return first && Range.createFromRange( first ) || this._getDefaultRange();
 	}
 
 	/**
 	 * Returns the first position in the selection. First position is the position that {@link core.treeModel.Position#isBefore is before}
 	 * any other position in the selection ranges.
 	 *
-	 * @returns {core.treeModel.Position|null}
+	 * @returns {core.treeModel.Position}
 	 */
 	getFirstPosition() {
-		let firstRange = this.getFirstRange();
-
-		return firstRange && Position.createFromPosition( firstRange.start );
+		return Position.createFromPosition( this.getFirstRange().start );
 	}
 
 	/**
@@ -334,16 +310,14 @@ export default class Selection {
 	 * @returns {Iterable.<*>}
 	 */
 	*_getStoredAttributes() {
-		if ( this.hasAnyRange ) {
-			const selectionParent = this.getFirstPosition().parent;
+		const selectionParent = this.getFirstPosition().parent;
 
-			if ( this.isCollapsed && selectionParent.getChildCount() === 0 ) {
-				for ( let attr of selectionParent.getAttributes() ) {
-					if ( attr[ 0 ].indexOf( storePrefix ) === 0 ) {
-						const realKey = attr[ 0 ].substr( storePrefix.length );
+		if ( this.isCollapsed && selectionParent.getChildCount() === 0 ) {
+			for ( let attr of selectionParent.getAttributes() ) {
+				if ( attr[ 0 ].indexOf( storePrefix ) === 0 ) {
+					const realKey = attr[ 0 ].substr( storePrefix.length );
 
-						yield [ realKey, attr[ 1 ] ];
-					}
+					yield [ realKey, attr[ 1 ] ];
 				}
 			}
 		}
@@ -356,16 +330,14 @@ export default class Selection {
 	 * @param {String} key Key of attribute to remove.
 	 */
 	_removeStoredAttribute( key ) {
-		if ( this.hasAnyRange ) {
-			const selectionParent = this.getFirstPosition().parent;
+		const selectionParent = this.getFirstPosition().parent;
 
-			if ( this.isCollapsed && selectionParent.getChildCount() === 0 ) {
-				const storeKey = Selection._getStoreAttributeKey( key );
+		if ( this.isCollapsed && selectionParent.getChildCount() === 0 ) {
+			const storeKey = Selection._getStoreAttributeKey( key );
 
-				this._document.enqueueChanges( () => {
-					this._document.batch().removeAttr( storeKey, selectionParent );
-				} );
-			}
+			this._document.enqueueChanges( () => {
+				this._document.batch().removeAttr( storeKey, selectionParent );
+			} );
 		}
 	}
 
@@ -378,16 +350,14 @@ export default class Selection {
 	 * @param {*} value Attribute value.
 	 */
 	_storeAttribute( key, value ) {
-		if ( this.hasAnyRange ) {
-			const selectionParent = this.getFirstPosition().parent;
+		const selectionParent = this.getFirstPosition().parent;
 
-			if ( this.isCollapsed && selectionParent.getChildCount() === 0 ) {
-				const storeKey = Selection._getStoreAttributeKey( key );
+		if ( this.isCollapsed && selectionParent.getChildCount() === 0 ) {
+			const storeKey = Selection._getStoreAttributeKey( key );
 
-				this._document.enqueueChanges( () => {
-					this._document.batch().setAttr( storeKey, value, selectionParent );
-				} );
-			}
+			this._document.enqueueChanges( () => {
+				this._document.batch().setAttr( storeKey, value, selectionParent );
+			} );
 		}
 	}
 
@@ -398,26 +368,24 @@ export default class Selection {
 	 * @private
 	 */
 	_setStoredAttributesTo( attrs ) {
-		if ( this.hasAnyRange ) {
-			const selectionParent = this.getFirstPosition().parent;
+		const selectionParent = this.getFirstPosition().parent;
 
-			if ( this.isCollapsed && selectionParent.getChildCount() === 0 ) {
-				this._document.enqueueChanges( () => {
-					const batch = this._document.batch();
+		if ( this.isCollapsed && selectionParent.getChildCount() === 0 ) {
+			this._document.enqueueChanges( () => {
+				const batch = this._document.batch();
 
-					for ( let attr of this._getStoredAttributes() ) {
-						const storeKey = Selection._getStoreAttributeKey( attr[ 0 ] );
+				for ( let attr of this._getStoredAttributes() ) {
+					const storeKey = Selection._getStoreAttributeKey( attr[ 0 ] );
 
-						batch.removeAttr( storeKey, selectionParent );
-					}
+					batch.removeAttr( storeKey, selectionParent );
+				}
 
-					for ( let attr of attrs ) {
-						const storeKey = Selection._getStoreAttributeKey( attr[ 0 ] );
+				for ( let attr of attrs ) {
+					const storeKey = Selection._getStoreAttributeKey( attr[ 0 ] );
 
-						batch.setAttr( storeKey, attr[ 1 ], selectionParent );
-					}
-				} );
-			}
+					batch.setAttr( storeKey, attr[ 1 ], selectionParent );
+				}
+			} );
 		}
 	}
 
@@ -427,72 +395,69 @@ export default class Selection {
 	 * @private
 	 */
 	_updateAttributes() {
-		if ( !this.hasAnyRange ) {
-			this.clearAttributes();
-		} else {
-			const position = this.getFirstPosition();
-			const positionParent = position.parent;
-			let attrs = null;
-
-			if ( this.isCollapsed === false ) {
-				// 1. If selection is a range...
-				const range = this.getFirstRange();
-
-				// ...look for a first character node in that range and take attributes from it.
-				for ( let item of range ) {
-					if ( item.type == 'TEXT' ) {
-						attrs = item.item.getAttributes();
-						break;
-					}
-				}
-			}
+		const position = this.getFirstPosition();
+		const positionParent = position.parent;
 
-			// 2. If the selection is a caret or the range does not contain a character node...
-			if ( !attrs && this.isCollapsed === true ) {
-				const nodeBefore = positionParent.getChild( position.offset - 1 );
-				const nodeAfter = positionParent.getChild( position.offset );
+		let attrs = null;
 
-				// ...look at the node before caret and take attributes from it if it is a character node.
-				attrs = getAttrsIfCharacter( nodeBefore );
+		if ( this.isCollapsed === false ) {
+			// 1. If selection is a range...
+			const range = this.getFirstRange();
 
-				// 3. If not, look at the node after caret...
-				if ( !attrs ) {
-					attrs = getAttrsIfCharacter( nodeAfter );
+			// ...look for a first character node in that range and take attributes from it.
+			for ( let item of range ) {
+				if ( item.type == 'TEXT' ) {
+					attrs = item.item.getAttributes();
+					break;
 				}
+			}
+		}
 
-				// 4. If not, try to find the first character on the left, that is in the same node.
-				if ( !attrs ) {
-					let node = nodeBefore;
+		// 2. If the selection is a caret or the range does not contain a character node...
+		if ( !attrs && this.isCollapsed === true ) {
+			const nodeBefore = positionParent.getChild( position.offset - 1 );
+			const nodeAfter = positionParent.getChild( position.offset );
 
-					while ( node && !attrs ) {
-						node = node.previousSibling;
-						attrs = getAttrsIfCharacter( node );
-					}
-				}
+			// ...look at the node before caret and take attributes from it if it is a character node.
+			attrs = getAttrsIfCharacter( nodeBefore );
 
-				// 5. If not found, try to find the first character on the right, that is in the same node.
-				if ( !attrs ) {
-					let node = nodeAfter;
+			// 3. If not, look at the node after caret...
+			if ( !attrs ) {
+				attrs = getAttrsIfCharacter( nodeAfter );
+			}
+
+			// 4. If not, try to find the first character on the left, that is in the same node.
+			if ( !attrs ) {
+				let node = nodeBefore;
 
-					while ( node && !attrs ) {
-						node = node.nextSibling;
-						attrs = getAttrsIfCharacter( node );
-					}
+				while ( node && !attrs ) {
+					node = node.previousSibling;
+					attrs = getAttrsIfCharacter( node );
 				}
+			}
 
-				// 6. If not found, selection should retrieve attributes from parent.
-				if ( !attrs ) {
-					attrs = this._getStoredAttributes();
+			// 5. If not found, try to find the first character on the right, that is in the same node.
+			if ( !attrs ) {
+				let node = nodeAfter;
+
+				while ( node && !attrs ) {
+					node = node.nextSibling;
+					attrs = getAttrsIfCharacter( node );
 				}
 			}
 
-			if ( attrs ) {
-				this._attrs = new Map( attrs );
-			} else {
-				this.clearAttributes();
+			// 6. If not found, selection should retrieve attributes from parent.
+			if ( !attrs ) {
+				attrs = this._getStoredAttributes();
 			}
 		}
 
+		if ( attrs ) {
+			this._attrs = new Map( attrs );
+		} else {
+			this.clearAttributes();
+		}
+
 		function getAttrsIfCharacter( node ) {
 			if ( node instanceof CharacterProxy ) {
 				return node.getAttributes();
@@ -502,6 +467,21 @@ export default class Selection {
 		}
 	}
 
+	/**
+	 * Returns a default range for this selection. The default range is a collapsed range that starts and ends
+	 * at the beginning of this selection's document {@link core.treeModel.Document#_getDefaultRoot default root}.
+	 * This "artificial" range is important for algorithms that base on selection, so they won't break or need
+	 * special logic if there are no real ranges in the selection.
+	 *
+	 * @private
+	 * @returns {core.treeModel.Range}
+	 */
+	_getDefaultRange() {
+		const defaultRoot = this._document._getDefaultRoot();
+
+		return new Range( new Position( defaultRoot, [ 0 ] ), new Position( defaultRoot, [ 0 ] ) );
+	}
+
 	/**
 	 * Generates and returns an attribute key for selection attributes store, basing on original attribute key.
 	 *

+ 0 - 11
packages/ckeditor5-engine/tests/commands/attributecommand.js

@@ -169,17 +169,6 @@ describe( '_doExecute', () => {
 		expect( modelDoc.selection.hasAttribute( 'bold' ) ).to.be.false;
 	} );
 
-	it( 'should not throw and do nothing if selection has no ranges', () => {
-		let spy = sinon.spy();
-		modelDoc.on( 'change', spy );
-
-		modelDoc.selection.removeAllRanges();
-		command._doExecute();
-
-		expect( spy.called ).to.be.false;
-		expect( Array.from( modelDoc.selection.getAttributes() ) ).to.deep.equal( [ ] );
-	} );
-
 	it( 'should not apply attribute change where it would invalid schema', () => {
 		p.insertChildren( 3, new Element( 'image' ) );
 		p.insertChildren( 12, new Element( 'image' ) );

+ 16 - 2
packages/ckeditor5-engine/tests/treemodel/document/document.js

@@ -20,12 +20,12 @@ describe( 'Document', () => {
 	} );
 
 	describe( 'constructor', () => {
-		it( 'should create Document with no data, empty graveyard and empty selection', () => {
+		it( 'should create Document with no data, empty graveyard and selection set to default range', () => {
 			expect( doc ).to.have.property( '_roots' ).that.is.instanceof( Map );
 			expect( doc._roots.size ).to.equal( 1 );
 			expect( doc.graveyard ).to.be.instanceof( RootElement );
 			expect( doc.graveyard.getChildCount() ).to.equal( 0 );
-			expect( doc.selection.getRanges().length ).to.equal( 0 );
+			expect( doc.selection.getRanges().length ).to.equal( 1 );
 		} );
 	} );
 
@@ -186,4 +186,18 @@ describe( 'Document', () => {
 
 		expect( doc.selection._updateAttributes.called ).to.be.true;
 	} );
+
+	describe( '_getDefaultRoot', () => {
+		it( 'should return graveyard root if there are no other roots in the document', () => {
+			expect( doc._getDefaultRoot() ).to.equal( doc.graveyard );
+		} );
+
+		it( 'should return the first root added to the document', () => {
+			let rootA = doc.createRoot( 'rootA' );
+			doc.createRoot( 'rootB' );
+			doc.createRoot( 'rootC' );
+
+			expect( doc._getDefaultRoot() ).to.equal( rootA );
+		} );
+	} );
 } );

+ 8 - 35
packages/ckeditor5-engine/tests/treemodel/selection.js

@@ -50,21 +50,17 @@ describe( 'Selection', () => {
 		liveRange.detach();
 	} );
 
-	it( 'should not have any range, attributes, anchor or focus position when just created', () => {
+	it( 'should be set to default range when just created', () => {
 		let ranges = selection.getRanges();
 
-		expect( ranges.length ).to.equal( 0 );
-		expect( selection.anchor ).to.be.null;
-		expect( selection.focus ).to.be.null;
+		expect( ranges.length ).to.equal( 1 );
+		expect( selection.anchor.isEqual( new Position( root, [ 0 ] ) ) ).to.be.true;
+		expect( selection.focus.isEqual( new Position( root, [ 0 ] ) ) ).to.be.true;
 		expect( selection._attrs ).to.be.instanceof( Map );
 		expect( selection._attrs.size ).to.equal( 0 );
 	} );
 
 	describe( 'isCollapsed', () => {
-		it( 'should be null if there are no ranges in it', () => {
-			expect( selection.isCollapsed ).to.be.null;
-		} );
-
 		it( 'should be true if all ranges are collapsed', () => {
 			selection.addRange( new Range( new Position( root, [ 0 ] ), new Position( root, [ 0 ] ) ) );
 
@@ -201,11 +197,10 @@ describe( 'Selection', () => {
 			ranges[ 1 ].detach.restore();
 		} );
 
-		it( 'should remove all stored ranges', () => {
-			expect( selection.getRanges().length ).to.equal( 0 );
-			expect( selection.anchor ).to.be.null;
-			expect( selection.focus ).to.be.null;
-			expect( selection.isCollapsed ).to.be.null;
+		it( 'should remove all stored ranges (and reset to default range)', () => {
+			expect( selection.getRanges().length ).to.equal( 1 );
+			expect( selection.anchor.isEqual( new Position( root, [ 0 ] ) ) ).to.be.true;
+			expect( selection.focus.isEqual( new Position( root, [ 0 ] ) ) ).to.be.true;
 		} );
 
 		it( 'should fire exactly one update event', () => {
@@ -280,24 +275,7 @@ describe( 'Selection', () => {
 		} );
 	} );
 
-	describe( 'hasAnyRange', () => {
-		it( 'should return false if there are no ranges added to the selection', () => {
-			selection.removeAllRanges();
-			expect( selection.hasAnyRange ).to.be.false;
-		} );
-
-		it( 'should return true if there is at least on range in the selection', () => {
-			selection.addRange( new Range( new Position( root, [ 0 ] ), new Position( root, [ 0 ] ) ) );
-			expect( selection.hasAnyRange ).to.be.true;
-		} );
-	} );
-
 	describe( 'getFirstRange', () => {
-		it( 'should return null if there are no ranges in selection', () => {
-			selection.removeAllRanges();
-			expect( selection.getFirstRange() ).to.be.null;
-		} );
-
 		it( 'should return a range which start position is before all other ranges\' start positions', () => {
 			// This will not be the first range despite being added as first
 			selection.addRange( new Range( new Position( root, [ 4 ] ), new Position( root, [ 5 ] ) ) );
@@ -316,11 +294,6 @@ describe( 'Selection', () => {
 	} );
 
 	describe( 'getFirstPosition', () => {
-		it( 'should return null if there are no ranges in selection', () => {
-			selection.removeAllRanges();
-			expect( selection.getFirstPosition() ).to.be.null;
-		} );
-
 		it( 'should return a position that is in selection and is before any other position from the selection', () => {
 			// This will not be a range containing the first position despite being added as first
 			selection.addRange( new Range( new Position( root, [ 4 ] ), new Position( root, [ 5 ] ) ) );