Table heading rows should be properly updated after removing rows as …

…a side effect of merging cells.
ckeditor · May 7, 2020 · dba9fac · dba9fac
1 parent 3d85aca
commit dba9fac
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 50 deletions.
diff --git a/packages/ckeditor5-table/src/commands/mergecellcommand.js b/packages/ckeditor5-table/src/commands/mergecellcommand.js
@@ -9,10 +9,7 @@
 
 import Command from '@ckeditor/ckeditor5-core/src/command';
 import TableWalker from '../tablewalker';
-import {
-	updateNumericAttribute,
-	isHeadingColumnCell
-} from './utils';
+import { isHeadingColumnCell, findAncestor } from './utils';
 import { getTableCellsContainingSelection } from '../utils';
 
 /**
@@ -85,6 +82,7 @@ export default class MergeCellCommand extends Command {
 		const tableCell = getTableCellsContainingSelection( doc.selection )[ 0 ];
 		const cellToMerge = this.value;
 		const direction = this.direction;
+		const table = findAncestor( 'table', tableCell );
 
 		model.change( writer => {
 			const isMergeNext = direction == 'right' || direction == 'down';
@@ -108,7 +106,8 @@ export default class MergeCellCommand extends Command {
 
 			// Remove empty row after merging.
 			if ( !removedTableCellRow.childCount ) {
-				removeEmptyRow( removedTableCellRow, writer );
+				const tableUtils = this.editor.plugins.get( 'TableUtils' );
+				tableUtils.removeRows( table, { at: table.getChildIndex( removedTableCellRow ) } );
 			}
 		} );
 	}
@@ -243,26 +242,6 @@ function getVerticalCell( tableCell, direction ) {
 	return cellToMergeData && cellToMergeData.cell;
 }
 
-// Properly removes an empty row from a table. It will update the `rowspan` attribute of cells that overlap the 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 );
-}
-
 // Merges two table cells. It will ensure that after merging cells with an empty paragraph, the resulting table cell will only have one
 // paragraph. If one of the merged table cells is empty, the merged table cell will have the contents of the non-empty table cell.
 // If both are empty, the merged table cell will have only one empty paragraph.

diff --git a/packages/ckeditor5-table/src/commands/mergecellscommand.js b/packages/ckeditor5-table/src/commands/mergecellscommand.js
@@ -8,7 +8,6 @@
  */
 
 import Command from '@ckeditor/ckeditor5-core/src/command';
-import TableWalker from '../tablewalker';
 import { findAncestor, updateNumericAttribute } from './utils';
 import TableUtils from '../tableutils';
 import { getColumnIndexes, getRowIndexes, getSelectedTableCells } from '../utils';
@@ -46,6 +45,7 @@ export default class MergeCellsCommand extends Command {
 
 			// All cells will be merged into the first one.
 			const firstTableCell = selectedTableCells.shift();
+			const table = findAncestor( 'table', firstTableCell );
 
 			// Set the selection in cell that other cells are being merged to prevent model-selection-range-intersects error in undo.
 			// See https://github.com/ckeditor/ckeditor5/issues/6634.
@@ -57,40 +57,25 @@ export default class MergeCellsCommand extends Command {
 			updateNumericAttribute( 'colspan', mergeWidth, firstTableCell, writer );
 			updateNumericAttribute( 'rowspan', mergeHeight, firstTableCell, writer );
 
+			const emptyRows = [];
+
 			for ( const tableCell of selectedTableCells ) {
 				const tableRow = tableCell.parent;
+
 				mergeTableCells( tableCell, firstTableCell, writer );
-				removeRowIfEmpty( tableRow, writer );
+
+				if ( !tableRow.childCount ) {
+					emptyRows.push( table.getChildIndex( tableRow ) );
+				}
 			}
 
+			emptyRows.reverse().forEach( row => tableUtils.removeRows( table, { at: row } ) );
+
 			writer.setSelection( firstTableCell, 'in' );
 		} );
 	}
 }
 
-// Properly removes an empty row from a table. Updates the `rowspan` attribute of cells that overlap the removed row.
-//
-// @param {module:engine/model/element~Element} row
-// @param {module:engine/model/writer~Writer} writer
-function removeRowIfEmpty( row, writer ) {
-	if ( row.childCount ) {
-		return;
-	}
-
-	const table = row.parent;
-	const removedRowIndex = table.getChildIndex( row );
-
-	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( row );
-}
-
 // Merges two table cells. It will ensure that after merging cells with empty paragraphs the resulting table cell will only have one
 // paragraph. If one of the merged table cells is empty, the merged table cell will have contents of the non-empty table cell.
 // If both are empty, the merged table cell will have only one empty paragraph.

diff --git a/packages/ckeditor5-table/tests/_utils/utils.js b/packages/ckeditor5-table/tests/_utils/utils.js
@@ -503,6 +503,9 @@ export function createTableAsciiArt( model, table ) {
 	const { row: lastRow, column: lastColumn } = tableMap[ tableMap.length - 1 ];
 	const columns = lastColumn + 1;
 
+	const headingRows = parseInt( table.getAttribute( 'headingRows' ) ) || 0;
+	const headingColumns = parseInt( table.getAttribute( 'headingColumns' ) ) || 0;
+
 	let result = '';
 
 	for ( let row = 0; row <= lastRow; row++ ) {
@@ -535,13 +538,25 @@ export function createTableAsciiArt( model, table ) {
 			if ( column == lastColumn ) {
 				gridLine += '+';
 				contentLine += '|';
+
+				if ( headingRows && row == headingRows ) {
+					gridLine += ' <-- heading rows';
+				}
 			}
 		}
 		result += gridLine + '\n';
 		result += contentLine + '\n';
 
 		if ( row == lastRow ) {
 			result += `+${ '----+'.repeat( columns ) }`;
+
+			if ( headingRows && row == headingRows - 1 ) {
+				result += ' <-- heading rows';
+			}
+
+			if ( headingColumns > 0 ) {
+				result += `\n${ '     '.repeat( headingColumns ) }^-- heading columns`;
+			}
 		}
 	}
 

diff --git a/packages/ckeditor5-table/tests/commands/mergecellscommand.js b/packages/ckeditor5-table/tests/commands/mergecellscommand.js
@@ -514,6 +514,28 @@ describe( 'MergeCellsCommand', () => {
 				] ) );
 			} );
 
+			it( 'should decrease heading rows if some heading rows were removed', () => {
+				setData( model, modelTable( [
+					[ '00' ],
+					[ '10' ],
+					[ '20' ]
+				], { headingRows: 2 } ) );
+
+				selectNodes( [
+					[ 0, 0, 0 ],
+					[ 0, 1, 0 ]
+				] );
+
+				command.execute();
+
+				assertEqualMarkup( getData( model ), modelTable( [
+					[
+						'<paragraph>[00</paragraph><paragraph>10]</paragraph>'
+					],
+					[ '20' ]
+				], { headingRows: 1 } ) );
+			} );
+
 			it( 'should decrease rowspan if cell overlaps removed row', () => {
 				setData( model, modelTable( [
 					[ '00', { rowspan: 2, contents: '01' }, { rowspan: 3, contents: '02' } ],