Merge pull request #286 from ckeditor/i/6406

Fix: Improved spanned cells handling for row and column removals. Closes ckeditor/ckeditor5#6406.

src/commands/removecolumncommand.js

@@ -76,28 +76,40 @@ export default class RemoveColumnCommand extends Command {
 				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 ) {
-						const cellRow = cell.parent;
-
-						// The cell in removed column has colspan of 1.
-						writer.remove( cell );
-
-						// If the cell was the last one in the row, get rid of the entire row.
-						// https://github.com/ckeditor/ckeditor5/issues/6429
-						if ( !cellRow.childCount ) {
-							writer.remove( cellRow );
-						}
-					}
-				}
+				this._removeColumn( removedColumnIndex, table, writer );
 			}
 
 			writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );
 		} );
 	}
+
+	/**
+	 * Removes a column from the given `table`.
+	 *
+	 * @private
+	 * @param {Number} removedColumnIndex Index of the column that should be removed.
+	 * @param {module:engine/model/element~Element} table
+	 * @param {module:engine/model/writer~Writer} writer
+	 */
+	_removeColumn( removedColumnIndex, table, writer ) {
+		for ( const { cell, column, colspan } of new TableWalker( table ) ) {
+			// 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 ) {
+				const cellRow = cell.parent;
+
+				// The cell in removed column has colspan of 1.
+				writer.remove( cell );
+
+				// If the cell was the last one in the row, get rid of the entire row.
+				// https://github.com/ckeditor/ckeditor5/issues/6429
+				if ( !cellRow.childCount ) {
+					writer.remove( cellRow );
+				}
+			}
+		}
+	}
 }
 
 // Updates heading columns attribute if removing a row from head section.

src/commands/removerowcommand.js

@@ -10,7 +10,7 @@
 import Command from '@ckeditor/ckeditor5-core/src/command';
 
 import TableWalker from '../tablewalker';
-import { updateNumericAttribute } from './utils';
+import { findAncestor, updateNumericAttribute } from './utils';
 import { getSelectionAffectedTableCells } from '../utils';
 
 /**
@@ -33,15 +33,16 @@ export default class RemoveRowCommand extends Command {
 		const firstCell = selectedCells[ 0 ];
 
 		if ( firstCell ) {
-			const table = firstCell.parent.parent;
+			const table = findAncestor( 'table', firstCell );
 			const tableRowCount = this.editor.plugins.get( 'TableUtils' ).getRows( table );
+			const lastRowIndex = tableRowCount - 1;
 
-			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 ];
+			const selectedRowIndexes = getRowIndexes( selectedCells );
 
-			this.isEnabled = maxRowIndex - minRowIndex < ( tableRowCount - 1 );
+			const areAllRowsSelected = selectedRowIndexes.first === 0 && selectedRowIndexes.last === lastRowIndex;
+
+			// Disallow selecting whole table -> delete whole table should be used instead.
+			this.isEnabled = !areAllRowsSelected;
 		} else {
 			this.isEnabled = false;
 		}
@@ -51,34 +52,35 @@ export default class RemoveRowCommand extends Command {
 	 * @inheritDoc
 	 */
 	execute() {
-		const referenceCells = getSelectionAffectedTableCells( this.editor.model.document.selection );
+		const model = this.editor.model;
+		const referenceCells = getSelectionAffectedTableCells( 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 );
+		const table = findAncestor( 'table', firstCell );
+
+		const batch = model.createBatch();
+		const columnIndexToFocus = this.editor.plugins.get( 'TableUtils' ).getCellLocation( firstCell ).column;
 
 		// 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 => {
+		model.enqueueChange( batch, writer => {
 			// This prevents the "model-selection-range-intersects" error, caused by removing row selected cells.
 			writer.setSelection( writer.createSelection( table, 'on' ) );
 		} );
 
 		let cellToFocus;
 
 		for ( let i = removedRowIndexes.last; i >= removedRowIndexes.first; i-- ) {
-			this.editor.model.enqueueChange( batch, writer => {
+			model.enqueueChange( batch, writer => {
 				const removedRowIndex = i;
-				this._removeRow( removedRowIndex, table, writer, tableMap );
+				this._removeRow( removedRowIndex, table, writer );
 
 				cellToFocus = getCellToFocus( table, removedRowIndex, columnIndexToFocus );
 			} );
 		}
 
-		this.editor.model.enqueueChange( batch, writer => {
+		model.enqueueChange( batch, writer => {
 			writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );
 		} );
 	}
@@ -90,12 +92,12 @@ export default class RemoveRowCommand extends Command {
 	 * @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 ) {
+	_removeRow( removedRowIndex, table, writer ) {
 		const cellsToMove = new Map();
 		const tableRow = table.getChild( removedRowIndex );
 		const headingRows = table.getAttribute( 'headingRows' ) || 0;
+		const tableMap = [ ...new TableWalker( table, { endRow: removedRowIndex } ) ];
 
 		if ( headingRows && removedRowIndex < headingRows ) {
 			updateNumericAttribute( 'headingRows', headingRows - 1, table, writer, 0 );
@@ -166,9 +168,3 @@ function getCellToFocus( table, removedRowIndex, columnToFocus ) {
 
 	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;
-}

tests/commands/removecolumncommand.js

@@ -324,6 +324,29 @@ describe( 'RemoveColumnCommand', () => {
 					[ '10', '[]14' ]
 				], { headingColumns: 1 } ) );
 			} );
+
+			it( 'should properly calculate truncated colspans', () => {
+				setData( model, modelTable( [
+					[ { contents: '00', colspan: 3 } ],
+					[ '10', '11', '12' ],
+					[ '20', '21', '22' ]
+				] ) );
+
+				const tableSelection = editor.plugins.get( TableSelection );
+				const modelRoot = model.document.getRoot();
+				tableSelection._setCellSelection(
+					modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
+					modelRoot.getNodeByPath( [ 0, 1, 1 ] )
+				);
+
+				command.execute();
+
+				assertEqualMarkup( getData( model ), modelTable( [
+					[ '00' ],
+					[ '[]12' ],
+					[ '22' ]
+				] ) );
+			} );
 		} );
 
 		it( 'should change heading columns if removing a heading column', () => {

tests/commands/removerowcommand.js

@@ -258,6 +258,27 @@ describe( 'RemoveRowCommand', () => {
 					[ '[]20', '21' ]
 				] ) );
 			} );
+
+			it( 'should properly calculate truncated rowspans', () => {
+				setData( model, modelTable( [
+					[ '00', { contents: '01', rowspan: 3 } ],
+					[ '10' ],
+					[ '20' ]
+				] ) );
+
+				const tableSelection = editor.plugins.get( TableSelection );
+				const modelRoot = model.document.getRoot();
+				tableSelection._setCellSelection(
+					modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
+					modelRoot.getNodeByPath( [ 0, 1, 0 ] )
+				);
+
+				command.execute();
+
+				assertEqualMarkup( getData( model ), modelTable( [
+					[ '[]20', '01' ]
+				] ) );
+			} );
 		} );
 
 		describe( 'with entire row selected', () => {