Merge pull request #305 from ckeditor/i/6569

Fix: Resolved various issues with handling bigger tables (due to sorting by indexes issues). Closes ckeditor/ckeditor5#6569. Closes ckeditor/ckeditor5#6544.
ckeditor · Apr 17, 2020 · 99242fb · 99242fb
2 parents c8d8d32 + 55ba7c1
commit 99242fb
Show file tree

Hide file tree

Showing 10 changed files with 325 additions and 48 deletions.
diff --git a/src/commands/removecolumncommand.js b/src/commands/removecolumncommand.js
@@ -10,7 +10,8 @@
 import Command from '@ckeditor/ckeditor5-core/src/command';
 
 import TableWalker from '../tablewalker';
-import { getSelectionAffectedTableCells } from '../utils';
+import { getColumnIndexes, getSelectionAffectedTableCells } from '../utils';
+import { findAncestor } from './utils';
 
 /**
  * The remove column command.
@@ -32,15 +33,12 @@ export default class RemoveColumnCommand extends Command {
 		const firstCell = selectedCells[ 0 ];
 
 		if ( firstCell ) {
-			const table = firstCell.parent.parent;
+			const table = findAncestor( 'table', firstCell );
 			const tableColumnCount = this.editor.plugins.get( 'TableUtils' ).getColumns( table );
 
-			const tableMap = [ ...new TableWalker( table ) ];
-			const columnIndexes = tableMap.filter( entry => selectedCells.includes( entry.cell ) ).map( el => el.column ).sort();
-			const minColumnIndex = columnIndexes[ 0 ];
-			const maxColumnIndex = columnIndexes[ columnIndexes.length - 1 ];
+			const { first, last } = getColumnIndexes( selectedCells );
 
-			this.isEnabled = maxColumnIndex - minColumnIndex < ( tableColumnCount - 1 );
+			this.isEnabled = last - first < ( tableColumnCount - 1 );
 		} else {
 			this.isEnabled = false;
 		}

diff --git a/src/commands/setheadercolumncommand.js b/src/commands/setheadercolumncommand.js
@@ -9,11 +9,8 @@
 
 import Command from '@ckeditor/ckeditor5-core/src/command';
 
-import {
-	updateNumericAttribute,
-	isHeadingColumnCell
-} from './utils';
-import { getSelectionAffectedTableCells } from '../utils';
+import { findAncestor, isHeadingColumnCell, updateNumericAttribute } from './utils';
+import { getColumnIndexes, getSelectionAffectedTableCells } from '../utils';
 
 /**
  * The header column command.
@@ -66,26 +63,19 @@ export default class SetHeaderColumnCommand extends Command {
 	 * the `forceValue` parameter instead of the current model state.
 	 */
 	execute( options = {} ) {
-		const model = this.editor.model;
-		const tableUtils = this.editor.plugins.get( 'TableUtils' );
-
-		const selectedCells = getSelectionAffectedTableCells( model.document.selection );
-		const firstCell = selectedCells[ 0 ];
-		const lastCell = selectedCells[ selectedCells.length - 1 ];
-		const tableRow = firstCell.parent;
-		const table = tableRow.parent;
-
-		const [ selectedColumnMin, selectedColumnMax ] =
-			// Returned cells might not necessary be in order, so make sure to sort it.
-			[ tableUtils.getCellLocation( firstCell ).column, tableUtils.getCellLocation( lastCell ).column ].sort();
-
 		if ( options.forceValue === this.value ) {
 			return;
 		}
 
-		const headingColumnsToSet = this.value ? selectedColumnMin : selectedColumnMax + 1;
+		const model = this.editor.model;
+		const selectedCells = getSelectionAffectedTableCells( model.document.selection );
+		const { first, last } = getColumnIndexes( selectedCells );
+
+		const headingColumnsToSet = this.value ? first : last + 1;
 
 		model.change( writer => {
+			const table = findAncestor( 'table', selectedCells[ 0 ] );
+
 			updateNumericAttribute( 'headingColumns', headingColumnsToSet, table, writer, 0 );
 		} );
 	}

diff --git a/src/commands/setheaderrowcommand.js b/src/commands/setheaderrowcommand.js
@@ -9,8 +9,8 @@
 
 import Command from '@ckeditor/ckeditor5-core/src/command';
 
-import { createEmptyTableCell, updateNumericAttribute } from './utils';
-import { getSelectionAffectedTableCells } from '../utils';
+import { createEmptyTableCell, findAncestor, updateNumericAttribute } from './utils';
+import { getRowIndexes, getSelectionAffectedTableCells } from '../utils';
 import TableWalker from '../tablewalker';
 
 /**
@@ -62,24 +62,16 @@ export default class SetHeaderRowCommand extends Command {
 	 * the `forceValue` parameter instead of the current model state.
 	 */
 	execute( options = {} ) {
-		const model = this.editor.model;
-
-		const selectedCells = getSelectionAffectedTableCells( model.document.selection );
-		const firstCell = selectedCells[ 0 ];
-		const lastCell = selectedCells[ selectedCells.length - 1 ];
-		const table = firstCell.parent.parent;
-
-		const currentHeadingRows = table.getAttribute( 'headingRows' ) || 0;
-
-		const [ selectedRowMin, selectedRowMax ] =
-			// Returned cells might not necessary be in order, so make sure to sort it.
-			[ firstCell.parent.index, lastCell.parent.index ].sort();
-
 		if ( options.forceValue === this.value ) {
 			return;
 		}
+		const model = this.editor.model;
+		const selectedCells = getSelectionAffectedTableCells( model.document.selection );
+		const table = findAncestor( 'table', selectedCells[ 0 ] );
 
-		const headingRowsToSet = this.value ? selectedRowMin : selectedRowMax + 1;
+		const { first, last } = getRowIndexes( selectedCells );
+		const headingRowsToSet = this.value ? first : last + 1;
+		const currentHeadingRows = table.getAttribute( 'headingRows' ) || 0;
 
 		model.change( writer => {
 			if ( headingRowsToSet ) {

diff --git a/src/utils.js b/src/utils.js
@@ -9,6 +9,7 @@
 
 import { isWidget, toWidget } from '@ckeditor/ckeditor5-widget/src/utils';
 import { findAncestor } from './commands/utils';
+import TableWalker from './tablewalker';
 
 /**
  * Converts a given {@link module:engine/view/element~Element} to a table widget:
@@ -146,16 +147,46 @@ export function getSelectionAffectedTableCells( selection ) {
  *
  *		console.log( `Selected rows ${ first } to ${ last }` );
  *
- * @package {Array.<module:engine/model/element~Element>}
+ * @param {Array.<module:engine/model/element~Element>}
  * @returns {Object} Returns an object with `first` and `last` table row indexes.
  */
 export function getRowIndexes( tableCells ) {
-	const allIndexesSorted = tableCells.map( cell => cell.parent.index ).sort();
+	const indexes = tableCells.map( cell => cell.parent.index );
 
-	return {
-		first: allIndexesSorted[ 0 ],
-		last: allIndexesSorted[ allIndexesSorted.length - 1 ]
-	};
+	return getFirstLastIndexesObject( indexes );
+}
+
+/**
+ * Returns a helper object with `first` and `last` column index contained in given `tableCells`.
+ *
+ *		const selectedTableCells = getSelectedTableCells( editor.model.document.selection );
+ *
+ *		const { first, last } = getColumnIndexes( selectedTableCells );
+ *
+ *		console.log( `Selected columns ${ first } to ${ last }` );
+ *
+ * @param {Array.<module:engine/model/element~Element>}
+ * @returns {Object} Returns an object with `first` and `last` table column indexes.
+ */
+export function getColumnIndexes( selectedCells ) {
+	const table = findAncestor( 'table', selectedCells[ 0 ] );
+	const tableMap = [ ...new TableWalker( table ) ];
+
+	const indexes = tableMap
+		.filter( entry => selectedCells.includes( entry.cell ) )
+		.map( entry => entry.column );
+
+	return getFirstLastIndexesObject( indexes );
+}
+
+// Helper method to get an object with `first` and `last` indexes from an unsorted array of indexes.
+function getFirstLastIndexesObject( indexes ) {
+	const allIndexesSorted = indexes.sort( ( indexA, indexB ) => indexA - indexB );
+
+	const first = allIndexesSorted[ 0 ];
+	const last = allIndexesSorted[ allIndexesSorted.length - 1 ];
+
+	return { first, last };
 }
 
 function sortRanges( rangesIterator ) {

diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js
@@ -208,6 +208,35 @@ describe( 'MergeCellsCommand', () => {
 
 			expect( command.isEnabled ).to.be.false;
 		} );
+
+		it( 'should be false if more than 10 rows selected and some are in heading section', () => {
+			setData( model, modelTable( [
+				[ '0' ],
+				[ '1' ],
+				[ '2' ],
+				[ '3' ],
+				[ '4' ],
+				[ '5' ],
+				[ '6' ],
+				[ '7' ],
+				[ '8' ],
+				[ '9' ],
+				[ '10' ],
+				[ '11' ],
+				[ '12' ],
+				[ '13' ],
+				[ '14' ]
+			], { headingRows: 10 } ) );
+
+			const tableSelection = editor.plugins.get( TableSelection );
+			const modelRoot = model.document.getRoot();
+			tableSelection._setCellSelection(
+				modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
+				modelRoot.getNodeByPath( [ 0, 12, 0 ] )
+			);
+
+			expect( command.isEnabled ).to.be.false;
+		} );
 	} );
 
 	describe( 'execute()', () => {

diff --git a/tests/commands/removecolumncommand.js b/tests/commands/removecolumncommand.js
@@ -86,6 +86,21 @@ describe( 'RemoveColumnCommand', () => {
 			expect( command.isEnabled ).to.be.false;
 		} );
 
+		it( 'should be false if all columns are selected - table with more than 10 columns (array sort bug)', () => {
+			setData( model, modelTable( [
+				[ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12' ]
+			] ) );
+
+			const tableSelection = editor.plugins.get( TableSelection );
+			const modelRoot = model.document.getRoot();
+			tableSelection._setCellSelection(
+				modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
+				modelRoot.getNodeByPath( [ 0, 0, 12 ] )
+			);
+
+			expect( command.isEnabled ).to.be.false;
+		} );
+
 		it( 'should be false if selection is outside a table', () => {
 			setData( model, '<paragraph>11[]</paragraph>' );
 

diff --git a/tests/commands/removerowcommand.js b/tests/commands/removerowcommand.js
@@ -105,6 +105,33 @@ describe( 'RemoveRowCommand', () => {
 
 			expect( command.isEnabled ).to.be.false;
 		} );
+
+		it( 'should be false if all the rows are selected - table with more than 10 rows (array sort bug)', () => {
+			setData( model, modelTable( [
+				[ '0' ],
+				[ '1' ],
+				[ '2' ],
+				[ '3' ],
+				[ '4' ],
+				[ '5' ],
+				[ '6' ],
+				[ '7' ],
+				[ '8' ],
+				[ '9' ],
+				[ '10' ],
+				[ '11' ],
+				[ '12' ]
+			] ) );
+
+			const tableSelection = editor.plugins.get( TableSelection );
+			const modelRoot = model.document.getRoot();
+			tableSelection._setCellSelection(
+				modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
+				modelRoot.getNodeByPath( [ 0, 12, 0 ] )
+			);
+
+			expect( command.isEnabled ).to.be.false;
+		} );
 	} );
 
 	describe( 'execute()', () => {
@@ -339,6 +366,41 @@ describe( 'RemoveRowCommand', () => {
 
 				expect( createdBatches.size ).to.equal( 1 );
 			} );
+
+			it( 'should properly remove more than 10 rows selected (array sort bug)', () => {
+				setData( model, modelTable( [
+					[ '0' ],
+					[ '1' ],
+					[ '2' ],
+					[ '3' ],
+					[ '4' ],
+					[ '5' ],
+					[ '6' ],
+					[ '7' ],
+					[ '8' ],
+					[ '9' ],
+					[ '10' ],
+					[ '11' ],
+					[ '12' ],
+					[ '13' ],
+					[ '14' ]
+				] ) );
+
+				const tableSelection = editor.plugins.get( TableSelection );
+				const modelRoot = model.document.getRoot();
+				tableSelection._setCellSelection(
+					modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
+					modelRoot.getNodeByPath( [ 0, 12, 0 ] )
+				);
+
+				command.execute();
+
+				assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
+					[ '0' ],
+					[ '13' ],
+					[ '14' ]
+				] ) );
+			} );
 		} );
 
 		describe( 'with entire row selected', () => {

diff --git a/tests/commands/selectrowcommand.js b/tests/commands/selectrowcommand.js
@@ -422,6 +422,53 @@ describe( 'SelectRowCommand', () => {
 					[ 0, 0 ]
 				] );
 			} );
+
+			it( 'should properly select more than 10 rows selected (array sort bug)', () => {
+				setData( model, modelTable( [
+					[ '0', 'x' ],
+					[ '1', 'x' ],
+					[ '2', 'x' ],
+					[ '3', 'x' ],
+					[ '4', 'x' ],
+					[ '5', 'x' ],
+					[ '6', 'x' ],
+					[ '7', 'x' ],
+					[ '8', 'x' ],
+					[ '9', 'x' ],
+					[ '10', 'x' ],
+					[ '11', 'x' ],
+					[ '12', 'x' ],
+					[ '13', 'x' ],
+					[ '14', 'x' ]
+				] ) );
+
+				const tableSelection = editor.plugins.get( TableSelection );
+				const modelRoot = model.document.getRoot();
+				tableSelection._setCellSelection(
+					modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
+					modelRoot.getNodeByPath( [ 0, 12, 0 ] )
+				);
+
+				command.execute();
+
+				assertSelectedCells( model, [
+					[ 0, 0 ], // '0'
+					[ 1, 1 ], // '1'
+					[ 1, 1 ], // '2'
+					[ 1, 1 ], // '3'
+					[ 1, 1 ], // '4'
+					[ 1, 1 ], // '5'
+					[ 1, 1 ], // '6'
+					[ 1, 1 ], // '7'
+					[ 1, 1 ], // '8'
+					[ 1, 1 ], // '9'
+					[ 1, 1 ], // '10'
+					[ 1, 1 ], // '11'
+					[ 1, 1 ], // '12'
+					[ 0, 0 ], // '13'
+					[ 0, 0 ] //  '14
+				] );
+			} );
 		} );
 
 		describe( 'with entire row selected', () => {

diff --git a/tests/commands/setheadercolumncommand.js b/tests/commands/setheadercolumncommand.js
@@ -364,6 +364,25 @@ describe( 'SetHeaderColumnCommand', () => {
 						[ 0, 1, 1, 0 ]
 					] );
 				} );
+
+				it( 'should set it correctly in table with more than 10 columns (array sort bug)', () => {
+					setData( model, modelTable( [
+						[ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14' ]
+					] ) );
+
+					const tableSelection = editor.plugins.get( TableSelection );
+					const modelRoot = model.document.getRoot();
+					tableSelection._setCellSelection(
+						modelRoot.getNodeByPath( [ 0, 0, 2 ] ),
+						modelRoot.getNodeByPath( [ 0, 0, 13 ] )
+					);
+
+					command.execute();
+
+					assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
+						[ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14' ]
+					], { headingColumns: 14 } ) );
+				} );
 			} );
 		} );