Merge pull request #6779 from ckeditor/i/6260

#6260: Fixed triggering of table cell refreshing when multiple elements added in one differ

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

@@ -28,10 +28,21 @@ function tableCellRefreshPostFixer( model ) {
 	// Stores cells to be refreshed so the table cell will be refreshed once for multiple changes.
 	const cellsToRefresh = new Set();
 
+	// Counting the paragraph inserts to verify if it increased to more than one paragraph in the current differ.
+	let insertCount = 0;
+
 	for ( const change of differ.getChanges() ) {
 		const parent = change.type == 'insert' || change.type == 'remove' ? change.position.parent : change.range.start.parent;
 
-		if ( parent.is( 'tableCell' ) && checkRefresh( parent, change.type ) ) {
+		if ( !parent.is( 'tableCell' ) ) {
+			continue;
+		}
+
+		if ( change.type == 'insert' ) {
+			insertCount++;
+		}
+
+		if ( checkRefresh( parent, change.type, insertCount ) ) {
 			cellsToRefresh.add( parent );
 		}
 	}
@@ -60,7 +71,8 @@ function tableCellRefreshPostFixer( model ) {
 //
 // @param {module:engine/model/element~Element} tableCell The table cell to check.
 // @param {String} type Type of change.
-function checkRefresh( tableCell, type ) {
+// @param {Number} insertCount The number of inserts in differ.
+function checkRefresh( tableCell, type, insertCount ) {
 	const hasInnerParagraph = Array.from( tableCell.getChildren() ).some( child => child.is( 'paragraph' ) );
 
 	// If there is no paragraph in table cell then the view doesn't require refreshing.
@@ -83,5 +95,7 @@ function checkRefresh( tableCell, type ) {
 	//
 	// - another element is added to a single paragraph (childCount becomes >= 2)
 	// - another element is removed and a single paragraph is left (childCount == 1)
-	return tableCell.childCount <= ( type == 'insert' ? 2 : 1 );
+	//
+	// Change is not needed if there were multiple blocks before change.
+	return tableCell.childCount <= ( type == 'insert' ? insertCount + 1 : 1 );
 }

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

@@ -55,18 +55,16 @@ describe( 'Table cell refresh post-fixer', () => {
 		return editor.destroy();
 	} );
 
-	it( 'should rename <span> to <p> when adding more <paragraph> elements to the same table cell', () => {
+	it( 'should rename <span> to <p> when adding <paragraph> element to the same table cell', () => {
 		editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );
 
 		const table = root.getChild( 0 );
 
 		model.change( writer => {
 			const nodeByPath = table.getNodeByPath( [ 0, 0, 0 ] );
-
 			const paragraph = writer.createElement( 'paragraph' );
 
 			writer.insert( paragraph, nodeByPath, 'after' );
-
 			writer.setSelection( nodeByPath.nextSibling, 0 );
 		} );
 
@@ -76,18 +74,37 @@ describe( 'Table cell refresh post-fixer', () => {
 		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
-	it( 'should rename <span> to <p> on adding other block element to the same table cell', () => {
+	it( 'should rename <span> to <p> when adding more <paragraph> elements to the same table cell', () => {
 		editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );
 
 		const table = root.getChild( 0 );
 
 		model.change( writer => {
 			const nodeByPath = table.getNodeByPath( [ 0, 0, 0 ] );
+			const paragraph1 = writer.createElement( 'paragraph' );
+			const paragraph2 = writer.createElement( 'paragraph' );
 
-			const paragraph = writer.createElement( 'block' );
+			writer.insert( paragraph1, nodeByPath, 'after' );
+			writer.insert( paragraph2, nodeByPath, 'after' );
+			writer.setSelection( nodeByPath.nextSibling, 0 );
+		} );
 
-			writer.insert( paragraph, nodeByPath, 'after' );
+		assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
+			[ '<p>00</p><p></p><p></p>' ]
+		], { asWidget: true } ) );
+		sinon.assert.calledOnce( refreshItemSpy );
+	} );
 
+	it( 'should rename <span> to <p> on adding other block element to the same table cell', () => {
+		editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );
+
+		const table = root.getChild( 0 );
+
+		model.change( writer => {
+			const nodeByPath = table.getNodeByPath( [ 0, 0, 0 ] );
+			const block = writer.createElement( 'block' );
+
+			writer.insert( block, nodeByPath, 'after' );
 			writer.setSelection( nodeByPath.nextSibling, 0 );
 		} );
 
@@ -97,6 +114,27 @@ describe( 'Table cell refresh post-fixer', () => {
 		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
+	it( 'should rename <span> to <p> on adding multiple other block elements to the same table cell', () => {
+		editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );
+
+		const table = root.getChild( 0 );
+
+		model.change( writer => {
+			const nodeByPath = table.getNodeByPath( [ 0, 0, 0 ] );
+			const block1 = writer.createElement( 'block' );
+			const block2 = writer.createElement( 'block' );
+
+			writer.insert( block1, nodeByPath, 'after' );
+			writer.insert( block2, nodeByPath, 'after' );
+			writer.setSelection( nodeByPath.nextSibling, 0 );
+		} );
+
+		assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
+			[ '<p>00</p><div></div><div></div>' ]
+		], { asWidget: true } ) );
+		sinon.assert.calledOnce( refreshItemSpy );
+	} );
+
 	it( 'should properly rename the same element on consecutive changes', () => {
 		editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );
 
@@ -140,7 +178,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
-	it( 'should rename <p> to <span> when removing all but one paragraph inside table cell', () => {
+	it( 'should rename <p> to <span> when removing one of two paragraphs inside table cell', () => {
 		editor.setData( viewTable( [ [ '<p>00</p><p>foo</p>' ] ] ) );
 
 		const table = root.getChild( 0 );
@@ -155,6 +193,22 @@ describe( 'Table cell refresh post-fixer', () => {
 		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
+	it( 'should rename <p> to <span> when removing all but one paragraph inside table cell', () => {
+		editor.setData( viewTable( [ [ '<p>00</p><p>foo</p><p>bar</p>' ] ] ) );
+
+		const table = root.getChild( 0 );
+
+		model.change( writer => {
+			writer.remove( table.getNodeByPath( [ 0, 0, 1 ] ) );
+			writer.remove( table.getNodeByPath( [ 0, 0, 1 ] ) );
+		} );
+
+		assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
+			[ '00' ]
+		], { asWidget: true } ) );
+		sinon.assert.calledOnce( refreshItemSpy );
+	} );
+
 	it( 'should rename <p> to <span> when removing attribute from <paragraph>', () => {
 		editor.setData( '<table><tr><td><p foo="bar">00</p></td></tr></table>' );
 
@@ -250,6 +304,21 @@ describe( 'Table cell refresh post-fixer', () => {
 		sinon.assert.notCalled( refreshItemSpy );
 	} );
 
+	it( 'should do nothing on adding <paragraph> to existing paragraphs', () => {
+		editor.setData( viewTable( [ [ '<p>a</p><p>b</p>' ] ] ) );
+
+		const table = root.getChild( 0 );
+
+		model.change( writer => {
+			writer.insertElement( 'paragraph', table.getNodeByPath( [ 0, 0, 1 ] ), 'after' );
+		} );
+
+		assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
+			[ '<p>a</p><p>b</p><p></p>' ]
+		], { asWidget: true } ) );
+		sinon.assert.notCalled( refreshItemSpy );
+	} );
+
 	it( 'should do nothing when setting attribute on block item other then <paragraph>', () => {
 		editor.setData( viewTable( [ [ '<div>foo</div>' ] ] ) );