Merge pull request #1834 from ckeditor/i/6501

Fix: Intersecting ranges resulting when fixing graveyard selection no longer break the editor. Closes ckeditor/ckeditor5#6501. Closes ckeditor/ckeditor5#6382.

Author: Reinmar

src/model/documentselection.js

@@ -1137,14 +1137,16 @@ class LiveSelection extends Selection {
 		liveRange.detach();
 
 		// If nearest valid selection range has been found - add it in the place of old range.
-		if ( selectionRange ) {
+		// If range is equal to any other selection ranges then it is probably due to contents
+		// of a multi-range selection being removed. See ckeditor/ckeditor5#6501.
+		if ( selectionRange && !isRangeCollidingWithSelection( selectionRange, this ) ) {
 			// Check the range, convert it to live range, bind events, etc.
 			const newRange = this._prepareRange( selectionRange );
 
 			// Add new range in the place of old range.
 			this._ranges.splice( index, 0, newRange );
 		}
-		// If nearest valid selection range cannot be found - just removing the old range is fine.
+		// If nearest valid selection range cannot be found or is intersecting with other selection ranges removing the old range is fine.
 	}
 }
 
@@ -1164,7 +1166,6 @@ function getAttrsIfCharacter( node ) {
 
 // Removes selection attributes from element which is not empty anymore.
 //
-// @private
 // @param {module:engine/model/model~Model} model
 // @param {module:engine/model/batch~Batch} batch
 function clearAttributesStoredInElement( model, batch ) {
@@ -1190,3 +1191,8 @@ function clearAttributesStoredInElement( model, batch ) {
 		}
 	}
 }
+
+// Checks if range collides with any of selection ranges.
+function isRangeCollidingWithSelection( range, selection ) {
+	return !selection._ranges.every( selectionRange => !range.isEqual( selectionRange ) );
+}

tests/model/documentselection.js

@@ -1710,7 +1710,7 @@ describe( 'DocumentSelection', () => {
 		} );
 
 		describe( 'MoveOperation to graveyard', () => {
-			it( 'fix selection range if it ends up in graveyard #1', () => {
+			it( 'fix selection range if it ends up in graveyard - collapsed selection', () => {
 				selection._setTo( new Position( root, [ 1, 3 ] ) );
 
 				model.applyOperation(
@@ -1725,7 +1725,7 @@ describe( 'DocumentSelection', () => {
 				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 2 ] );
 			} );
 
-			it( 'fix selection range if it ends up in graveyard #2', () => {
+			it( 'fix selection range if it ends up in graveyard - text from non-collapsed selection is moved', () => {
 				selection._setTo( [ new Range( new Position( root, [ 1, 2 ] ), new Position( root, [ 1, 4 ] ) ) ] );
 
 				model.applyOperation(
@@ -1740,7 +1740,7 @@ describe( 'DocumentSelection', () => {
 				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 2 ] );
 			} );
 
-			it( 'fix selection range if it ends up in graveyard #3', () => {
+			it( 'fix selection range if it ends up in graveyard - parent of non-collapsed selection is moved', () => {
 				selection._setTo( [ new Range( new Position( root, [ 1, 1 ] ), new Position( root, [ 1, 2 ] ) ) ] );
 
 				model.applyOperation(
@@ -1755,7 +1755,7 @@ describe( 'DocumentSelection', () => {
 				expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 6 ] );
 			} );
 
-			it( 'fix selection range if it ends up in graveyard #4 - whole content removed', () => {
+			it( 'fix selection range if it ends up in graveyard - whole content removed', () => {
 				model.applyOperation(
 					new MoveOperation(
 						new Position( root, [ 0 ] ),
@@ -1778,6 +1778,63 @@ describe( 'DocumentSelection', () => {
 				// Now it's clear that it's the default range.
 				expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] );
 			} );
+
+			it( 'handles multi-range selection in a text node by merging it into one range (resulting in collapsed ranges)', () => {
+				const ranges = [
+					new Range( new Position( root, [ 1, 1 ] ), new Position( root, [ 1, 2 ] ) ),
+					new Range( new Position( root, [ 1, 3 ] ), new Position( root, [ 1, 4 ] ) )
+				];
+
+				selection._setTo( ranges );
+
+				model.applyOperation(
+					new MoveOperation(
+						new Position( root, [ 1, 1 ] ),
+						4,
+						new Position( doc.graveyard, [ 0 ] ),
+						doc.version
+					)
+				);
+
+				expect( selection.rangeCount ).to.equal( 1 );
+				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 1 ] );
+				expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 1 ] );
+			} );
+
+			it( 'handles multi-range selection on object nodes by merging it into one range (resulting in non-collapsed ranges)', () => {
+				model.schema.register( 'outer', {
+					isObject: true
+				} );
+				model.schema.register( 'inner', {
+					isObject: true,
+					allowIn: 'outer'
+				} );
+
+				root._removeChildren( 0, root.childCount );
+				root._insertChild( 0, [
+					new Element( 'outer', [], [ new Element( 'inner' ), new Element( 'inner' ), new Element( 'inner' ) ] )
+				] );
+
+				const ranges = [
+					new Range( new Position( root, [ 0, 0 ] ), new Position( root, [ 0, 1 ] ) ),
+					new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 0, 2 ] ) )
+				];
+
+				selection._setTo( ranges );
+
+				model.applyOperation(
+					new MoveOperation(
+						new Position( root, [ 0, 0 ] ),
+						2,
+						new Position( doc.graveyard, [ 0 ] ),
+						doc.version
+					)
+				);
+
+				expect( selection.rangeCount ).to.equal( 1 );
+				expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] );
+				expect( selection.getLastPosition().path ).to.deep.equal( [ 0, 1 ] );
+			} );
 		} );
 
 		it( '`DocumentSelection#change:range` event should be fire once even if selection contains multi-ranges', () => {

tests/model/range.js

@@ -156,6 +156,13 @@ describe( 'Range', () => {
 			const otherRange = new Range( new Position( otherRoot, [ 0 ] ), new Position( otherRoot, [ 1, 4 ] ) );
 			expect( range.isIntersecting( otherRange ) ).to.be.false;
 		} );
+
+		it( 'should return false if ranges are collapsed and equal', () => {
+			range = new Range( new Position( root, [ 1, 1 ] ) );
+			const otherRange = new Range( new Position( otherRoot, [ 1, 1 ] ) );
+
+			expect( range.isIntersecting( otherRange ) ).to.be.false;
+		} );
 	} );
 
 	describe( 'static constructors', () => {