From 5391f7df5a72b0e7e4fa0bee85b01c8d803c1424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 25 Mar 2020 08:28:16 +0100 Subject: [PATCH 1/5] Recalculate table geometry on each table row removal. --- src/commands/removerowcommand.js | 31 +++++++++++++----------------- tests/commands/removerowcommand.js | 21 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/commands/removerowcommand.js b/src/commands/removerowcommand.js index 17ac3487..05d51bdc 100644 --- a/src/commands/removerowcommand.js +++ b/src/commands/removerowcommand.js @@ -10,7 +10,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; import TableWalker from '../tablewalker'; -import { updateNumericAttribute } from './utils'; +import { findAncestor, updateNumericAttribute } from './utils'; import { getSelectionAffectedTableCells } from '../utils'; /** @@ -51,18 +51,19 @@ export default class RemoveRowCommand extends Command { * @inheritDoc */ execute() { - const referenceCells = getSelectionAffectedTableCells( this.editor.model.document.selection ); + const model = this.editor.model; + const referenceCells = getSelectionAffectedTableCells( model.document.selection ); const removedRowIndexes = getRowIndexes( referenceCells ); const firstCell = referenceCells[ 0 ]; - const table = firstCell.parent.parent; - const tableMap = [ ...new TableWalker( table, { endRow: removedRowIndexes.last } ) ]; - const batch = this.editor.model.createBatch(); - const columnIndexToFocus = getColumnIndexToFocus( tableMap, firstCell ); + const table = findAncestor( 'table', firstCell ); + + const batch = model.createBatch(); + const columnIndexToFocus = this.editor.plugins.get( 'TableUtils' ).getCellLocation( firstCell ).column; // Doing multiple model.enqueueChange() calls, to get around ckeditor/ckeditor5#6391. // Ideally we want to do this in a single model.change() block. - this.editor.model.enqueueChange( batch, writer => { + model.enqueueChange( batch, writer => { // This prevents the "model-selection-range-intersects" error, caused by removing row selected cells. writer.setSelection( writer.createSelection( table, 'on' ) ); } ); @@ -70,15 +71,15 @@ export default class RemoveRowCommand extends Command { let cellToFocus; for ( let i = removedRowIndexes.last; i >= removedRowIndexes.first; i-- ) { - this.editor.model.enqueueChange( batch, writer => { + model.enqueueChange( batch, writer => { const removedRowIndex = i; - this._removeRow( removedRowIndex, table, writer, tableMap ); + this._removeRow( removedRowIndex, table, writer ); cellToFocus = getCellToFocus( table, removedRowIndex, columnIndexToFocus ); } ); } - this.editor.model.enqueueChange( batch, writer => { + model.enqueueChange( batch, writer => { writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) ); } ); } @@ -90,12 +91,12 @@ export default class RemoveRowCommand extends Command { * @param {Number} removedRowIndex Index of the row that should be removed. * @param {module:engine/model/element~Element} table * @param {module:engine/model/writer~Writer} writer - * @param {module:engine/model/element~Element[]} tableMap Table map retrieved from {@link module:table/tablewalker~TableWalker}. */ - _removeRow( removedRowIndex, table, writer, tableMap ) { + _removeRow( removedRowIndex, table, writer ) { const cellsToMove = new Map(); const tableRow = table.getChild( removedRowIndex ); const headingRows = table.getAttribute( 'headingRows' ) || 0; + const tableMap = [ ...new TableWalker( table, { endRow: removedRowIndex } ) ]; if ( headingRows && removedRowIndex < headingRows ) { updateNumericAttribute( 'headingRows', headingRows - 1, table, writer, 0 ); @@ -166,9 +167,3 @@ function getCellToFocus( table, removedRowIndex, columnToFocus ) { return cellToFocus; } - -// Returns the index of column that should be focused after rows are removed. -function getColumnIndexToFocus( tableMap, firstCell ) { - const firstCellData = tableMap.find( value => value.cell === firstCell ); - return firstCellData.column; -} diff --git a/tests/commands/removerowcommand.js b/tests/commands/removerowcommand.js index 101f32c6..03d24405 100644 --- a/tests/commands/removerowcommand.js +++ b/tests/commands/removerowcommand.js @@ -258,6 +258,27 @@ describe( 'RemoveRowCommand', () => { [ '[]20', '21' ] ] ) ); } ); + + it( 'should properly calculate truncated rowspans', () => { + setData( model, modelTable( [ + [ '00', { contents: '01', rowspan: 3 } ], + [ '10' ], + [ '20' ] + ] ) ); + + const tableSelection = editor.plugins.get( TableSelection ); + const modelRoot = model.document.getRoot(); + tableSelection._setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 0 ] ) + ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ '[]20', '01' ] + ] ) ); + } ); } ); describe( 'with entire row selected', () => { From 073504a8073f054c3a78bbe962d47fca3c9938f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 25 Mar 2020 08:39:25 +0100 Subject: [PATCH 2/5] Use existing helpers & make code more readable. --- src/commands/removerowcommand.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/commands/removerowcommand.js b/src/commands/removerowcommand.js index 05d51bdc..443c1f7d 100644 --- a/src/commands/removerowcommand.js +++ b/src/commands/removerowcommand.js @@ -33,15 +33,16 @@ export default class RemoveRowCommand extends Command { const firstCell = selectedCells[ 0 ]; if ( firstCell ) { - const table = firstCell.parent.parent; + const table = findAncestor( 'table', firstCell ); const tableRowCount = this.editor.plugins.get( 'TableUtils' ).getRows( table ); + const lastRowIndex = tableRowCount - 1; - const tableMap = [ ...new TableWalker( table ) ]; - const rowIndexes = tableMap.filter( entry => selectedCells.includes( entry.cell ) ).map( el => el.row ); - const minRowIndex = rowIndexes[ 0 ]; - const maxRowIndex = rowIndexes[ rowIndexes.length - 1 ]; + const removedRowIndexes = getRowIndexes( selectedCells ); - this.isEnabled = maxRowIndex - minRowIndex < ( tableRowCount - 1 ); + const areAllRowsSelected = removedRowIndexes.first === 0 && removedRowIndexes.last === lastRowIndex; + + // Disallow selecting whole table -> delete whole table should be used instead. + this.isEnabled = !areAllRowsSelected; } else { this.isEnabled = false; } From 8f6460a44ad2b28586f6a2638d0a9c5e3db5cefe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 25 Mar 2020 09:23:12 +0100 Subject: [PATCH 3/5] Recalculate table geometry on each table column removal. --- src/commands/removecolumncommand.js | 2 +- tests/commands/removecolumncommand.js | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/commands/removecolumncommand.js b/src/commands/removecolumncommand.js index 75054e2a..8c621609 100644 --- a/src/commands/removecolumncommand.js +++ b/src/commands/removecolumncommand.js @@ -76,7 +76,7 @@ export default class RemoveColumnCommand extends Command { removedColumnIndex >= removedColumnIndexes.first; removedColumnIndex-- ) { - for ( const { cell, column, colspan } of tableMap ) { + 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 ); diff --git a/tests/commands/removecolumncommand.js b/tests/commands/removecolumncommand.js index 8c658080..ad76d108 100644 --- a/tests/commands/removecolumncommand.js +++ b/tests/commands/removecolumncommand.js @@ -324,6 +324,29 @@ describe( 'RemoveColumnCommand', () => { [ '10', '[]14' ] ], { headingColumns: 1 } ) ); } ); + + it( 'should properly calculate truncated colspans', () => { + setData( model, modelTable( [ + [ { contents: '00', colspan: 3 } ], + [ '10', '11', '12' ], + [ '20', '21', '22' ] + ] ) ); + + const tableSelection = editor.plugins.get( TableSelection ); + const modelRoot = model.document.getRoot(); + tableSelection._setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ '00' ], + [ '[]12' ], + [ '22' ] + ] ) ); + } ); } ); it( 'should change heading columns if removing a heading column', () => { From a9dfb775bd89d9a684c9e0950af5d9a7439dcf0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 25 Mar 2020 09:26:05 +0100 Subject: [PATCH 4/5] Extract single column removal to own method in remove column command. --- src/commands/removecolumncommand.js | 46 ++++++++++++++++++----------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/src/commands/removecolumncommand.js b/src/commands/removecolumncommand.js index 8c621609..d21aa7a3 100644 --- a/src/commands/removecolumncommand.js +++ b/src/commands/removecolumncommand.js @@ -76,28 +76,40 @@ export default class RemoveColumnCommand extends Command { removedColumnIndex >= removedColumnIndexes.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 ) { - writer.remove( cellRow ); - } - } - } + this._removeColumn( removedColumnIndex, table, writer ); } 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 ) { + 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 ) { + writer.remove( cellRow ); + } + } + } + } } // Updates heading columns attribute if removing a row from head section. From df173482063567d9514bcacfca32a448a1406607 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Thu, 26 Mar 2020 15:32:41 +0100 Subject: [PATCH 5/5] Internal: Variable name adjusted. --- src/commands/removerowcommand.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands/removerowcommand.js b/src/commands/removerowcommand.js index 443c1f7d..20779122 100644 --- a/src/commands/removerowcommand.js +++ b/src/commands/removerowcommand.js @@ -37,9 +37,9 @@ export default class RemoveRowCommand extends Command { const tableRowCount = this.editor.plugins.get( 'TableUtils' ).getRows( table ); const lastRowIndex = tableRowCount - 1; - const removedRowIndexes = getRowIndexes( selectedCells ); + const selectedRowIndexes = getRowIndexes( selectedCells ); - const areAllRowsSelected = removedRowIndexes.first === 0 && removedRowIndexes.last === lastRowIndex; + const areAllRowsSelected = selectedRowIndexes.first === 0 && selectedRowIndexes.last === lastRowIndex; // Disallow selecting whole table -> delete whole table should be used instead. this.isEnabled = !areAllRowsSelected;