Merge pull request #288 from ckeditor/i/6391

Fix: Remove table row command no longer breaks table heading downcast conversion. Closes ckeditor/ckeditor5#6391.

src/commands/removerowcommand.js

@@ -59,28 +59,34 @@ export default class RemoveRowCommand extends Command {
 		const firstCell = referenceCells[ 0 ];
 		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.
-		model.enqueueChange( batch, writer => {
+		model.change( writer => {
 			// This prevents the "model-selection-range-intersects" error, caused by removing row selected cells.
 			writer.setSelection( writer.createSelection( table, 'on' ) );
-		} );
 
-		let cellToFocus;
+			let cellToFocus;
 
-		for ( let i = removedRowIndexes.last; i >= removedRowIndexes.first; i-- ) {
-			model.enqueueChange( batch, writer => {
+			for ( let i = removedRowIndexes.last; i >= removedRowIndexes.first; i-- ) {
 				const removedRowIndex = i;
 				this._removeRow( removedRowIndex, table, writer );
 
 				cellToFocus = getCellToFocus( table, removedRowIndex, columnIndexToFocus );
-			} );
-		}
+			}
+
+			const model = this.editor.model;
+			const headingRows = table.getAttribute( 'headingRows' ) || 0;
+
+			if ( headingRows && removedRowIndexes.first < headingRows ) {
+				const newRows = getNewHeadingRowsValue( removedRowIndexes, headingRows );
+
+				// Must be done after the changes in table structure (removing rows).
+				// Otherwise the downcast converter for headingRows attribute will fail. ckeditor/ckeditor5#6391.
+				model.enqueueChange( writer.batch, writer => {
+					updateNumericAttribute( 'headingRows', newRows, table, writer, 0 );
+				} );
+			}
 
-		model.enqueueChange( batch, writer => {
 			writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );
 		} );
 	}
@@ -96,13 +102,8 @@ export default class RemoveRowCommand extends Command {
 	_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 );
-		}
-
 		// Get cells from removed row that are spanned over multiple rows.
 		tableMap
 			.filter( ( { row, rowspan } ) => row === removedRowIndex && rowspan > 1 )
@@ -168,3 +169,12 @@ function getCellToFocus( table, removedRowIndex, columnToFocus ) {
 
 	return cellToFocus;
 }
+
+// Calculates a new heading rows value for removing rows from heading section.
+function getNewHeadingRowsValue( removedRowIndexes, headingRows ) {
+	if ( removedRowIndexes.last < headingRows ) {
+		return headingRows - ( ( removedRowIndexes.last - removedRowIndexes.first ) + 1 );
+	}
+
+	return removedRowIndexes.first;
+}

tests/commands/removerowcommand.js

@@ -3,19 +3,20 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
-import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor';
+import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
 import { getData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
+import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 
 import RemoveRowCommand from '../../src/commands/removerowcommand';
 import TableSelection from '../../src/tableselection';
-import { defaultConversion, defaultSchema, modelTable } from '../_utils/utils';
+import { defaultConversion, defaultSchema, modelTable, viewTable } from '../_utils/utils';
 import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils';
 
 describe( 'RemoveRowCommand', () => {
 	let editor, model, command;
 
 	beforeEach( () => {
-		return ModelTestEditor.create( { plugins: [ TableSelection ] } )
+		return VirtualTestEditor.create( { plugins: [ TableSelection ] } )
 			.then( newEditor => {
 				editor = newEditor;
 				model = editor.model;
@@ -215,7 +216,7 @@ describe( 'RemoveRowCommand', () => {
 				] ) );
 			} );
 
-			it( 'should support removing multiple headings', () => {
+			it( 'should support removing multiple headings (removed rows in heading section)', () => {
 				setData( model, modelTable( [
 					[ '00', '01' ],
 					[ '10', '11' ],
@@ -238,6 +239,37 @@ describe( 'RemoveRowCommand', () => {
 				], { headingRows: 1 } ) );
 			} );
 
+			it( 'should support removing multiple headings (removed rows in heading and body section)', () => {
+				setData( model, modelTable( [
+					[ '00', '01' ],
+					[ '10', '11' ],
+					[ '20', '21' ],
+					[ '30', '31' ],
+					[ '40', '41' ]
+				], { headingRows: 3 } ) );
+
+				const tableSelection = editor.plugins.get( TableSelection );
+				const modelRoot = model.document.getRoot();
+
+				tableSelection._setCellSelection(
+					modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
+					modelRoot.getNodeByPath( [ 0, 3, 0 ] )
+				);
+
+				command.execute();
+
+				assertEqualMarkup( getData( model ), modelTable( [
+					[ '00', '01' ],
+					[ '[]40', '41' ]
+				], { headingRows: 1 } ) );
+
+				// The view should also be properly downcasted.
+				assertEqualMarkup( getViewData( editor.editing.view, { withoutSelection: true } ), viewTable( [
+					[ '00', '01' ],
+					[ '40', '41' ]
+				], { headingRows: 1 } ) );
+			} );
+
 			it( 'should support removing mixed heading and cell rows', () => {
 				setData( model, modelTable( [
 					[ '00', '01' ],
@@ -279,6 +311,34 @@ describe( 'RemoveRowCommand', () => {
 					[ '[]20', '01' ]
 				] ) );
 			} );
+
+			it( 'should create one undo step (1 batch)', () => {
+				setData( model, modelTable( [
+					[ '00', '01' ],
+					[ '10', '11' ],
+					[ '20', '21' ],
+					[ '30', '31' ]
+				], { headingRows: 3 } ) );
+
+				const createdBatches = new Set();
+
+				model.on( 'applyOperation', ( evt, args ) => {
+					const operation = args[ 0 ];
+
+					createdBatches.add( operation.batch );
+				} );
+
+				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();
+
+				expect( createdBatches.size ).to.equal( 1 );
+			} );
 		} );
 
 		describe( 'with entire row selected', () => {