Browse Source

Merge pull request #653 from ckeditor/t/331

T/331 Selection range should not end up in graveyard.
Piotrek Koszuliński 9 years ago
parent
commit
54facc81cf

+ 1 - 1
packages/ckeditor5-engine/src/controller/insert.js

@@ -291,7 +291,7 @@ class Insertion {
 
 			// OK:  <p>xx[]</p> + <p>yy</p> => <p>xx[]yy</p> (when sticks to previous)
 			// NOK: <p>xx[]</p> + <p>yy</p> => <p>xxyy[]</p> (when sticks to next)
-			position = new LivePosition( this.position.root, this.position.path, 'STICKS_TO_PREVIOUS' );
+			position = new LivePosition( this.position.root, this.position.path, 'sticksToPrevious' );
 
 			this.batch.merge( mergePosRight );
 

+ 24 - 10
packages/ckeditor5-engine/src/model/liveposition.js

@@ -30,7 +30,7 @@ export default class LivePosition extends Position {
 	 * @see engine.model.Position
 	 * @param {engine.model.RootElement} root
 	 * @param {Array.<Number>} path
-	 * @param {engine.model.PositionStickiness} [stickiness] Defaults to `'STICKS_TO_NEXT'`.
+	 * @param {engine.model.PositionStickiness} [stickiness] Defaults to `'sticksToNext'`.
 	 * See {@link engine.model.LivePosition#stickiness}.
 	 */
 	constructor( root, path, stickiness ) {
@@ -65,7 +65,7 @@ export default class LivePosition extends Position {
 		 *
 		 * @member {engine.model.PositionStickiness} engine.model.LivePosition#stickiness
 		 */
-		this.stickiness = stickiness || 'STICKS_TO_NEXT';
+		this.stickiness = stickiness || 'sticksToNext';
 
 		bindWithDocument.call( this );
 	}
@@ -111,6 +111,13 @@ export default class LivePosition extends Position {
 	 * @param {engine.model.Position} position
 	 * @returns {engine.model.LivePosition}
 	 */
+
+	/**
+	 * Fired when `LivePosition` instance is changed due to changes on {@link engine.model.Document}.
+	 *
+	 * @event engine.model.LivePosition#change
+	 * @param {engine.model.Position} oldPosition Position equal to this live position before it got changed.
+	 */
 }
 
 /**
@@ -133,7 +140,8 @@ function bindWithDocument() {
 			if ( supportedTypes.has( type ) ) {
 				transform.call( this, type, changes.range, changes.sourcePosition );
 			}
-		}
+		},
+		{ priority: 'high' }
 	);
 }
 
@@ -155,7 +163,7 @@ function transform( type, range, position ) {
 
 	switch ( type ) {
 		case 'insert':
-			let insertBefore = this.stickiness == 'STICKS_TO_NEXT';
+			let insertBefore = this.stickiness == 'sticksToNext';
 			transformed = this._getTransformedByInsertion( range.start, howMany, insertBefore );
 			break;
 
@@ -165,28 +173,34 @@ function transform( type, range, position ) {
 			let originalRange = Range.createFromPositionAndShift( position, howMany );
 
 			let gotMoved = originalRange.containsPosition( this ) ||
-				( originalRange.start.isEqual( this ) && this.stickiness == 'STICKS_TO_NEXT' ) ||
-				( originalRange.end.isEqual( this ) && this.stickiness == 'STICKS_TO_PREVIOUS' );
+				( originalRange.start.isEqual( this ) && this.stickiness == 'sticksToNext' ) ||
+				( originalRange.end.isEqual( this ) && this.stickiness == 'sticksToPrevious' );
 
 			// We can't use ._getTransformedByMove() because we have a different if-condition.
 			if ( gotMoved ) {
 				transformed = this._getCombined( position, range.start );
 			} else {
-				let insertBefore = this.stickiness == 'STICKS_TO_NEXT';
+				let insertBefore = this.stickiness == 'sticksToNext';
 				transformed = this._getTransformedByMove( position, range.start, howMany, insertBefore );
 			}
 			break;
 	}
 
-	this.path = transformed.path;
-	this.root = transformed.root;
+	if ( !this.isEqual( transformed ) ) {
+		const oldPosition = Position.createFromPosition( this );
+
+		this.path = transformed.path;
+		this.root = transformed.root;
+
+		this.fire( 'change', oldPosition );
+	}
 }
 
 mix( LivePosition, EmitterMixin );
 
 /**
  * Enum representing how position is "sticking" with their neighbour nodes.
- * Possible values: `'STICKS_TO_NEXT'`, `'STICKS_TO_PREVIOUS'`.
+ * Possible values: `'sticksToNext'`, `'sticksToPrevious'`.
  *
  * @typedef {String} engine.model.PositionStickiness
  */

+ 7 - 2
packages/ckeditor5-engine/src/model/liverange.js

@@ -77,6 +77,8 @@ export default class LiveRange extends Range {
 	 * Fired when `LiveRange` instance is changed due to changes on {@link engine.model.Document}.
 	 *
 	 * @event engine.model.LiveRange#change
+	 * @param {engine.model.Range} oldRange Range with start and end position equal to start and end position of this live range
+	 * before it got changed.
 	 */
 }
 
@@ -99,7 +101,8 @@ function bindWithDocument() {
 			if ( supportedTypes.has( type ) ) {
 				transform.call( this, type, changes.range, changes.sourcePosition );
 			}
-		}
+		},
+		{ priority: 'high' }
 	);
 }
 
@@ -158,10 +161,12 @@ function transform( type, range, position ) {
 
 	// If anything changed, update the range and fire an event.
 	if ( !updated.start.isEqual( this.start ) || !updated.end.isEqual( this.end ) ) {
+		const oldRange = Range.createFromRange( this );
+
 		this.start = updated.start;
 		this.end = updated.end;
 
-		this.fire( 'change' );
+		this.fire( 'change', oldRange );
 	}
 }
 

+ 95 - 11
packages/ckeditor5-engine/src/model/liveselection.js

@@ -3,12 +3,14 @@
  * For licensing, see LICENSE.md.
  */
 
+import Position from './position.js';
 import Range from './range.js';
 import LiveRange from './liverange.js';
 import Text from './text.js';
 import TextProxy from './textproxy.js';
 import toMap from '../../utils/tomap.js';
 import CKEditorError from '../../utils/ckeditorerror.js';
+import log from '../../utils/log.js';
 
 import Selection from './selection.js';
 
@@ -63,14 +65,6 @@ export default class LiveSelection extends Selection {
 		 */
 		this._attributePriority = new Map();
 
-		// Whenever selection range changes, if the change comes directly from selection API (direct user change).
-		this.on( 'change:range', ( evt, data ) => {
-			if ( data.directChange ) {
-				// Reset attributes on selection (clear attributes and priorities) and get attributes from surrounding nodes.
-				this._updateAttributes( true );
-			}
-		}, { priority: 'high' } );
-
 		// Whenever attribute operation is performed on document, update attributes. This is not the most efficient
 		// way to update selection attributes, but should be okay for now. `_updateAttributes` will be fired too often,
 		// but it won't change attributes or fire `change:attribute` event if not needed.
@@ -150,6 +144,30 @@ export default class LiveSelection extends Selection {
 	/**
 	 * @inheritDoc
 	 */
+	addRange( range, isBackward = false ) {
+		super.addRange( range, isBackward );
+		this.refreshAttributes();
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	removeAllRanges() {
+		super.removeAllRanges();
+		this.refreshAttributes();
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	setRanges( newRanges, isLastBackward = false ) {
+		super.setRanges( newRanges, isLastBackward );
+		this.refreshAttributes();
+	}
+
+	/**
+	 * @inheritDoc
+	 */
 	setAttribute( key, value ) {
 		// Store attribute in parent element if the selection is collapsed in an empty node.
 		if ( this.isCollapsed && this.anchor.parent.childCount === 0 ) {
@@ -206,6 +224,13 @@ export default class LiveSelection extends Selection {
 	}
 
 	/**
+	 * Removes all attributes from the selection and sets attributes according to the surrounding nodes.
+	 */
+	refreshAttributes() {
+		this._updateAttributes( true );
+	}
+
+	/**
 	 * Creates and returns an instance of `LiveSelection` that is a clone of given selection, meaning that it has same
 	 * ranges and same direction as this selection.
 	 *
@@ -230,22 +255,58 @@ export default class LiveSelection extends Selection {
 	 * @inheritDoc
 	 */
 	_pushRange( range ) {
+		const liveRange = this._prepareRange( range );
+
+		// `undefined` is returned when given `range` is in graveyard root.
+		if ( liveRange ) {
+			this._ranges.push( liveRange );
+		}
+	}
+
+	/**
+	 * Prepares given range to be added to selection. Checks if it is correct, converts it to {@link engine.model.LiveRange LiveRange}
+	 * and sets listeners listening to the range's change event.
+	 *
+	 * @private
+	 * @param {engine.model.Range} range
+	 */
+	_prepareRange( range ) {
 		if ( !( range instanceof Range ) ) {
+			/**
+			 * Trying to add an object that is not an instance of Range.
+			 *
+			 * @error model-selection-added-not-range
+			 */
 			throw new CKEditorError( 'model-selection-added-not-range: Trying to add an object that is not an instance of Range.' );
 		}
 
+		if ( range.root == this._document.graveyard ) {
+			/**
+			 * Trying to add a Range that is in the graveyard root. Range rejected.
+			 *
+			 * @warning model-selection-range-in-graveyard
+			 */
+			log.warn( 'model-selection-range-in-graveyard: Trying to add a Range that is in the graveyard root. Range rejected.' );
+
+			return;
+		}
+
 		this._checkRange( range );
 
 		const liveRange = LiveRange.createFromRange( range );
-		this.listenTo( liveRange, 'change', () => {
+		this.listenTo( liveRange, 'change', ( evt, oldRange ) => {
+			if ( liveRange.root == this._document.graveyard ) {
+				this._fixGraveyardSelection( liveRange, oldRange );
+			}
+
 			this.fire( 'change:range', { directChange: false } );
 		} );
 
-		this._ranges.push( liveRange );
+		return liveRange;
 	}
 
 	/**
-	 * Updates this selection attributes according to it's ranges and the {@link engine.model.Document model document}.
+	 * Updates this selection attributes according to its ranges and the {@link engine.model.Document model document}.
 	 *
 	 * @protected
 	 * @param {Boolean} clearAll
@@ -544,6 +605,29 @@ export default class LiveSelection extends Selection {
 
 		return attrs;
 	}
+
+	/**
+	 * Fixes a selection range after it ends up in graveyard root.
+	 *
+	 * @private
+	 * @param {engine.model.LiveRange} gyRange The range added in selection, that ended up in graveyard root.
+	 * @param {engine.model.Range} oldRange The state of that range before it was added to graveyard root.
+	 */
+	_fixGraveyardSelection( gyRange, oldRange ) {
+		const gyPath = gyRange.start.path;
+
+		const newPathLength = oldRange.start.path.length - ( gyPath.length - 2 );
+		const newPath = oldRange.start.path.slice( 0, newPathLength );
+		newPath[ newPath.length - 1 ] -= gyPath[ 1 ];
+
+		const newPosition = new Position( oldRange.root, newPath );
+		const newRange = this._prepareRange( new Range( newPosition, newPosition ) );
+
+		const index = this._ranges.indexOf( gyRange );
+		this._ranges.splice( index, 1, newRange );
+
+		gyRange.detach();
+	}
 }
 
 // Helper function for {@link engine.model.LiveSelection#_updateAttributes}. It takes model item, checks whether

+ 12 - 18
packages/ckeditor5-engine/tests/model/composer/deletecontents.js

@@ -93,9 +93,7 @@ describe( 'Delete utils', () => {
 			} );
 
 			it( 'deletes characters (first half has attrs)', () => {
-				setData( doc, '<$text bold="true">fo[o</$text>b]ar', { selectionAttributes: {
-					bold: true
-				} } );
+				setData( doc, '<$text bold="true">fo[o</$text>b]ar' );
 
 				deleteContents( doc.batch(), doc.selection );
 
@@ -104,9 +102,7 @@ describe( 'Delete utils', () => {
 			} );
 
 			it( 'deletes characters (2nd half has attrs)', () => {
-				setData( doc, 'fo[o<$text bold="true">b]ar</$text>', { selectionAttributes: {
-					bold: true
-				} } );
+				setData( doc, 'fo[o<$text bold="true">b]ar</$text>' );
 
 				deleteContents( doc.batch(), doc.selection );
 
@@ -115,15 +111,7 @@ describe( 'Delete utils', () => {
 			} );
 
 			it( 'clears selection attrs when emptied content', () => {
-				setData(
-					doc,
-					'<paragraph>x</paragraph><paragraph>[<$text bold="true">foo</$text>]</paragraph><paragraph>y</paragraph>',
-					{
-						selectionAttributes: {
-							bold: true
-						}
-					}
-				);
+				setData( doc, '<paragraph>x</paragraph><paragraph>[<$text bold="true">foo</$text>]</paragraph><paragraph>y</paragraph>' );
 
 				deleteContents( doc.batch(), doc.selection );
 
@@ -132,9 +120,15 @@ describe( 'Delete utils', () => {
 			} );
 
 			it( 'leaves selection attributes when text contains them', () => {
-				setData( doc, '<paragraph>x<$text bold="true">a[foo]b</$text>y</paragraph>', { selectionAttributes: {
-					bold: true
-				} } );
+				setData(
+					doc,
+					'<paragraph>x<$text bold="true">a[foo]b</$text>y</paragraph>',
+					{
+						selectionAttributes: {
+							bold: true
+						}
+					}
+				);
 
 				deleteContents( doc.batch(), doc.selection );
 

+ 0 - 8
packages/ckeditor5-engine/tests/model/document/document.js

@@ -225,14 +225,6 @@ describe( 'Document', () => {
 	} );
 
 	describe( 'selection', () => {
-		it( 'should get updated attributes whenever selection gets updated', () => {
-			sinon.spy( doc.selection, '_updateAttributes' );
-
-			doc.selection.fire( 'change:range', { directChange: true } );
-
-			expect( doc.selection._updateAttributes.called ).to.be.true;
-		} );
-
 		it( 'should get updated attributes whenever attribute operation is applied', () => {
 			sinon.spy( doc.selection, '_updateAttributes' );
 

+ 42 - 8
packages/ckeditor5-engine/tests/model/liveposition.js

@@ -89,10 +89,13 @@ describe( 'LivePosition', () => {
 	} );
 
 	describe( 'should get transformed if', () => {
-		let live;
+		let live, spy;
 
 		beforeEach( () => {
 			live = new LivePosition( root, [ 1, 4, 6 ] );
+
+			spy = sinon.spy();
+			live.on( 'change', spy );
 		} );
 
 		afterEach( () => {
@@ -106,6 +109,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.path ).to.deep.equal( [ 1, 4, 9 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is at the same position and live position is sticking to right side', () => {
@@ -114,6 +118,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.path ).to.deep.equal( [ 1, 4, 9 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is before a node from the live position path', () => {
@@ -122,6 +127,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.path ).to.deep.equal( [ 1, 6, 6 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 		} );
 
@@ -137,6 +143,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( [ 1, 4, 9 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is at the same position and live position is sticking to right side', () => {
@@ -150,6 +157,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( [ 1, 4, 9 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is at a position before a node from the live position path', () => {
@@ -163,6 +171,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( [ 1, 6, 6 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is from the same parent and closer offset', () => {
@@ -176,6 +185,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( [ 1, 4, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is from a position before a node from the live position path', () => {
@@ -189,6 +199,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( [ 1, 0, 6 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'contains live position (same level)', () => {
@@ -202,6 +213,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( [ 2, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'contains live position (deep)', () => {
@@ -215,12 +227,13 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( [ 2, 1, 6 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 		} );
 	} );
 
 	describe( 'should not get transformed if', () => {
-		let path, otherRoot;
+		let path, otherRoot, spy;
 
 		before( () => {
 			path = [ 1, 4, 6 ];
@@ -231,6 +244,9 @@ describe( 'LivePosition', () => {
 
 		beforeEach( () => {
 			live = new LivePosition( root, path );
+
+			spy = sinon.spy();
+			live.on( 'change', spy );
 		} );
 
 		afterEach( () => {
@@ -244,17 +260,22 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is at the same position and live position is sticking to left side', () => {
-				let live = new LivePosition( root, path, 'STICKS_TO_PREVIOUS' );
+				let newLive = new LivePosition( root, path, 'sticksToPrevious' );
+				spy = sinon.spy();
+				newLive.on( 'change', spy );
+
 				let insertRange = new Range( new Position( root, [ 1, 4, 6 ] ), new Position( root, [ 1, 4, 9 ] ) );
 
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
-				expect( live.path ).to.deep.equal( path );
+				expect( newLive.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 
-				live.detach();
+				newLive.detach();
 			} );
 
 			it( 'is after a node from the position path', () => {
@@ -263,6 +284,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is in different root', () => {
@@ -271,6 +293,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 			} );
 		} );
 
@@ -286,10 +309,14 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is at the same position and live position is sticking to left side', () => {
-				let live = new LivePosition( root, path, 'STICKS_TO_PREVIOUS' );
+				let newLive = new LivePosition( root, path, 'sticksToPrevious' );
+				spy = sinon.spy();
+				newLive.on( 'change', spy );
+
 				let moveSource = new Position( root, [ 2 ] );
 				let moveRange = new Range( new Position( root, [ 1, 4, 6 ] ), new Position( root, [ 1, 4, 9 ] ) );
 
@@ -299,9 +326,10 @@ describe( 'LivePosition', () => {
 				};
 				doc.fire( 'change', 'move', changes, null );
 
-				expect( live.path ).to.deep.equal( path );
+				expect( newLive.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 
-				live.detach();
+				newLive.detach();
 			} );
 
 			it( 'is at a position after a node from the live position path', () => {
@@ -315,6 +343,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is from the same parent and further offset', () => {
@@ -328,6 +357,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is from a position after a node from the live position path', () => {
@@ -341,6 +371,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is to different root', () => {
@@ -354,6 +385,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is from different root', () => {
@@ -367,6 +399,7 @@ describe( 'LivePosition', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.path ).to.deep.equal( path );
+				expect( spy.called ).to.be.false;
 			} );
 		} );
 
@@ -381,6 +414,7 @@ describe( 'LivePosition', () => {
 			doc.fire( 'change', 'setAttribute', changes, null );
 
 			expect( live.path ).to.deep.equal( path );
+			expect( spy.called ).to.be.false;
 		} );
 	} );
 } );

+ 56 - 18
packages/ckeditor5-engine/tests/model/liverange.js

@@ -94,10 +94,13 @@ describe( 'LiveRange', () => {
 	// structure is representing what is _after_ operation is executed. So live LiveRange properties are describing
 	// virtual tree that is not existing anymore and event ranges are operating on the tree generated above.
 	describe( 'should get transformed if', () => {
-		let live;
+		let live, spy;
 
 		beforeEach( () => {
 			live = new LiveRange( new Position( root, [ 0, 1, 4 ] ), new Position( root, [ 0, 2, 2 ] ) );
+
+			spy = sinon.spy();
+			live.on( 'change', spy );
 		} );
 
 		afterEach( () => {
@@ -112,6 +115,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 8 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 2, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is in the same parent as range end and before it', () => {
@@ -121,6 +125,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 2, 5 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is at a position before a node from range start path', () => {
@@ -130,6 +135,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 3, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 4, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is at a position before a node from range end path', () => {
@@ -139,6 +145,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 3, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is at the live range start position and live range is collapsed', () => {
@@ -150,6 +157,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 8 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 1, 8 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 		} );
 
@@ -166,6 +174,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 8 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 2, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is to the same parent as range end and before it', () => {
@@ -180,6 +189,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 2, 6 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is to a position before a node from range start path', () => {
@@ -194,6 +204,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 3, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 4, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is to a position before a node from range end path', () => {
@@ -208,6 +219,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 3, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is from the same parent as range start and before it', () => {
@@ -222,6 +234,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 1 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 2, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is from the same parent as range end and before it', () => {
@@ -236,6 +249,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 2, 0 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is from a position before a node from range start path', () => {
@@ -250,6 +264,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 0, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 1, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'intersects on live range left side', () => {
@@ -264,6 +279,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 2 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 2, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'intersects on live range right side', () => {
@@ -278,6 +294,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 2, 1 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'intersects on live range left side and live range new start is touching moved range end', () => {
@@ -292,6 +309,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 5 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 7, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'intersects on live range right side and live range new end is touching moved range start', () => {
@@ -308,6 +326,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 3, 2 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is equal to live range', () => {
@@ -324,6 +343,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 3, 0 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 3, 3 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'contains live range', () => {
@@ -340,22 +360,7 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 3, 1 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 3, 4 ] );
-			} );
-
-			it( 'is inside live range and points to live range', () => {
-				live.end.path = [ 0, 1, 12 ];
-
-				let moveSource = new Position( root, [ 0, 1, 6 ] );
-				let moveRange = new Range( new Position( root, [ 0, 1, 8 ] ), new Position( root, [ 0, 1, 10 ] ) );
-
-				let changes = {
-					range: moveRange,
-					sourcePosition: moveSource
-				};
-				doc.fire( 'change', 'move', changes, null );
-
-				expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
-				expect( live.end.path ).to.deep.equal( [ 0, 1, 12 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 
 			it( 'is intersecting with live range and points to live range', () => {
@@ -372,12 +377,13 @@ describe( 'LiveRange', () => {
 
 				expect( live.start.path ).to.deep.equal( [ 0, 1, 2 ] );
 				expect( live.end.path ).to.deep.equal( [ 0, 1, 12 ] );
+				expect( spy.calledOnce ).to.be.true;
 			} );
 		} );
 	} );
 
 	describe( 'should not get transformed if', () => {
-		let otherRoot;
+		let otherRoot, spy;
 
 		before( () => {
 			otherRoot = doc.createRoot( '$root', 'otherRoot' );
@@ -388,6 +394,9 @@ describe( 'LiveRange', () => {
 		beforeEach( () => {
 			live = new LiveRange( new Position( root, [ 0, 1, 4 ] ), new Position( root, [ 0, 2, 2 ] ) );
 			clone = Range.createFromRange( live );
+
+			spy = sinon.spy();
+			live.on( 'change', spy );
 		} );
 
 		afterEach( () => {
@@ -401,6 +410,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is in the same parent as range end and after it', () => {
@@ -409,6 +419,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is to a position after a node from range end path', () => {
@@ -417,6 +428,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is in different root', () => {
@@ -425,6 +437,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'insert', { range: insertRange }, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 		} );
 
@@ -440,6 +453,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is to the same parent as range end and after it', () => {
@@ -453,6 +467,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is to a position after a node from range end path', () => {
@@ -466,6 +481,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is from the same parent as range start and after it', () => {
@@ -479,6 +495,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is from the same parent as range end and after it', () => {
@@ -492,6 +509,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is from a position after a node from range end path', () => {
@@ -505,6 +523,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is to different root', () => {
@@ -518,6 +537,7 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
 			} );
 
 			it( 'is from different root', () => {
@@ -531,6 +551,24 @@ describe( 'LiveRange', () => {
 				doc.fire( 'change', 'move', changes, null );
 
 				expect( live.isEqual( clone ) ).to.be.true;
+				expect( spy.called ).to.be.false;
+			} );
+
+			it( 'is inside live range and points to live range', () => {
+				live.end.path = [ 0, 1, 12 ];
+
+				let moveSource = new Position( root, [ 0, 1, 6 ] );
+				let moveRange = new Range( new Position( root, [ 0, 1, 8 ] ), new Position( root, [ 0, 1, 10 ] ) );
+
+				let changes = {
+					range: moveRange,
+					sourcePosition: moveSource
+				};
+				doc.fire( 'change', 'move', changes, null );
+
+				expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
+				expect( live.end.path ).to.deep.equal( [ 0, 1, 12 ] );
+				expect( spy.calledOnce ).to.be.false;
 			} );
 		} );
 	} );

+ 77 - 3
packages/ckeditor5-engine/tests/model/liveselection.js

@@ -14,6 +14,7 @@ import LiveRange from '/ckeditor5/engine/model/liverange.js';
 import LiveSelection from '/ckeditor5/engine/model/liveselection.js';
 import InsertOperation from '/ckeditor5/engine/model/operation/insertoperation.js';
 import MoveOperation from '/ckeditor5/engine/model/operation/moveoperation.js';
+import RemoveOperation from '/ckeditor5/engine/model/operation/removeoperation.js';
 import AttributeOperation from '/ckeditor5/engine/model/operation/attributeoperation.js';
 import CKEditorError from '/ckeditor5/utils/ckeditorerror.js';
 import count from '/ckeditor5/utils/count.js';
@@ -125,9 +126,7 @@ describe( 'LiveSelection', () => {
 		it( 'should convert added Range to LiveRange', () => {
 			selection.addRange( range );
 
-			const ranges = selection._ranges;
-
-			expect( ranges[ 0 ] ).to.be.instanceof( LiveRange );
+			expect( selection._ranges[ 0 ] ).to.be.instanceof( LiveRange );
 		} );
 
 		it( 'should throw an error when range is invalid', () => {
@@ -135,6 +134,20 @@ describe( 'LiveSelection', () => {
 				selection.addRange( { invalid: 'range' } );
 			} ).to.throw( CKEditorError, /model-selection-added-not-range/ );
 		} );
+
+		it( 'should not add a range that is in graveyard', () => {
+			selection.addRange( Range.createIn( doc.graveyard ) );
+
+			expect( selection._ranges.length ).to.equal( 0 );
+		} );
+
+		it( 'should refresh attributes', () => {
+			const spy = sinon.spy( selection, '_updateAttributes' );
+
+			selection.addRange( range );
+
+			expect( spy.called ).to.be.true;
+		} );
 	} );
 
 	describe( 'collapse', () => {
@@ -227,6 +240,14 @@ describe( 'LiveSelection', () => {
 			expect( ranges[ 0 ].detach.called ).to.be.true;
 			expect( ranges[ 1 ].detach.called ).to.be.true;
 		} );
+
+		it( 'should refresh attributes', () => {
+			const spy = sinon.spy( selection, '_updateAttributes' );
+
+			selection.removeAllRanges();
+
+			expect( spy.called ).to.be.true;
+		} );
 	} );
 
 	describe( 'setRanges', () => {
@@ -250,6 +271,14 @@ describe( 'LiveSelection', () => {
 			expect( oldRanges[ 0 ].detach.called ).to.be.true;
 			expect( oldRanges[ 1 ].detach.called ).to.be.true;
 		} );
+
+		it( 'should refresh attributes', () => {
+			const spy = sinon.spy( selection, '_updateAttributes' );
+
+			selection.setRanges( [ range ] );
+
+			expect( spy.called ).to.be.true;
+		} );
 	} );
 
 	describe( 'getFirstRange', () => {
@@ -284,6 +313,7 @@ describe( 'LiveSelection', () => {
 		let spyRange;
 
 		beforeEach( () => {
+			root.removeChildren( 0, root.childCount );
 			root.insertChildren( 0, [
 				new Element( 'ul', [], new Text( 'abcdef' ) ),
 				new Element( 'p', [], new Text( 'foobar' ) ),
@@ -518,6 +548,50 @@ describe( 'LiveSelection', () => {
 				expect( spyAttribute.called ).to.be.false;
 			} );
 		} );
+
+		describe( 'RemoveOperation', () => {
+			it( 'fix selection range if it ends up in graveyard #1', () => {
+				selection.collapse( new Position( root, [ 1, 3 ] ) );
+
+				doc.applyOperation( wrapInDelta(
+					new RemoveOperation(
+						new Position( root, [ 1, 2 ] ),
+						2,
+						doc.version
+					)
+				) );
+
+				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 2 ] );
+			} );
+
+			it( 'fix selection range if it ends up in graveyard #2', () => {
+				selection.setRanges( [ new Range( new Position( root, [ 1, 2 ] ), new Position( root, [ 1, 4 ] ) ) ] );
+
+				doc.applyOperation( wrapInDelta(
+					new RemoveOperation(
+						new Position( root, [ 1, 2 ] ),
+						2,
+						doc.version
+					)
+				) );
+
+				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 2 ] );
+			} );
+
+			it( 'fix selection range if it ends up in graveyard #3', () => {
+				selection.setRanges( [ new Range( new Position( root, [ 1, 2 ] ), new Position( root, [ 1, 4 ] ) ) ] );
+
+				doc.applyOperation( wrapInDelta(
+					new RemoveOperation(
+						new Position( root, [ 0 ] ),
+						3,
+						doc.version
+					)
+				) );
+
+				expect( selection.getFirstPosition().path ).to.deep.equal( [ 0 ] );
+			} );
+		} );
 	} );
 
 	describe( 'attributes interface', () => {