Merge pull request #7508 from ckeditor/i/6639

Other (table): Restoring the document selection to the ranges as they were before undoing table cells merge. Closes #6639.
Feature (engine): Added `Range#getJoined()` method for joining ranges.
Internal (table): Removed workaround for `model-selection-range-intersects` errors.
Internal (undo): Fixed normalization of restored selection ranges.

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

@@ -276,6 +276,63 @@ export default class Range {
 		return null;
 	}
 
+	/**
+	 * Returns a range created by joining this {@link ~Range range} with the given {@link ~Range range}.
+	 * If ranges have no common part, returns `null`.
+	 *
+	 * Examples:
+	 *
+	 *		let range = model.createRange(
+	 *			model.createPositionFromPath( root, [ 2, 7 ] ),
+	 *			model.createPositionFromPath( root, [ 4, 0, 1 ] )
+	 *		);
+	 *		let otherRange = model.createRange(
+	 *			model.createPositionFromPath( root, [ 1 ] ),
+	 *			model.createPositionFromPath( root, [ 2 ] )
+ 	 *		);
+	 *		let transformed = range.getJoined( otherRange ); // null - ranges have no common part
+	 *
+	 *		otherRange = model.createRange(
+	 *			model.createPositionFromPath( root, [ 3 ] ),
+	 *			model.createPositionFromPath( root, [ 5 ] )
+	 *		);
+	 *		transformed = range.getJoined( otherRange ); // range from [ 2, 7 ] to [ 5 ]
+	 *
+	 * @param {module:engine/model/range~Range} otherRange Range to be joined.
+	 * @param {Boolean} [loose=false] Whether the intersection check is loose or strict. If the check is strict (`false`),
+	 * ranges are tested for intersection or whether start/end positions are equal. If the check is loose (`true`),
+	 * compared range is also checked if it's {@link module:engine/model/position~Position#isTouching touching} current range.
+	 * @returns {module:engine/model/range~Range|null} A sum of given ranges or `null` if ranges have no common part.
+	 */
+	getJoined( otherRange, loose = false ) {
+		let shouldJoin = this.isIntersecting( otherRange );
+
+		if ( !shouldJoin ) {
+			if ( this.start.isBefore( otherRange.start ) ) {
+				shouldJoin = loose ? this.end.isTouching( otherRange.start ) : this.end.isEqual( otherRange.start );
+			} else {
+				shouldJoin = loose ? otherRange.end.isTouching( this.start ) : otherRange.end.isEqual( this.start );
+			}
+		}
+
+		if ( !shouldJoin ) {
+			return null;
+		}
+
+		let startPosition = this.start;
+		let endPosition = this.end;
+
+		if ( otherRange.start.isBefore( startPosition ) ) {
+			startPosition = otherRange.start;
+		}
+
+		if ( otherRange.end.isAfter( endPosition ) ) {
+			endPosition = otherRange.end;
+		}
+
+		return new Range( startPosition, endPosition );
+	}
+
 	/**
 	 * Computes and returns the smallest set of {@link #isFlat flat} ranges, that covers this range in whole.
 	 *

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

@@ -789,6 +789,116 @@ describe( 'Range', () => {
 		} );
 	} );
 
+	describe( 'getJoined()', () => {
+		let range;
+
+		beforeEach( () => {
+			range = new Range( new Position( root, [ 3, 2 ] ), new Position( root, [ 5, 4 ] ) );
+		} );
+
+		it( 'should return null if ranges do not intersect nor have equal start/end', () => {
+			const otherRange = new Range( new Position( root, [ 5, 5 ] ), new Position( root, [ 7 ] ) );
+			const sum = range.getJoined( otherRange );
+
+			expect( sum ).to.be.null;
+		} );
+
+		it( 'should return a range spanning both of the ranges if the ranges have equal start/end positions', () => {
+			const otherRange = new Range( new Position( root, [ 5, 4 ] ), new Position( root, [ 7 ] ) );
+			const sum = range.getJoined( otherRange );
+
+			expect( sum.start.path ).to.deep.equal( [ 3, 2 ] );
+			expect( sum.end.path ).to.deep.equal( [ 7 ] );
+		} );
+
+		it( 'should return a range spanning both of the ranges if the ranges have equal start/end positions (different order)', () => {
+			const otherRange = new Range( new Position( root, [ 1, 4 ] ), new Position( root, [ 3, 2 ] ) );
+			const sum = range.getJoined( otherRange );
+
+			expect( sum.start.path ).to.deep.equal( [ 1, 4 ] );
+			expect( sum.end.path ).to.deep.equal( [ 5, 4 ] );
+		} );
+
+		it( 'should return a range spanning both of the ranges - original range contains the other range', () => {
+			const otherRange = new Range( new Position( root, [ 4 ] ), new Position( root, [ 5 ] ) );
+			const sum = range.getJoined( otherRange );
+
+			expect( sum.isEqual( range ) ).to.be.true;
+		} );
+
+		it( 'should return a range spanning both of the ranges - original range is contained by the other range', () => {
+			const otherRange = new Range( new Position( root, [ 3 ] ), new Position( root, [ 6 ] ) );
+			const sum = range.getJoined( otherRange );
+
+			expect( sum.isEqual( otherRange ) ).to.be.true;
+		} );
+
+		it( 'should return a range spanning both of the ranges - original range intersects with the other range', () => {
+			const otherRange = new Range( new Position( root, [ 3 ] ), new Position( root, [ 4, 7 ] ) );
+			const sum = range.getJoined( otherRange );
+
+			expect( sum.start.path ).to.deep.equal( [ 3 ] );
+			expect( sum.end.path ).to.deep.equal( [ 5, 4 ] );
+		} );
+
+		it( 'should return a range spanning both of the ranges if both ranges are equal', () => {
+			const otherRange = range.clone();
+			const sum = range.getJoined( otherRange );
+
+			expect( sum.isEqual( range ) ).to.be.true;
+		} );
+
+		describe( 'with `loose` option enabled', () => {
+			beforeEach( () => {
+				prepareRichRoot( root );
+			} );
+
+			it( 'should return null if ranges are not intersecting nor touching', () => {
+				const range = new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 3 ] ) );
+				const otherRange = new Range( new Position( root, [ 3, 1 ] ), new Position( root, [ 3, 2 ] ) );
+				const sum = range.getJoined( otherRange, true );
+
+				expect( sum ).to.be.null;
+			} );
+
+			it( 'should return a range spanning both of the ranges - original range end is equal to other range start position', () => {
+				const range = new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 3 ] ) );
+				const otherRange = new Range( new Position( root, [ 3 ] ), new Position( root, [ 3, 2 ] ) );
+				const sum = range.getJoined( otherRange, true );
+
+				expect( sum.start.path ).to.deep.equal( [ 0, 1 ] );
+				expect( sum.end.path ).to.deep.equal( [ 3, 2 ] );
+			} );
+
+			it( 'should return a range spanning both of the ranges - original range start is equal to other range end position', () => {
+				const range = new Range( new Position( root, [ 3 ] ), new Position( root, [ 3, 2 ] ) );
+				const otherRange = new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 3 ] ) );
+				const sum = range.getJoined( otherRange, true );
+
+				expect( sum.start.path ).to.deep.equal( [ 0, 1 ] );
+				expect( sum.end.path ).to.deep.equal( [ 3, 2 ] );
+			} );
+
+			it( 'should return a range spanning both of the ranges - original range is touching other range on the right side', () => {
+				const range = new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 3 ] ) );
+				const otherRange = new Range( new Position( root, [ 3, 0 ] ), new Position( root, [ 3, 2 ] ) );
+				const sum = range.getJoined( otherRange, true );
+
+				expect( sum.start.path ).to.deep.equal( [ 0, 1 ] );
+				expect( sum.end.path ).to.deep.equal( [ 3, 2 ] );
+			} );
+
+			it( 'should return a range spanning both of the ranges - original range is touching other range on the left side', () => {
+				const range = new Range( new Position( root, [ 1, 0 ] ), new Position( root, [ 3, 2 ] ) );
+				const otherRange = new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 0, 2 ] ) );
+				const sum = range.getJoined( otherRange, true );
+
+				expect( sum.start.path ).to.deep.equal( [ 0, 1 ] );
+				expect( sum.end.path ).to.deep.equal( [ 3, 2 ] );
+			} );
+		} );
+	} );
+
 	// Note: We don't create model element structure in these tests because this method
 	// is used by OT so it must not check the structure.
 	describe( 'getTransformedByOperation()', () => {

packages/ckeditor5-table/src/commands/mergecellscommand.js

@@ -48,11 +48,6 @@ export default class MergeCellsCommand extends Command {
 			// All cells will be merged into the first one.
 			const firstTableCell = selectedTableCells.shift();
 
-			// Set the selection in cell that other cells are being merged to prevent model-selection-range-intersects error in undo.
-			// See https://github.com/ckeditor/ckeditor5/issues/6634.
-			// May be fixed by: https://github.com/ckeditor/ckeditor5/issues/6639.
-			writer.setSelection( firstTableCell, 0 );
-
 			// Update target cell dimensions.
 			const { mergeWidth, mergeHeight } = getMergeDimensions( firstTableCell, selectedTableCells, tableUtils );
 			updateNumericAttribute( 'colspan', mergeWidth, firstTableCell, writer );

packages/ckeditor5-table/src/commands/removerowcommand.js

@@ -63,10 +63,7 @@ export default class RemoveRowCommand extends Command {
 		// Use single batch to modify table in steps but in one undo step.
 		const batch = model.createBatch();
 
-		model.enqueueChange( batch, writer => {
-			// This prevents the "model-selection-range-intersects" error, caused by removing row selected cells.
-			writer.setSelection( writer.createSelection( table, 'on' ) );
-
+		model.enqueueChange( batch, () => {
 			const rowsToRemove = removedRowIndexes.last - removedRowIndexes.first + 1;
 
 			this.editor.plugins.get( 'TableUtils' ).removeRows( table, {

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

@@ -18,7 +18,7 @@ import TableClipboard from '../src/tableclipboard';
 
 import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 
-import { modelTable } from './_utils/utils';
+import { assertSelectedCells, modelTable } from './_utils/utils';
 import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils';
 import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata';
 import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard';
@@ -227,7 +227,6 @@ describe( 'TableSelection - integration', () => {
 			await setupEditor( [ UndoEditing ] );
 		} );
 
-		// See https://github.com/ckeditor/ckeditor5/issues/6634.
 		it( 'works with merge cells command', () => {
 			setModelData( editor.model, modelTable( [
 				[ '00', '01' ],
@@ -248,10 +247,15 @@ describe( 'TableSelection - integration', () => {
 
 			editor.execute( 'undo' );
 
-			assertEqualMarkup( getModelData( model ), modelTable( [
-				[ '[]00', '01' ],
+			assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [
+				[ '00', '01' ],
 				[ '10', '11' ]
 			] ) );
+
+			assertSelectedCells( model, [
+				[ 1, 1 ],
+				[ 0, 0 ]
+			] );
 		} );
 	} );
 

packages/ckeditor5-undo/package.json

@@ -22,7 +22,8 @@
     "@ckeditor/ckeditor5-heading": "^20.0.0",
     "@ckeditor/ckeditor5-paragraph": "^20.0.0",
     "@ckeditor/ckeditor5-typing": "^20.0.0",
-    "@ckeditor/ckeditor5-utils": "^20.0.0"
+    "@ckeditor/ckeditor5-utils": "^20.0.0",
+    "@ckeditor/ckeditor5-table": "^20.0.0"
   },
   "engines": {
     "node": ">=12.0.0",

packages/ckeditor5-undo/src/basecommand.js

@@ -93,15 +93,23 @@ export default class BaseCommand extends Command {
 		const selectionRanges = [];
 
 		// Transform all ranges from the restored selection.
-		for ( const range of ranges ) {
-			const transformed = transformSelectionRange( range, operations );
+		const transformedRangeGroups = ranges.map( range => range.getTransformedByOperations( operations ) );
+		const allRanges = transformedRangeGroups.flat();
+
+		for ( const rangeGroup of transformedRangeGroups ) {
+			// While transforming there could appear ranges that are contained by other ranges, we shall ignore them.
+			const transformed = rangeGroup.filter( range => !isRangeContainedByAnyOtherRange( range, allRanges ) );
+
+			// After the range got transformed, we have an array of ranges. Some of those
+			// ranges may be "touching" -- they can be next to each other and could be merged.
+			normalizeRanges( transformed );
 
 			// For each `range` from `ranges`, we take only one transformed range.
 			// This is because we want to prevent situation where single-range selection
 			// got transformed to multi-range selection. We will take the first range that
 			// is not in the graveyard.
 			const newRange = transformed.find(
-				range => range.start.root != document.graveyard
+				range => range.root != document.graveyard
 			);
 
 			// `transformedRange` might be `undefined` if transformed range ended up in graveyard.
@@ -110,6 +118,8 @@ export default class BaseCommand extends Command {
 			}
 		}
 
+		// @if CK_DEBUG_ENGINE // console.log( `Restored selection by undo: ${ selectionRanges.join( ', ' ) }` );
+
 		// `selectionRanges` may be empty if all ranges ended up in graveyard. If that is the case, do not restore selection.
 		if ( selectionRanges.length ) {
 			model.change( writer => {
@@ -167,28 +177,25 @@ export default class BaseCommand extends Command {
 	}
 }
 
-// Transforms given range `range` by given `operations`.
-// Returns an array containing one or more ranges, which are result of the transformation.
-function transformSelectionRange( range, operations ) {
-	const transformed = range.getTransformedByOperations( operations );
-
-	// After `range` got transformed, we have an array of ranges. Some of those
-	// ranges may be "touching" -- they can be next to each other and could be merged.
-	// First, we have to sort those ranges to assure that they are in order.
-	transformed.sort( ( a, b ) => a.start.isBefore( b.start ) ? -1 : 1 );
-
-	// Then, we check if two consecutive ranges are touching.
-	for ( let i = 1; i < transformed.length; i++ ) {
-		const a = transformed[ i - 1 ];
-		const b = transformed[ i ];
-
-		if ( a.end.isTouching( b.start ) ) {
-			// And join them together if they are.
-			a.end = b.end;
-			transformed.splice( i, 1 );
+// Normalizes list of ranges by joining intersecting or "touching" ranges.
+//
+// @param {Array.<module:engine/model/range~Range>} ranges
+//
+function normalizeRanges( ranges ) {
+	ranges.sort( ( a, b ) => a.start.isBefore( b.start ) ? -1 : 1 );
+
+	for ( let i = 1; i < ranges.length; i++ ) {
+		const previousRange = ranges[ i - 1 ];
+		const joinedRange = previousRange.getJoined( ranges[ i ], true );
+
+		if ( joinedRange ) {
+			// Replace the ranges on the list with the new joined range.
 			i--;
+			ranges.splice( i, 2, joinedRange );
 		}
 	}
+}
 
-	return transformed;
+function isRangeContainedByAnyOtherRange( range, ranges ) {
+	return ranges.some( otherRange => otherRange !== range && otherRange.containsRange( range, true ) );
 }