Introduced support for removing multiple rows / columns using multi-cell selection

src/commands/removecolumncommand.js

@@ -11,7 +11,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command';
 
 import TableWalker from '../tablewalker';
 import { updateNumericAttribute } from './utils';
-import { getTableCellsContainingSelection } from '../utils';
+import { getSelectionAffectedTableCells } from '../utils';
 
 /**
  * The remove column command.
@@ -29,66 +29,102 @@ export default class RemoveColumnCommand extends Command {
 	 * @inheritDoc
 	 */
 	refresh() {
-		const editor = this.editor;
-		const selection = editor.model.document.selection;
-		const tableUtils = editor.plugins.get( 'TableUtils' );
-		const tableCell = getTableCellsContainingSelection( selection )[ 0 ];
-
-		this.isEnabled = !!tableCell && tableUtils.getColumns( tableCell.parent.parent ) > 1;
+		const selectedCells = getSelectionAffectedTableCells( this.editor.model.document.selection );
+		const firstCell = selectedCells[ 0 ];
+
+		if ( firstCell ) {
+			const table = firstCell.parent.parent;
+			const tableColumnCount = this.editor.plugins.get( 'TableUtils' ).getColumns( table );
+
+			const tableMap = [ ...new TableWalker( table ) ];
+			const columnIndexes = tableMap.filter( entry => selectedCells.includes( entry.cell ) ).map( el => el.column ).sort();
+			const minColumnIndex = columnIndexes[ 0 ];
+			const maxColumnIndex = columnIndexes[ columnIndexes.length - 1 ];
+
+			this.isEnabled = maxColumnIndex - minColumnIndex < ( tableColumnCount - 1 );
+		} else {
+			this.isEnabled = false;
+		}
 	}
 
 	/**
 	 * @inheritDoc
 	 */
 	execute() {
-		const model = this.editor.model;
-		const selection = model.document.selection;
-
-		const tableCell = getTableCellsContainingSelection( selection )[ 0 ];
-		const tableRow = tableCell.parent;
-		const table = tableRow.parent;
-
-		const headingColumns = table.getAttribute( 'headingColumns' ) || 0;
+		const [ firstCell, lastCell ] = getBoundaryCells( this.editor.model.document.selection );
+		const table = firstCell.parent.parent;
 
 		// Cache the table before removing or updating colspans.
 		const tableMap = [ ...new TableWalker( table ) ];
 
-		// Get column index of removed column.
-		const cellData = tableMap.find( value => value.cell === tableCell );
-		const removedColumn = cellData.column;
-		const selectionRow = cellData.row;
-		const cellToFocus = getCellToFocus( tableCell );
-
-		model.change( writer => {
-			// Update heading columns attribute if removing a row from head section.
-			if ( headingColumns && selectionRow <= headingColumns ) {
-				writer.setAttribute( 'headingColumns', headingColumns - 1, table );
-			}
-
-			for ( const { cell, column, colspan } of tableMap ) {
-				// If colspaned cell overlaps removed column decrease it's span.
-				if ( column <= removedColumn && colspan > 1 && column + colspan > removedColumn ) {
-					updateNumericAttribute( 'colspan', colspan - 1, cell, writer );
-				} else if ( column === removedColumn ) {
-					// The cell in removed column has colspan of 1.
-					writer.remove( cell );
+		// Store column indexes of removed columns.
+		const removedColumnIndexes = {
+			first: tableMap.find( value => value.cell === firstCell ).column,
+			last: tableMap.find( value => value.cell === lastCell ).column
+		};
+
+		const cellsToFocus = getCellToFocus( firstCell, lastCell );
+
+		this.editor.model.change( writer => {
+			// A temporary workaround to avoid the "model-selection-range-intersects" error.
+			writer.setSelection( writer.createRangeOn( table ) );
+
+			adjustHeadingColumns( table, removedColumnIndexes, writer );
+
+			for (
+				let removedColumnIndex = removedColumnIndexes.last;
+				removedColumnIndex >= removedColumnIndexes.first;
+				removedColumnIndex--
+			) {
+				for ( const { cell, column, colspan } of tableMap ) {
+					// If colspaned cell overlaps removed column decrease its span.
+					if ( column <= removedColumnIndex && colspan > 1 && column + colspan > removedColumnIndex ) {
+						updateNumericAttribute( 'colspan', colspan - 1, cell, writer );
+					} else if ( column === removedColumnIndex ) {
+						// The cell in removed column has colspan of 1.
+						writer.remove( cell );
+					}
 				}
 			}
 
-			writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );
+			writer.setSelection( writer.createPositionAt( cellsToFocus.reverse().filter( item => item != null )[ 0 ], 0 ) );
 		} );
 	}
 }
 
+// Updates heading columns attribute if removing a row from head section.
+function adjustHeadingColumns( table, removedColumnIndexes, writer ) {
+	const headingColumns = table.getAttribute( 'headingColumns' ) || 0;
+
+	if ( headingColumns && removedColumnIndexes.first <= headingColumns ) {
+		const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) -
+			removedColumnIndexes.first + 1;
+
+		writer.setAttribute( 'headingColumns', headingColumns - headingsRemoved, table );
+	}
+}
+
 // Returns a proper table cell to focus after removing a column. It should be a next sibling to selection visually stay in place but:
 // - selection is on last table cell it will return previous cell.
 // - table cell is spanned over 2+ columns - it will be truncated so the selection should stay in that cell.
-function getCellToFocus( tableCell ) {
-	const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 );
+function getCellToFocus( firstCell, lastCell ) {
+	const colspan = parseInt( lastCell.getAttribute( 'colspan' ) || 1 );
 
 	if ( colspan > 1 ) {
-		return tableCell;
+		return [ firstCell, lastCell ];
 	}
 
-	return tableCell.nextSibling ? tableCell.nextSibling : tableCell.previousSibling;
+	// return lastCell.nextSibling ? lastCell.nextSibling : lastCell.previousSibling;
+	return [ firstCell.previousSibling, lastCell.nextSibling ];
+}
+
+// Returns helper object returning the first and the last cell contained in given selection, based on DOM order.
+function getBoundaryCells( selection ) {
+	const referenceCells = getSelectionAffectedTableCells( selection );
+	const firstCell = referenceCells[ 0 ];
+	const lastCell = referenceCells.pop();
+
+	const returnValue = [ firstCell, lastCell ];
+
+	return firstCell.isBefore( lastCell ) ? returnValue : returnValue.reverse();
 }

src/commands/removerowcommand.js

@@ -11,7 +11,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command';
 
 import TableWalker from '../tablewalker';
 import { updateNumericAttribute } from './utils';
-import { getTableCellsContainingSelection } from '../utils';
+import { getSelectionAffectedTableCells } from '../utils';
 
 /**
  * The remove row command.
@@ -29,80 +29,122 @@ export default class RemoveRowCommand extends Command {
 	 * @inheritDoc
 	 */
 	refresh() {
-		const model = this.editor.model;
-		const doc = model.document;
-		const tableCell = getTableCellsContainingSelection( doc.selection )[ 0 ];
+		const selectedCells = getSelectionAffectedTableCells( this.editor.model.document.selection );
+		const firstCell = selectedCells[ 0 ];
 
-		this.isEnabled = !!tableCell && tableCell.parent.parent.childCount > 1;
+		if ( firstCell ) {
+			const table = firstCell.parent.parent;
+			const tableRowCount = this.editor.plugins.get( 'TableUtils' ).getRows( table );
+
+			const tableMap = [ ...new TableWalker( table ) ];
+			const rowIndexes = tableMap.filter( entry => selectedCells.includes( entry.cell ) ).map( el => el.row );
+			const minRowIndex = rowIndexes[ 0 ];
+			const maxRowIndex = rowIndexes[ rowIndexes.length - 1 ];
+
+			this.isEnabled = maxRowIndex - minRowIndex < ( tableRowCount - 1 );
+		} else {
+			this.isEnabled = false;
+		}
 	}
 
 	/**
 	 * @inheritDoc
 	 */
 	execute() {
-		const model = this.editor.model;
-		const selection = model.document.selection;
-		const tableCell = getTableCellsContainingSelection( selection )[ 0 ];
-		const tableRow = tableCell.parent;
-		const table = tableRow.parent;
+		const referenceCells = getSelectionAffectedTableCells( this.editor.model.document.selection );
+		const removedRowIndexes = getRowIndexes( referenceCells );
+
+		const firstCell = referenceCells[ 0 ];
+		const table = firstCell.parent.parent;
+		const tableMap = [ ...new TableWalker( table, { endRow: removedRowIndexes.last } ) ];
+		const batch = this.editor.model.createBatch();
+		const columnIndexToFocus = getColumnIndexToFocus( tableMap, firstCell );
+
+		// Doing multiple model.enqueueChange() calls, to get around ckeditor/ckeditor5#6391.
+		// Ideally we want to do this in a single model.change() block.
+		this.editor.model.enqueueChange( batch, writer => {
+			// This prevents the "model-selection-range-intersects" error, caused by removing row selected cells.
+			writer.setSelection( writer.createSelection( table, 'on' ) );
+		} );
 
-		const removedRowIndex = table.getChildIndex( tableRow );
+		let cellToFocus;
 
-		const tableMap = [ ...new TableWalker( table, { endRow: removedRowIndex } ) ];
+		for ( let i = removedRowIndexes.last; i >= removedRowIndexes.first; i-- ) {
+			this.editor.model.enqueueChange( batch, writer => {
+				const removedRowIndex = i;
+				this._removeRow( removedRowIndex, table, writer, tableMap );
 
-		const cellData = tableMap.find( value => value.cell === tableCell );
+				cellToFocus = getCellToFocus( table, removedRowIndex, columnIndexToFocus );
+			} );
+		}
 
+		this.editor.model.enqueueChange( batch, writer => {
+			writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );
+		} );
+	}
+
+	/**
+	 * Removes a row from the given `table`.
+	 *
+	 * @private
+	 * @param {Number} removedRowIndex Index of the row that should be removed.
+	 * @param {module:engine/model/element~Element} table
+	 * @param {module:engine/model/writer~Writer} writer
+	 * @param {module:engine/model/element~Element[]} tableMap Table map retrieved from {@link module:table/tablewalker~TableWalker}.
+	 */
+	_removeRow( removedRowIndex, table, writer, tableMap ) {
+		const cellsToMove = new Map();
+		const tableRow = table.getChild( removedRowIndex );
 		const headingRows = table.getAttribute( 'headingRows' ) || 0;
 
-		const columnToFocus = cellData.column;
+		if ( headingRows && removedRowIndex < headingRows ) {
+			updateNumericAttribute( 'headingRows', headingRows - 1, table, writer, 0 );
+		}
 
-		model.change( writer => {
-			if ( headingRows && removedRowIndex <= headingRows ) {
-				updateNumericAttribute( 'headingRows', headingRows - 1, table, writer, 0 );
+		// Get cells from removed row that are spanned over multiple rows.
+		tableMap
+			.filter( ( { row, rowspan } ) => row === removedRowIndex && rowspan > 1 )
+			.forEach( ( { column, cell, rowspan } ) => cellsToMove.set( column, { cell, rowspanToSet: rowspan - 1 } ) );
+
+		// Reduce rowspan on cells that are above removed row and overlaps removed row.
+		tableMap
+			.filter( ( { row, rowspan } ) => row <= removedRowIndex - 1 && row + rowspan > removedRowIndex )
+			.forEach( ( { cell, rowspan } ) => updateNumericAttribute( 'rowspan', rowspan - 1, cell, writer ) );
+
+		// Move cells to another row.
+		const targetRow = removedRowIndex + 1;
+		const tableWalker = new TableWalker( table, { includeSpanned: true, startRow: targetRow, endRow: targetRow } );
+		let previousCell;
+
+		for ( const { row, column, cell } of [ ...tableWalker ] ) {
+			if ( cellsToMove.has( column ) ) {
+				const { cell: cellToMove, rowspanToSet } = cellsToMove.get( column );
+				const targetPosition = previousCell ?
+					writer.createPositionAfter( previousCell ) :
+					writer.createPositionAt( table.getChild( row ), 0 );
+				writer.move( writer.createRangeOn( cellToMove ), targetPosition );
+				updateNumericAttribute( 'rowspan', rowspanToSet, cellToMove, writer );
+				previousCell = cellToMove;
 			}
-
-			const cellsToMove = new Map();
-
-			// Get cells from removed row that are spanned over multiple rows.
-			tableMap
-				.filter( ( { row, rowspan } ) => row === removedRowIndex && rowspan > 1 )
-				.forEach( ( { column, cell, rowspan } ) => cellsToMove.set( column, { cell, rowspanToSet: rowspan - 1 } ) );
-
-			// Reduce rowspan on cells that are above removed row and overlaps removed row.
-			tableMap
-				.filter( ( { row, rowspan } ) => row <= removedRowIndex - 1 && row + rowspan > removedRowIndex )
-				.forEach( ( { cell, rowspan } ) => updateNumericAttribute( 'rowspan', rowspan - 1, cell, writer ) );
-
-			// Move cells to another row.
-			const targetRowIndex = removedRowIndex + 1;
-			const tableWalker = new TableWalker( table, { includeSpanned: true, startRow: targetRowIndex, endRow: targetRowIndex } );
-
-			let previousCell;
-
-			for ( const { row, column, cell } of [ ...tableWalker ] ) {
-				if ( cellsToMove.has( column ) ) {
-					const { cell: cellToMove, rowspanToSet } = cellsToMove.get( column );
-					const targetPosition = previousCell ?
-						writer.createPositionAfter( previousCell ) :
-						writer.createPositionAt( table.getChild( row ), 0 );
-
-					writer.move( writer.createRangeOn( cellToMove ), targetPosition );
-					updateNumericAttribute( 'rowspan', rowspanToSet, cellToMove, writer );
-
-					previousCell = cellToMove;
-				} else {
-					previousCell = cell;
-				}
+			else {
+				previousCell = cell;
 			}
+		}
 
-			writer.remove( tableRow );
-
-			const cellToFocus = getCellToFocus( table, removedRowIndex, columnToFocus );
-			writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );
-		} );
+		writer.remove( tableRow );
 	}
 }
 
+// Returns a helper object with first and last row index contained in given `referenceCells`.
+function getRowIndexes( referenceCells ) {
+	const allIndexesSorted = referenceCells.map( cell => cell.parent.index ).sort();
+
+	return {
+		first: allIndexesSorted[ 0 ],
+		last: allIndexesSorted[ allIndexesSorted.length - 1 ]
+	};
+}
+
 // Returns a cell that should be focused before removing the row, belonging to the same column as the currently focused cell.
 // * If the row was not the last one, the cell to focus will be in the row that followed it (before removal).
 // * If the row was the last one, the cell to focus will be in the row that preceded it (before removal).
@@ -121,4 +163,12 @@ function getCellToFocus( table, removedRowIndex, columnToFocus ) {
 		cellToFocus = tableCell;
 		column += parseInt( tableCell.getAttribute( 'colspan' ) || 1 );
 	}
+
+	return cellToFocus;
+}
+
+// Returns the index of column that should be focused after rows are removed.
+function getColumnIndexToFocus( tableMap, firstCell ) {
+	const firstCellData = tableMap.find( value => value.cell === firstCell );
+	return firstCellData.column;
 }

src/tableutils.js

@@ -550,6 +550,22 @@ export default class TableUtils extends Plugin {
 			return columns + columnWidth;
 		}, 0 );
 	}
+
+	/**
+	 * Returns the number of rows for a given table.
+	 *
+	 *		editor.plugins.get( 'TableUtils' ).getRows( table );
+	 *
+	 * @param {module:engine/model/element~Element} table The table to analyze.
+	 * @returns {Number}
+	 */
+	getRows( table ) {
+		return [ ...table.getChildren() ].reduce( ( rows, row ) => {
+			const currentRowCount = parseInt( row.getChild( 0 ).getAttribute( 'rowspan' ) || 1 );
+
+			return rows + currentRowCount;
+		}, 0 );
+	}
 }
 
 // Creates empty rows at the given index in an existing table.