Merge pull request #212 from ckeditor/t/209

Fix: Table cell post-fixer will refresh a cell only when it is needed. Closes #209.

Author: scofalik

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

@@ -42,19 +42,20 @@ function tableCellContentsPostFixer( writer, model ) {
 	let wasFixed = false;
 
 	for ( const entry of changes ) {
-		// Analyze table cells on insertion.
-		if ( entry.type == 'insert' ) {
-			if ( entry.name == 'table' ) {
-				wasFixed = fixTable( entry.position.nodeAfter, writer ) || wasFixed;
-			}
-
-			if ( entry.name == 'tableRow' ) {
-				wasFixed = fixTableRow( entry.position.nodeAfter, writer ) || wasFixed;
-			}
-
-			if ( entry.name == 'tableCell' ) {
-				wasFixed = fixTableCellContent( entry.position.nodeAfter, writer ) || wasFixed;
-			}
+		if ( entry.type == 'insert' && entry.name == 'table' ) {
+			wasFixed = fixTable( entry.position.nodeAfter, writer ) || wasFixed;
+		}
+
+		if ( entry.type == 'insert' && entry.name == 'tableRow' ) {
+			wasFixed = fixTableRow( entry.position.nodeAfter, writer ) || wasFixed;
+		}
+
+		if ( entry.type == 'insert' && entry.name == 'tableCell' ) {
+			wasFixed = fixTableCellContent( entry.position.nodeAfter, writer ) || wasFixed;
+		}
+
+		if ( checkTableCellChange( entry ) ) {
+			wasFixed = fixTableCellContent( entry.position.parent, writer ) || wasFixed;
 		}
 	}
 
@@ -115,3 +116,17 @@ function fixTableCellContent( tableCell, writer ) {
 	// Return true when there were text nodes to fix.
 	return !!textNodes.length;
 }
+
+// Check if differ change should fix table cell. This happens on:
+// - removing content from table cell (ie tableCell can be left empty).
+// - adding text node directly into a table cell.
+//
+// @param {Object} differ change entry
+// @returns {Boolean}
+function checkTableCellChange( entry ) {
+	if ( !entry.position || !entry.position.parent.is( 'tableCell' ) ) {
+		return false;
+	}
+
+	return entry.type == 'insert' && entry.name == '$text' || entry.type == 'remove';
+}

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

@@ -25,17 +25,63 @@ export default function injectTableCellRefreshPostFixer( model ) {
 function tableCellRefreshPostFixer( model ) {
 	const differ = model.document.differ;
 
-	let fixed = false;
+	// Stores cells to be refreshed so the table cell will be refreshed once for multiple changes.
+	const cellsToRefresh = new Set();
 
 	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' ) ) {
-			differ.refreshItem( parent );
+		if ( parent.is( 'tableCell' ) && checkRefresh( parent, change.type ) ) {
+			cellsToRefresh.add( parent );
+		}
+	}
 
-			fixed = true;
+	if ( cellsToRefresh.size ) {
+		for ( const tableCell of cellsToRefresh.values() ) {
+			differ.refreshItem( tableCell );
 		}
+
+		return true;
+	}
+
+	return false;
+}
+
+// Checks if the model table cell requires refreshing to be re-rendered to a proper state in the view.
+//
+// This methods detects changes that will require renaming <span> to <p> (or vice versa) in the view.
+//
+// This method is a simple heuristic that checks only a single change and will sometimes give a false positive result when multiple changes
+// will result in a state that does not require renaming in the view (but will be seen as requiring a refresh).
+//
+// For instance: a `<span>` should be renamed to `<p>` when adding an attribute to a `<paragraph>`.
+// But adding one attribute and removing another one will result in a false positive: the check for added attribute will see one attribute
+// on a paragraph and will falsy qualify such change as adding an attribute to a paragraph without any attribute.
+//
+// @param {module:engine/model/element~Element} tableCell Table cell to check.
+// @param {String} type Type of change.
+function checkRefresh( tableCell, type ) {
+	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.
+	//
+	// Why? What we really want to achieve is to make all the old paragraphs (which weren't added in this batch) to be
+	// converted once again, so that the paragraph-in-table-cell converter can correctly create a `<p>` or a `<span>` element.
+	// If there are no paragraphs in the table cell, we don't care.
+	if ( !hasInnerParagraph ) {
+		return false;
+	}
+
+	// For attribute change we only refresh if there is a single paragraph as in this case we may want to change existing `<span>` to `<p>`.
+	if ( type == 'attribute' ) {
+		const attributesCount = Array.from( tableCell.getChild( 0 ).getAttributeKeys() ).length;
+
+		return tableCell.childCount === 1 && attributesCount < 2;
 	}
 
-	return fixed;
+	// For other changes (insert, remove) the `<span>` to `<p>` change is needed when:
+	//
+	// - 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 );
 }

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

@@ -17,7 +17,7 @@ import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictest
 import Delete from '@ckeditor/ckeditor5-typing/src/delete';
 
 describe( 'Table cell refresh post-fixer', () => {
-	let editor, model, doc, root, view;
+	let editor, model, doc, root, view, refreshItemSpy;
 
 	testUtils.createSinonSandbox();
 
@@ -44,10 +44,12 @@ describe( 'Table cell refresh post-fixer', () => {
 				} );
 				editor.conversion.elementToElement( { model: 'block', view: 'div' } );
 
-				model.schema.extend( '$block', { allowAttributes: 'foo' } );
+				model.schema.extend( '$block', { allowAttributes: [ 'foo', 'bar' ] } );
 				editor.conversion.attributeToAttribute( { model: 'foo', view: 'foo' } );
+				editor.conversion.attributeToAttribute( { model: 'bar', view: 'bar' } );
 
 				injectTableCellRefreshPostFixer( model );
+				refreshItemSpy = sinon.spy( model.document.differ, 'refreshItem' );
 			} );
 	} );
 
@@ -69,6 +71,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<p>00</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', () => {
@@ -89,6 +92,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<p>00</p><div></div>' ]
 		], { asWidget: true } ) );
+		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
 	it( 'should properly rename the same element on consecutive changes', () => {
@@ -107,6 +111,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<p>00</p><p></p>' ]
 		], { asWidget: true } ) );
+		sinon.assert.calledOnce( refreshItemSpy );
 
 		model.change( writer => {
 			writer.remove( table.getNodeByPath( [ 0, 0, 1 ] ) );
@@ -115,6 +120,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '00' ]
 		], { asWidget: true } ) );
+		sinon.assert.calledTwice( refreshItemSpy );
 	} );
 
 	it( 'should rename <span> to <p> when setting attribute on <paragraph>', () => {
@@ -129,6 +135,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<p foo="bar">00</p>' ]
 		], { asWidget: true } ) );
+		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
 	it( 'should rename <p> to <span> when removing all but one paragraph inside table cell', () => {
@@ -143,6 +150,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '00' ]
 		], { asWidget: true } ) );
+		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
 	it( 'should rename <p> to <span> when removing attribute from <paragraph>', () => {
@@ -157,6 +165,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<span>00</span>' ]
 		], { asWidget: true } ) );
+		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
 	it( 'should keep <p> in the view when <paragraph> attribute value is changed', () => {
@@ -171,6 +180,42 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<p foo="baz">00</p>' ]
 		], { asWidget: true } ) );
+		// False positive: should not be called.
+		sinon.assert.calledOnce( refreshItemSpy );
+	} );
+
+	it( 'should keep <p> in the view when adding another attribute to a <paragraph> with other attributes', () => {
+		editor.setData( viewTable( [ [ '<p foo="bar">00</p>' ] ] ) );
+
+		const table = root.getChild( 0 );
+
+		model.change( writer => {
+			writer.setAttribute( 'bar', 'bar', table.getNodeByPath( [ 0, 0, 0 ] ) );
+		} );
+
+		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
+			[ '<p bar="bar" foo="bar">00</p>' ]
+		], { asWidget: true } ) );
+
+		// False positive
+		sinon.assert.notCalled( refreshItemSpy );
+	} );
+
+	it( 'should keep <p> in the view when adding another attribute to a <paragraph> and removing attribute that is already set', () => {
+		editor.setData( viewTable( [ [ '<p foo="bar">00</p>' ] ] ) );
+
+		const table = root.getChild( 0 );
+
+		model.change( writer => {
+			writer.setAttribute( 'bar', 'bar', table.getNodeByPath( [ 0, 0, 0 ] ) );
+			writer.removeAttribute( 'foo', table.getNodeByPath( [ 0, 0, 0 ] ) );
+		} );
+
+		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
+			[ '<p bar="bar">00</p>' ]
+		], { asWidget: true } ) );
+		// False positive: should not be called.
+		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
 	it( 'should keep <p> in the view when <paragraph> attribute value is changed (table cell with multiple blocks)', () => {
@@ -185,6 +230,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<p foo="baz">00</p><p>00</p>' ]
 		], { asWidget: true } ) );
+		sinon.assert.notCalled( refreshItemSpy );
 	} );
 
 	it( 'should do nothing on rename <paragraph> to other block', () => {
@@ -199,6 +245,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<div>00</div>' ]
 		], { asWidget: true } ) );
+		sinon.assert.notCalled( refreshItemSpy );
 	} );
 
 	it( 'should do nothing when setting attribute on block item other then <paragraph>', () => {
@@ -213,9 +260,10 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<div foo="bar">foo</div>' ]
 		], { asWidget: true } ) );
+		sinon.assert.notCalled( refreshItemSpy );
 	} );
 
-	it( 'should keep <p> in the view when <paragraph> attribute value is changed (table cell with multiple blocks)', () => {
+	it( 'should rename <p> in to <span> when removing <paragraph> (table cell with 2 paragraphs)', () => {
 		editor.setData( viewTable( [ [ '<p>00</p><p>00</p>' ] ] ) );
 
 		const table = root.getChild( 0 );
@@ -227,6 +275,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( formatTable( getViewData( view, { withoutSelection: true } ) ) ).to.equal( formattedViewTable( [
 			[ '<span>00</span>' ]
 		], { asWidget: true } ) );
+		sinon.assert.calledOnce( refreshItemSpy );
 	} );
 
 	it( 'should update view selection after deleting content', () => {