Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

Commit 99242fb

Browse files
ReinmarReinmar
authoredApr 17, 2020
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.
·
v19.0.0
2 parents c8d8d32 + 55ba7c1 commit 99242fb

10 files changed

+325
-48
lines changed
 

‎src/commands/removecolumncommand.js

+5-7
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010
import Command from '@ckeditor/ckeditor5-core/src/command';
1111

1212
import TableWalker from '../tablewalker';
13-
import { getSelectionAffectedTableCells } from '../utils';
13+
import { getColumnIndexes, getSelectionAffectedTableCells } from '../utils';
14+
import { findAncestor } from './utils';
1415

1516
/**
1617
* The remove column command.
@@ -32,15 +33,12 @@ export default class RemoveColumnCommand extends Command {
3233
const firstCell = selectedCells[ 0 ];
3334

3435
if ( firstCell ) {
35-
const table = firstCell.parent.parent;
36+
const table = findAncestor( 'table', firstCell );
3637
const tableColumnCount = this.editor.plugins.get( 'TableUtils' ).getColumns( table );
3738

38-
const tableMap = [ ...new TableWalker( table ) ];
39-
const columnIndexes = tableMap.filter( entry => selectedCells.includes( entry.cell ) ).map( el => el.column ).sort();
40-
const minColumnIndex = columnIndexes[ 0 ];
41-
const maxColumnIndex = columnIndexes[ columnIndexes.length - 1 ];
39+
const { first, last } = getColumnIndexes( selectedCells );
4240

43-
this.isEnabled = maxColumnIndex - minColumnIndex < ( tableColumnCount - 1 );
41+
this.isEnabled = last - first < ( tableColumnCount - 1 );
4442
} else {
4543
this.isEnabled = false;
4644
}

‎src/commands/setheadercolumncommand.js

+9-19
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,8 @@
99

1010
import Command from '@ckeditor/ckeditor5-core/src/command';
1111

12-
import {
13-
updateNumericAttribute,
14-
isHeadingColumnCell
15-
} from './utils';
16-
import { getSelectionAffectedTableCells } from '../utils';
12+
import { findAncestor, isHeadingColumnCell, updateNumericAttribute } from './utils';
13+
import { getColumnIndexes, getSelectionAffectedTableCells } from '../utils';
1714

1815
/**
1916
* The header column command.
@@ -66,26 +63,19 @@ export default class SetHeaderColumnCommand extends Command {
6663
* the `forceValue` parameter instead of the current model state.
6764
*/
6865
execute( options = {} ) {
69-
const model = this.editor.model;
70-
const tableUtils = this.editor.plugins.get( 'TableUtils' );
71-
72-
const selectedCells = getSelectionAffectedTableCells( model.document.selection );
73-
const firstCell = selectedCells[ 0 ];
74-
const lastCell = selectedCells[ selectedCells.length - 1 ];
75-
const tableRow = firstCell.parent;
76-
const table = tableRow.parent;
77-
78-
const [ selectedColumnMin, selectedColumnMax ] =
79-
// Returned cells might not necessary be in order, so make sure to sort it.
80-
[ tableUtils.getCellLocation( firstCell ).column, tableUtils.getCellLocation( lastCell ).column ].sort();
81-
8266
if ( options.forceValue === this.value ) {
8367
return;
8468
}
8569

86-
const headingColumnsToSet = this.value ? selectedColumnMin : selectedColumnMax + 1;
70+
const model = this.editor.model;
71+
const selectedCells = getSelectionAffectedTableCells( model.document.selection );
72+
const { first, last } = getColumnIndexes( selectedCells );
73+
74+
const headingColumnsToSet = this.value ? first : last + 1;
8775

8876
model.change( writer => {
77+
const table = findAncestor( 'table', selectedCells[ 0 ] );
78+
8979
updateNumericAttribute( 'headingColumns', headingColumnsToSet, table, writer, 0 );
9080
} );
9181
}

‎src/commands/setheaderrowcommand.js

+8-16
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99

1010
import Command from '@ckeditor/ckeditor5-core/src/command';
1111

12-
import { createEmptyTableCell, updateNumericAttribute } from './utils';
13-
import { getSelectionAffectedTableCells } from '../utils';
12+
import { createEmptyTableCell, findAncestor, updateNumericAttribute } from './utils';
13+
import { getRowIndexes, getSelectionAffectedTableCells } from '../utils';
1414
import TableWalker from '../tablewalker';
1515

1616
/**
@@ -62,24 +62,16 @@ export default class SetHeaderRowCommand extends Command {
6262
* the `forceValue` parameter instead of the current model state.
6363
*/
6464
execute( options = {} ) {
65-
const model = this.editor.model;
66-
67-
const selectedCells = getSelectionAffectedTableCells( model.document.selection );
68-
const firstCell = selectedCells[ 0 ];
69-
const lastCell = selectedCells[ selectedCells.length - 1 ];
70-
const table = firstCell.parent.parent;
71-
72-
const currentHeadingRows = table.getAttribute( 'headingRows' ) || 0;
73-
74-
const [ selectedRowMin, selectedRowMax ] =
75-
// Returned cells might not necessary be in order, so make sure to sort it.
76-
[ firstCell.parent.index, lastCell.parent.index ].sort();
77-
7865
if ( options.forceValue === this.value ) {
7966
return;
8067
}
68+
const model = this.editor.model;
69+
const selectedCells = getSelectionAffectedTableCells( model.document.selection );
70+
const table = findAncestor( 'table', selectedCells[ 0 ] );
8171

82-
const headingRowsToSet = this.value ? selectedRowMin : selectedRowMax + 1;
72+
const { first, last } = getRowIndexes( selectedCells );
73+
const headingRowsToSet = this.value ? first : last + 1;
74+
const currentHeadingRows = table.getAttribute( 'headingRows' ) || 0;
8375

8476
model.change( writer => {
8577
if ( headingRowsToSet ) {

‎src/utils.js

+37-6
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import { isWidget, toWidget } from '@ckeditor/ckeditor5-widget/src/utils';
1111
import { findAncestor } from './commands/utils';
12+
import TableWalker from './tablewalker';
1213

1314
/**
1415
* Converts a given {@link module:engine/view/element~Element} to a table widget:
@@ -146,16 +147,46 @@ export function getSelectionAffectedTableCells( selection ) {
146147
*
147148
* console.log( `Selected rows ${ first } to ${ last }` );
148149
*
149-
* @package {Array.<module:engine/model/element~Element>}
150+
* @param {Array.<module:engine/model/element~Element>}
150151
* @returns {Object} Returns an object with `first` and `last` table row indexes.
151152
*/
152153
export function getRowIndexes( tableCells ) {
153-
const allIndexesSorted = tableCells.map( cell => cell.parent.index ).sort();
154+
const indexes = tableCells.map( cell => cell.parent.index );
154155

155-
return {
156-
first: allIndexesSorted[ 0 ],
157-
last: allIndexesSorted[ allIndexesSorted.length - 1 ]
158-
};
156+
return getFirstLastIndexesObject( indexes );
157+
}
158+
159+
/**
160+
* Returns a helper object with `first` and `last` column index contained in given `tableCells`.
161+
*
162+
* const selectedTableCells = getSelectedTableCells( editor.model.document.selection );
163+
*
164+
* const { first, last } = getColumnIndexes( selectedTableCells );
165+
*
166+
* console.log( `Selected columns ${ first } to ${ last }` );
167+
*
168+
* @param {Array.<module:engine/model/element~Element>}
169+
* @returns {Object} Returns an object with `first` and `last` table column indexes.
170+
*/
171+
export function getColumnIndexes( selectedCells ) {
172+
const table = findAncestor( 'table', selectedCells[ 0 ] );
173+
const tableMap = [ ...new TableWalker( table ) ];
174+
175+
const indexes = tableMap
176+
.filter( entry => selectedCells.includes( entry.cell ) )
177+
.map( entry => entry.column );
178+
179+
return getFirstLastIndexesObject( indexes );
180+
}
181+
182+
// Helper method to get an object with `first` and `last` indexes from an unsorted array of indexes.
183+
function getFirstLastIndexesObject( indexes ) {
184+
const allIndexesSorted = indexes.sort( ( indexA, indexB ) => indexA - indexB );
185+
186+
const first = allIndexesSorted[ 0 ];
187+
const last = allIndexesSorted[ allIndexesSorted.length - 1 ];
188+
189+
return { first, last };
159190
}
160191

161192
function sortRanges( rangesIterator ) {

‎tests/commands/mergecellscommand.js

+29
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,35 @@ describe( 'MergeCellsCommand', () => {
208208

209209
expect( command.isEnabled ).to.be.false;
210210
} );
211+
212+
it( 'should be false if more than 10 rows selected and some are in heading section', () => {
213+
setData( model, modelTable( [
214+
[ '0' ],
215+
[ '1' ],
216+
[ '2' ],
217+
[ '3' ],
218+
[ '4' ],
219+
[ '5' ],
220+
[ '6' ],
221+
[ '7' ],
222+
[ '8' ],
223+
[ '9' ],
224+
[ '10' ],
225+
[ '11' ],
226+
[ '12' ],
227+
[ '13' ],
228+
[ '14' ]
229+
], { headingRows: 10 } ) );
230+
231+
const tableSelection = editor.plugins.get( TableSelection );
232+
const modelRoot = model.document.getRoot();
233+
tableSelection._setCellSelection(
234+
modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
235+
modelRoot.getNodeByPath( [ 0, 12, 0 ] )
236+
);
237+
238+
expect( command.isEnabled ).to.be.false;
239+
} );
211240
} );
212241

213242
describe( 'execute()', () => {

‎tests/commands/removecolumncommand.js

+15
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,21 @@ describe( 'RemoveColumnCommand', () => {
8686
expect( command.isEnabled ).to.be.false;
8787
} );
8888

89+
it( 'should be false if all columns are selected - table with more than 10 columns (array sort bug)', () => {
90+
setData( model, modelTable( [
91+
[ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12' ]
92+
] ) );
93+
94+
const tableSelection = editor.plugins.get( TableSelection );
95+
const modelRoot = model.document.getRoot();
96+
tableSelection._setCellSelection(
97+
modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
98+
modelRoot.getNodeByPath( [ 0, 0, 12 ] )
99+
);
100+
101+
expect( command.isEnabled ).to.be.false;
102+
} );
103+
89104
it( 'should be false if selection is outside a table', () => {
90105
setData( model, '<paragraph>11[]</paragraph>' );
91106

‎tests/commands/removerowcommand.js

+62
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,33 @@ describe( 'RemoveRowCommand', () => {
105105

106106
expect( command.isEnabled ).to.be.false;
107107
} );
108+
109+
it( 'should be false if all the rows are selected - table with more than 10 rows (array sort bug)', () => {
110+
setData( model, modelTable( [
111+
[ '0' ],
112+
[ '1' ],
113+
[ '2' ],
114+
[ '3' ],
115+
[ '4' ],
116+
[ '5' ],
117+
[ '6' ],
118+
[ '7' ],
119+
[ '8' ],
120+
[ '9' ],
121+
[ '10' ],
122+
[ '11' ],
123+
[ '12' ]
124+
] ) );
125+
126+
const tableSelection = editor.plugins.get( TableSelection );
127+
const modelRoot = model.document.getRoot();
128+
tableSelection._setCellSelection(
129+
modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
130+
modelRoot.getNodeByPath( [ 0, 12, 0 ] )
131+
);
132+
133+
expect( command.isEnabled ).to.be.false;
134+
} );
108135
} );
109136

110137
describe( 'execute()', () => {
@@ -339,6 +366,41 @@ describe( 'RemoveRowCommand', () => {
339366

340367
expect( createdBatches.size ).to.equal( 1 );
341368
} );
369+
370+
it( 'should properly remove more than 10 rows selected (array sort bug)', () => {
371+
setData( model, modelTable( [
372+
[ '0' ],
373+
[ '1' ],
374+
[ '2' ],
375+
[ '3' ],
376+
[ '4' ],
377+
[ '5' ],
378+
[ '6' ],
379+
[ '7' ],
380+
[ '8' ],
381+
[ '9' ],
382+
[ '10' ],
383+
[ '11' ],
384+
[ '12' ],
385+
[ '13' ],
386+
[ '14' ]
387+
] ) );
388+
389+
const tableSelection = editor.plugins.get( TableSelection );
390+
const modelRoot = model.document.getRoot();
391+
tableSelection._setCellSelection(
392+
modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
393+
modelRoot.getNodeByPath( [ 0, 12, 0 ] )
394+
);
395+
396+
command.execute();
397+
398+
assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
399+
[ '0' ],
400+
[ '13' ],
401+
[ '14' ]
402+
] ) );
403+
} );
342404
} );
343405

344406
describe( 'with entire row selected', () => {

‎tests/commands/selectrowcommand.js

+47
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,53 @@ describe( 'SelectRowCommand', () => {
422422
[ 0, 0 ]
423423
] );
424424
} );
425+
426+
it( 'should properly select more than 10 rows selected (array sort bug)', () => {
427+
setData( model, modelTable( [
428+
[ '0', 'x' ],
429+
[ '1', 'x' ],
430+
[ '2', 'x' ],
431+
[ '3', 'x' ],
432+
[ '4', 'x' ],
433+
[ '5', 'x' ],
434+
[ '6', 'x' ],
435+
[ '7', 'x' ],
436+
[ '8', 'x' ],
437+
[ '9', 'x' ],
438+
[ '10', 'x' ],
439+
[ '11', 'x' ],
440+
[ '12', 'x' ],
441+
[ '13', 'x' ],
442+
[ '14', 'x' ]
443+
] ) );
444+
445+
const tableSelection = editor.plugins.get( TableSelection );
446+
const modelRoot = model.document.getRoot();
447+
tableSelection._setCellSelection(
448+
modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
449+
modelRoot.getNodeByPath( [ 0, 12, 0 ] )
450+
);
451+
452+
command.execute();
453+
454+
assertSelectedCells( model, [
455+
[ 0, 0 ], // '0'
456+
[ 1, 1 ], // '1'
457+
[ 1, 1 ], // '2'
458+
[ 1, 1 ], // '3'
459+
[ 1, 1 ], // '4'
460+
[ 1, 1 ], // '5'
461+
[ 1, 1 ], // '6'
462+
[ 1, 1 ], // '7'
463+
[ 1, 1 ], // '8'
464+
[ 1, 1 ], // '9'
465+
[ 1, 1 ], // '10'
466+
[ 1, 1 ], // '11'
467+
[ 1, 1 ], // '12'
468+
[ 0, 0 ], // '13'
469+
[ 0, 0 ] // '14
470+
] );
471+
} );
425472
} );
426473

427474
describe( 'with entire row selected', () => {

‎tests/commands/setheadercolumncommand.js

+19
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,25 @@ describe( 'SetHeaderColumnCommand', () => {
364364
[ 0, 1, 1, 0 ]
365365
] );
366366
} );
367+
368+
it( 'should set it correctly in table with more than 10 columns (array sort bug)', () => {
369+
setData( model, modelTable( [
370+
[ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14' ]
371+
] ) );
372+
373+
const tableSelection = editor.plugins.get( TableSelection );
374+
const modelRoot = model.document.getRoot();
375+
tableSelection._setCellSelection(
376+
modelRoot.getNodeByPath( [ 0, 0, 2 ] ),
377+
modelRoot.getNodeByPath( [ 0, 0, 13 ] )
378+
);
379+
380+
command.execute();
381+
382+
assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
383+
[ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14' ]
384+
], { headingColumns: 14 } ) );
385+
} );
367386
} );
368387
} );
369388

‎tests/commands/setheaderrowcommand.js

+94
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,100 @@ describe( 'SetHeaderRowCommand', () => {
346346
] );
347347
} );
348348

349+
it( 'should set it correctly in table with more than 10 columns (array sort bug)', () => {
350+
setData( model, modelTable( [
351+
[ '0', 'x' ],
352+
[ '1', 'x' ],
353+
[ '2', 'x' ],
354+
[ '3', 'x' ],
355+
[ '4', 'x' ],
356+
[ '5', 'x' ],
357+
[ '6', 'x' ],
358+
[ '7', 'x' ],
359+
[ '8', 'x' ],
360+
[ '9', 'x' ],
361+
[ '10', 'x' ],
362+
[ '11', 'x' ],
363+
[ '12', 'x' ],
364+
[ '13', 'x' ],
365+
[ '14', 'x' ]
366+
] ) );
367+
368+
const tableSelection = editor.plugins.get( TableSelection );
369+
const modelRoot = model.document.getRoot();
370+
tableSelection._setCellSelection(
371+
modelRoot.getNodeByPath( [ 0, 2, 0 ] ),
372+
modelRoot.getNodeByPath( [ 0, 13, 0 ] )
373+
);
374+
375+
command.execute();
376+
377+
assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
378+
[ '0', 'x' ],
379+
[ '1', 'x' ],
380+
[ '2', 'x' ],
381+
[ '3', 'x' ],
382+
[ '4', 'x' ],
383+
[ '5', 'x' ],
384+
[ '6', 'x' ],
385+
[ '7', 'x' ],
386+
[ '8', 'x' ],
387+
[ '9', 'x' ],
388+
[ '10', 'x' ],
389+
[ '11', 'x' ],
390+
[ '12', 'x' ],
391+
[ '13', 'x' ],
392+
[ '14', 'x' ]
393+
], { headingRows: 14 } ) );
394+
} );
395+
396+
it( 'should set it correctly in table with more than 10 columns (array sort bug, reversed selection)', () => {
397+
setData( model, modelTable( [
398+
[ '0', 'x' ],
399+
[ '1', 'x' ],
400+
[ '2', 'x' ],
401+
[ '3', 'x' ],
402+
[ '4', 'x' ],
403+
[ '5', 'x' ],
404+
[ '6', 'x' ],
405+
[ '7', 'x' ],
406+
[ '8', 'x' ],
407+
[ '9', 'x' ],
408+
[ '10', 'x' ],
409+
[ '11', 'x' ],
410+
[ '12', 'x' ],
411+
[ '13', 'x' ],
412+
[ '14', 'x' ]
413+
] ) );
414+
415+
const tableSelection = editor.plugins.get( TableSelection );
416+
const modelRoot = model.document.getRoot();
417+
tableSelection._setCellSelection(
418+
modelRoot.getNodeByPath( [ 0, 13, 0 ] ),
419+
modelRoot.getNodeByPath( [ 0, 2, 1 ] )
420+
);
421+
422+
command.execute();
423+
424+
assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
425+
[ '0', 'x' ],
426+
[ '1', 'x' ],
427+
[ '2', 'x' ],
428+
[ '3', 'x' ],
429+
[ '4', 'x' ],
430+
[ '5', 'x' ],
431+
[ '6', 'x' ],
432+
[ '7', 'x' ],
433+
[ '8', 'x' ],
434+
[ '9', 'x' ],
435+
[ '10', 'x' ],
436+
[ '11', 'x' ],
437+
[ '12', 'x' ],
438+
[ '13', 'x' ],
439+
[ '14', 'x' ]
440+
], { headingRows: 14 } ) );
441+
} );
442+
349443
it( 'should remove header rows in case of multiple cell selection', () => {
350444
setData( model, modelTable( [
351445
[ '00' ],

0 commit comments

Comments
 (0)
This repository has been archived.