Merge pull request #301 from ckeditor/i/6437

Fix: Removing the last header row no longer breaks the table in the editing view. Closes ckeditor/ckeditor5#6437.

src/commands/removerowcommand.js

@@ -60,17 +60,23 @@ export default class RemoveRowCommand extends Command {
 
 		const columnIndexToFocus = this.editor.plugins.get( 'TableUtils' ).getCellLocation( firstCell ).column;
 
-		model.change( writer => {
+		// Use single batch to modify table in steps but in one undo step.
+		const batch = model.createBatch();
+
+		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 rowsToRemove = removedRowIndexes.last - removedRowIndexes.first + 1;
 
 			this.editor.plugins.get( 'TableUtils' ).removeRows( table, {
 				at: removedRowIndexes.first,
-				rows: rowsToRemove
+				rows: rowsToRemove,
+				batch
 			} );
+		} );
 
+		model.enqueueChange( batch, writer => {
 			const cellToFocus = getCellToFocus( table, removedRowIndexes.first, columnIndexToFocus );
 
 			writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );

src/converters/downcast.js

@@ -209,9 +209,6 @@ export function downcastTableHeadingRowsChange( options = {} ) {
 					renameViewTableCell( tableCell, 'th', conversionApi, asWidget );
 				}
 			}
-
-			// Cleanup: this will remove any empty section from the view which may happen when moving all rows from a table section.
-			removeTableSectionIfEmpty( 'tbody', viewTable, conversionApi );
 		}
 		// The head section has shrunk so move rows from <thead> to <tbody>.
 		else {
@@ -234,11 +231,12 @@ export function downcastTableHeadingRowsChange( options = {} ) {
 			for ( const tableWalkerValue of tableWalker ) {
 				renameViewTableCellIfRequired( tableWalkerValue, tableAttributes, conversionApi, asWidget );
 			}
-
-			// Cleanup: this will remove any empty section from the view which may happen when moving all rows from a table section.
-			removeTableSectionIfEmpty( 'thead', viewTable, conversionApi );
 		}
 
+		// Cleanup: Ensure that thead & tbody sections are removed if left empty after moving rows. See #6437, #6391.
+		removeTableSectionIfEmpty( 'thead', viewTable, conversionApi );
+		removeTableSectionIfEmpty( 'tbody', viewTable, conversionApi );
+
 		function isBetween( index, lower, upper ) {
 			return index > lower && index < upper;
 		}
@@ -298,6 +296,7 @@ export function downcastRemoveRow() {
 		const viewStart = mapper.toViewPosition( data.position ).getLastMatchingPosition( value => !value.item.is( 'tr' ) );
 		const viewItem = viewStart.nodeAfter;
 		const tableSection = viewItem.parent;
+		const viewTable = tableSection.parent;
 
 		// Remove associated <tr> from the view.
 		const removeRange = viewWriter.createRangeOn( viewItem );
@@ -307,11 +306,9 @@ export function downcastRemoveRow() {
 			mapper.unbindViewElement( child );
 		}
 
-		// Check if table section has any children left - if not remove it from the view.
-		if ( !tableSection.childCount ) {
-			// No need to unbind anything as table section is not represented in the model.
-			viewWriter.remove( viewWriter.createRangeOn( tableSection ) );
-		}
+		// Cleanup: Ensure that thead & tbody sections are removed if left empty after removing rows. See #6437, #6391.
+		removeTableSectionIfEmpty( 'thead', viewTable, conversionApi );
+		removeTableSectionIfEmpty( 'tbody', viewTable, conversionApi );
 	}, { priority: 'higher' } );
 }
 

src/tableutils.js

@@ -287,14 +287,15 @@ export default class TableUtils extends Plugin {
 		const rowsToRemove = options.rows || 1;
 		const first = options.at;
 		const last = first + rowsToRemove - 1;
+		const batch = options.batch || 'default';
 
 		// Removing rows from table requires most calculations to be done prior to changing table structure.
 
 		// 1. Preparation - get row-spanned cells that have to be modified after removing rows.
 		const { cellsToMove, cellsToTrim } = getCellsToMoveAndTrimOnRemoveRow( table, first, last );
 
 		// 2. Execution
-		model.change( writer => {
+		model.enqueueChange( batch, writer => {
 			// 2a. Move cells from removed rows that extends over a removed section - must be done before removing rows.
 			// This will fill any gaps in a rows below that previously were empty because of row-spanned cells.
 			const rowAfterRemovedSection = last + 1;
@@ -311,7 +312,7 @@ export default class TableUtils extends Plugin {
 			}
 
 			// 2d. Adjust heading rows if removed rows were in a heading section.
-			updateHeadingRows( table, first, last, model, writer.batch );
+			updateHeadingRows( table, first, last, model, batch );
 		} );
 	}
 

tests/commands/removerowcommand.js

@@ -9,22 +9,19 @@ import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils
 
 import RemoveRowCommand from '../../src/commands/removerowcommand';
 import TableSelection from '../../src/tableselection';
-import { defaultConversion, defaultSchema, modelTable, viewTable } from '../_utils/utils';
+import { modelTable, viewTable } from '../_utils/utils';
 import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils';
+import TableEditing from '../../src/tableediting';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 
 describe( 'RemoveRowCommand', () => {
 	let editor, model, command;
 
-	beforeEach( () => {
-		return VirtualTestEditor.create( { plugins: [ TableSelection ] } )
-			.then( newEditor => {
-				editor = newEditor;
-				model = editor.model;
-				command = new RemoveRowCommand( editor );
+	beforeEach( async () => {
+		editor = await VirtualTestEditor.create( { plugins: [ Paragraph, TableEditing, TableSelection ] } );
 
-				defaultSchema( model.schema );
-				defaultConversion( editor.conversion );
-			} );
+		model = editor.model;
+		command = new RemoveRowCommand( editor );
 	} );
 
 	afterEach( () => {
@@ -290,11 +287,11 @@ describe( 'RemoveRowCommand', () => {
 					[ '[]40', '41' ]
 				], { headingRows: 1 } ) );
 
-				// The view should also be properly downcasted.
+				// The editing view should also be properly downcasted.
 				assertEqualMarkup( getViewData( editor.editing.view, { withoutSelection: true } ), viewTable( [
 					[ '00', '01' ],
 					[ '40', '41' ]
-				], { headingRows: 1 } ) );
+				], { headingRows: 1, asWidget: true } ) );
 			} );
 
 			it( 'should support removing mixed heading and cell rows', () => {
@@ -509,16 +506,16 @@ describe( 'RemoveRowCommand', () => {
 			setData( model, modelTable( [
 				[ { rowspan: 4, contents: '00' }, { rowspan: 3, contents: '01' }, { rowspan: 2, contents: '02' }, '03', '04' ],
 				[ { rowspan: 2, contents: '13' }, '14' ],
-				[ '22[]', '23', '24' ],
-				[ '30', '31', '32', '33', '34' ]
+				[ '22[]', '24' ],
+				[ '31', '32', '33', '34' ]
 			] ) );
 
 			command.execute();
 
 			assertEqualMarkup( getData( model ), modelTable( [
 				[ { rowspan: 3, contents: '00' }, { rowspan: 2, contents: '01' }, { rowspan: 2, contents: '02' }, '03', '04' ],
 				[ '13', '14' ],
-				[ '30', '31', '[]32', '33', '34' ]
+				[ '31', '32', '[]33', '34' ]
 			] ) );
 		} );