From 555b22a6eec54431396487b54cc0b5750d4851f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 3 Apr 2020 14:15:46 +0200 Subject: [PATCH 01/11] Fix wrong assertions in remove column command tests. --- .../tests/commands/removecolumncommand.js | 35 +++++++++++++------ 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/packages/ckeditor5-table/tests/commands/removecolumncommand.js b/packages/ckeditor5-table/tests/commands/removecolumncommand.js index ad76d108292..08e5f677c6c 100644 --- a/packages/ckeditor5-table/tests/commands/removecolumncommand.js +++ b/packages/ckeditor5-table/tests/commands/removecolumncommand.js @@ -418,7 +418,7 @@ describe( 'RemoveColumnCommand', () => { ] ) ); } ); - it( 'should work property if the rowspan is in the first column (#1)', () => { + it( 'should work property if the rowspan is in the first column (the other cell in row is selected)', () => { setData( model, modelTable( [ [ { rowspan: 2, contents: '00' }, '[]01' ], [ '10' ] @@ -427,11 +427,11 @@ describe( 'RemoveColumnCommand', () => { command.execute(); assertEqualMarkup( getData( model ), modelTable( [ - [ { rowspan: 2, contents: '[]00' } ] + [ '[]00' ] ] ) ); } ); - it( 'should work property if the rowspan is in the first column (#2)', () => { + it( 'should work property if the rowspan is in the first column (the cell in row below is selected)', () => { setData( model, modelTable( [ [ { rowspan: 2, contents: '00' }, '01' ], [ '[]10' ] @@ -440,11 +440,11 @@ describe( 'RemoveColumnCommand', () => { command.execute(); assertEqualMarkup( getData( model ), modelTable( [ - [ { rowspan: 2, contents: '[]00' } ] + [ '[]00' ] ] ) ); } ); - it( 'should work property if the rowspan is in the first column (#3)', () => { + it( 'should work property if the rowspan is in the first column (the cell with rowspan is selected)', () => { setData( model, modelTable( [ [ { rowspan: 2, contents: '00[]' }, '01' ], [ '10' ] @@ -458,7 +458,7 @@ describe( 'RemoveColumnCommand', () => { ] ) ); } ); - it( 'should work property if the rowspan is in the last column (#1)', () => { + it( 'should work property if the rowspan is in the last column (the other cell in row is selected)', () => { setData( model, modelTable( [ [ '[]00', { rowspan: 2, contents: '01' } ], [ '10' ] @@ -467,11 +467,11 @@ describe( 'RemoveColumnCommand', () => { command.execute(); assertEqualMarkup( getData( model ), modelTable( [ - [ { rowspan: 2, contents: '[]01' } ] + [ '[]01' ] ] ) ); } ); - it( 'should work property if the rowspan is in the last column (#2)', () => { + it( 'should work property if the rowspan is in the last column (the cell in row below is selected)', () => { setData( model, modelTable( [ [ '00', { rowspan: 2, contents: '01' } ], [ '[]10' ] @@ -480,11 +480,11 @@ describe( 'RemoveColumnCommand', () => { command.execute(); assertEqualMarkup( getData( model ), modelTable( [ - [ { rowspan: 2, contents: '[]01' } ] + [ '[]01' ] ] ) ); } ); - it( 'should work property if the rowspan is in the last column (#3)', () => { + it( 'should work property if the rowspan is in the last column (the cell with rowspan is selected)', () => { setData( model, modelTable( [ [ '00', { rowspan: 2, contents: '[]01' } ], [ '10' ] @@ -497,5 +497,20 @@ describe( 'RemoveColumnCommand', () => { [ '10' ] ] ) ); } ); + + it( 'should remove column if removing row with one column - other columns are spanned', () => { + setData( model, modelTable( [ + [ '[]00', { rowspan: 2, contents: '01' }, { rowspan: 2, contents: '02' } ], + [ '10' ], + [ '20', '21', '22' ] + ] ) ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ '[]01', '02' ], + [ '21', '22' ] + ] ) ); + } ); } ); } ); From d7542b03765d5f777829e2a9f8bf43b67d028147 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 3 Apr 2020 14:25:48 +0200 Subject: [PATCH 02/11] Use removeRow() method from TableUtils. --- .../ckeditor5-table/src/commands/removecolumncommand.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/removecolumncommand.js b/packages/ckeditor5-table/src/commands/removecolumncommand.js index d21aa7a3cdf..1494284f4e0 100644 --- a/packages/ckeditor5-table/src/commands/removecolumncommand.js +++ b/packages/ckeditor5-table/src/commands/removecolumncommand.js @@ -92,7 +92,9 @@ export default class RemoveColumnCommand extends Command { * @param {module:engine/model/writer~Writer} writer */ _removeColumn( removedColumnIndex, table, writer ) { - for ( const { cell, column, colspan } of new TableWalker( table ) ) { + const tableUtils = this.editor.plugins.get( 'TableUtils' ); + + for ( const { cell, column, colspan } of [ ...new TableWalker( table ) ] ) { // If colspaned cell overlaps removed column decrease its span. if ( column <= removedColumnIndex && colspan > 1 && column + colspan > removedColumnIndex ) { updateNumericAttribute( 'colspan', colspan - 1, cell, writer ); @@ -105,7 +107,7 @@ export default class RemoveColumnCommand extends Command { // If the cell was the last one in the row, get rid of the entire row. // https://github.com/ckeditor/ckeditor5/issues/6429 if ( !cellRow.childCount ) { - writer.remove( cellRow ); + tableUtils.removeRow( cellRow.index, table, writer ); } } } From 3cbaaa312c82746b20eb3b34ca7b2cd722a974b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 7 Apr 2020 10:27:48 +0200 Subject: [PATCH 03/11] Add test cases for TableUtils#removeColumn(). --- packages/ckeditor5-table/tests/tableutils.js | 191 +++++++++++++++++++ 1 file changed, 191 insertions(+) diff --git a/packages/ckeditor5-table/tests/tableutils.js b/packages/ckeditor5-table/tests/tableutils.js index 8412907bc02..5cd8a7d1c88 100644 --- a/packages/ckeditor5-table/tests/tableutils.js +++ b/packages/ckeditor5-table/tests/tableutils.js @@ -960,4 +960,195 @@ describe( 'TableUtils', () => { } ); } ); } ); + + describe( 'removeColumn()', () => { + describe( 'single row', () => { + it( 'should remove a given column', () => { + setData( model, modelTable( [ + [ '00', '01', '02' ], + [ '10', '11', '12' ], + [ '20', '21', '22' ] + ] ) ); + + tableUtils.removeColumn( { at: 1 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '02' ], + [ '10', '12' ], + [ '20', '22' ] + ] ) ); + } ); + + it( 'should remove a given column from a table start', () => { + setData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ] ) ); + + tableUtils.removeColumn( { at: 0 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '01' ], + [ '11' ], + [ '21' ] + ] ) ); + } ); + + it( 'should change heading columns if removing a heading column', () => { + setData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ], { headingColumns: 2 } ) ); + + tableUtils.removeColumn( { at: 0 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '01' ], + [ '11' ], + [ '21' ] + ], { headingColumns: 1 } ) ); + } ); + + it( 'should decrease colspan of table cells from previous column', () => { + setData( model, modelTable( [ + [ { colspan: 4, contents: '00' }, '03' ], + [ { colspan: 3, contents: '10' }, '13' ], + [ { colspan: 2, contents: '20' }, '22', '23' ], + [ '30', { colspan: 2, contents: '31' }, '33' ], + [ '40', '41', '42', '43' ] + ] ) ); + + tableUtils.removeColumn( { at: 2 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ { colspan: 3, contents: '00' }, '03' ], + [ { colspan: 2, contents: '10' }, '13' ], + [ { colspan: 2, contents: '20' }, '23' ], + [ '30', '31', '33' ], + [ '40', '41', '43' ] + + ] ) ); + } ); + + it( 'should decrease colspan of cells that are on removed column', () => { + setData( model, modelTable( [ + [ { colspan: 3, contents: '00' }, '03' ], + [ { colspan: 2, contents: '10' }, '13' ], + [ '20', '21', '22', '23' ] + ] ) ); + + tableUtils.removeColumn( { at: 0 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ { colspan: 2, contents: '00' }, '03' ], + [ '10', '13' ], + [ '21', '22', '23' ] + ] ) ); + } ); + + it( 'should move focus to previous column of removed cell if in last column', () => { + setData( model, modelTable( [ + [ '00', '01', '02' ], + [ '10', '11', '12' ], + [ '20', '21', '22' ] + ] ) ); + + tableUtils.removeColumn( { at: 2 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ] ) ); + } ); + } ); + + describe( 'multiple columns', () => { + it( 'should properly remove two first columns', () => { + setData( model, modelTable( [ + [ '00', '01', '02' ], + [ '10', '11', '12' ], + [ '20', '21', '22' ], + [ '30', '31', '32' ] + ] ) ); + + tableUtils.removeColumn( { at: 0, columns: 2 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '02' ], + [ '12' ], + [ '22' ], + [ '32' ] + ] ) ); + } ); + + it( 'should properly remove two middle columns', () => { + setData( model, modelTable( [ + [ '00', '01', '02', '03' ], + [ '10', '11', '12', '13' ], + [ '20', '21', '22', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + tableUtils.removeColumn( { at: 1, columns: 2 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '03' ], + [ '10', '13' ], + [ '20', '23' ], + [ '30', '33' ] + ] ) ); + } ); + + it( 'should properly remove two last columns', () => { + setData( model, modelTable( [ + [ '00', '01', '02' ], + [ '10', '11', '12' ], + [ '20', '21', '22' ], + [ '30', '31', '32' ] + ] ) ); + + tableUtils.removeColumn( { at: 1, columns: 2 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '00' ], + [ '10' ], + [ '20' ], + [ '30' ] + ] ) ); + } ); + + it( 'should properly remove multiple heading columns', () => { + setData( model, modelTable( [ + [ '00', '01', '02', '03', '04' ], + [ '10', '11', '12', '13', '14' ] + ], { headingColumns: 3 } ) ); + + tableUtils.removeColumn( { at: 1, columns: 3 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '04' ], + [ '10', '14' ] + ], { headingColumns: 1 } ) ); + } ); + + it( 'should properly calculate truncated colspans', () => { + setData( model, modelTable( [ + [ { contents: '00', colspan: 3 } ], + [ '10', '11', '12' ], + [ '20', '21', '22' ] + ] ) ); + + tableUtils.removeColumn( { at: 0, columns: 2 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '00' ], + [ '12' ], + [ '22' ] + ] ) ); + } ); + } ); + } ); } ); From 97bc2cb9ebd35d2be965989ade10a6c839082787 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 7 Apr 2020 11:58:59 +0200 Subject: [PATCH 04/11] Update remove column test cases. --- packages/ckeditor5-table/tests/tableutils.js | 89 ++++++++++++++++---- 1 file changed, 71 insertions(+), 18 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableutils.js b/packages/ckeditor5-table/tests/tableutils.js index 5cd8a7d1c88..9ea9400f3ea 100644 --- a/packages/ckeditor5-table/tests/tableutils.js +++ b/packages/ckeditor5-table/tests/tableutils.js @@ -961,7 +961,7 @@ describe( 'TableUtils', () => { } ); } ); - describe( 'removeColumn()', () => { + describe( 'removeColumns()', () => { describe( 'single row', () => { it( 'should remove a given column', () => { setData( model, modelTable( [ @@ -970,7 +970,7 @@ describe( 'TableUtils', () => { [ '20', '21', '22' ] ] ) ); - tableUtils.removeColumn( { at: 1 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 1 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ '00', '02' ], @@ -986,7 +986,7 @@ describe( 'TableUtils', () => { [ '20', '21' ] ] ) ); - tableUtils.removeColumn( { at: 0 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 0 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ '01' ], @@ -1002,7 +1002,7 @@ describe( 'TableUtils', () => { [ '20', '21' ] ], { headingColumns: 2 } ) ); - tableUtils.removeColumn( { at: 0 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 0 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ '01' ], @@ -1020,7 +1020,7 @@ describe( 'TableUtils', () => { [ '40', '41', '42', '43' ] ] ) ); - tableUtils.removeColumn( { at: 2 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 2 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ { colspan: 3, contents: '00' }, '03' ], @@ -1039,7 +1039,7 @@ describe( 'TableUtils', () => { [ '20', '21', '22', '23' ] ] ) ); - tableUtils.removeColumn( { at: 0 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 0 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ { colspan: 2, contents: '00' }, '03' ], @@ -1048,19 +1048,72 @@ describe( 'TableUtils', () => { ] ) ); } ); - it( 'should move focus to previous column of removed cell if in last column', () => { + it( 'should remove column with rowspan (first column)', () => { setData( model, modelTable( [ - [ '00', '01', '02' ], - [ '10', '11', '12' ], + [ { rowspan: 2, contents: '00' }, '01' ], + [ '10' ] + ] ) ); + + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 0 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '01' ], + [ '10' ] + ] ) ); + } ); + + it( 'should remove column with rowspan (last column)', () => { + setData( model, modelTable( [ + [ '00', { rowspan: 2, contents: '01' } ], + [ '10' ] + ] ) ); + + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 1 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '00' ], + [ '10' ] + ] ) ); + } ); + + it( 'should remove column if other column is rowspanned (last column)', () => { + setData( model, modelTable( [ + [ '00', { rowspan: 2, contents: '01' } ], + [ '10' ] + ] ) ); + + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 0 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '01' ] + ] ) ); + } ); + + it( 'should remove column if other column is rowspanned (first column)', () => { + setData( model, modelTable( [ + [ { rowspan: 2, contents: '00' }, '01' ], + [ '11' ] + ] ) ); + + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 1 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '00' ] + ] ) ); + } ); + + it( 'should remove column if removing row with one column - other columns are spanned', () => { + setData( model, modelTable( [ + [ '00', { rowspan: 2, contents: '01' }, { rowspan: 2, contents: '02' } ], + [ '10' ], [ '20', '21', '22' ] ] ) ); - tableUtils.removeColumn( { at: 2 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 0 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ - [ '00', '01' ], - [ '10', '11' ], - [ '20', '21' ] + [ '01', '02' ], + [ '21', '22' ] ] ) ); } ); } ); @@ -1074,7 +1127,7 @@ describe( 'TableUtils', () => { [ '30', '31', '32' ] ] ) ); - tableUtils.removeColumn( { at: 0, columns: 2 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 0, columns: 2 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ '02' ], @@ -1092,7 +1145,7 @@ describe( 'TableUtils', () => { [ '30', '31', '32', '33' ] ] ) ); - tableUtils.removeColumn( { at: 1, columns: 2 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 1, columns: 2 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ '00', '03' ], @@ -1110,7 +1163,7 @@ describe( 'TableUtils', () => { [ '30', '31', '32' ] ] ) ); - tableUtils.removeColumn( { at: 1, columns: 2 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 1, columns: 2 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ '00' ], @@ -1126,7 +1179,7 @@ describe( 'TableUtils', () => { [ '10', '11', '12', '13', '14' ] ], { headingColumns: 3 } ) ); - tableUtils.removeColumn( { at: 1, columns: 3 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 1, columns: 3 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ '00', '04' ], @@ -1141,7 +1194,7 @@ describe( 'TableUtils', () => { [ '20', '21', '22' ] ] ) ); - tableUtils.removeColumn( { at: 0, columns: 2 } ); + tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 0, columns: 2 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ '00' ], From f7c92f39b3548163acd36c64d371e9b39eeb0cd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 7 Apr 2020 11:59:28 +0200 Subject: [PATCH 05/11] Move column removing logic to TableUtils#removeColumns. --- .../src/commands/removecolumncommand.js | 56 ++----------------- packages/ckeditor5-table/src/tableutils.js | 43 ++++++++++++++ 2 files changed, 47 insertions(+), 52 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/removecolumncommand.js b/packages/ckeditor5-table/src/commands/removecolumncommand.js index 1494284f4e0..5523a52cfe7 100644 --- a/packages/ckeditor5-table/src/commands/removecolumncommand.js +++ b/packages/ckeditor5-table/src/commands/removecolumncommand.js @@ -10,7 +10,6 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; import TableWalker from '../tablewalker'; -import { updateNumericAttribute } from './utils'; import { getSelectionAffectedTableCells } from '../utils'; /** @@ -69,61 +68,14 @@ export default class RemoveColumnCommand extends Command { // A temporary workaround to avoid the "model-selection-range-intersects" error. writer.setSelection( writer.createRangeOn( table ) ); - adjustHeadingColumns( table, removedColumnIndexes, writer ); - - for ( - let removedColumnIndex = removedColumnIndexes.last; - removedColumnIndex >= removedColumnIndexes.first; - removedColumnIndex-- - ) { - this._removeColumn( removedColumnIndex, table, writer ); - } + this.editor.plugins.get( 'TableUtils' ).removeColumns( { + at: removedColumnIndexes.first, + columns: removedColumnIndexes.last - removedColumnIndexes.first + } ); writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) ); } ); } - - /** - * Removes a column from the given `table`. - * - * @private - * @param {Number} removedColumnIndex Index of the column that should be removed. - * @param {module:engine/model/element~Element} table - * @param {module:engine/model/writer~Writer} writer - */ - _removeColumn( removedColumnIndex, table, writer ) { - const tableUtils = this.editor.plugins.get( 'TableUtils' ); - - for ( const { cell, column, colspan } of [ ...new TableWalker( table ) ] ) { - // If colspaned cell overlaps removed column decrease its span. - if ( column <= removedColumnIndex && colspan > 1 && column + colspan > removedColumnIndex ) { - updateNumericAttribute( 'colspan', colspan - 1, cell, writer ); - } else if ( column === removedColumnIndex ) { - const cellRow = cell.parent; - - // The cell in removed column has colspan of 1. - writer.remove( cell ); - - // If the cell was the last one in the row, get rid of the entire row. - // https://github.com/ckeditor/ckeditor5/issues/6429 - if ( !cellRow.childCount ) { - tableUtils.removeRow( cellRow.index, table, writer ); - } - } - } - } -} - -// Updates heading columns attribute if removing a row from head section. -function adjustHeadingColumns( table, removedColumnIndexes, writer ) { - const headingColumns = table.getAttribute( 'headingColumns' ) || 0; - - if ( headingColumns && removedColumnIndexes.first <= headingColumns ) { - const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) - - removedColumnIndexes.first + 1; - - writer.setAttribute( 'headingColumns', headingColumns - headingsRemoved, table ); - } } // Returns a proper table cell to focus after removing a column. diff --git a/packages/ckeditor5-table/src/tableutils.js b/packages/ckeditor5-table/src/tableutils.js index 69bd7a2267d..f4bf3a81b35 100644 --- a/packages/ckeditor5-table/src/tableutils.js +++ b/packages/ckeditor5-table/src/tableutils.js @@ -307,6 +307,37 @@ export default class TableUtils extends Plugin { } ); } + removeColumns( table, options ) { + const model = this.editor.model; + const first = options.at; + const columnsToRemove = options.columns || 1; + const last = options.at + columnsToRemove - 1; + + model.change( writer => { + adjustHeadingColumns( table, { first, last }, writer ); + + for ( let removedColumnIndex = last; removedColumnIndex >= first; removedColumnIndex-- ) { + for ( const { cell, column, colspan } of [ ...new TableWalker( table ) ] ) { + // If colspaned cell overlaps removed column decrease its span. + if ( column <= removedColumnIndex && colspan > 1 && column + colspan > removedColumnIndex ) { + updateNumericAttribute( 'colspan', colspan - 1, cell, writer ); + } else if ( column === removedColumnIndex ) { + const cellRow = cell.parent; + + // The cell in removed column has colspan of 1. + writer.remove( cell ); + + // If the cell was the last one in the row, get rid of the entire row. + // https://github.com/ckeditor/ckeditor5/issues/6429 + if ( !cellRow.childCount ) { + this.removeRows( table, { at: cellRow.index } ); + } + } + } + } + } ); + } + /** * Divides a table cell vertically into several ones. * @@ -715,3 +746,15 @@ function getNewHeadingRowsValue( first, last, headingRows ) { return first; } + +// Updates heading columns attribute if removing a row from head section. +function adjustHeadingColumns( table, removedColumnIndexes, writer ) { + const headingColumns = table.getAttribute( 'headingColumns' ) || 0; + + if ( headingColumns && removedColumnIndexes.first <= headingColumns ) { + const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) - + removedColumnIndexes.first + 1; + + writer.setAttribute( 'headingColumns', headingColumns - headingsRemoved, table ); + } +} From 686afd7d3dce7762bff060db9794eeacd6648995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 7 Apr 2020 12:15:53 +0200 Subject: [PATCH 06/11] Pass proper parameters to TableUtils.removeColumns() ins RemoveColumnCommand. --- .../ckeditor5-table/src/commands/removecolumncommand.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/removecolumncommand.js b/packages/ckeditor5-table/src/commands/removecolumncommand.js index 5523a52cfe7..0e159ed5167 100644 --- a/packages/ckeditor5-table/src/commands/removecolumncommand.js +++ b/packages/ckeditor5-table/src/commands/removecolumncommand.js @@ -68,9 +68,11 @@ export default class RemoveColumnCommand extends Command { // A temporary workaround to avoid the "model-selection-range-intersects" error. writer.setSelection( writer.createRangeOn( table ) ); - this.editor.plugins.get( 'TableUtils' ).removeColumns( { + const columnsToRemove = removedColumnIndexes.last - removedColumnIndexes.first + 1; + + this.editor.plugins.get( 'TableUtils' ).removeColumns( table, { at: removedColumnIndexes.first, - columns: removedColumnIndexes.last - removedColumnIndexes.first + columns: columnsToRemove } ); writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) ); From 2636b225ffc618d3322d765d1018be057786defe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 7 Apr 2020 13:27:34 +0200 Subject: [PATCH 07/11] Add docs for TableUtils.removeColumns() method. --- packages/ckeditor5-table/src/tableutils.js | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/packages/ckeditor5-table/src/tableutils.js b/packages/ckeditor5-table/src/tableutils.js index f4bf3a81b35..46ff1de3144 100644 --- a/packages/ckeditor5-table/src/tableutils.js +++ b/packages/ckeditor5-table/src/tableutils.js @@ -307,6 +307,35 @@ export default class TableUtils extends Plugin { } ); } + /** + * Removes columns from the given `table`. + * + * This method re-calculates the table geometry including `colspan` attribute of table cells overlapping removed columns + * and table headings values. + * + * editor.plugins.get( 'TableUtils' ).removeColumns( table, { at: 1, columns: 2 } ); + * + * Executing the above code in the context of the table on the left will transform its structure as presented on the right: + * + * 0 1 2 3 4 0 1 2 + * ┌───────────────┬───┐ ┌───────┬───┐ + * │ a │ b │ │ a │ b │ + * │ ├───┤ │ ├───┤ + * │ │ c │ │ │ c │ + * ├───┬───┬───┬───┼───┤ will give: ├───┬───┼───┤ + * │ d │ e │ f │ g │ h │ │ d │ g │ h │ + * ├───┼───┼───┤ ├───┤ ├───┤ ├───┤ + * │ i │ j │ k │ │ l │ │ i │ │ l │ + * ├───┴───┴───┴───┴───┤ ├───┴───┴───┤ + * │ m │ │ m │ + * └───────────────────┘ └───────────┘ + * ^---- remove from here, `at` = 1, `columns` = 2 + * + * @param {module:engine/model/element~Element} table + * @param {Object} options + * @param {Number} options.at The row index at which the removing columns will start. + * @param {Number} [options.columns=1] The number of columns to remove. + */ removeColumns( table, options ) { const model = this.editor.model; const first = options.at; From e48d63df3867bbdf5a27a04e3ee6d32b58e0a599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 8 Apr 2020 12:08:22 +0200 Subject: [PATCH 08/11] Fix tables used in tests. --- .../tests/commands/removecolumncommand.js | 32 +++++++++---------- packages/ckeditor5-table/tests/tableutils.js | 28 ++++++++-------- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/packages/ckeditor5-table/tests/commands/removecolumncommand.js b/packages/ckeditor5-table/tests/commands/removecolumncommand.js index 08e5f677c6c..905727c963e 100644 --- a/packages/ckeditor5-table/tests/commands/removecolumncommand.js +++ b/packages/ckeditor5-table/tests/commands/removecolumncommand.js @@ -367,21 +367,21 @@ describe( 'RemoveColumnCommand', () => { it( 'should decrease colspan of table cells from previous column', () => { setData( model, modelTable( [ - [ { colspan: 4, contents: '00' }, '03' ], - [ { colspan: 3, contents: '10' }, '13' ], - [ { colspan: 2, contents: '20' }, '22[]', '23' ], - [ '30', { colspan: 2, contents: '31' }, '33' ], - [ '40', '41', '42', '43' ] + [ { colspan: 4, contents: '00' }, '04' ], + [ { colspan: 3, contents: '10' }, '14' ], + [ { colspan: 2, contents: '20' }, '22[]', '23', '24' ], + [ '30', { colspan: 2, contents: '31' }, '33', '34' ], + [ '40', '41', '42', '43', '44' ] ] ) ); command.execute(); assertEqualMarkup( getData( model ), modelTable( [ - [ { colspan: 3, contents: '00' }, '03' ], - [ { colspan: 2, contents: '10' }, '13' ], - [ { colspan: 2, contents: '20' }, '[]23' ], - [ '30', '31', '33' ], - [ '40', '41', '43' ] + [ { colspan: 3, contents: '00' }, '04' ], + [ { colspan: 2, contents: '10' }, '14' ], + [ { colspan: 2, contents: '20' }, '[]23', '24' ], + [ '30', '31', '33', '34' ], + [ '40', '41', '43', '44' ] ] ) ); } ); @@ -389,7 +389,7 @@ describe( 'RemoveColumnCommand', () => { it( 'should decrease colspan of cells that are on removed column', () => { setData( model, modelTable( [ [ { colspan: 3, contents: '[]00' }, '03' ], - [ { colspan: 2, contents: '10' }, '13' ], + [ { colspan: 2, contents: '10' }, '12', '13' ], [ '20', '21', '22', '23' ] ] ) ); @@ -397,7 +397,7 @@ describe( 'RemoveColumnCommand', () => { assertEqualMarkup( getData( model ), modelTable( [ [ { colspan: 2, contents: '[]00' }, '03' ], - [ '10', '13' ], + [ '10', '12', '13' ], [ '21', '22', '23' ] ] ) ); } ); @@ -421,7 +421,7 @@ describe( 'RemoveColumnCommand', () => { it( 'should work property if the rowspan is in the first column (the other cell in row is selected)', () => { setData( model, modelTable( [ [ { rowspan: 2, contents: '00' }, '[]01' ], - [ '10' ] + [ '11' ] ] ) ); command.execute(); @@ -434,7 +434,7 @@ describe( 'RemoveColumnCommand', () => { it( 'should work property if the rowspan is in the first column (the cell in row below is selected)', () => { setData( model, modelTable( [ [ { rowspan: 2, contents: '00' }, '01' ], - [ '[]10' ] + [ '[]11' ] ] ) ); command.execute(); @@ -447,14 +447,14 @@ describe( 'RemoveColumnCommand', () => { it( 'should work property if the rowspan is in the first column (the cell with rowspan is selected)', () => { setData( model, modelTable( [ [ { rowspan: 2, contents: '00[]' }, '01' ], - [ '10' ] + [ '11' ] ] ) ); command.execute(); assertEqualMarkup( getData( model ), modelTable( [ [ '[]01' ], - [ '10' ] + [ '11' ] ] ) ); } ); diff --git a/packages/ckeditor5-table/tests/tableutils.js b/packages/ckeditor5-table/tests/tableutils.js index facdc5e565f..b8930c8cc84 100644 --- a/packages/ckeditor5-table/tests/tableutils.js +++ b/packages/ckeditor5-table/tests/tableutils.js @@ -1013,21 +1013,21 @@ describe( 'TableUtils', () => { it( 'should decrease colspan of table cells from previous column', () => { setData( model, modelTable( [ - [ { colspan: 4, contents: '00' }, '03' ], - [ { colspan: 3, contents: '10' }, '13' ], - [ { colspan: 2, contents: '20' }, '22', '23' ], - [ '30', { colspan: 2, contents: '31' }, '33' ], - [ '40', '41', '42', '43' ] + [ { colspan: 4, contents: '00' }, '04' ], + [ { colspan: 3, contents: '10' }, '14' ], + [ { colspan: 2, contents: '20' }, '22', '23', '24' ], + [ '30', { colspan: 2, contents: '31' }, '33', '34' ], + [ '40', '41', '42', '43', '44' ] ] ) ); tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 2 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ - [ { colspan: 3, contents: '00' }, '03' ], - [ { colspan: 2, contents: '10' }, '13' ], - [ { colspan: 2, contents: '20' }, '23' ], - [ '30', '31', '33' ], - [ '40', '41', '43' ] + [ { colspan: 3, contents: '00' }, '04' ], + [ { colspan: 2, contents: '10' }, '14' ], + [ { colspan: 2, contents: '20' }, '23', '24' ], + [ '30', '31', '33', '34' ], + [ '40', '41', '43', '44' ] ] ) ); } ); @@ -1035,7 +1035,7 @@ describe( 'TableUtils', () => { it( 'should decrease colspan of cells that are on removed column', () => { setData( model, modelTable( [ [ { colspan: 3, contents: '00' }, '03' ], - [ { colspan: 2, contents: '10' }, '13' ], + [ { colspan: 2, contents: '10' }, '12', '13' ], [ '20', '21', '22', '23' ] ] ) ); @@ -1043,7 +1043,7 @@ describe( 'TableUtils', () => { assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ { colspan: 2, contents: '00' }, '03' ], - [ '10', '13' ], + [ '10', '12', '13' ], [ '21', '22', '23' ] ] ) ); } ); @@ -1051,14 +1051,14 @@ describe( 'TableUtils', () => { it( 'should remove column with rowspan (first column)', () => { setData( model, modelTable( [ [ { rowspan: 2, contents: '00' }, '01' ], - [ '10' ] + [ '11' ] ] ) ); tableUtils.removeColumns( root.getNodeByPath( [ 0 ] ), { at: 0 } ); assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ '01' ], - [ '10' ] + [ '11' ] ] ) ); } ); From 4822543193cd77361a09f5d51389e10a84289f87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 8 Apr 2020 12:11:04 +0200 Subject: [PATCH 09/11] Fix condition for updating heading columns in adjustHeadingColumns(). --- packages/ckeditor5-table/src/tableutils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/src/tableutils.js b/packages/ckeditor5-table/src/tableutils.js index b99984ba81a..f1a283276f3 100644 --- a/packages/ckeditor5-table/src/tableutils.js +++ b/packages/ckeditor5-table/src/tableutils.js @@ -780,7 +780,7 @@ function getNewHeadingRowsValue( first, last, headingRows ) { function adjustHeadingColumns( table, removedColumnIndexes, writer ) { const headingColumns = table.getAttribute( 'headingColumns' ) || 0; - if ( headingColumns && removedColumnIndexes.first <= headingColumns ) { + if ( headingColumns && removedColumnIndexes.first < headingColumns ) { const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) - removedColumnIndexes.first + 1; From 627c61e07309cc6a1792d4d05071aa2e71e50fe0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 8 Apr 2020 12:22:26 +0200 Subject: [PATCH 10/11] Fix tables used in tests. --- .../ckeditor5-table/tests/commands/removecolumncommand.js | 4 ++-- packages/ckeditor5-table/tests/tableutils.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-table/tests/commands/removecolumncommand.js b/packages/ckeditor5-table/tests/commands/removecolumncommand.js index 905727c963e..487a2f16d9f 100644 --- a/packages/ckeditor5-table/tests/commands/removecolumncommand.js +++ b/packages/ckeditor5-table/tests/commands/removecolumncommand.js @@ -368,7 +368,7 @@ describe( 'RemoveColumnCommand', () => { it( 'should decrease colspan of table cells from previous column', () => { setData( model, modelTable( [ [ { colspan: 4, contents: '00' }, '04' ], - [ { colspan: 3, contents: '10' }, '14' ], + [ { colspan: 3, contents: '10' }, '13', '14' ], [ { colspan: 2, contents: '20' }, '22[]', '23', '24' ], [ '30', { colspan: 2, contents: '31' }, '33', '34' ], [ '40', '41', '42', '43', '44' ] @@ -378,7 +378,7 @@ describe( 'RemoveColumnCommand', () => { assertEqualMarkup( getData( model ), modelTable( [ [ { colspan: 3, contents: '00' }, '04' ], - [ { colspan: 2, contents: '10' }, '14' ], + [ { colspan: 2, contents: '10' }, '13', '14' ], [ { colspan: 2, contents: '20' }, '[]23', '24' ], [ '30', '31', '33', '34' ], [ '40', '41', '43', '44' ] diff --git a/packages/ckeditor5-table/tests/tableutils.js b/packages/ckeditor5-table/tests/tableutils.js index b8930c8cc84..ae1c4f56fab 100644 --- a/packages/ckeditor5-table/tests/tableutils.js +++ b/packages/ckeditor5-table/tests/tableutils.js @@ -1014,7 +1014,7 @@ describe( 'TableUtils', () => { it( 'should decrease colspan of table cells from previous column', () => { setData( model, modelTable( [ [ { colspan: 4, contents: '00' }, '04' ], - [ { colspan: 3, contents: '10' }, '14' ], + [ { colspan: 3, contents: '10' }, '13', '14' ], [ { colspan: 2, contents: '20' }, '22', '23', '24' ], [ '30', { colspan: 2, contents: '31' }, '33', '34' ], [ '40', '41', '42', '43', '44' ] @@ -1024,7 +1024,7 @@ describe( 'TableUtils', () => { assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ [ { colspan: 3, contents: '00' }, '04' ], - [ { colspan: 2, contents: '10' }, '14' ], + [ { colspan: 2, contents: '10' }, '13', '14' ], [ { colspan: 2, contents: '20' }, '23', '24' ], [ '30', '31', '33', '34' ], [ '40', '41', '43', '44' ] From 2a09cf10d86b6fa1a600d2fe14ff8558704060c9 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 8 Apr 2020 14:13:47 +0200 Subject: [PATCH 11/11] Update src/tableutils.js --- packages/ckeditor5-table/src/tableutils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/src/tableutils.js b/packages/ckeditor5-table/src/tableutils.js index f1a283276f3..a1e1b17bd66 100644 --- a/packages/ckeditor5-table/src/tableutils.js +++ b/packages/ckeditor5-table/src/tableutils.js @@ -310,7 +310,7 @@ export default class TableUtils extends Plugin { /** * Removes columns from the given `table`. * - * This method re-calculates the table geometry including `colspan` attribute of table cells overlapping removed columns + * This method re-calculates the table geometry including the `colspan` attribute of table cells overlapping removed columns * and table headings values. * * editor.plugins.get( 'TableUtils' ).removeColumns( table, { at: 1, columns: 2 } );