Merge pull request #7662 from ckeditor/i/7659

Fix (engine): Fixed incorrect selection fixing in some multi-cell selection scenarios. Closes #7659.

packages/ckeditor5-engine/src/model/documentselection.js

@@ -567,7 +567,6 @@ mix( DocumentSelection, EmitterMixin );
 //
 // @extends module:engine/model/selection~Selection
 //
-
 class LiveSelection extends Selection {
 	// Creates an empty live selection for given {@link module:engine/model/document~Document}.
 	// @param {module:engine/model/document~Document} doc Document which owns this selection.
@@ -602,10 +601,10 @@ class LiveSelection extends Selection {
 		// @member {Map} module:engine/model/liveselection~LiveSelection#_attributePriority
 		this._attributePriority = new Map();
 
-		// Contains data required to fix ranges which have been moved to the graveyard.
+		// Position to which the selection should be set if the last selection range was moved to the graveyard.
 		// @private
-		// @member {Array} module:engine/model/liveselection~LiveSelection#_fixGraveyardRangesData
-		this._fixGraveyardRangesData = [];
+		// @member {module:engine/model/position~Position} module:engine/model/liveselection~LiveSelection#_selectionRestorePosition
+		this._selectionRestorePosition = null;
 
 		// Flag that informs whether the selection ranges have changed. It is changed on true when `LiveRange#change:range` event is fired.
 		// @private
@@ -628,12 +627,14 @@ class LiveSelection extends Selection {
 				return;
 			}
 
-			while ( this._fixGraveyardRangesData.length ) {
-				const { liveRange, sourcePosition } = this._fixGraveyardRangesData.shift();
-
-				this._fixGraveyardSelection( liveRange, sourcePosition );
+			// Fix selection if the last range was removed from it and we have a position to which we can restore the selection.
+			if ( this._ranges.length == 0 && this._selectionRestorePosition ) {
+				this._fixGraveyardSelection( this._selectionRestorePosition );
 			}
 
+			// "Forget" the restore position even if it was not "used".
+			this._selectionRestorePosition = null;
+
 			if ( this._hasChangedRange ) {
 				this._hasChangedRange = false;
 				this.fire( 'change:range', { directChange: false } );
@@ -827,15 +828,17 @@ class LiveSelection extends Selection {
 
 		const liveRange = LiveRange.fromRange( range );
 
+		// If selection range is moved to the graveyard remove it from the selection object.
+		// Also, save some data that can be used to restore selection later, on `Model#applyOperation` event.
 		liveRange.on( 'change:range', ( evt, oldRange, data ) => {
 			this._hasChangedRange = true;
 
-			// If `LiveRange` is in whole moved to the graveyard, save necessary data. It will be fixed on `Model#applyOperation` event.
 			if ( liveRange.root == this._document.graveyard ) {
-				this._fixGraveyardRangesData.push( {
-					liveRange,
-					sourcePosition: data.deletionPosition
-				} );
+				this._selectionRestorePosition = data.deletionPosition;
+
+				const index = this._ranges.indexOf( liveRange );
+				this._ranges.splice( index, 1 );
+				liveRange.detach();
 			}
 		} );
 
@@ -1117,36 +1120,20 @@ class LiveSelection extends Selection {
 		return attrs;
 	}
 
-	// Fixes a selection range after it ends up in graveyard root.
+	// Fixes the selection after all its ranges got removed.
 	//
 	// @private
-	// @param {module:engine/model/liverange~LiveRange} liveRange The range from selection, that ended up in the graveyard root.
-	// @param {module:engine/model/position~Position} removedRangeStart Start position of a range which was removed.
-	_fixGraveyardSelection( liveRange, removedRangeStart ) {
-		// The start of the removed range is the closest position to the `liveRange` - the original selection range.
-		// This is a good candidate for a fixed selection range.
-		const positionCandidate = removedRangeStart.clone();
-
-		// Find a range that is a correct selection range and is closest to the start of removed range.
-		const selectionRange = this._model.schema.getNearestSelectionRange( positionCandidate );
-
-		// Remove the old selection range before preparing and adding new selection range. This order is important,
-		// because new range, in some cases, may intersect with old range (it depends on `getNearestSelectionRange()` result).
-		const index = this._ranges.indexOf( liveRange );
-		this._ranges.splice( index, 1 );
-		liveRange.detach();
+	// @param {module:engine/model/position~Position} deletionPosition Position where the deletion happened.
+	_fixGraveyardSelection( deletionPosition ) {
+		// Find a range that is a correct selection range and is closest to the position where the deletion happened.
+		const selectionRange = this._model.schema.getNearestSelectionRange( deletionPosition );
 
 		// If nearest valid selection range has been found - add it in the place of old range.
-		// 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 ) ) {
+		if ( selectionRange ) {
 			// 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 );
+			this._pushRange( selectionRange );
 		}
-		// If nearest valid selection range cannot be found or is intersecting with other selection ranges removing the old range is fine.
+		// If nearest valid selection range cannot be found don't add any range. Selection will be set to the default range.
 	}
 }
 
@@ -1191,8 +1178,3 @@ 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 ) );
-}

packages/ckeditor5-engine/tests/model/documentselection.js

@@ -1779,7 +1779,7 @@ describe( 'DocumentSelection', () => {
 				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)', () => {
+			it( 'handles multi-range selection in a text node by merging it into one range (resulting in a collapsed range)', () => {
 				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 ] ) )
@@ -1789,19 +1789,19 @@ describe( 'DocumentSelection', () => {
 
 				model.applyOperation(
 					new MoveOperation(
-						new Position( root, [ 1, 1 ] ),
-						4,
+						new Position( root, [ 1, 0 ] ),
+						5,
 						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 ] );
+				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 0 ] );
+				expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 0 ] );
 			} );
 
-			it( 'handles multi-range selection on object nodes by merging it into one range (resulting in non-collapsed ranges)', () => {
+			it( 'handles multi-range selection on object nodes by merging it into one range (resulting in a non-collapsed range)', () => {
 				model.schema.register( 'outer', {
 					isObject: true
 				} );
@@ -1835,6 +1835,28 @@ describe( 'DocumentSelection', () => {
 				expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] );
 				expect( selection.getLastPosition().path ).to.deep.equal( [ 0, 1 ] );
 			} );
+
+			it( 'should not fix the selection if not all ranges were removed', () => {
+				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 ] ),
+						1,
+						new Position( doc.graveyard, [ 0 ] ),
+						doc.version
+					)
+				);
+
+				expect( selection.rangeCount ).to.equal( 1 );
+				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 2 ] );
+				expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 3 ] );
+			} );
 		} );
 
 		it( '`DocumentSelection#change:range` event should be fire once even if selection contains multi-ranges', () => {

packages/ckeditor5-table/tests/tableselection-integration.js

@@ -220,6 +220,40 @@ describe( 'TableSelection - integration', () => {
 				'</table>'
 			);
 		} );
+
+		// https://github.com/ckeditor/ckeditor5/issues/7659.
+		// The fix is in the `DocumentSelection` class but this test is here to make sure that the fix works
+		// and that the behavior won't change in the future.
+		it( 'should not fix selection if not all ranges were removed', () => {
+			// [ ][ ][ ]
+			// [x][x][ ]
+			// [x][x][ ]
+			tableSelection.setCellSelection(
+				modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
+				modelRoot.getNodeByPath( [ 0, 2, 1 ] )
+			);
+
+			editor.model.change( writer => {
+				// Remove second row.
+				writer.remove( modelRoot.getNodeByPath( [ 0, 1 ] ) );
+			} );
+
+			assertEqualMarkup(
+				getModelData( model ),
+				'<table>' +
+					'<tableRow>' +
+						'<tableCell><paragraph>11</paragraph></tableCell>' +
+						'<tableCell><paragraph>12</paragraph></tableCell>' +
+						'<tableCell><paragraph>13</paragraph></tableCell>' +
+					'</tableRow>' +
+					'<tableRow>' +
+						'[<tableCell><paragraph>31</paragraph></tableCell>]' +
+						'[<tableCell><paragraph>32</paragraph></tableCell>]' +
+						'<tableCell><paragraph>33</paragraph></tableCell>' +
+					'</tableRow>' +
+				'</table>'
+			);
+		} );
 	} );
 
 	describe( 'with undo', () => {

packages/ckeditor5-typing/tests/inputcommand.js

@@ -259,7 +259,7 @@ describe( 'InputCommand', () => {
 				model.change( writer => {
 					let rangeSelection;
 
-					for ( const range of selection.getRanges() ) {
+					for ( const range of Array.from( selection.getRanges() ) ) {
 						rangeSelection = writer.createSelection( range );
 
 						model.deleteContent( rangeSelection );