I/6122: Handling spanned table cells in pasting scenarios

packages/ckeditor5-table/src/tableclipboard.js

@@ -142,15 +142,19 @@ export default class TableClipboard extends Plugin {
 			// Content table to which we insert a pasted table.
 			const selectedTable = findAncestor( 'table', selectedTableCells[ 0 ] );
 
-			if ( selectedTableCells.length === 1 ) {
+			// Single cell selected - expand selection to pasted table dimensions.
+			const shouldExpandSelection = selectedTableCells.length === 1;
+
+			if ( shouldExpandSelection ) {
 				lastRowOfSelection += pasteHeight - 1;
 				lastColumnOfSelection += pasteWidth - 1;
 
 				expandTableSize( selectedTable, lastRowOfSelection + 1, lastColumnOfSelection + 1, writer, tableUtils );
 			}
 
-			// Beyond this point we will operate on fixed content table.
-			if ( selectedTableCells.length === 1 || !isSelectionRectangular( selectedTableCells, tableUtils ) ) {
+			// In case of expanding selection we do not reset the selection so in this case we will always try to fix selection
+			// like in the case of a non-rectangular area. This might be fixed by re-setting selected cells array but this shortcut is safe.
+			if ( shouldExpandSelection || !isSelectionRectangular( selectedTableCells, tableUtils ) ) {
 				const splitDimensions = {
 					firstRow: firstRowOfSelection,
 					lastRow: lastRowOfSelection,
@@ -173,6 +177,8 @@ export default class TableClipboard extends Plugin {
 				lastColumnOfSelection = adjustLastColumnIndex( selectedTable, rowIndexes, columnIndexes );
 			}
 
+			// Beyond this point we operate on a fixed content table with rectangular selection and proper last row/column values.
+
 			const selectionHeight = lastRowOfSelection - firstRowOfSelection + 1;
 			const selectionWidth = lastColumnOfSelection - firstColumnOfSelection + 1;
 
@@ -306,24 +312,24 @@ function replaceSelectedCellsWithPasted( pastedTable, selectedTable, selectionDi
 	writer.setSelection( cellsToSelect.map( cell => writer.createRangeOn( cell ) ) );
 }
 
-// Expand table (in place) to expected size (rows and columns).
-function expandTableSize( table, rows, columns, writer, tableUtils ) {
+// Expand table (in place) to expected size.
+function expandTableSize( table, expectedHeight, expectedWidth, writer, tableUtils ) {
 	const tableWidth = tableUtils.getColumns( table );
 	const tableHeight = tableUtils.getRows( table );
 
-	if ( columns > tableWidth ) {
+	if ( expectedWidth > tableWidth ) {
 		tableUtils.insertColumns( table, {
 			batch: writer.batch,
 			at: tableWidth,
-			columns: columns - tableWidth
+			columns: expectedWidth - tableWidth
 		} );
 	}
 
-	if ( rows > tableHeight ) {
+	if ( expectedHeight > tableHeight ) {
 		tableUtils.insertRows( table, {
 			batch: writer.batch,
 			at: tableHeight,
-			rows: rows - tableHeight
+			rows: expectedHeight - tableHeight
 		} );
 	}
 }
@@ -482,6 +488,19 @@ function isAffectedBySelection( index, span, limit ) {
 }
 
 // Returns adjusted last row index if selection covers part of a row with empty slots (spanned by other cells).
+// The rowIndexes.last is equal to last row index but selection might be bigger.
+//
+// This happens *only* on rectangular selection so we analyze a case like this:
+//
+//    +---+---+---+---+
+//  0 | a | b | c | d |
+//    +   +   +---+---+
+//  1 |   | e | f | g |
+//    +   +---+   +---+
+//  2 |   | h |   | i | <- last row, each cell has rowspan = 2,
+//    +   +   +   +   +    so we need to return 3, not 2
+//  3 |   |   |   |   |
+//    +---+---+---+---+
 function adjustLastRowIndex( table, rowIndexes, columnIndexes ) {
 	const tableIterator = new TableWalker( table, {
 		startRow: rowIndexes.last,
@@ -493,16 +512,50 @@ function adjustLastRowIndex( table, rowIndexes, columnIndexes ) {
 		return columnIndexes.first <= column && column <= columnIndexes.last;
 	} );
 
-	return rowIndexes.last + Math.max( 1, ...lastRowMap.map( ( { rowspan } ) => rowspan ) ) - 1;
+	const everyCellHasSingleRowspan = lastRowMap.every( ( { rowspan } ) => rowspan === 1 );
+
+	// It is a "flat" row, so the last row index is OK.
+	if ( everyCellHasSingleRowspan ) {
+		return rowIndexes.last;
+	}
+
+	// Otherwise get any cell's rowspan and adjust the last row index.
+	const rowspanAdjustment = lastRowMap.pop().rowspan - 1;
+	return rowIndexes.last + rowspanAdjustment;
 }
 
 // Returns adjusted last column index if selection covers part of a column with empty slots (spanned by other cells).
+// The columnIndexes.last is equal to last column index but selection might be bigger.
+//
+// This happens *only* on rectangular selection so we analyze a case like this:
+//
+//   0   1   2   3
+// +---+---+---+---+
+// | a             |
+// +---+---+---+---+
+// | b | c | d     |
+// +---+---+---+---+
+// | e     | f     |
+// +---+---+---+---+
+// | g | h         |
+// +---+---+---+---+
+//           ^
+//          last column, each cell has colspan = 2, so we need to return 3, not 2
 function adjustLastColumnIndex( table, rowIndexes, columnIndexes ) {
 	const lastColumnMap = Array.from( new TableWalker( table, {
 		startRow: rowIndexes.first,
 		endRow: rowIndexes.last,
 		column: columnIndexes.last
 	} ) );
 
-	return columnIndexes.last + Math.max( 1, ...lastColumnMap.map( ( { colspan } ) => colspan ) ) - 1;
+	const everyCellHasSingleColspan = lastColumnMap.every( ( { colspan } ) => colspan === 1 );
+
+	// It is a "flat" column, so the last column index is OK.
+	if ( everyCellHasSingleColspan ) {
+		return columnIndexes.last;
+	}
+
+	// Otherwise get any cell's colspan and adjust the last column index.
+	const colspanAdjustment = lastColumnMap.pop().colspan - 1;
+	return columnIndexes.last + colspanAdjustment;
 }

packages/ckeditor5-table/src/utils.js

@@ -248,7 +248,7 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) {
 }
 
 /**
- * Returns cells that starts above and overlaps a given row.
+ * Returns slot info of cells that starts above and overlaps a given row.
  *
  * In a table below, passing `overlapRow = 3`
  *
@@ -264,11 +264,11 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) {
  *    4  │ o │ p │   │   │ q │
  *       └───┴───┴───┴───┴───┘
  *
- * will return cells: "j", "f", "k".
+ * will return slot info for cells: "j", "f", "k".
  *
  * @param {module:engine/model/element~Element} table The table to check.
  * @param {Number} overlapRow The index of the row to check.
- * @param {Number} [startRow=0] A row to start analysis if it is known where.
+ * @param {Number} [startRow=0] A row to start analysis. Use it when it is known that cells below that row will not overlap.
  * @returns {Array.<module:table/tablewalker~TableWalkerValue>}
  */
 export function getVerticallyOverlappingCells( table, overlapRow, startRow = 0 ) {
@@ -278,8 +278,9 @@ export function getVerticallyOverlappingCells( table, overlapRow, startRow = 0 )
 
 	for ( const slotInfo of tableWalker ) {
 		const { row, rowspan } = slotInfo;
+		const slotEndRow = row + rowspan - 1;
 
-		if ( rowspan > 1 && row + rowspan > overlapRow ) {
+		if ( row < overlapRow && overlapRow <= slotEndRow ) {
 			cells.push( slotInfo );
 		}
 	}
@@ -339,7 +340,7 @@ export function splitHorizontally( tableCell, splitRow, writer ) {
 }
 
 /**
- * Returns cells that starts before and overlaps a given column.
+ * Returns slot info of cells that starts before and overlaps a given column.
  *
  * In a table below, passing `overlapColumn = 3`
  *
@@ -358,7 +359,7 @@ export function splitHorizontally( tableCell, splitRow, writer ) {
  *                  ^
  *                  Overlap column to check
  *
- * will return cells: "b", "e", "i".
+ * will return slot info for cells: "b", "e", "i".
  *
  * @param {module:engine/model/element~Element} table The table to check.
  * @param {Number} overlapColumn The index of the column to check.
@@ -371,9 +372,9 @@ export function getHorizontallyOverlappingCells( table, overlapColumn ) {
 
 	for ( const slotInfo of tableWalker ) {
 		const { column, colspan } = slotInfo;
-		const endColumn = column + colspan - 1;
+		const slotEndColumn = column + colspan - 1;
 
-		if ( column < overlapColumn && overlapColumn <= endColumn ) {
+		if ( column < overlapColumn && overlapColumn <= slotEndColumn ) {
 			cellsToSplit.push( slotInfo );
 		}
 	}

packages/ckeditor5-table/tests/commands/setheadercolumncommand.js

@@ -198,6 +198,18 @@ describe( 'SetHeaderColumnCommand', () => {
 			], { headingColumns: 1 } ) );
 		} );
 
+		it( 'should remove "headingColumns" attribute from table if no value was given', () => {
+			setData( model, modelTable( [
+				[ '[]00', '01', '02', '03' ]
+			], { headingColumns: 3 } ) );
+
+			command.execute();
+
+			assertEqualMarkup( getData( model ), modelTable( [
+				[ '[]00', '01', '02', '03' ]
+			] ) );
+		} );
+
 		describe( 'multi-cell selection', () => {
 			describe( 'setting header', () => {
 				it( 'should set it correctly in a middle of single-row, multiple cell selection', () => {
@@ -404,7 +416,7 @@ describe( 'SetHeaderColumnCommand', () => {
 			], { headingColumns: 2 } ) );
 		} );
 
-		it( 'should respect forceValue parameter #1', () => {
+		it( 'should respect forceValue parameter (forceValue=true)', () => {
 			setData( model, modelTable( [
 				[ '00', '01[]', '02', '03' ]
 			], { headingColumns: 3 } ) );
@@ -416,7 +428,7 @@ describe( 'SetHeaderColumnCommand', () => {
 			], { headingColumns: 3 } ) );
 		} );
 
-		it( 'should respect forceValue parameter #2', () => {
+		it( 'should respect forceValue parameter (forceValue=false)', () => {
 			setData( model, modelTable( [
 				[ '00', '01[]', '02', '03' ]
 			], { headingColumns: 1 } ) );

packages/ckeditor5-table/tests/commands/setheaderrowcommand.js

@@ -225,7 +225,7 @@ describe( 'SetHeaderRowCommand', () => {
 			], { headingRows: 1 } ) );
 		} );
 
-		it( 'should unsetset heading rows attribute', () => {
+		it( 'should remove "headingRows" attribute from table if no value was given', () => {
 			setData( model, modelTable( [
 				[ '[]00' ],
 				[ '10' ],
@@ -551,7 +551,7 @@ describe( 'SetHeaderRowCommand', () => {
 			} );
 		} );
 
-		it( 'should respect forceValue parameter #1', () => {
+		it( 'should respect forceValue parameter (forceValue=true)', () => {
 			setData( model, modelTable( [
 				[ '00' ],
 				[ '[]10' ],
@@ -569,7 +569,7 @@ describe( 'SetHeaderRowCommand', () => {
 			], { headingRows: 3 } ) );
 		} );
 
-		it( 'should respect forceValue parameter #2', () => {
+		it( 'should respect forceValue parameter (forceValue=false)', () => {
 			setData( model, modelTable( [
 				[ '00' ],
 				[ '[]10' ],

packages/ckeditor5-table/tests/tableclipboard-paste.js

@@ -765,6 +765,39 @@ describe( 'table clipboard', () => {
 						[ '', '', '', 'ea', 'eb', 'ec', 'ed' ]
 					] ) );
 				} );
+
+				it( 'should fix non-rectangular are on matched table fragment', () => {
+					// +----+----+----+
+					// | 00 | 01 | 02 |
+					// +----+----+    +
+					// | 10 | 11 |    |
+					// +----+----+----+
+					// | 20      | 22 |
+					// +----+----+----+
+					setModelData( model, modelTable( [
+						[ '00', '01', { contents: '02', rowspan: 2 } ],
+						[ '10', '11[]' ],
+						[ { contents: '20', colspan: 2 }, '22' ]
+					] ) );
+
+					pasteTable( [
+						[ 'aa', 'ab' ],
+						[ 'ba', 'bb' ]
+					] );
+
+					// +----+----+----+
+					// | 00 | 01 | 02 |
+					// +----+----+----+
+					// | 10 | aa | ab |
+					// +----+----+----+
+					// | 20 | ba | bb |
+					// +----+----+----+
+					assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [
+						[ '00', '01', '02' ],
+						[ '10', 'aa', 'ab' ],
+						[ '20', 'ba', 'bb' ]
+					] ) );
+				} );
 			} );
 		} );