8
0
Просмотр исходного кода

Changed: Make table post-fixer a bit less clever.

Maciej Gołaszewski 7 лет назад
Родитель
Сommit
dce37ae9bb
2 измененных файлов с 190 добавлено и 175 удалено
  1. 16 73
      packages/ckeditor5-table/src/tableediting.js
  2. 174 102
      packages/ckeditor5-table/tests/tableediting.js

+ 16 - 73
packages/ckeditor5-table/src/tableediting.js

@@ -255,34 +255,28 @@ function tablePostFixer( writer, model, tableUtils ) {
 
 	let wasFixed = false;
 
-	const tableRowsOfRemovedCells = [];
-
 	for ( const entry of changes ) {
-		if ( entry.type == 'insert' ) {
-			let table;
-
-			if ( entry.name == 'table' ) {
-				table = entry.position.nodeAfter;
-			}
+		let table;
 
-			if ( entry.name == 'tableRow' ) {
-				const tableRow = entry.position.nodeAfter;
-				table = tableRow.parent;
-			}
+		if ( entry.name == 'table' ) {
+			table = entry.position.nodeAfter;
+		}
 
-			if ( table ) {
-				wasFixed = fixTableOnInsert( tableUtils, table, writer, wasFixed );
-			}
+		if ( entry.name == 'tableRow' ) {
+			const tableRow = entry.position.nodeAfter;
+			table = tableRow.parent;
 		}
 
-		if ( entry.type == 'remove' && entry.name == 'tableCell' ) {
+		if ( entry.name == 'tableCell' ) {
 			const tableRow = entry.position.parent;
 
-			tableRowsOfRemovedCells.push( tableRow );
+			table = tableRow.parent;
 		}
-	}
 
-	wasFixed = fixTableOnRemoveCells( tableRowsOfRemovedCells, writer ) || wasFixed;
+		if ( table ) {
+			wasFixed = makeTableRowsSameLength( tableUtils, table, writer );
+		}
+	}
 
 	return wasFixed;
 }
@@ -327,7 +321,9 @@ function getRowsLengths( table ) {
 	return { lengths, cellsToTrim };
 }
 
-function fixTableOnInsert( tableUtils, table, writer, wasFixed ) {
+function makeTableRowsSameLength( tableUtils, table, writer ) {
+	let wasFixed = false;
+
 	const tableSize = tableUtils.getColumns( table );
 
 	const { lengths, cellsToTrim } = getRowsLengths( table );
@@ -358,56 +354,3 @@ function fixTableOnInsert( tableUtils, table, writer, wasFixed ) {
 
 	return wasFixed;
 }
-
-function fixTableOnRemoveCells( tableRowsToCheck, writer ) {
-	let wasFixed = false;
-
-	if ( !tableRowsToCheck.length ) {
-		return wasFixed;
-	}
-
-	const indexes = tableRowsToCheck.map( tableRow => tableRow.index );
-
-	const table = tableRowsToCheck[ 0 ].parent;
-	const allIndexes = [ ...table.getChildren() ].map( tableRow => tableRow.index );
-
-	const other = allIndexes.filter( index => !indexes.includes( index ) );
-
-	const { lengths } = getRowsLengths( table );
-
-	const areSame = indexes.every( index => lengths[ index ] === lengths[ 0 ] );
-
-	if ( areSame ) {
-		const properLength = lengths[ 0 ];
-
-		other.map( index => {
-			let length = lengths[ index ];
-
-			const tableRow = table.getChild( index );
-
-			while ( length > properLength ) {
-				const tableCell = tableRow.getChild( tableRow.childCount - 1 );
-
-				const howMany = length - properLength;
-
-				const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 );
-
-				if ( colspan > howMany ) {
-					writer.setAttribute( 'colspan', colspan - howMany, tableCell );
-
-					length = properLength;
-				} else {
-					writer.remove( tableCell );
-
-					length -= colspan;
-				}
-
-				length = properLength - 10;
-
-				wasFixed = true;
-			}
-		} );
-	}
-
-	return wasFixed;
-}

+ 174 - 102
packages/ckeditor5-table/tests/tableediting.js

@@ -6,7 +6,7 @@
 import Range from '@ckeditor/ckeditor5-engine/src/model/range';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
-import { getData as getModelData, setData as setModelData, parse } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
+import { getData as getModelData, parse, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard';
 
 import TableEditing from '../src/tableediting';
@@ -20,6 +20,7 @@ import SplitCellCommand from '../src/commands/splitcellcommand';
 import MergeCellCommand from '../src/commands/mergecellcommand';
 import SetHeaderRowCommand from '../src/commands/setheaderrowcommand';
 import SetHeaderColumnCommand from '../src/commands/setheadercolumncommand';
+import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting';
 
 describe( 'TableEditing', () => {
 	let editor, model;
@@ -27,7 +28,7 @@ describe( 'TableEditing', () => {
 	beforeEach( () => {
 		return VirtualTestEditor
 			.create( {
-				plugins: [ TableEditing, Paragraph ]
+				plugins: [ TableEditing, Paragraph, UndoEditing ]
 			} )
 			.then( newEditor => {
 				editor = newEditor;
@@ -110,11 +111,11 @@ describe( 'TableEditing', () => {
 
 				expect( editor.getData() ).to.equal(
 					'<figure class="table">' +
-						'<table>' +
-							'<tbody>' +
-								'<tr><td>foo</td></tr>' +
-							'</tbody>' +
-						'</table>' +
+					'<table>' +
+					'<tbody>' +
+					'<tr><td>foo</td></tr>' +
+					'</tbody>' +
+					'</table>' +
 					'</figure>'
 				);
 			} );
@@ -124,11 +125,11 @@ describe( 'TableEditing', () => {
 
 				expect( editor.getData() ).to.equal(
 					'<figure class="table">' +
-						'<table>' +
-							'<thead>' +
-								'<tr><th>foo</th></tr>' +
-							'</thead>' +
-						'</table>' +
+					'<table>' +
+					'<thead>' +
+					'<tr><th>foo</th></tr>' +
+					'</thead>' +
+					'</table>' +
 					'</figure>'
 				);
 			} );
@@ -443,116 +444,187 @@ describe( 'TableEditing', () => {
 			], { headingRows: 1 } ) );
 		} );
 
-		// Client A: insert row. Client B: insert column.
-		it( 'should fix missing insert column before insert row', () => {
-			setModelData( model, modelTable( [
-				[ '00[]', '01', '02' ],
-				[ '10', '11', '12' ],
-				[ '20', '21', '22' ]
-			] ) );
+		it( 'collab remove column vs insert row', () => {
+			_testExternal(
+				modelTable( [
+					[ '00[]', '01' ],
+					[ '10', '11' ]
+				] ),
+				writer => _removeColumn( writer, 1, [ 0, 1 ] ),
+				writer => _insertRow( writer, 1, [ 'a', 'b' ] ),
+				formattedModelTable( [
+					[ '00', '' ],
+					[ 'a', 'b' ],
+					[ '10', '' ]
+				] ),
+				formattedModelTable( [
+					[ '00', '01', '' ],
+					[ 'a', 'b', '' ],
+					[ '10', '11', '' ]
+				] ) );
+		} );
 
-			const table = root.getChild( 0 );
+		it( 'collab insert row vs remove column', () => {
+			_testExternal(
+				modelTable( [
+					[ '00[]', '01' ],
+					[ '10', '11' ]
+				] ),
+				writer => _insertRow( writer, 1, [ 'a', 'b' ] ),
+				writer => _removeColumn( writer, 1, [ 0, 2 ] ),
+				formattedModelTable( [
+					[ '00', '' ],
+					[ 'a', 'b' ],
+					[ '10', '' ]
+				] ),
+				formattedModelTable( [
+					[ '00', '' ],
+					[ '10', '' ]
+				] ) );
+		} );
 
-			// Insert column:
-			model.change( writer => {
-				for ( const tableRow of table.getChildren() ) {
-					const tableCell = writer.createElement( 'tableCell' );
-					writer.insert( tableCell, tableRow, 1 );
-					writer.insertText( 'x', tableCell );
-				}
-			} );
+		it( 'collab insert row vs insert column', () => {
+			_testExternal(
+				modelTable( [
+					[ '00[]', '01' ],
+					[ '10', '11' ]
+				] ),
+				writer => _insertRow( writer, 1, [ 'a', 'b' ] ),
+				writer => _insertColumn( writer, 1, [ 0, 2 ] ),
+				formattedModelTable( [
+					[ '00', '', '01' ],
+					[ 'a', 'b', '' ],
+					[ '10', '', '11' ]
+				] ),
+				formattedModelTable( [
+					[ '00', '', '01' ],
+					[ '10', '', '11' ]
+				] ) );
+		} );
 
-			// Insert table row
-			model.change( writer => {
-				const parsedTable = parse(
-					modelTable( [ [ 'a', 'b', 'c' ] ] ),
-					model.schema
-				);
+		it( 'collab insert column vs insert row', () => {
+			_testExternal(
+				modelTable( [
+					[ '00[]', '01' ],
+					[ '10', '11' ]
+				] ),
+				writer => _insertColumn( writer, 1, [ 0, 1 ] ),
+				writer => _insertRow( writer, 1, [ 'a', 'b' ] ),
+				formattedModelTable( [
+					[ '00', '', '01' ],
+					[ 'a', 'b', '' ],
+					[ '10', '', '11' ]
+				] ),
+				formattedModelTable( [
+					[ '00', '01', '' ],
+					[ 'a', 'b', '' ],
+					[ '10', '11', '' ]
+				] ) );
+		} );
 
-				writer.insert( parsedTable.getChild( 0 ), table, 2 );
-			} );
+		it( 'collab insert column vs insert column - other row has spanned cell', () => {
+			_testExternal(
+				modelTable( [
+					[ { colspan: 3, contents: '00' } ],
+					[ '10', '11', '12' ]
+				] ),
+				writer => {
+					_setAttribute( writer, 'colspan', 4, [ 0, 0 ] );
+					_insertColumn( writer, 2, [ 1 ] );
+				},
+				writer => {
+					_setAttribute( writer, 'colspan', 4, [ 0, 0 ] );
+					_insertColumn( writer, 1, [ 1 ] );
+				},
+				formattedModelTable( [
+					[ { colspan: 4, contents: '00' }, '' ],
+					[ '10', '', '11', '', '12' ]
+				] ),
+				formattedModelTable( [
+					[ { colspan: 3, contents: '00' }, '' ],
+					[ '10', '', '11', '12' ]
+				] ) );
+		} );
 
-			expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formattedModelTable( [
-				[ '00', 'x', '01', '02' ],
-				[ '10', 'x', '11', '12' ],
-				[ 'a', 'b', 'c', '' ],
-				[ '20', 'x', '21', '22' ]
-			] ) );
+		it( 'collab insert column vs insert column - other row has spanned cell (inverted)', () => {
+			_testExternal(
+				modelTable( [
+					[ { colspan: 3, contents: '00' } ],
+					[ '10', '11', '12' ]
+				] ),
+				writer => {
+					_setAttribute( writer, 'colspan', 4, [ 0, 0 ] );
+					_insertColumn( writer, 1, [ 1 ] );
+				},
+				writer => {
+					_setAttribute( writer, 'colspan', 4, [ 0, 0 ] );
+					_insertColumn( writer, 3, [ 1 ] );
+				},
+				formattedModelTable( [
+					[ { colspan: 4, contents: '00' }, '' ],
+					[ '10', '', '11', '', '12' ]
+				] ),
+				formattedModelTable( [
+					[ { colspan: 3, contents: '00' }, '' ],
+					[ '10', '11', '', '12' ]
+				] ) );
 		} );
 
-		// Client A: insert row. Client B: insert column.
-		it( 'should fix (undo of add tableRow & add tableCell)', () => {
-			setModelData( model, modelTable( [
-				[ '00', 'x', '01', '02' ],
-				[ '10', 'x', '11', '12' ],
-				[ 'a', 'b', 'c', '' ],
-				[ '20', 'x', '21', '22' ]
-			] ) );
+		// Case: remove same column (undo does nothing on one client - NOOP in batch).
+		// Case: remove same row (undo does nothing on one client - NOOP in batch).
+		// Case: Typing over user selecting - typing in marker...
 
-			const table = root.getChild( 0 );
+		function _testExternal( initialData, localCallback, externalCallback, modelAfter, modelAfterUndo ) {
+			setModelData( model, initialData );
 
-			// Insert column:
-			model.change( writer => {
-				[ 0, 1, 3 ].map( index => writer.remove( table.getChild( index ).getChild( 1 ) ) );
-			} );
+			model.change( localCallback );
 
-			expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formattedModelTable( [
-				[ '00', '01', '02' ],
-				[ '10', '11', '12' ],
-				[ 'a', 'b', 'c' ],
-				[ '20', '21', '22' ]
-			] ) );
-		} );
+			model.enqueueChange( 'transparent', externalCallback );
 
-		// Client A: insert row. Client B: insert column.
-		it( 'should fix (undo of add tableRow & add tableCell) fff', () => {
-			setModelData( model, modelTable( [
-				[ '00', 'x', '01', '02' ],
-				[ '10', 'x', '11', '12' ],
-				[ 'a', { colspan: 3, contents: 'b' } ],
-				[ '20', 'x', '21', '22' ]
-			] ) );
+			expect( formatTable( getModelData( model, { withoutSelection: true } ) ), 'after operations' ).to.equal( modelAfter );
+
+			editor.execute( 'undo' );
+
+			expect( formatTable( getModelData( model, { withoutSelection: true } ) ), 'after undo' ).to.equal( modelAfterUndo );
+		}
 
+		function _removeColumn( writer, columnIndex, rows ) {
 			const table = root.getChild( 0 );
 
-			// Insert column:
-			model.change( writer => {
-				[ 0, 1, 3 ].map( index => writer.remove( table.getChild( index ).getChild( 1 ) ) );
-			} );
+			for ( const index of rows ) {
+				const tableRow = table.getChild( index );
+				writer.remove( tableRow.getChild( columnIndex ) );
+			}
+		}
 
-			expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formattedModelTable( [
-				[ '00', '01', '02' ],
-				[ '10', '11', '12' ],
-				[ 'a', { colspan: 2, contents: 'b' } ],
-				[ '20', '21', '22' ]
-			] ) );
-		} );
+		function _insertRow( writer, rowIndex, rowData ) {
+			const table = root.getChild( 0 );
 
-		// Client A: insert row. Client B: insert column.
-		it( 'should not fix (undo of add tableRow & add tableCell) - OK', () => {
-			setModelData( model, modelTable( [
-				[ '00', 'x', '01', '02' ],
-				[ '10', 'x', '11', '12' ],
-				[ 'a', 'b', 'c', '' ],
-				[ '20', 'x', '21', '22' ]
-			] ) );
+			const parsedTable = parse(
+				modelTable( [ rowData ] ),
+				model.schema
+			);
 
+			writer.insert( parsedTable.getChild( 0 ), table, rowIndex );
+		}
+
+		function _setAttribute( writer, attributeKey, attributeValue, path ) {
 			const table = root.getChild( 0 );
 
-			// Insert column:
-			model.change( writer => {
-				[ 0, 1, 3 ].map( index => writer.remove( table.getChild( index ).getChild( 1 ) ) );
+			const node = table.getNodeByPath( path );
 
-				// Remove one more cell... (shouldn't occur??)
-				writer.remove( table.getChild( 0 ).getChild( 1 ) );
-			} );
+			writer.setAttribute( attributeKey, attributeValue, node );
+		}
 
-			expect( formatTable( getModelData( model, { withoutSelection: true } ) ) ).to.equal( formattedModelTable( [
-				[ '00', '02' ],
-				[ '10', '11', '12' ],
-				[ 'a', 'b', 'c', '' ],
-				[ '20', '21', '22' ]
-			] ) );
-		} );
+		function _insertColumn( writer, columnIndex, rows ) {
+			const table = root.getChild( 0 );
+
+			for ( const index of rows ) {
+				const tableRow = table.getChild( index );
+				const tableCell = writer.createElement( 'tableCell' );
+
+				writer.insert( tableCell, tableRow, columnIndex );
+			}
+		}
 	} );
 } );