I/6502: Refactor TableUtils.removeRows() logic.

src/tableutils.js

@@ -290,32 +290,8 @@ export default class TableUtils extends Plugin {
 
 		// Removing rows from table requires most calculations to be done prior to changing table structure.
 
-		// 1. Preparation
-
-		// 1a. Cells from removed rows section might stick out of. Such cells are moved to a next row after a removed section.
-		const cellsToMove = new Map();
-		// 1b. Similarly, if a cell is "above" removed rows sections we must trim their rowspan.
-		const cellsToTrim = [];
-
-		for ( const { row, column, rowspan, cell } of new TableWalker( table, { endRow: last } ) ) {
-			const rowspanInRemovedSection = last - row + 1;
-			const lastRowOfCell = row + rowspan - 1;
-
-			const isCellStickingOutFromRemovedRows = row >= first && row <= last && lastRowOfCell > last;
-
-			if ( isCellStickingOutFromRemovedRows ) {
-				const rowSpanToSet = rowspan - rowspanInRemovedSection;
-				cellsToMove.set( column, { cell, rowspan: rowSpanToSet } );
-			}
-
-			const isCellOverlappingRemovedRows = row < first && lastRowOfCell >= first;
-
-			if ( isCellOverlappingRemovedRows ) {
-				const rowspanAdjustment = lastRowOfCell >= last ? rowsToRemove : first - row;
-				const rowSpanToSet = rowspan - rowspanAdjustment;
-				cellsToTrim.push( { cell, rowspan: rowSpanToSet } );
-			}
-		}
+		// 1. Preparation - get row-spanned cells that have to be modified after removing rows.
+		const { cellsToMove, cellsToTrim } = getCellsToMoveAndTrimOnRemoveRow( table, last, first, rowsToRemove );
 
 		// 2. Execution
 		model.change( writer => {
@@ -789,6 +765,69 @@ function updateHeadingRows( table, first, last, model, batch ) {
 	}
 }
 
+// Finds cells that will be:
+// - trimmed - Cells that are "above" removed rows sections and overlaps removed section - their rowspan must be trimmed.
+// - moved - Cells from removed rows section might stick out of. Such cells are moved to a next row after a removed section.
+//
+// Sample table with overlapping & sticking out cells:
+//
+//      +----+----+----+----+----+
+//      | 00 | 01 | 02 | 03 | 04 |
+//      +----+    +    +    +    +
+//      | 10 |    |    |    |    |
+//      +----+----+    +    +    +
+//      | 20 | 21 |    |    |    | <-- removed row
+//      +    +    +----+    +    +
+//      |    |    | 32 |    |    | <-- removed row
+//      +----+    +    +----+    +
+//      | 40 |    |    | 43 |    |
+//      +----+----+----+----+----+
+//
+// In a table above:
+// - cells to trim: '02', '03' & '04'.
+// - cells to move: '21' & '32'.
+function getCellsToMoveAndTrimOnRemoveRow( table, last, first, rowsToRemove ) {
+	const cellsToMove = new Map();
+	const cellsToTrim = [];
+
+	for ( const { row, column, rowspan, cell } of new TableWalker( table, { endRow: last } ) ) {
+		const lastRowOfCell = row + rowspan - 1;
+
+		const isCellStickingOutFromRemovedRows = row >= first && row <= last && lastRowOfCell > last;
+
+		if ( isCellStickingOutFromRemovedRows ) {
+			const rowspanInRemovedSection = last - row + 1;
+			const rowSpanToSet = rowspan - rowspanInRemovedSection;
+
+			cellsToMove.set( column, {
+				cell,
+				rowspan: rowSpanToSet
+			} );
+		}
+
+		const isCellOverlappingRemovedRows = row < first && lastRowOfCell >= first;
+
+		if ( isCellOverlappingRemovedRows ) {
+			let rowspanAdjustment;
+
+			// Cell fully covers removed section - trim it by removed rows count.
+			if ( lastRowOfCell >= last ) {
+				rowspanAdjustment = rowsToRemove;
+			}
+			// Cell partially overlaps removed section - calculate cell's span that is in removed section.
+			else {
+				rowspanAdjustment = lastRowOfCell - first + 1;
+			}
+
+			cellsToTrim.push( {
+				cell,
+				rowspan: rowspan - rowspanAdjustment
+			} );
+		}
+	}
+	return { cellsToMove, cellsToTrim };
+}
+
 function moveCellsToRow( table, targetRowIndex, cellsToMove, writer ) {
 	const tableWalker = new TableWalker( table, {
 		includeSpanned: true,

tests/tableutils.js

@@ -414,7 +414,7 @@ describe( 'TableUtils', () => {
 			], { headingColumns: 4 } ) );
 		} );
 
-		it( 'should properly insert column while table has rowspanned cells', () => {
+		it( 'should properly insert column while table has row-spanned cells', () => {
 			setData( model, modelTable( [
 				[ { rowspan: 4, contents: '00[]' }, { rowspan: 2, contents: '01' }, '02' ],
 				[ '12' ],
@@ -642,7 +642,7 @@ describe( 'TableUtils', () => {
 			] ) );
 		} );
 
-		it( 'should properly update rowspanned cells overlapping selected cell', () => {
+		it( 'should properly update row-spanned cells overlapping selected cell', () => {
 			setData( model, modelTable( [
 				[ { rowspan: 2, contents: '00' }, '01', { rowspan: 3, contents: '02' } ],
 				[ '[]11' ],
@@ -660,7 +660,7 @@ describe( 'TableUtils', () => {
 			] ) );
 		} );
 
-		it( 'should split rowspanned cell', () => {
+		it( 'should split row-spanned cell', () => {
 			setData( model, modelTable( [
 				[ '00', { rowspan: 2, contents: '01[]' } ],
 				[ '10' ],
@@ -678,7 +678,7 @@ describe( 'TableUtils', () => {
 			] ) );
 		} );
 
-		it( 'should copy colspan while splitting rowspanned cell', () => {
+		it( 'should copy colspan while splitting row-spanned cell', () => {
 			setData( model, modelTable( [
 				[ '00', { rowspan: 2, colspan: 2, contents: '01[]' } ],
 				[ '10' ],
@@ -724,7 +724,7 @@ describe( 'TableUtils', () => {
 			] ) );
 		} );
 
-		it( 'should split rowspanned cell and updated other cells rowspan when splitting to bigger number of cells', () => {
+		it( 'should split row-spanned cell and updated other cells rowspan when splitting to bigger number of cells', () => {
 			setData( model, modelTable( [
 				[ '00', { rowspan: 2, contents: '01[]' } ],
 				[ '10' ],
@@ -743,7 +743,7 @@ describe( 'TableUtils', () => {
 			] ) );
 		} );
 
-		it( 'should split rowspanned & colspaned cell', () => {
+		it( 'should split row-spanned & col-spanned cell', () => {
 			setData( model, modelTable( [
 				[ '00', { colspan: 2, contents: '01[]' } ],
 				[ '10', '11' ]
@@ -905,6 +905,52 @@ describe( 'TableUtils', () => {
 			} );
 
 			it( 'should decrease rowspan of table cells from previous rows', () => {
+				// +----+----+----+----+----+
+				// | 00 | 01 | 02 | 03 | 04 |
+				// +----+    +    +    +    +
+				// | 10 |    |    |    |    |
+				// +----+----+    +    +    +
+				// | 20 | 21 |    |    |    |
+				// +----+----+----+    +    +
+				// | 30 | 31 | 32 |    |    |
+				// +----+----+----+----+    +
+				// | 40 | 41 | 42 | 43 |    |
+				// +----+----+----+----+----+
+				// | 50 | 51 | 52 | 53 | 54 |
+				// +----+----+----+----+----+
+				setData( model, modelTable( [
+					[ '00', { contents: '01', rowspan: 2 }, { contents: '02', rowspan: 3 }, { contents: '03', rowspan: 4 },
+						{ contents: '04', rowspan: 5 } ],
+					[ '10' ],
+					[ '20', '21' ],
+					[ '30', '31', '32' ],
+					[ '40', '41', '42', '43' ],
+					[ '50', '51', '52', '53', '54' ]
+				] ) );
+
+				tableUtils.removeRows( root.getChild( 0 ), { at: 1, rows: 1 } );
+
+				// +----+----+----+----+----+
+				// | 00 | 01 | 02 | 03 | 04 |
+				// +----+----+    +    +    +
+				// | 20 | 21 |    |    |    |
+				// +----+----+----+    +    +
+				// | 30 | 31 | 32 |    |    |
+				// +----+----+----+----+    +
+				// | 40 | 41 | 42 | 43 |    |
+				// +----+----+----+----+----+
+				// | 50 | 51 | 52 | 53 | 54 |
+				// +----+----+----+----+----+
+				assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
+					[ '00', '01', { contents: '02', rowspan: 2 }, { contents: '03', rowspan: 3 }, { contents: '04', rowspan: 4 } ],
+					[ '20', '21' ],
+					[ '30', '31', '32' ],
+					[ '40', '41', '42', '43' ],
+					[ '50', '51', '52', '53', '54' ]
+				] ) );
+			} );
+
+			it( 'should decrease rowspan of table cells from previous rows (row-spanned cells on different rows)', () => {
 				setData( model, modelTable( [
 					[ { rowspan: 4, contents: '00' }, { rowspan: 3, contents: '01' }, { rowspan: 2, contents: '02' }, '03', '04' ],
 					[ { rowspan: 2, contents: '13' }, '14' ],
@@ -921,7 +967,7 @@ describe( 'TableUtils', () => {
 				] ) );
 			} );
 
-			it( 'should move rowspaned cells to a row below removing it\'s row', () => {
+			it( 'should move row-spanned cells to a row below removing it\'s row', () => {
 				setData( model, modelTable( [
 					[ { rowspan: 3, contents: '00' }, { rowspan: 2, contents: '01' }, '02' ],
 					[ '12' ],
@@ -938,7 +984,7 @@ describe( 'TableUtils', () => {
 				] ) );
 			} );
 
-			it( 'should move rowspaned cells to a row below removing it\'s row (other cell is overlapping removed row)', () => {
+			it( 'should move row-spanned cells to a row below removing it\'s row (other cell is overlapping removed row)', () => {
 				setData( model, modelTable( [
 					[ '00', { rowspan: 3, contents: '01' }, '02', '03', '04' ],
 					[ '10', { rowspan: 2, contents: '12' }, '13', '14' ],
@@ -1050,7 +1096,7 @@ describe( 'TableUtils', () => {
 				] ) );
 			} );
 
-			it( 'should move row-spaned cells to a row after removed rows section', () => {
+			it( 'should move row-spanned cells to a row after removed rows section', () => {
 				setData( model, modelTable( [
 					[ '00', '01', '02', '03' ],
 					[ { rowspan: 4, contents: '10' }, { rowspan: 3, contents: '11' }, { rowspan: 2, contents: '12' }, '13' ],
@@ -1084,6 +1130,48 @@ describe( 'TableUtils', () => {
 				] ) );
 			} );
 
+			it( 'should decrease rowspan of table cells from previous rows', () => {
+				// +----+----+----+----+----+
+				// | 00 | 01 | 02 | 03 | 04 |
+				// +----+    +    +    +    +
+				// | 10 |    |    |    |    |
+				// +----+----+    +    +    +
+				// | 20 | 21 |    |    |    |
+				// +----+----+----+    +    +
+				// | 30 | 31 | 32 |    |    |
+				// +----+----+----+----+    +
+				// | 40 | 41 | 42 | 43 |    |
+				// +----+----+----+----+----+
+				// | 50 | 51 | 52 | 53 | 44 |
+				// +----+----+----+----+----+
+				setData( model, modelTable( [
+					[ '00', { contents: '01', rowspan: 2 }, { contents: '02', rowspan: 3 }, { contents: '03', rowspan: 4 },
+						{ contents: '04', rowspan: 5 } ],
+					[ '10' ],
+					[ '20', '21' ],
+					[ '30', '31', '32' ],
+					[ '40', '41', '42', '43' ],
+					[ '50', '51', '52', '53', '54' ]
+				] ) );
+
+				tableUtils.removeRows( root.getChild( 0 ), { at: 2, rows: 2 } );
+
+				// +----+----+----+----+----+
+				// | 00 | 01 | 02 | 03 | 04 |
+				// +----+    +    +    +    +
+				// | 10 |    |    |    |    |
+				// +----+----+----+----+    +
+				// | 40 | 41 | 42 | 43 |    |
+				// +----+----+----+----+----+
+				assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
+					[ '00', { contents: '01', rowspan: 2 }, { contents: '02', rowspan: 2 }, { contents: '03', rowspan: 2 },
+						{ contents: '04', rowspan: 3 } ],
+					[ '10' ],
+					[ '40', '41', '42', '43' ],
+					[ '50', '51', '52', '53', '54' ]
+				] ) );
+			} );
+
 			it( 'should create one undo step (1 batch)', () => {
 				setData( model, modelTable( [
 					[ '00', '01' ],
@@ -1236,7 +1324,7 @@ describe( 'TableUtils', () => {
 				] ) );
 			} );
 
-			it( 'should remove column if other column is rowspanned (last column)', () => {
+			it( 'should remove column if other column is row-spanned (last column)', () => {
 				setData( model, modelTable( [
 					[ '00', { rowspan: 2, contents: '01' } ],
 					[ '10' ]
@@ -1249,7 +1337,7 @@ describe( 'TableUtils', () => {
 				] ) );
 			} );
 
-			it( 'should remove column if other column is rowspanned (first column)', () => {
+			it( 'should remove column if other column is row-spanned (first column)', () => {
 				setData( model, modelTable( [
 					[ { rowspan: 2, contents: '00' }, '01' ],
 					[ '11' ]