浏览代码

Restoring multi-cell selection on undo.

Kuba Niegowski 5 年之前
父节点
当前提交
f1845bcced

+ 38 - 0
packages/ckeditor5-engine/src/model/range.js

@@ -277,6 +277,44 @@ export default class Range {
 	}
 
 	/**
+	 * TODO
+	 *
+	 * @param {module:engine/model/range~Range} otherRange Range to sum with.
+	 * @param {Boolean} [loose=false] Whether the sum intersection check is loose or strict. If the check is strict (`false`),
+	 * summed range is tested for intersection only. If the check is loose (`true`), compared range is also checked if it's "touching"
+	 * current range.
+	 * @returns {module:engine/model/range~Range|null} A sum of given ranges or `null` if ranges have no common part.
+	 */
+	getSum( otherRange, loose = false ) {
+		let shouldSum = this.isIntersecting( otherRange );
+
+		if ( loose && !shouldSum ) {
+			if ( this.start.isBefore( otherRange.start ) ) {
+				shouldSum = this.end.isTouching( otherRange.start );
+			} else {
+				shouldSum = otherRange.end.isTouching( this.start );
+			}
+		}
+
+		if ( !shouldSum ) {
+			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.
 	 *
 	 * See an example of a model structure (`[` and `]` are range boundaries):

+ 25 - 10
packages/ckeditor5-table/src/commands/mergecellscommand.js

@@ -46,21 +46,19 @@ export default class MergeCellsCommand extends Command {
 			const selectedTableCells = getSelectedTableCells( model.document.selection );
 
 			// 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 );
+			const firstTableCell = selectedTableCells[ 0 ];
 
 			// Update target cell dimensions.
-			const { mergeWidth, mergeHeight } = getMergeDimensions( firstTableCell, selectedTableCells, tableUtils );
+			const { mergeWidth, mergeHeight } = getMergeDimensions( selectedTableCells, tableUtils );
 			updateNumericAttribute( 'colspan', mergeWidth, firstTableCell, writer );
 			updateNumericAttribute( 'rowspan', mergeHeight, firstTableCell, writer );
 
-			for ( const tableCell of selectedTableCells ) {
-				mergeTableCells( tableCell, firstTableCell, writer );
+			for ( let i = 1; i < selectedTableCells.length; i++ ) {
+				mergeTableCells( selectedTableCells[ i ], firstTableCell, writer );
 			}
+			// for ( let i = selectedTableCells.length - 1; i > 0; i-- ) {
+			// 	mergeTableCells( selectedTableCells[ i ], selectedTableCells[ i - 1 ], writer );
+			// }
 
 			const table = findAncestor( 'table', firstTableCell );
 
@@ -90,6 +88,23 @@ function mergeTableCells( cellBeingMerged, targetCell, writer ) {
 
 	// Remove merged table cell.
 	writer.remove( cellBeingMerged );
+
+	// 	const positionAfterTargetCell = writer.createPositionAfter( targetCell );
+	// 	const positionBeforeMergedCell = writer.createPositionBefore( cellBeingMerged );
+	//
+	// 	const targetCellWasEmpty = !writer.model.hasContent( targetCell );
+	// 	const mergedCellWasEmpty = !writer.model.hasContent( cellBeingMerged );
+	// 	const positionAtEndOfTargetContent = writer.createPositionAt( targetCell, 'end' );
+	//
+	// 	if ( !positionAfterTargetCell.isEqual( positionBeforeMergedCell ) ) {
+	// 		writer.move( writer.createRangeOn( cellBeingMerged ), positionAfterTargetCell );
+	// 	}
+	//
+	// 	writer.merge( positionAfterTargetCell );
+	//
+	// 	if ( targetCellWasEmpty || mergedCellWasEmpty ) {
+	// 		writer.merge( positionAtEndOfTargetContent );
+	// 	}
 }
 
 // Checks if the passed table cell contains an empty paragraph.
@@ -100,7 +115,7 @@ function isEmpty( tableCell ) {
 	return tableCell.childCount == 1 && tableCell.getChild( 0 ).is( 'paragraph' ) && tableCell.getChild( 0 ).isEmpty;
 }
 
-function getMergeDimensions( firstTableCell, selectedTableCells, tableUtils ) {
+function getMergeDimensions( [ firstTableCell, ...selectedTableCells ], tableUtils ) {
 	let maxWidthOffset = 0;
 	let maxHeightOffset = 0;
 

+ 8 - 4
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 ]
+			] );
 		} );
 	} );
 

+ 21 - 40
packages/ckeditor5-undo/src/basecommand.js

@@ -89,26 +89,16 @@ export default class BaseCommand extends Command {
 		const model = this.editor.model;
 		const document = model.document;
 
-		// This will keep the transformed selection ranges.
-		const selectionRanges = [];
-
 		// Transform all ranges from the restored selection.
-		for ( const range of ranges ) {
-			const transformed = transformSelectionRange( range, operations );
-
-			// 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
-			);
+		const selectionRanges = ranges
+			.flatMap( range => range.getTransformedByOperations( operations ) )
+			.filter( range => range.root != document.graveyard )
+			.sort( ( a, b ) => a.start.isBefore( b.start ) ? -1 : 1 );
 
-			// `transformedRange` might be `undefined` if transformed range ended up in graveyard.
-			if ( newRange ) {
-				selectionRanges.push( newRange );
-			}
-		}
+		normalizeRanges( selectionRanges );
+		normalizeRanges( selectionRanges, [ 'tableCell' ] );
+
+		// @if CK_DEBUG_ENGINE // console.log( `Restored selection from 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 ) {
@@ -167,28 +157,19 @@ 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 );
-			i--;
+function normalizeRanges( ranges, dontSumElements = null ) {
+	for ( let i = 1; i < ranges.length; i++ ) {
+		const previousRange = ranges[ i - 1 ];
+		const containedElement = dontSumElements && previousRange.getContainedElement();
+
+		if ( containedElement && dontSumElements.includes( containedElement.name ) ) {
+			continue;
 		}
-	}
 
-	return transformed;
+		const summedRange = previousRange.getSum( ranges[ i ], !!dontSumElements );
+
+		if ( summedRange ) {
+			ranges.splice( --i, 2, summedRange );
+		}
+	}
 }

+ 49 - 0
packages/ckeditor5-undo/tests/undoediting-integration.js

@@ -428,6 +428,55 @@ describe( 'UndoEditing integration', () => {
 
 			undoDisabled();
 		} );
+
+		it.skip( 'move, merge and undo', () => {
+			// Add a "div feature".
+			model.schema.register( 'div', {
+				allowWhere: '$block',
+				allowContentOf: '$root'
+			} );
+
+			editor.conversion.for( 'downcast' ).elementToElement( { model: 'div', view: 'div' } );
+			editor.conversion.for( 'upcast' ).elementToElement( { model: 'div', view: 'div' } );
+
+			input(
+				'<div><paragraph>[]00.</paragraph><paragraph>01.</paragraph><paragraph>02.</paragraph></div>' +
+				'<div><paragraph>10.</paragraph><paragraph>11.</paragraph><paragraph>12.</paragraph></div>'
+			);
+
+			const elements = [
+				root.getNodeByPath( [ 0, 1 ] ),
+				root.getNodeByPath( [ 1, 0 ] ),
+				root.getNodeByPath( [ 1, 1 ] )
+			];
+
+			model.change( writer => {
+				for ( let i = elements.length - 1; i > 0; i-- ) {
+					merge( elements[ i ], elements[ i - 1 ], writer );
+				}
+			} );
+			output(
+				'<div><paragraph>[]00.</paragraph><paragraph>01.10.11.</paragraph><paragraph>02.</paragraph></div>' +
+				'<div><paragraph>12.</paragraph></div>'
+			);
+
+			editor.execute( 'undo' );
+			output(
+				'<div><paragraph>[]00.</paragraph><paragraph>01.</paragraph><paragraph>02.</paragraph></div>' +
+				'<div><paragraph>10.</paragraph><paragraph>11.</paragraph><paragraph>12.</paragraph></div>'
+			);
+
+			function merge( element, targetElement, writer ) {
+				const positionAfterTarget = writer.createPositionAfter( targetElement );
+				const positionBeforeElement = writer.createPositionBefore( element );
+
+				if ( !positionAfterTarget.isEqual( positionBeforeElement ) ) {
+					writer.move( writer.createRangeOn( element ), positionAfterTarget );
+				}
+
+				writer.merge( positionAfterTarget );
+			}
+		} );
 	} );
 
 	// Restoring selection in those examples may be completely off.