Merge pull request #60 from ckeditor/t/16

Fix: The `MergeCellCommand` should check if merging cells results in an empty row and remove it. Closes #16.

Author: Reinmar

src/commands/mergecellcommand.js

@@ -11,6 +11,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command';
 import Position from '@ckeditor/ckeditor5-engine/src/model/position';
 import Range from '@ckeditor/ckeditor5-engine/src/model/range';
 import TableWalker from '../tablewalker';
+import { updateNumericAttribute } from './utils';
 
 /**
  * The merge cell command.
@@ -92,16 +93,25 @@ export default class MergeCellCommand extends Command {
 			const cellToExpand = isMergeNext ? tableCell : cellToMerge;
 			const cellToRemove = isMergeNext ? cellToMerge : tableCell;
 
+			// Cache the parent of cell to remove for later check.
+			const removedTableCellRow = cellToRemove.parent;
+
+			// Remove table cell and merge it contents with merged cell.
 			writer.move( Range.createIn( cellToRemove ), Position.createAt( cellToExpand, 'end' ) );
 			writer.remove( cellToRemove );
 
 			const spanAttribute = this.isHorizontal ? 'colspan' : 'rowspan';
 			const cellSpan = parseInt( tableCell.getAttribute( spanAttribute ) || 1 );
 			const cellToMergeSpan = parseInt( cellToMerge.getAttribute( spanAttribute ) || 1 );
 
+			// Update table cell span attribute and merge set selection on merged contents.
 			writer.setAttribute( spanAttribute, cellSpan + cellToMergeSpan, cellToExpand );
-
 			writer.setSelection( Range.createIn( cellToExpand ) );
+
+			// Remove empty row after merging.
+			if ( !removedTableCellRow.childCount ) {
+				removeEmptyRow( removedTableCellRow, writer );
+			}
 		} );
 	}
 
@@ -195,3 +205,23 @@ function getVerticalCell( tableCell, direction ) {
 
 	return cellToMergeData && cellToMergeData.cell;
 }
+
+// Properly removes empty row from a table. Will update `rowspan` attribute of cells that overlaps removed row.
+//
+// @param {module:engine/model/element~Element} removedTableCellRow
+// @param {module:engine/model/writer~Writer} writer
+function removeEmptyRow( removedTableCellRow, writer ) {
+	const table = removedTableCellRow.parent;
+
+	const removedRowIndex = table.getChildIndex( removedTableCellRow );
+
+	for ( const { cell, row, rowspan } of new TableWalker( table, { endRow: removedRowIndex } ) ) {
+		const overlapsRemovedRow = row + rowspan - 1 >= removedRowIndex;
+
+		if ( overlapsRemovedRow ) {
+			updateNumericAttribute( 'rowspan', rowspan - 1, cell, writer );
+		}
+	}
+
+	writer.remove( removedTableCellRow );
+}

tests/commands/mergecellcommand.js

@@ -394,6 +394,40 @@ describe( 'MergeCellCommand', () => {
 					[ '10' ]
 				] ) );
 			} );
+
+			it( 'should remove empty row if merging table cells ', () => {
+				setData( model, modelTable( [
+					[ { rowspan: 2, contents: '00' }, '01[]', { rowspan: 3, contents: '02' } ],
+					[ '11' ],
+					[ '20', '21' ]
+				] ) );
+
+				command.execute();
+
+				expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
+					[ '00', '[0111]', { rowspan: 2, contents: '02' } ],
+					[ '20', '21' ]
+				] ) );
+			} );
+
+			it( 'should not reduce rowspan on cells above removed empty row when merging table cells ', () => {
+				setData( model, modelTable( [
+					[ { rowspan: 2, contents: '00' }, '01', '02' ],
+					[ '11', '12' ],
+					[ { rowspan: 2, contents: '20' }, '21[]', { rowspan: 3, contents: '22' } ],
+					[ '31' ],
+					[ '40', '41' ]
+				] ) );
+
+				command.execute();
+
+				expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
+					[ { rowspan: 2, contents: '00' }, '01', '02' ],
+					[ '11', '12' ],
+					[ '20', '[2131]', { rowspan: 2, contents: '22' } ],
+					[ '40', '41' ]
+				] ) );
+			} );
 		} );
 	} );
 
@@ -542,6 +576,40 @@ describe( 'MergeCellCommand', () => {
 					[ { colspan: 2, contents: '40' }, '42' ]
 				] ) );
 			} );
+
+			it( 'should remove empty row if merging table cells ', () => {
+				setData( model, modelTable( [
+					[ { rowspan: 2, contents: '00' }, '01', { rowspan: 3, contents: '02' } ],
+					[ '11[]' ],
+					[ '20', '21' ]
+				] ) );
+
+				command.execute();
+
+				expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
+					[ '00', '[0111]', { rowspan: 2, contents: '02' } ],
+					[ '20', '21' ]
+				] ) );
+			} );
+
+			it( 'should not reduce rowspan on cells above removed empty row when merging table cells ', () => {
+				setData( model, modelTable( [
+					[ { rowspan: 2, contents: '00' }, '01', '02' ],
+					[ '11', '12' ],
+					[ { rowspan: 2, contents: '20' }, '21', { rowspan: 3, contents: '22' } ],
+					[ '31[]' ],
+					[ '40', '41' ]
+				] ) );
+
+				command.execute();
+
+				expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
+					[ { rowspan: 2, contents: '00' }, '01', '02' ],
+					[ '11', '12' ],
+					[ '20', '[2131]', { rowspan: 2, contents: '22' } ],
+					[ '40', '41' ]
+				] ) );
+			} );
 		} );
 	} );
 } );