Merge pull request #6762 from ckeditor/i/3274

Fix (table): Empty table rows are properly handled during conversion and layout post-fixing. Closes #3274.

packages/ckeditor5-table/src/converters/table-cell-paragraph-post-fixer.js

@@ -100,6 +100,8 @@ function fixTableRow( tableRow, writer ) {
 function fixTableCellContent( tableCell, writer ) {
 	// Insert paragraph to an empty table cell.
 	if ( tableCell.childCount == 0 ) {
+		// @if CK_DEBUG_TABLE // console.log( 'Post-fixing table: insert paragraph in empty cell.' );
+
 		writer.insertElement( 'paragraph', tableCell );
 
 		return true;
@@ -109,6 +111,8 @@ function fixTableCellContent( tableCell, writer ) {
 	// Temporary solution. See https://github.com/ckeditor/ckeditor5/issues/1464.
 	const textNodes = Array.from( tableCell.getChildren() ).filter( child => child.is( 'text' ) );
 
+	// @if CK_DEBUG_TABLE // textNodes.length && console.log( 'Post-fixing table: wrap cell content with paragraph.' );
+
 	for ( const child of textNodes ) {
 		writer.wrap( writer.createRangeOn( child ), 'paragraph' );
 	}

packages/ckeditor5-table/src/converters/table-cell-refresh-post-fixer.js

@@ -48,6 +48,8 @@ function tableCellRefreshPostFixer( model ) {
 	}
 
 	if ( cellsToRefresh.size ) {
+		// @if CK_DEBUG_TABLE // console.log( `Post-fixing table: refreshing cells (${ cellsToRefresh.size }).` );
+
 		for ( const tableCell of cellsToRefresh.values() ) {
 			differ.refreshItem( tableCell );
 		}

packages/ckeditor5-table/src/converters/table-layout-post-fixer.js

@@ -271,6 +271,8 @@ function fixTableCellsRowspan( table, writer ) {
 	const cellsToTrim = findCellsToTrim( table );
 
 	if ( cellsToTrim.length ) {
+		// @if CK_DEBUG_TABLE // console.log( `Post-fixing table: trimming cells row-spans (${ cellsToTrim.length }).` );
+
 		wasFixed = true;
 
 		for ( const data of cellsToTrim ) {
@@ -290,14 +292,38 @@ function fixTableRowsSizes( table, writer ) {
 	let wasFixed = false;
 
 	const rowsLengths = getRowsLengths( table );
-	const tableSize = rowsLengths[ 0 ];
+	const rowsToRemove = [];
+
+	// Find empty rows.
+	for ( const [ rowIndex, size ] of rowsLengths.entries() ) {
+		if ( !size ) {
+			rowsToRemove.push( rowIndex );
+		}
+	}
+
+	// Remove empty rows.
+	if ( rowsToRemove.length ) {
+		// @if CK_DEBUG_TABLE // console.log( `Post-fixing table: remove empty rows (${ rowsToRemove.length }).` );
 
-	const isValid = Object.values( rowsLengths ).every( length => length === tableSize );
+		wasFixed = true;
+
+		for ( const rowIndex of rowsToRemove.reverse() ) {
+			writer.remove( table.getChild( rowIndex ) );
+			rowsLengths.splice( rowIndex, 1 );
+		}
+	}
+
+	// Verify if all the rows have the same number of columns.
+	const tableSize = rowsLengths[ 0 ];
+	const isValid = rowsLengths.every( length => length === tableSize );
 
 	if ( !isValid ) {
-		const maxColumns = Object.values( rowsLengths ).reduce( ( prev, current ) => current > prev ? current : prev, 0 );
+		// @if CK_DEBUG_TABLE // console.log( 'Post-fixing table: adding missing cells.' );
 
-		for ( const [ rowIndex, size ] of Object.entries( rowsLengths ) ) {
+		// Find the maximum number of columns.
+		const maxColumns = rowsLengths.reduce( ( prev, current ) => current > prev ? current : prev, 0 );
+
+		for ( const [ rowIndex, size ] of rowsLengths.entries() ) {
 			const columnsToInsert = maxColumns - size;
 
 			if ( columnsToInsert ) {
@@ -346,19 +372,16 @@ function findCellsToTrim( table ) {
 	return cellsToTrim;
 }
 
-// Returns an object with lengths of rows assigned to the corresponding row index.
+// Returns an array with lengths of rows assigned to the corresponding row index.
 //
 // @param {module:engine/model/element~Element} table
-// @returns {Object}
+// @returns {Array.<Number>}
 function getRowsLengths( table ) {
-	const lengths = {};
+	// TableWalker will not provide items for the empty rows, we need to pre-fill this array.
+	const lengths = new Array( table.childCount ).fill( 0 );
 
 	for ( const { row } of new TableWalker( table, { includeSpanned: true } ) ) {
-		if ( !lengths[ row ] ) {
-			lengths[ row ] = 0;
-		}
-
-		lengths[ row ] += 1;
+		lengths[ row ]++;
 	}
 
 	return lengths;

packages/ckeditor5-table/src/converters/upcasttable.js

@@ -52,11 +52,11 @@ export default function upcastTable() {
 			conversionApi.writer.insert( table, splitResult.position );
 			conversionApi.consumable.consume( viewTable, { name: true } );
 
-			if ( rows.length ) {
-				// Upcast table rows in proper order (heading rows first).
-				rows.forEach( row => conversionApi.convertItem( row, conversionApi.writer.createPositionAt( table, 'end' ) ) );
-			} else {
-				// Create one row and one table cell for empty table.
+			// Upcast table rows in proper order (heading rows first).
+			rows.forEach( row => conversionApi.convertItem( row, conversionApi.writer.createPositionAt( table, 'end' ) ) );
+
+			// Create one row and one table cell for empty table.
+			if ( table.isEmpty ) {
 				const row = conversionApi.writer.createElement( 'tableRow' );
 				conversionApi.writer.insert( row, conversionApi.writer.createPositionAt( table, 'end' ) );
 
@@ -90,6 +90,23 @@ export default function upcastTable() {
 	};
 }
 
+/**
+ * Conversion helper that skips empty <tr> from upcasting.
+ *
+ * Empty row is considered a table model error.
+ *
+ * @returns {Function} Conversion helper.
+ */
+export function skipEmptyTableRow() {
+	return dispatcher => {
+		dispatcher.on( 'element:tr', ( evt, data ) => {
+			if ( data.viewItem.isEmpty ) {
+				evt.stop();
+			}
+		}, { priority: 'high' } );
+	};
+}
+
 export function upcastTableCell( elementName ) {
 	return dispatcher => {
 		dispatcher.on( `element:${ elementName }`, ( evt, data, conversionApi ) => {

packages/ckeditor5-table/src/tableediting.js

@@ -9,7 +9,7 @@
 
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 
-import upcastTable, { upcastTableCell } from './converters/upcasttable';
+import upcastTable, { upcastTableCell, skipEmptyTableRow } from './converters/upcasttable';
 import {
 	downcastInsertCell,
 	downcastInsertRow,
@@ -98,6 +98,7 @@ export default class TableEditing extends Plugin {
 
 		// Table row conversion.
 		conversion.for( 'upcast' ).elementToElement( { model: 'tableRow', view: 'tr' } );
+		conversion.for( 'upcast' ).add( skipEmptyTableRow() );
 
 		conversion.for( 'editingDowncast' ).add( downcastInsertRow( { asWidget: true } ) );
 		conversion.for( 'dataDowncast' ).add( downcastInsertRow() );

packages/ckeditor5-table/tests/_utils/utils.js

@@ -500,6 +500,10 @@ function getClassToSet( attributes ) {
 export function createTableAsciiArt( model, table ) {
 	const tableMap = [ ...new TableWalker( table, { includeSpanned: true } ) ];
 
+	if ( !tableMap.length ) {
+		return '';
+	}
+
 	const { row: lastRow, column: lastColumn } = tableMap[ tableMap.length - 1 ];
 	const columns = lastColumn + 1;
 

packages/ckeditor5-table/tests/converters/table-layout-post-fixer.js

@@ -89,6 +89,25 @@ describe( 'Table layout post-fixer', () => {
 			] ) );
 		} );
 
+		it( 'should remove empty rows', () => {
+			const parsed = parse( modelTable( [
+				[ '00', '01' ],
+				[ ],
+				[ '20', '21', '22' ],
+				[ ]
+			] ), model.schema );
+
+			model.change( writer => {
+				writer.remove( writer.createRangeIn( root ) );
+				writer.insert( parsed, root );
+			} );
+
+			assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [
+				[ '00', '01', '' ],
+				[ '20', '21', '22' ]
+			] ) );
+		} );
+
 		it( 'should fix the wrong rowspan attribute of a table cell inside the header', () => {
 			const parsed = parse( modelTable( [
 				[ { rowspan: 2, contents: '00' }, { rowspan: 3, contents: '01' }, '02' ],

packages/ckeditor5-table/tests/converters/upcasttable.js

@@ -180,6 +180,32 @@ describe( 'upcastTable()', () => {
 		);
 	} );
 
+	it( 'should create valid table model from all empty rows', () => {
+		editor.setData(
+			'<table>' +
+				'<tr></tr>' +
+				'<tr></tr>' +
+			'</table>'
+		);
+
+		expectModel(
+			'<table><tableRow><tableCell><paragraph></paragraph></tableCell></tableRow></table>'
+		);
+	} );
+
+	it( 'should skip empty table rows', () => {
+		editor.setData(
+			'<table>' +
+				'<tr></tr>' +
+				'<tr><td>bar</td></tr>' +
+			'</table>'
+		);
+
+		expectModel(
+			'<table><tableRow><tableCell><paragraph>bar</paragraph></tableCell></tableRow></table>'
+		);
+	} );
+
 	it( 'should skip unknown table children', () => {
 		editor.setData(
 			'<table>' +