Table converters must not read model when converting to view

[scofalik]

One of the rules when creating converters is to perform view changes only depending on the view state and on the data passed by the dispatcher to the converter. It is incorrect to use the current model state when converting to view. Why? Because if there are multiple changes introduced in one batch, the model state is very different than the view state, so making decisions basing on data read from the model will lead to errors.

This didn't happen yet because tables weren't tested heavily in real-time collaboration, or, more importantly, in track changes.

One of the issues happens in the part of the code shown below. The change was to remove two rows, but one of them was heading so it also changed headingRows attribute.

export function downcastTableHeadingRowsChange( options = {} ) {
	// ... (cut for readibility)

	// The head section has shrunk so move rows from <thead> to <tbody>.
	// Filter out only those rows that are in wrong section.
	const rowsToMove = Array.from( table.getChildren() )
		.filter( ( { index } ) => isBetween( index, newRows - 1, oldRows ) )
		.reverse(); // The rows will be moved from <thead> to <tbody> in reverse order at the beginning of a <tbody>.

	const viewTableBody = getOrCreateTableSection( 'tbody', viewTable, conversionApi );
	moveViewRowsToTableSection( rowsToMove, viewTableBody, conversionApi, 0 );

	// Check if cells moved from <thead> to <tbody> requires renaming to <td> as this depends on current heading columns attribute.
	const tableWalker = new TableWalker( table, { startRow: newRows ? newRows - 1 : newRows, endRow: oldRows - 1 } );

	// ...
}

This is the headingRows attribute converter which is fired before remove rows converter. It uses the model to get view items which should be moved. But the rows that are supposed to be moved are not existing in the model anymore! rowsToMove contains wrong rows, which leads to weird stuff happening and ultimately into a crash.

[jodator]

@scofalik this was a very good insight and we "fixed" that in recent iterations. Unfortunately this relates to #6506. The current "fix" uses beloved model.enqueChange() for the reasons described in #6506 and linked tickets.

This was fixed by #6437.