8
0
Pārlūkot izejas kodu

Refactor, again, how table cell refresh post-fixer check which paragraph should be refreshed.

Using mapper should guarantee that we do a minimal set of changes. The previous implementations did not check for attribute changes.
Maciej Gołaszewski 5 gadi atpakaļ
vecāks
revīzija
7d825c357d

+ 20 - 4
packages/ckeditor5-table/src/converters/downcast.js

@@ -255,10 +255,7 @@ export function convertParagraphInTableCell( modelElement, conversionApi ) {
 		return;
 	}
 
-	const tableCell = modelElement.parent;
-	const isSingleParagraph = tableCell.childCount === 1;
-
-	if ( isSingleParagraph && !hasAnyAttribute( modelElement ) ) {
+	if ( isSingleParagraphWithoutAttributes( modelElement ) ) {
 		// Use display:inline-block to force Chrome/Safari to limit text mutations to this element.
 		// See #6062.
 		return writer.createContainerElement( 'span', { style: 'display:inline-block' } );
@@ -267,6 +264,25 @@ export function convertParagraphInTableCell( modelElement, conversionApi ) {
 	}
 }
 
+/**
+ * Checks if given model `<paragraph>` is an only child of a parent (`<tableCell>`) and if it has any attribute set.
+ *
+ * The paragraph should be converted in the editing view to:
+ *
+ * * If returned `true` - to a `<span style="display:inline-block">`
+ * * If returned `false` - to a `<p>`
+ *
+ * @param {module:engine/model/element~Element} modelElement
+ * @returns {Boolean}
+ */
+export function isSingleParagraphWithoutAttributes( modelElement ) {
+	const tableCell = modelElement.parent;
+
+	const isSingleParagraph = tableCell.childCount === 1;
+
+	return isSingleParagraph && !hasAnyAttribute( modelElement );
+}
+
 // Converts a given {@link module:engine/view/element~Element} to a table widget:
 // * Adds a {@link module:engine/view/element~Element#_setCustomProperty custom property} allowing to recognize the table widget element.
 // * Calls the {@link module:widget/utils~toWidget} function with the proper element's label creator.

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

@@ -7,6 +7,8 @@
  * @module table/converters/table-cell-refresh-post-fixer
  */
 
+import { isSingleParagraphWithoutAttributes } from './downcast';
+
 /**
  * Injects a table cell post-fixer into the model which marks the table cell in the differ to have it re-rendered.
  *
@@ -17,66 +19,33 @@
  * re-rendered so it changes from `<span>` to `<p>`. The easiest way to do it is to re-render the entire table cell.
  *
  * @param {module:engine/model/model~Model} model
+ * @param {module:engine/conversion/mapper~Mapper} mapper
  */
-export default function injectTableCellRefreshPostFixer( model ) {
-	model.document.registerPostFixer( () => tableCellRefreshPostFixer( model ) );
+export default function injectTableCellRefreshPostFixer( model, mapper ) {
+	model.document.registerPostFixer( () => tableCellRefreshPostFixer( model.document.differ, mapper ) );
 }
 
-function tableCellRefreshPostFixer( model ) {
-	const differ = model.document.differ;
-
-	const changesForCells = new Map();
-
-	// 1. Gather all changes inside table cell.
-	differ.getChanges().forEach( change => {
-		const parent = change.type == 'attribute' ? change.range.start.parent : change.position.parent;
-
-		if ( !parent.is( 'element', 'tableCell' ) ) {
-			return;
-		}
-
-		if ( !changesForCells.has( parent ) ) {
-			changesForCells.set( parent, [] );
-		}
-
-		changesForCells.get( parent ).push( change );
-	} );
-
+function tableCellRefreshPostFixer( differ, mapper ) {
 	// Stores cells to be refreshed, so the table cell will be refreshed once for multiple changes.
-	const cellsToRefresh = new Set();
 
-	// 2. For each table cell:
-	for ( const [ tableCell, changes ] of changesForCells.entries() ) {
-		// 2a. Count inserts/removes as diff and marks any attribute change.
-		const { childDiff, attribute } = getChangesSummary( changes );
-
-		// 2b. If we detect that number of children has changed...
-		if ( childDiff !== 0 ) {
-			const currentChildren = tableCell.childCount;
-			const prevChildren = currentChildren - childDiff;
-
-			// Might need refresh if previous children was different from 1. Eg.: it was 2 before, now is 1 (or the opposite).
-			if ( currentChildren === 1 || prevChildren === 1 ) {
-				cellsToRefresh.add( tableCell );
-			}
-		}
+	// 1. Gather all changes inside table cell.
+	const changedCells = differ.getChanges()
+		.map( change => change.type == 'attribute' ? change.range.start.parent : change.position.parent )
+		.filter( parent => parent.is( 'element', 'tableCell' ) );
 
-		// ... 2c or some attribute has changed.
-		if ( attribute ) {
-			cellsToRefresh.add( tableCell );
-		}
+	if ( !changedCells.length ) {
+		return false;
 	}
 
-	// Having cells to refresh we need to
-	if ( cellsToRefresh.size ) {
-		// @if CK_DEBUG_TABLE // console.log( `Post-fixing table: Checking table cell to refresh (${ cellsToRefresh.size }).` );
-		// @if CK_DEBUG_TABLE // let paragraphsRefreshed = 0;
+	const cellsToCheck = new Set( changedCells );
 
-		for ( const tableCell of cellsToRefresh.values() ) {
-			for ( const paragraph of [ ...tableCell.getChildren() ].filter( child => child.is( 'element', 'paragraph' ) ) ) {
-				// @if CK_DEBUG_TABLE // console.log( `Post-fixing table: refreshing paragraph in table cell (${++paragraphsRefreshed}).` );
-				differ.refreshItem( paragraph );
-			}
+	// @if CK_DEBUG_TABLE // console.log( `Post-fixing table: Checking table cell to refresh (${ cellsToCheck.size }).` );
+	// @if CK_DEBUG_TABLE // let paragraphsRefreshed = 0;
+
+	for ( const tableCell of cellsToCheck.values() ) {
+		for ( const paragraph of [ ...tableCell.getChildren() ].filter( child => shouldRefresh( child, mapper ) ) ) {
+			// @if CK_DEBUG_TABLE // console.log( `Post-fixing table: refreshing paragraph in table cell (${++paragraphsRefreshed}).` );
+			differ.refreshItem( paragraph );
 		}
 	}
 
@@ -85,22 +54,21 @@ function tableCellRefreshPostFixer( model ) {
 	return false;
 }
 
-function updateSummaryFromChange( summary, change ) {
-	if ( change.type === 'remove' ) {
-		summary.childDiff--;
+// Check if given model element needs refreshing.
+//
+// @param {module:engine/model/element~Element} modelElement
+// @param {module:engine/conversion/mapper~Mapper} mapper
+// @returns {Boolean}
+function shouldRefresh( child, mapper ) {
+	if ( !child.is( 'element', 'paragraph' ) ) {
+		return false;
 	}
 
-	if ( change.type === 'insert' ) {
-		summary.childDiff++;
-	}
+	const viewElement = mapper.toViewElement( child );
 
-	if ( change.type === 'attribute' ) {
-		summary.attribute = true;
+	if ( !viewElement ) {
+		return false;
 	}
 
-	return summary;
-}
-
-function getChangesSummary( changes ) {
-	return changes.reduce( updateSummaryFromChange, { childDiff: 0, attribute: false } );
+	return isSingleParagraphWithoutAttributes( child ) !== viewElement.is( 'element', 'span' );
 }

+ 1 - 1
packages/ckeditor5-table/src/tableediting.js

@@ -154,7 +154,7 @@ export default class TableEditing extends Plugin {
 
 		injectTableHeadingRowsRefreshPostFixer( model );
 		injectTableLayoutPostFixer( model );
-		injectTableCellRefreshPostFixer( model );
+		injectTableCellRefreshPostFixer( model, editor.editing.mapper );
 		injectTableCellParagraphPostFixer( model );
 	}
 

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

@@ -52,7 +52,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		return editor.editing.mapper.toViewElement( table.getNodeByPath( [ 0, 0, 0 ] ) );
 	}
 
-	it( 'should rename <span> to <p> when adding <paragraph> element to the same table cell', () => {
+	it( 'should rename <span> to <p> when adding <paragraph> element to the same table cell (append)', () => {
 		editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );
 
 		const table = root.getChild( 0 );
@@ -72,6 +72,25 @@ describe( 'Table cell refresh post-fixer', () => {
 		expect( getViewForParagraph( table ) ).to.not.equal( previousView );
 	} );
 
+	it( 'should rename <span> to <p> when adding <paragraph> element to the same table cell (prepend)', () => {
+		editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );
+
+		const table = root.getChild( 0 );
+		const previousView = getViewForParagraph( table );
+
+		model.change( writer => {
+			const nodeByPath = table.getNodeByPath( [ 0, 0, 0 ] );
+			const paragraph = writer.createElement( 'paragraph' );
+
+			writer.insert( paragraph, nodeByPath, 'before' );
+		} );
+
+		assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
+			[ '<p></p><p>00</p>' ]
+		], { asWidget: true } ) );
+		expect( getViewForParagraph( table ) ).to.not.equal( previousView );
+	} );
+
 	it( 'should rename <span> to <p> when adding more <paragraph> elements to the same table cell', () => {
 		editor.setData( viewTable( [ [ '<p>00</p>' ] ] ) );
 
@@ -262,7 +281,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
 			[ '<p foo="baz">00</p>' ]
 		], { asWidget: true } ) );
-		expect( getViewForParagraph( table ) ).to.not.equal( previousView );
+		expect( getViewForParagraph( table ) ).to.equal( previousView );
 	} );
 
 	it( 'should keep <p> in the view when adding another attribute to a <paragraph> with other attributes', () => {
@@ -278,7 +297,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
 			[ '<p bar="bar" foo="bar">00</p>' ]
 		], { asWidget: true } ) );
-		expect( getViewForParagraph( table ) ).to.not.equal( previousView );
+		expect( getViewForParagraph( table ) ).to.equal( previousView );
 	} );
 
 	it( 'should keep <p> in the view when adding another attribute to a <paragraph> and removing attribute that is already set', () => {
@@ -295,7 +314,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
 			[ '<p bar="bar">00</p>' ]
 		], { asWidget: true } ) );
-		expect( getViewForParagraph( table ) ).to.not.equal( previousView );
+		expect( getViewForParagraph( table ) ).to.equal( previousView );
 	} );
 
 	it( 'should keep <p> in the view when <paragraph> attribute value is changed (table cell with multiple blocks)', () => {
@@ -311,7 +330,7 @@ describe( 'Table cell refresh post-fixer', () => {
 		assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
 			[ '<p foo="baz">00</p><p>00</p>' ]
 		], { asWidget: true } ) );
-		expect( getViewForParagraph( table ) ).to.not.equal( previousView );
+		expect( getViewForParagraph( table ) ).to.equal( previousView );
 	} );
 
 	it( 'should do nothing on rename <paragraph> to other block', () => {