From 830d58ea8f837096560140257dc9342ac72c1248 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 15 May 2020 11:24:34 +0200 Subject: [PATCH 01/39] Move non-rectangular tests to own suite. --- .../tests/tableclipboard-paste.js | 58 ++++++++++--------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 8772ffa4976..d1c48969eeb 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -248,33 +248,6 @@ describe( 'table clipboard', () => { ] ) ); } ); - it( 'should block non-rectangular selection', () => { - setModelData( model, modelTable( [ - [ { contents: '00', colspan: 3 } ], - [ '10', '11', '12' ], - [ '20', '21', '22' ] - ] ) ); - - tableSelection.setCellSelection( - modelRoot.getNodeByPath( [ 0, 0, 0 ] ), - modelRoot.getNodeByPath( [ 0, 1, 1 ] ) - ); - - // Catches the temporary console log in the CK_DEBUG mode. - sinon.stub( console, 'log' ); - - pasteTable( [ - [ 'aa', 'ab' ], - [ 'ba', 'bb' ] - ] ); - - assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ - [ { contents: '00', colspan: 3 } ], - [ '10', '11', '12' ], - [ '20', '21', '22' ] - ] ) ); - } ); - describe( 'single cell selected', () => { it( 'blocks this case', () => { setModelData( model, modelTable( [ @@ -1120,13 +1093,42 @@ describe( 'table clipboard', () => { /* eslint-disable no-multi-spaces */ assertSelectedCells( model, [ [ 0, 0, 0, 0 ], - [ 0, 1, 0 ], + [ 0, 1, 0 ], [ 0, 1, 1, 0 ], [ 0, 0, 0, 0 ] ] ); /* eslint-enable no-multi-spaces */ } ); } ); + + describe( 'non-rectangular content table', () => { + it( 'should block non-rectangular selection', () => { + setModelData( model, modelTable( [ + [ { contents: '00', colspan: 3 } ], + [ '10', '11', '12' ], + [ '20', '21', '22' ] + ] ) ); + + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + // Catches the temporary console log in the CK_DEBUG mode. + sinon.stub( console, 'log' ); + + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', 'bb' ] + ] ); + + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ { contents: '00', colspan: 3 } ], + [ '10', '11', '12' ], + [ '20', '21', '22' ] + ] ) ); + } ); + } ); } ); describe( 'pasted table is bigger than the selected area', () => { From 1b1f1fff4f7233d8a6ccbcf772ac36d5d84a1d90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 15 May 2020 13:08:58 +0200 Subject: [PATCH 02/39] Add tests for non-rectangular table selection. --- .../tests/tableclipboard-paste.js | 342 +++++++++++++++++- 1 file changed, 331 insertions(+), 11 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index d1c48969eeb..a8982ad20c6 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -1101,31 +1101,351 @@ describe( 'table clipboard', () => { } ); } ); - describe( 'non-rectangular content table', () => { - it( 'should block non-rectangular selection', () => { + describe( 'non-rectangular content table selection', () => { + it( 'should split cells outside the selected area before pasting (rowspan ends in selection)', () => { + // +----+----+----+ + // | 00 | 01 | 02 | + // +----+ +----+ + // | 10 | | 12 | + // +----+ +----+ + // | 20 | | 22 | + // +----+ +----+ + // | 30 | | 32 | + // +----+----+----+ + // | 40 | 41 | 42 | + // +----+----+----+ setModelData( model, modelTable( [ - [ { contents: '00', colspan: 3 } ], - [ '10', '11', '12' ], - [ '20', '21', '22' ] + [ '00', { contents: '01', rowspan: 4 }, '02' ], + [ '10', '12' ], + [ '20', '22' ], + [ '30', '32' ], + [ '40', '41', '42' ] ] ) ); + // Select 20 -> 32 + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 2, 0 ] ), + modelRoot.getNodeByPath( [ 0, 3, 1 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab', 'ac' ], + [ 'ba', 'bb', 'bc' ] + ] ); + + // +----+----+----+ + // | 00 | 01 | 02 | + // +----+ +----+ + // | 10 | | 12 | + // +----+----+----+ + // | aa | ab | ac | + // +----+----+----+ + // | ba | bb | bc | + // +----+----+----+ + // | 40 | 41 | 42 | + // +----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', { contents: '01', rowspan: 2 }, '02' ], + [ '10', '12' ], + [ 'aa', 'ab', 'ac' ], + [ 'ba', 'bb', 'bc' ], + [ '40', '41', '42' ] + ] ) ); + } ); + + it( 'should split cells outside the selected area before pasting (rowspan ends after the selection)', () => { + // +----+----+----+ + // | 00 | 01 | 02 | + // +----+ +----+ + // | 10 | | 12 | + // +----+ +----+ + // | 20 | | 22 | + // +----+ +----+ + // | 30 | | 32 | + // +----+----+----+ + // | 40 | 41 | 42 | + // +----+----+----+ + setModelData( model, modelTable( [ + [ '00', { contents: '01', rowspan: 4 }, '02' ], + [ '10', '12' ], + [ '20', '22' ], + [ '30', '32' ], + [ '40', '41', '42' ] + ] ) ); + + // Select 10 -> 22 + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 1, 0 ] ), + modelRoot.getNodeByPath( [ 0, 2, 1 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab', 'ac' ], + [ 'ba', 'bb', 'bc' ] + ] ); + + // +----+----+----+ + // | 00 | 01 | 02 | + // +----+----+----+ + // | aa | ab | ac | + // +----+----+----+ + // | ba | bb | bc | + // +----+----+----+ + // | 30 | | 32 | + // +----+----+----+ + // | 40 | 41 | 42 | + // +----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01', '02' ], + [ 'aa', 'ab', 'ac' ], + [ 'ba', 'bb', 'bc' ], + [ '30', '', '32' ], + [ '40', '41', '42' ] + ] ) ); + } ); + + it( 'should split cells inside the selected area before pasting (rowspan ends after the selection)', () => { + // +----+----+----+ + // | 00 | 01 | 02 | + // +----+ +----+ + // | 10 | | 12 | + // +----+ +----+ + // | 20 | | 22 | + // +----+ +----+ + // | 30 | | 32 | + // +----+----+----+ + // | 40 | 41 | 42 | + // +----+----+----+ + setModelData( model, modelTable( [ + [ '00', { contents: '01', rowspan: 4 }, '02' ], + [ '10', '12' ], + [ '20', '22' ], + [ '30', '32' ], + [ '40', '41', '42' ] + ] ) ); + + // Select 00 -> 12 tableSelection.setCellSelection( modelRoot.getNodeByPath( [ 0, 0, 0 ] ), modelRoot.getNodeByPath( [ 0, 1, 1 ] ) ); - // Catches the temporary console log in the CK_DEBUG mode. - sinon.stub( console, 'log' ); + pasteTable( [ + [ 'aa', 'ab', 'ac' ], + [ 'ba', 'bb', 'bc' ] + ] ); + + // +----+----+----+ + // | aa | ab | ac | + // +----+----+----+ + // | ba | bb | bc | + // +----+----+----+ + // | 20 | | 22 | + // +----+ +----+ + // | 30 | | 32 | + // +----+----+----+ + // | 40 | 41 | 42 | + // +----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ 'aa', 'ab', 'ac' ], + [ 'ba', 'bb', 'bc' ], + [ '20', { rowspan: 2, contents: '' }, '22' ], + [ '30', '32' ], + [ '40', '41', '42' ] + ] ) ); + } ); + + it( 'should split cells outside the selected area before pasting (colspan ends in selection)', () => { + // +----+----+----+----+----+ + // | 00 | 01 | 02 | 03 | 04 | + // +----+----+----+----+----+ + // | 10 | 14 | + // +----+----+----+----+----+ + // | 20 | 21 | 22 | 23 | 24 | + // +----+----+----+----+----+ + setModelData( model, modelTable( [ + [ '00', '01', '02', '03', '04' ], + [ { contents: '10', colspan: 4 }, '14' ], + [ '20', '21', '22', '23', '24' ] + ] ) ); + + // Select 02 -> 23 + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 2, 0 ] ), + modelRoot.getNodeByPath( [ 0, 3, 1 ] ) + ); pasteTable( [ [ 'aa', 'ab' ], - [ 'ba', 'bb' ] + [ 'ba', 'bb' ], + [ 'ca', 'cb' ] + ] ); + + // +----+----+----+----+----+ + // | 00 | 01 | aa | ab | 04 | + // +----+----+----+----+----+ + // | 10 | ba | bb | 14 | + // +----+----+----+----+----+ + // | 20 | 21 | ca | cb | 24 | + // +----+----+----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01', 'aa', 'ab', '04' ], + [ { contents: '10', colspan: 2 }, 'ba', 'bb', '14' ], + [ '20', '21', 'ca', 'cb', '24' ] + ] ) ); + } ); + + it( 'should split cells outside the selected area before pasting (colspan ends after the selection)', () => { + // +----+----+----+----+----+ + // | 00 | 01 | 02 | 03 | 04 | + // +----+----+----+----+----+ + // | 10 | 14 | + // +----+----+----+----+----+ + // | 20 | 21 | 22 | 23 | 24 | + // +----+----+----+----+----+ + setModelData( model, modelTable( [ + [ '00', '01', '02', '03', '04' ], + [ { contents: '10', colspan: 4 }, '14' ], + [ '20', '21', '22', '23', '24' ] + ] ) ); + + // Select 01 -> 22 + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 1, 0 ] ), + modelRoot.getNodeByPath( [ 0, 2, 1 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', 'bb' ], + [ 'ca', 'cb' ] + ] ); + + // +----+----+----+----+----+ + // | 00 | aa | ab | 03 | 04 | + // +----+----+----+----+----+ + // | 10 | ba | bb | | 14 | + // +----+----+----+----+----+ + // | 20 | ca | cb | 23 | 24 | + // +----+----+----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01', 'aa', 'ab', '04' ], + [ { contents: '10', colspan: 2 }, 'ba', 'bb', '14' ], + [ '20', '21', 'ca', 'cb', '24' ] + ] ) ); + } ); + + it( 'should split cells inside the selected area before pasting (colspan ends after the selection)', () => { + // +----+----+----+----+----+ + // | 00 | 01 | 02 | 03 | 04 | + // +----+----+----+----+----+ + // | 10 | 14 | + // +----+----+----+----+----+ + // | 20 | 21 | 22 | 23 | 24 | + // +----+----+----+----+----+ + setModelData( model, modelTable( [ + [ '00', '01', '02', '03', '04' ], + [ { contents: '10', colspan: 4 }, '14' ], + [ '20', '21', '22', '23', '24' ] + ] ) ); + + // Select 00 -> 21 + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', 'bb' ], + [ 'ca', 'cb' ] + ] ); + + // +----+----+----+----+----+ + // | aa | ab | 02 | 03 | 04 | + // +----+----+----+----+----+ + // | ba | bb | | | 14 | + // +----+----+----+----+----+ + // | ca | cb | 22 | 23 | 24 | + // +----+----+----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01', 'aa', 'ab', '04' ], + [ { contents: '10', colspan: 2 }, 'ba', 'bb', '14' ], + [ '20', '21', 'ca', 'cb', '24' ] + ] ) ); + } ); + + it( 'should properly handle complex case', () => { + // +----+----+----+----+----+----+----+ + // | 00 | 03 | 04 | 05 | 06 | + // + + +----+----+----+ + // | | | 14 | 15 | 16 | + // + + +----+----+----+ + // | | | 24 | + // +----+----+----+----+----+----+----+ + // | 30 | 31 | 33 | 34 | 35 | 36 | + // + +----+----+ +----+----+----+ + // | | 41 | 42 | | 44 | 45 | + // + +----+----+ +----+ + + // | | 51 | 52 | | 54 | | + // +----+----+----+ +----+ + + // | 60 | 61 | 62 | | 64 | | + // +----+----+----+----+----+----+----+ + setModelData( model, modelTable( [ + [ { contents: '00', colspan: 3, rowspan: 3 }, { contents: '03', rowspan: 3 }, '04', '05', '06' ], + [ '14', '15', '16' ], + [ { contents: '24', colspan: 3 } ], + [ + { contents: '30', rowspan: 3 }, + { contents: '31', colspan: 2 }, + { contents: '33', rowspan: 4 }, + '34', '35', '36' + ], + [ '41', '42', '44', { contents: '45', colspan: 2, rowspan: 3 } ], + [ '51', '52', '54' ], + [ '60', '61', '62', '64' ] + ] ) ); + + // Select 42 -> 24 (3x3 selection) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 4, 1 ] ), + modelRoot.getNodeByPath( [ 0, 2, 0 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab', 'ac' ], + [ 'ba', 'bb', 'bc' ], + [ 'ca', 'cb', 'cc' ] ] ); + // +----+----+----+----+----+----+----+ + // | 00 | | 03 | 04 | 05 | 06 | + // + + + +----+----+----+ + // | | | | 14 | 15 | 16 | + // +----+----+----+----+----+----+----+ + // | | aa | ab | ac | | + // +----+----+----+----+----+----+----+ + // | 30 | 31 | ba | bb | bc | 35 | 36 | + // + +----+----+----+----+----+----+ + // | | 41 | ca | cb | cc | 45 | + // + +----+----+----+----+ + + // | | 51 | 52 | 53 | 54 | | + // +----+----+----+ +----+ + + // | 60 | 61 | 62 | | 64 | | + // +----+----+----+----+----+----+----+ assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ - [ { contents: '00', colspan: 3 } ], - [ '10', '11', '12' ], - [ '20', '21', '22' ] + [ + { contents: '00', colspan: 2, rowspan: 2 }, + { contents: '', rowspan: 2 }, + { contents: '03', rowspan: 2 }, + '04', '05', '06' + ], + [ '14', '15', '16' ], + [ { contents: '', colspan: 2 }, 'aa', 'ab', 'ac', { contents: '', colspan: 2 } ], + [ { contents: '30', rowspan: 3 }, '31', 'ba', 'bb', 'bc', '35', '36' ], + [ '41', 'ca', 'cb', 'cc', { contents: '45', colspan: 2, rowspan: 3 } ], + [ '51', '52', { contents: '53', rowspan: 2 }, '54' ], + [ '60', '61', '62', '64' ] ] ) ); } ); } ); From 22cb4001239cc9a753d7a12c935eef5275726e67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 15 May 2020 13:58:52 +0200 Subject: [PATCH 03/39] Handle horizontal split when making rectangular selection. --- .../src/commands/setheaderrowcommand.js | 4 +- .../ckeditor5-table/src/tableclipboard.js | 63 +++++++++++++------ 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js index a4fc96804bb..26ae832b9bd 100644 --- a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js +++ b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js @@ -109,7 +109,7 @@ export default class SetHeaderRowCommand extends Command { // @param {Number} headingRowsToSet New heading rows attribute. // @param {Number} currentHeadingRows Current heading rows attribute. // @returns {Array.} -function getOverlappingCells( table, headingRowsToSet, currentHeadingRows ) { +export function getOverlappingCells( table, headingRowsToSet, currentHeadingRows ) { const cellsToSplit = []; const startAnalysisRow = headingRowsToSet > currentHeadingRows ? currentHeadingRows : 0; @@ -132,7 +132,7 @@ function getOverlappingCells( table, headingRowsToSet, currentHeadingRows ) { // @param {module:engine/model/element~Element} tableCell // @param {Number} headingRows // @param {module:engine/model/writer~Writer} writer -function splitHorizontally( tableCell, headingRows, writer ) { +export function splitHorizontally( tableCell, headingRows, writer ) { const tableRow = tableCell.parent; const table = tableRow.parent; const rowIndex = tableRow.index; diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 9ae9e7af8fd..b1ac471c84e 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -14,6 +14,7 @@ import TableWalker from './tablewalker'; import { getColumnIndexes, getRowIndexes, isSelectionRectangular } from './utils'; import { findAncestor } from './commands/utils'; import { cropTableToDimensions } from './tableselection/croptable'; +import { getOverlappingCells, splitHorizontally } from './commands/setheaderrowcommand'; import TableUtils from './tableutils'; /** @@ -127,34 +128,36 @@ export default class TableClipboard extends Plugin { const tableUtils = this.editor.plugins.get( TableUtils ); - // Currently not handled. The selected table content should be trimmed to a rectangular selection. - // See: https://github.com/ckeditor/ckeditor5/issues/6122. - if ( !isSelectionRectangular( selectedTableCells, tableUtils ) ) { - // @if CK_DEBUG // console.log( 'NOT IMPLEMENTED YET: Selection is not rectangular (non-mergeable).' ); + const model = this.editor.model; - return; - } + model.change( writer => { + // Currently not handled. The selected table content should be trimmed to a rectangular selection. + // See: https://github.com/ckeditor/ckeditor5/issues/6122. + if ( !isSelectionRectangular( selectedTableCells, tableUtils ) ) { + // @if CK_DEBUG // console.log( 'NOT IMPLEMENTED YET: Selection is not rectangular (non-mergeable).' ); - const { last: lastColumnOfSelection, first: firstColumnOfSelection } = getColumnIndexes( selectedTableCells ); - const { first: firstRowOfSelection, last: lastRowOfSelection } = getRowIndexes( selectedTableCells ); + prepareLandingPlace( selectedTableCells, writer ); - const selectionHeight = lastRowOfSelection - firstRowOfSelection + 1; - const selectionWidth = lastColumnOfSelection - firstColumnOfSelection + 1; + // return; + } - const pasteHeight = tableUtils.getRows( pastedTable ); - const pasteWidth = tableUtils.getColumns( pastedTable ); + const { last: lastColumnOfSelection, first: firstColumnOfSelection } = getColumnIndexes( selectedTableCells ); + const { first: firstRowOfSelection, last: lastRowOfSelection } = getRowIndexes( selectedTableCells ); - // The if below is temporal and will be removed when handling this case. - // See: https://github.com/ckeditor/ckeditor5/issues/6769. - if ( selectionHeight > pasteHeight || selectionWidth > pasteWidth ) { - // @if CK_DEBUG // console.log( 'NOT IMPLEMENTED YET: Pasted table is smaller than selection area.' ); + const selectionHeight = lastRowOfSelection - firstRowOfSelection + 1; + const selectionWidth = lastColumnOfSelection - firstColumnOfSelection + 1; - return; - } + const pasteHeight = tableUtils.getRows( pastedTable ); + const pasteWidth = tableUtils.getColumns( pastedTable ); - const model = this.editor.model; + // The if below is temporal and will be removed when handling this case. + // See: https://github.com/ckeditor/ckeditor5/issues/6769. + if ( selectionHeight > pasteHeight || selectionWidth > pasteWidth ) { + // @if CK_DEBUG // console.log( 'NOT IMPLEMENTED YET: Pasted table is smaller than selection area.' ); + + return; + } - model.change( writer => { // Crop pasted table if it extends selection area. if ( selectionHeight < pasteHeight || selectionWidth < pasteWidth ) { const cropDimensions = { @@ -299,3 +302,23 @@ function createLocationMap( table, width, height ) { return map; } + +function prepareLandingPlace( selectedTableCells, writer ) { + const table = findAncestor( 'table', selectedTableCells[ 0 ] ); + + const { first: firstRow, last: lastRow } = getRowIndexes( selectedTableCells ); + + if ( firstRow > 0 ) { + const cellsToSplitHorizontallyAtFirstRow = getOverlappingCells( table, firstRow, 0 ); + + for ( const cell of cellsToSplitHorizontallyAtFirstRow ) { + splitHorizontally( cell, firstRow, writer ); + } + } + + const cellsToSplitHorizontallyAtLastRow = getOverlappingCells( table, lastRow + 1, firstRow ); + + for ( const cell of cellsToSplitHorizontallyAtLastRow ) { + splitHorizontally( cell, lastRow + 1, writer ); + } +} From 01002e86a7d75f2e0613a091c17b9a6336c8d744 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 15 May 2020 14:07:07 +0200 Subject: [PATCH 04/39] Change the cut cells horizontally utility to single function. --- .../src/commands/setheaderrowcommand.js | 18 +++++++++++------- packages/ckeditor5-table/src/tableclipboard.js | 14 +++----------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js index 26ae832b9bd..06e6fee1403 100644 --- a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js +++ b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js @@ -77,11 +77,7 @@ export default class SetHeaderRowCommand extends Command { if ( headingRowsToSet ) { // Changing heading rows requires to check if any of a heading cell is overlapping vertically the table head. // Any table cell that has a rowspan attribute > 1 will not exceed the table head so we need to fix it in rows below. - const cellsToSplit = getOverlappingCells( table, headingRowsToSet, currentHeadingRows ); - - for ( const cell of cellsToSplit ) { - splitHorizontally( cell, headingRowsToSet, writer ); - } + cutCellsHorizontallyAt( table, headingRowsToSet, currentHeadingRows, writer ); } updateNumericAttribute( 'headingRows', headingRowsToSet, table, writer, 0 ); @@ -103,13 +99,21 @@ export default class SetHeaderRowCommand extends Command { } } +export function cutCellsHorizontallyAt( table, headingRowsToSet, currentHeadingRows, writer ) { + const cellsToSplit = getOverlappingCells( table, headingRowsToSet, currentHeadingRows ); + + for ( const cell of cellsToSplit ) { + splitHorizontally( cell, headingRowsToSet, writer ); + } +} + // Returns cells that span beyond the new heading section. // // @param {module:engine/model/element~Element} table The table to check. // @param {Number} headingRowsToSet New heading rows attribute. // @param {Number} currentHeadingRows Current heading rows attribute. // @returns {Array.} -export function getOverlappingCells( table, headingRowsToSet, currentHeadingRows ) { +function getOverlappingCells( table, headingRowsToSet, currentHeadingRows ) { const cellsToSplit = []; const startAnalysisRow = headingRowsToSet > currentHeadingRows ? currentHeadingRows : 0; @@ -132,7 +136,7 @@ export function getOverlappingCells( table, headingRowsToSet, currentHeadingRows // @param {module:engine/model/element~Element} tableCell // @param {Number} headingRows // @param {module:engine/model/writer~Writer} writer -export function splitHorizontally( tableCell, headingRows, writer ) { +function splitHorizontally( tableCell, headingRows, writer ) { const tableRow = tableCell.parent; const table = tableRow.parent; const rowIndex = tableRow.index; diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index b1ac471c84e..33de18756e6 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -14,7 +14,7 @@ import TableWalker from './tablewalker'; import { getColumnIndexes, getRowIndexes, isSelectionRectangular } from './utils'; import { findAncestor } from './commands/utils'; import { cropTableToDimensions } from './tableselection/croptable'; -import { getOverlappingCells, splitHorizontally } from './commands/setheaderrowcommand'; +import { cutCellsHorizontallyAt } from './commands/setheaderrowcommand'; import TableUtils from './tableutils'; /** @@ -309,16 +309,8 @@ function prepareLandingPlace( selectedTableCells, writer ) { const { first: firstRow, last: lastRow } = getRowIndexes( selectedTableCells ); if ( firstRow > 0 ) { - const cellsToSplitHorizontallyAtFirstRow = getOverlappingCells( table, firstRow, 0 ); - - for ( const cell of cellsToSplitHorizontallyAtFirstRow ) { - splitHorizontally( cell, firstRow, writer ); - } + cutCellsHorizontallyAt( table, firstRow, 0, writer ); } - const cellsToSplitHorizontallyAtLastRow = getOverlappingCells( table, lastRow + 1, firstRow ); - - for ( const cell of cellsToSplitHorizontallyAtLastRow ) { - splitHorizontally( cell, lastRow + 1, writer ); - } + cutCellsHorizontallyAt( table, lastRow + 1, firstRow, writer ); } From 8161c727cdd257eb9cf9597b228baa9ba086ac2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 15 May 2020 14:09:39 +0200 Subject: [PATCH 05/39] Move cutCellsHorizontallyAt() to general utils. --- .../src/commands/setheaderrowcommand.js | 87 +------------------ .../ckeditor5-table/src/tableclipboard.js | 3 +- packages/ckeditor5-table/src/utils.js | 85 +++++++++++++++++- 3 files changed, 87 insertions(+), 88 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js index 06e6fee1403..0d7237b1872 100644 --- a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js +++ b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js @@ -9,9 +9,8 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; -import { createEmptyTableCell, findAncestor, updateNumericAttribute } from './utils'; -import { getRowIndexes, getSelectionAffectedTableCells } from '../utils'; -import TableWalker from '../tablewalker'; +import { findAncestor, updateNumericAttribute } from './utils'; +import { cutCellsHorizontallyAt, getRowIndexes, getSelectionAffectedTableCells } from '../utils'; /** * The header row command. @@ -98,85 +97,3 @@ export default class SetHeaderRowCommand extends Command { return !!headingRows && tableCell.parent.index < headingRows; } } - -export function cutCellsHorizontallyAt( table, headingRowsToSet, currentHeadingRows, writer ) { - const cellsToSplit = getOverlappingCells( table, headingRowsToSet, currentHeadingRows ); - - for ( const cell of cellsToSplit ) { - splitHorizontally( cell, headingRowsToSet, writer ); - } -} - -// Returns cells that span beyond the new heading section. -// -// @param {module:engine/model/element~Element} table The table to check. -// @param {Number} headingRowsToSet New heading rows attribute. -// @param {Number} currentHeadingRows Current heading rows attribute. -// @returns {Array.} -function getOverlappingCells( table, headingRowsToSet, currentHeadingRows ) { - const cellsToSplit = []; - - const startAnalysisRow = headingRowsToSet > currentHeadingRows ? currentHeadingRows : 0; - // We're analyzing only when headingRowsToSet > 0. - const endAnalysisRow = headingRowsToSet - 1; - - const tableWalker = new TableWalker( table, { startRow: startAnalysisRow, endRow: endAnalysisRow } ); - - for ( const { row, rowspan, cell } of tableWalker ) { - if ( rowspan > 1 && row + rowspan > headingRowsToSet ) { - cellsToSplit.push( cell ); - } - } - - return cellsToSplit; -} - -// Splits the table cell horizontally. -// -// @param {module:engine/model/element~Element} tableCell -// @param {Number} headingRows -// @param {module:engine/model/writer~Writer} writer -function splitHorizontally( tableCell, headingRows, writer ) { - const tableRow = tableCell.parent; - const table = tableRow.parent; - const rowIndex = tableRow.index; - - const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) ); - const newRowspan = headingRows - rowIndex; - - const attributes = {}; - - const spanToSet = rowspan - newRowspan; - - if ( spanToSet > 1 ) { - attributes.rowspan = spanToSet; - } - - const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 ); - - if ( colspan > 1 ) { - attributes.colspan = colspan; - } - - const startRow = table.getChildIndex( tableRow ); - const endRow = startRow + newRowspan; - const tableMap = [ ...new TableWalker( table, { startRow, endRow, includeSpanned: true } ) ]; - - let columnIndex; - - for ( const { row, column, cell, cellIndex } of tableMap ) { - if ( cell === tableCell && columnIndex === undefined ) { - columnIndex = column; - } - - if ( columnIndex !== undefined && columnIndex === column && row === endRow ) { - const tableRow = table.getChild( row ); - const tableCellPosition = writer.createPositionAt( tableRow, cellIndex ); - - createEmptyTableCell( writer, tableCellPosition, attributes ); - } - } - - // Update the rowspan attribute after updating table. - updateNumericAttribute( 'rowspan', newRowspan, tableCell, writer ); -} diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 33de18756e6..d4196bbdff8 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -11,10 +11,9 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import TableSelection from './tableselection'; import TableWalker from './tablewalker'; -import { getColumnIndexes, getRowIndexes, isSelectionRectangular } from './utils'; +import { cutCellsHorizontallyAt, getColumnIndexes, getRowIndexes, isSelectionRectangular } from './utils'; import { findAncestor } from './commands/utils'; import { cropTableToDimensions } from './tableselection/croptable'; -import { cutCellsHorizontallyAt } from './commands/setheaderrowcommand'; import TableUtils from './tableutils'; /** diff --git a/packages/ckeditor5-table/src/utils.js b/packages/ckeditor5-table/src/utils.js index d6cdae0e439..4a207ee5f82 100644 --- a/packages/ckeditor5-table/src/utils.js +++ b/packages/ckeditor5-table/src/utils.js @@ -8,7 +8,7 @@ */ import { isWidget, toWidget } from '@ckeditor/ckeditor5-widget/src/utils'; -import { findAncestor } from './commands/utils'; +import { createEmptyTableCell, findAncestor, updateNumericAttribute } from './commands/utils'; import TableWalker from './tablewalker'; /** @@ -247,6 +247,89 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) { return areaOfValidSelection == areaOfSelectedCells; } +// TODO: refactor it to a better, general util. +export function cutCellsHorizontallyAt( table, headingRowsToSet, currentHeadingRows, writer ) { + const cellsToSplit = getHorizontallyOverlappingCells( table, headingRowsToSet, currentHeadingRows ); + + for ( const cell of cellsToSplit ) { + splitHorizontally( cell, headingRowsToSet, writer ); + } +} + +// Returns cells that span beyond the new heading section. +// +// @param {module:engine/model/element~Element} table The table to check. +// @param {Number} headingRowsToSet New heading rows attribute. +// @param {Number} currentHeadingRows Current heading rows attribute. +// @returns {Array.} +function getHorizontallyOverlappingCells( table, headingRowsToSet, currentHeadingRows ) { + const cellsToSplit = []; + + const startAnalysisRow = headingRowsToSet > currentHeadingRows ? currentHeadingRows : 0; + // We're analyzing only when headingRowsToSet > 0. + const endAnalysisRow = headingRowsToSet - 1; + + const tableWalker = new TableWalker( table, { startRow: startAnalysisRow, endRow: endAnalysisRow } ); + + for ( const { row, rowspan, cell } of tableWalker ) { + if ( rowspan > 1 && row + rowspan > headingRowsToSet ) { + cellsToSplit.push( cell ); + } + } + + return cellsToSplit; +} + +// Splits the table cell horizontally. +// +// @param {module:engine/model/element~Element} tableCell +// @param {Number} headingRows +// @param {module:engine/model/writer~Writer} writer +function splitHorizontally( tableCell, headingRows, writer ) { + const tableRow = tableCell.parent; + const table = tableRow.parent; + const rowIndex = tableRow.index; + + const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) ); + const newRowspan = headingRows - rowIndex; + + const attributes = {}; + + const spanToSet = rowspan - newRowspan; + + if ( spanToSet > 1 ) { + attributes.rowspan = spanToSet; + } + + const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 ); + + if ( colspan > 1 ) { + attributes.colspan = colspan; + } + + const startRow = table.getChildIndex( tableRow ); + const endRow = startRow + newRowspan; + const tableMap = [ ...new TableWalker( table, { startRow, endRow, includeSpanned: true } ) ]; + + let columnIndex; + + for ( const { row, column, cell, cellIndex } of tableMap ) { + if ( cell === tableCell && columnIndex === undefined ) { + columnIndex = column; + } + + if ( columnIndex !== undefined && columnIndex === column && row === endRow ) { + const tableRow = table.getChild( row ); + const tableCellPosition = writer.createPositionAt( tableRow, cellIndex ); + + createEmptyTableCell( writer, tableCellPosition, attributes ); + } + } + + // Update the rowspan attribute after updating table. + updateNumericAttribute( 'rowspan', newRowspan, tableCell, writer ); +} + // 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 ); From 785c7c650f7d1dd18dd34a89cbb50c9a4b2daa75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 15 May 2020 14:39:47 +0200 Subject: [PATCH 06/39] Fix tests scenarios. --- .../tests/tableclipboard-paste.js | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index a8982ad20c6..ecaa160790e 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -1271,8 +1271,8 @@ describe( 'table clipboard', () => { // Select 02 -> 23 tableSelection.setCellSelection( - modelRoot.getNodeByPath( [ 0, 2, 0 ] ), - modelRoot.getNodeByPath( [ 0, 3, 1 ] ) + modelRoot.getNodeByPath( [ 0, 0, 2 ] ), + modelRoot.getNodeByPath( [ 0, 2, 3 ] ) ); pasteTable( [ @@ -1311,8 +1311,8 @@ describe( 'table clipboard', () => { // Select 01 -> 22 tableSelection.setCellSelection( - modelRoot.getNodeByPath( [ 0, 1, 0 ] ), - modelRoot.getNodeByPath( [ 0, 2, 1 ] ) + modelRoot.getNodeByPath( [ 0, 0, 1 ] ), + modelRoot.getNodeByPath( [ 0, 2, 2 ] ) ); pasteTable( [ @@ -1329,9 +1329,9 @@ describe( 'table clipboard', () => { // | 20 | ca | cb | 23 | 24 | // +----+----+----+----+----+ assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ - [ '00', '01', 'aa', 'ab', '04' ], - [ { contents: '10', colspan: 2 }, 'ba', 'bb', '14' ], - [ '20', '21', 'ca', 'cb', '24' ] + [ '00', 'aa', 'ab', '03', '04' ], + [ '10', 'ba', 'bb', '', '14' ], + [ '20', 'ca', 'cb', '23', '24' ] ] ) ); } ); @@ -1352,7 +1352,7 @@ describe( 'table clipboard', () => { // Select 00 -> 21 tableSelection.setCellSelection( modelRoot.getNodeByPath( [ 0, 0, 0 ] ), - modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + modelRoot.getNodeByPath( [ 0, 2, 1 ] ) ); pasteTable( [ @@ -1364,20 +1364,20 @@ describe( 'table clipboard', () => { // +----+----+----+----+----+ // | aa | ab | 02 | 03 | 04 | // +----+----+----+----+----+ - // | ba | bb | | | 14 | + // | ba | bb | | 14 | // +----+----+----+----+----+ // | ca | cb | 22 | 23 | 24 | // +----+----+----+----+----+ assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ - [ '00', '01', 'aa', 'ab', '04' ], - [ { contents: '10', colspan: 2 }, 'ba', 'bb', '14' ], - [ '20', '21', 'ca', 'cb', '24' ] + [ 'aa', 'ab', '02', '03', '04' ], + [ 'ba', 'bb', { colspan: 2, contents: '' }, '14' ], + [ 'ca', 'cb', '22', '23', '24' ] ] ) ); } ); it( 'should properly handle complex case', () => { // +----+----+----+----+----+----+----+ - // | 00 | 03 | 04 | 05 | 06 | + // | 00 | 03 | 04 | // + + +----+----+----+ // | | | 14 | 15 | 16 | // + + +----+----+----+ @@ -1392,7 +1392,7 @@ describe( 'table clipboard', () => { // | 60 | 61 | 62 | | 64 | | // +----+----+----+----+----+----+----+ setModelData( model, modelTable( [ - [ { contents: '00', colspan: 3, rowspan: 3 }, { contents: '03', rowspan: 3 }, '04', '05', '06' ], + [ { contents: '00', colspan: 3, rowspan: 3 }, { contents: '03', rowspan: 3 }, { colspan: 3, contents: '04' } ], [ '14', '15', '16' ], [ { contents: '24', colspan: 3 } ], [ @@ -1419,7 +1419,7 @@ describe( 'table clipboard', () => { ] ); // +----+----+----+----+----+----+----+ - // | 00 | | 03 | 04 | 05 | 06 | + // | 00 | | 03 | 04 | // + + + +----+----+----+ // | | | | 14 | 15 | 16 | // +----+----+----+----+----+----+----+ From 1b41a3fd7c92d41146de1846d99b988ce501cfb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 15 May 2020 14:41:34 +0200 Subject: [PATCH 07/39] Introduce cutCellsVerticallyAt() and use it pasting scenario. --- .../ckeditor5-table/src/tableclipboard.js | 9 ++- packages/ckeditor5-table/src/utils.js | 67 +++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index d4196bbdff8..424f7610318 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -11,7 +11,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import TableSelection from './tableselection'; import TableWalker from './tablewalker'; -import { cutCellsHorizontallyAt, getColumnIndexes, getRowIndexes, isSelectionRectangular } from './utils'; +import { cutCellsHorizontallyAt, cutCellsVerticallyAt, getColumnIndexes, getRowIndexes, isSelectionRectangular } from './utils'; import { findAncestor } from './commands/utils'; import { cropTableToDimensions } from './tableselection/croptable'; import TableUtils from './tableutils'; @@ -306,10 +306,17 @@ function prepareLandingPlace( selectedTableCells, writer ) { const table = findAncestor( 'table', selectedTableCells[ 0 ] ); const { first: firstRow, last: lastRow } = getRowIndexes( selectedTableCells ); + const { first: firstColumn, last: lastColumn } = getColumnIndexes( selectedTableCells ); if ( firstRow > 0 ) { cutCellsHorizontallyAt( table, firstRow, 0, writer ); } cutCellsHorizontallyAt( table, lastRow + 1, firstRow, writer ); + + if ( firstColumn > 0 ) { + cutCellsVerticallyAt( table, firstColumn, 0, writer ); + } + + cutCellsVerticallyAt( table, lastColumn + 1, firstColumn, writer ); } diff --git a/packages/ckeditor5-table/src/utils.js b/packages/ckeditor5-table/src/utils.js index 4a207ee5f82..b19c3db3ef6 100644 --- a/packages/ckeditor5-table/src/utils.js +++ b/packages/ckeditor5-table/src/utils.js @@ -256,6 +256,15 @@ export function cutCellsHorizontallyAt( table, headingRowsToSet, currentHeadingR } } +// TODO: refactor it to a better, general util. +export function cutCellsVerticallyAt( table, headingColumnsToSet, currentHeadingColumns, writer ) { + const cellsToSplit = getVerticallyOverlappingCells( table, headingColumnsToSet, currentHeadingColumns ); + + for ( const { cell, column } of cellsToSplit ) { + splitVertically( cell, column, headingColumnsToSet, writer ); + } +} + // Returns cells that span beyond the new heading section. // // @param {module:engine/model/element~Element} table The table to check. @@ -330,6 +339,64 @@ function splitHorizontally( tableCell, headingRows, writer ) { updateNumericAttribute( 'rowspan', newRowspan, tableCell, writer ); } +// Returns cells that span beyond the new heading section. +// +// @param {module:engine/model/element~Element} table The table to check. +// @param {Number} headingColumnsToSet New heading columns attribute. +// @param {Number} currentHeadingColumns Current heading columns attribute. +// @returns {Array.} +function getVerticallyOverlappingCells( table, headingColumnsToSet, currentHeadingColumns ) { + const cellsToSplit = []; + + const startAnalysisColumn = headingColumnsToSet > currentHeadingColumns ? currentHeadingColumns : 0; + // We're analyzing only when headingColumnsToSet > 0. + const endAnalysisColumn = headingColumnsToSet - 1; + + // todo: end/start column + const tableWalker = new TableWalker( table ); + + for ( const { column, colspan, cell } of tableWalker ) { + // Skip slots outside the cropped area. + // Could use startColumn, endColumn. See: https://github.com/ckeditor/ckeditor5/issues/6785. + if ( startAnalysisColumn > column || column > endAnalysisColumn ) { + continue; + } + if ( colspan > 1 && column + colspan > headingColumnsToSet ) { + cellsToSplit.push( { cell, column } ); + } + } + + return cellsToSplit; +} + +// Splits the table cell vertically. +// +// @param {module:engine/model/element~Element} tableCell +// @param {Number} headingColumns +// @param {module:engine/model/writer~Writer} writer +function splitVertically( tableCell, columnIndex, headingColumns, writer ) { + const colspan = parseInt( tableCell.getAttribute( 'colspan' ) ); + const newColspan = headingColumns - columnIndex; + + const attributes = {}; + + const spanToSet = colspan - newColspan; + + if ( spanToSet > 1 ) { + attributes.colspan = spanToSet; + } + + const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) || 1 ); + + if ( rowspan > 1 ) { + attributes.rowspan = rowspan; + } + + createEmptyTableCell( writer, writer.createPositionAfter( tableCell ), attributes ); + // Update the colspan attribute after updating table. + updateNumericAttribute( 'colspan', newColspan, tableCell, writer ); +} + // 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 ); From 94f582b8e32ea67ad298ee87ac79df04d4fb94e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 15 May 2020 15:08:01 +0200 Subject: [PATCH 08/39] Filter cells to check when splitting table. --- .../ckeditor5-table/src/tableclipboard.js | 8 ++-- packages/ckeditor5-table/src/utils.js | 46 +++++++++++++++---- 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 424f7610318..01e7e7df411 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -309,14 +309,14 @@ function prepareLandingPlace( selectedTableCells, writer ) { const { first: firstColumn, last: lastColumn } = getColumnIndexes( selectedTableCells ); if ( firstRow > 0 ) { - cutCellsHorizontallyAt( table, firstRow, 0, writer ); + cutCellsHorizontallyAt( table, firstRow, 0, writer, { firstRow: 0, firstColumn, lastRow: 1000, lastColumn } ); } - cutCellsHorizontallyAt( table, lastRow + 1, firstRow, writer ); + cutCellsHorizontallyAt( table, lastRow + 1, firstRow, writer, { firstRow: 0, firstColumn, lastRow: 1000, lastColumn } ); if ( firstColumn > 0 ) { - cutCellsVerticallyAt( table, firstColumn, 0, writer ); + cutCellsVerticallyAt( table, firstColumn, 0, writer, { firstRow, firstColumn: 0, lastRow, lastColumn: 1000 } ); } - cutCellsVerticallyAt( table, lastColumn + 1, firstColumn, writer ); + cutCellsVerticallyAt( table, lastColumn + 1, firstColumn, writer, { firstRow, firstColumn: 0, lastRow, lastColumn: 1000 } ); } diff --git a/packages/ckeditor5-table/src/utils.js b/packages/ckeditor5-table/src/utils.js index b19c3db3ef6..8bcb66fea17 100644 --- a/packages/ckeditor5-table/src/utils.js +++ b/packages/ckeditor5-table/src/utils.js @@ -248,23 +248,48 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) { } // TODO: refactor it to a better, general util. -export function cutCellsHorizontallyAt( table, headingRowsToSet, currentHeadingRows, writer ) { - const cellsToSplit = getHorizontallyOverlappingCells( table, headingRowsToSet, currentHeadingRows ); +export function cutCellsHorizontallyAt( table, headingRowsToSet, currentHeadingRows, writer, boundingBox ) { + const overlappingCells = getHorizontallyOverlappingCells( table, headingRowsToSet, currentHeadingRows ); - for ( const cell of cellsToSplit ) { + let cellsToSplit; + + if ( boundingBox === undefined ) { + cellsToSplit = overlappingCells; + } else { + cellsToSplit = overlappingCells.filter( filterToBoundingBox( boundingBox ) ); + } + + for ( const { cell } of cellsToSplit ) { splitHorizontally( cell, headingRowsToSet, writer ); } } // TODO: refactor it to a better, general util. -export function cutCellsVerticallyAt( table, headingColumnsToSet, currentHeadingColumns, writer ) { - const cellsToSplit = getVerticallyOverlappingCells( table, headingColumnsToSet, currentHeadingColumns ); +export function cutCellsVerticallyAt( table, headingColumnsToSet, currentHeadingColumns, writer, boundingBox ) { + const overlappingCells = getVerticallyOverlappingCells( table, headingColumnsToSet, currentHeadingColumns ); + + let cellsToSplit; + + if ( boundingBox === undefined ) { + cellsToSplit = overlappingCells; + } else { + cellsToSplit = overlappingCells.filter( filterToBoundingBox( boundingBox ) ); + } for ( const { cell, column } of cellsToSplit ) { splitVertically( cell, column, headingColumnsToSet, writer ); } } +// TODO: better fit to bounding box to match criteria.. should check also spans because sometimes we need to split them. +function filterToBoundingBox( boundingBox ) { + const { firstRow, firstColumn, lastRow, lastColumn } = boundingBox; + + return ( { row, column } ) => { + return ( ( firstRow <= row ) && ( row <= lastRow ) ) && ( firstColumn <= column && column <= lastColumn ); + }; +} + // Returns cells that span beyond the new heading section. // // @param {module:engine/model/element~Element} table The table to check. @@ -280,9 +305,11 @@ function getHorizontallyOverlappingCells( table, headingRowsToSet, currentHeadin const tableWalker = new TableWalker( table, { startRow: startAnalysisRow, endRow: endAnalysisRow } ); - for ( const { row, rowspan, cell } of tableWalker ) { + for ( const twv of tableWalker ) { + const { row, rowspan } = twv; + if ( rowspan > 1 && row + rowspan > headingRowsToSet ) { - cellsToSplit.push( cell ); + cellsToSplit.push( twv ); } } @@ -355,14 +382,15 @@ function getVerticallyOverlappingCells( table, headingColumnsToSet, currentHeadi // todo: end/start column const tableWalker = new TableWalker( table ); - for ( const { column, colspan, cell } of tableWalker ) { + for ( const twv of tableWalker ) { + const { column, colspan } = twv; // Skip slots outside the cropped area. // Could use startColumn, endColumn. See: https://github.com/ckeditor/ckeditor5/issues/6785. if ( startAnalysisColumn > column || column > endAnalysisColumn ) { continue; } if ( colspan > 1 && column + colspan > headingColumnsToSet ) { - cellsToSplit.push( { cell, column } ); + cellsToSplit.push( twv ); } } From 5ad37bfc64d0d10f48b95960038cc82435a57be7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 18 May 2020 09:57:47 +0200 Subject: [PATCH 09/39] Cut table cells to a selection bounding box before paste. --- packages/ckeditor5-table/src/tableclipboard.js | 16 ++++++---------- packages/ckeditor5-table/src/utils.js | 5 +++-- .../tests/tableclipboard-paste.js | 12 ++++++------ 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 01e7e7df411..05e9f55cfa7 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -133,11 +133,7 @@ export default class TableClipboard extends Plugin { // Currently not handled. The selected table content should be trimmed to a rectangular selection. // See: https://github.com/ckeditor/ckeditor5/issues/6122. if ( !isSelectionRectangular( selectedTableCells, tableUtils ) ) { - // @if CK_DEBUG // console.log( 'NOT IMPLEMENTED YET: Selection is not rectangular (non-mergeable).' ); - prepareLandingPlace( selectedTableCells, writer ); - - // return; } const { last: lastColumnOfSelection, first: firstColumnOfSelection } = getColumnIndexes( selectedTableCells ); @@ -308,15 +304,15 @@ function prepareLandingPlace( selectedTableCells, writer ) { const { first: firstRow, last: lastRow } = getRowIndexes( selectedTableCells ); const { first: firstColumn, last: lastColumn } = getColumnIndexes( selectedTableCells ); - if ( firstRow > 0 ) { - cutCellsHorizontallyAt( table, firstRow, 0, writer, { firstRow: 0, firstColumn, lastRow: 1000, lastColumn } ); - } - - cutCellsHorizontallyAt( table, lastRow + 1, firstRow, writer, { firstRow: 0, firstColumn, lastRow: 1000, lastColumn } ); - if ( firstColumn > 0 ) { cutCellsVerticallyAt( table, firstColumn, 0, writer, { firstRow, firstColumn: 0, lastRow, lastColumn: 1000 } ); } cutCellsVerticallyAt( table, lastColumn + 1, firstColumn, writer, { firstRow, firstColumn: 0, lastRow, lastColumn: 1000 } ); + + if ( firstRow > 0 ) { + cutCellsHorizontallyAt( table, firstRow, 0, writer, { firstRow: 0, firstColumn, lastRow: 1000, lastColumn } ); + } + + cutCellsHorizontallyAt( table, lastRow + 1, firstRow, writer, { firstRow: 0, firstColumn, lastRow: 1000, lastColumn } ); } diff --git a/packages/ckeditor5-table/src/utils.js b/packages/ckeditor5-table/src/utils.js index 8bcb66fea17..9075cd2c6a0 100644 --- a/packages/ckeditor5-table/src/utils.js +++ b/packages/ckeditor5-table/src/utils.js @@ -285,8 +285,9 @@ export function cutCellsVerticallyAt( table, headingColumnsToSet, currentHeading function filterToBoundingBox( boundingBox ) { const { firstRow, firstColumn, lastRow, lastColumn } = boundingBox; - return ( { row, column } ) => { - return ( ( firstRow <= row ) && ( row <= lastRow ) ) && ( firstColumn <= column && column <= lastColumn ); + return ( { row, column, colspan, rowspan } ) => { + return ( ( firstRow <= row + rowspan - 1 ) && ( row + rowspan - 1 <= lastRow ) ) && + ( firstColumn <= column + colspan - 1 && column + colspan - 1 <= lastColumn ); }; } diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index ecaa160790e..15bc0e0d355 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -1422,29 +1422,29 @@ describe( 'table clipboard', () => { // | 00 | | 03 | 04 | // + + + +----+----+----+ // | | | | 14 | 15 | 16 | - // +----+----+----+----+----+----+----+ + // + +----+----+----+----+----+ // | | aa | ab | ac | | // +----+----+----+----+----+----+----+ // | 30 | 31 | ba | bb | bc | 35 | 36 | // + +----+----+----+----+----+----+ // | | 41 | ca | cb | cc | 45 | // + +----+----+----+----+ + - // | | 51 | 52 | 53 | 54 | | + // | | 51 | 52 | | 54 | | // +----+----+----+ +----+ + // | 60 | 61 | 62 | | 64 | | // +----+----+----+----+----+----+----+ assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ [ - { contents: '00', colspan: 2, rowspan: 2 }, + { contents: '00', colspan: 2, rowspan: 3 }, { contents: '', rowspan: 2 }, { contents: '03', rowspan: 2 }, - '04', '05', '06' + { contents: '04', colspan: 3 } ], [ '14', '15', '16' ], - [ { contents: '', colspan: 2 }, 'aa', 'ab', 'ac', { contents: '', colspan: 2 } ], + [ 'aa', 'ab', 'ac', { contents: '', colspan: 2 } ], [ { contents: '30', rowspan: 3 }, '31', 'ba', 'bb', 'bc', '35', '36' ], [ '41', 'ca', 'cb', 'cc', { contents: '45', colspan: 2, rowspan: 3 } ], - [ '51', '52', { contents: '53', rowspan: 2 }, '54' ], + [ '51', '52', { contents: '', rowspan: 2 }, '54' ], [ '60', '61', '62', '64' ] ] ) ); } ); From c67e84d0f8c9ae59760c771427d3c2d9cd2cb61e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 18 May 2020 12:55:13 +0200 Subject: [PATCH 10/39] Refactor utility methods. --- .../src/commands/setheaderrowcommand.js | 9 +- .../ckeditor5-table/src/tableclipboard.js | 68 +++++-- packages/ckeditor5-table/src/utils.js | 177 ++++++++---------- 3 files changed, 147 insertions(+), 107 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js index 0d7237b1872..9c92dd26d1f 100644 --- a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js +++ b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js @@ -10,7 +10,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; import { findAncestor, updateNumericAttribute } from './utils'; -import { cutCellsHorizontallyAt, getRowIndexes, getSelectionAffectedTableCells } from '../utils'; +import { getHorizontallyOverlappingCells, getRowIndexes, getSelectionAffectedTableCells, splitHorizontally } from '../utils'; /** * The header row command. @@ -76,7 +76,12 @@ export default class SetHeaderRowCommand extends Command { if ( headingRowsToSet ) { // Changing heading rows requires to check if any of a heading cell is overlapping vertically the table head. // Any table cell that has a rowspan attribute > 1 will not exceed the table head so we need to fix it in rows below. - cutCellsHorizontallyAt( table, headingRowsToSet, currentHeadingRows, writer ); + const startRow = headingRowsToSet > currentHeadingRows ? currentHeadingRows : 0; + const overlappingCells = getHorizontallyOverlappingCells( table, headingRowsToSet, startRow ); + + for ( const { cell } of overlappingCells ) { + splitHorizontally( cell, headingRowsToSet, writer ); + } } updateNumericAttribute( 'headingRows', headingRowsToSet, table, writer, 0 ); diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 05e9f55cfa7..f836a0da8cc 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -11,7 +11,15 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import TableSelection from './tableselection'; import TableWalker from './tablewalker'; -import { cutCellsHorizontallyAt, cutCellsVerticallyAt, getColumnIndexes, getRowIndexes, isSelectionRectangular } from './utils'; +import { + getColumnIndexes, + getRowIndexes, + getHorizontallyOverlappingCells, + getVerticallyOverlappingCells, + isSelectionRectangular, + splitHorizontally, + splitVertically +} from './utils'; import { findAncestor } from './commands/utils'; import { cropTableToDimensions } from './tableselection/croptable'; import TableUtils from './tableutils'; @@ -133,7 +141,7 @@ export default class TableClipboard extends Plugin { // Currently not handled. The selected table content should be trimmed to a rectangular selection. // See: https://github.com/ckeditor/ckeditor5/issues/6122. if ( !isSelectionRectangular( selectedTableCells, tableUtils ) ) { - prepareLandingPlace( selectedTableCells, writer ); + makeSelectedCellsRectangular( selectedTableCells, writer ); } const { last: lastColumnOfSelection, first: firstColumnOfSelection } = getColumnIndexes( selectedTableCells ); @@ -298,21 +306,59 @@ function createLocationMap( table, width, height ) { return map; } -function prepareLandingPlace( selectedTableCells, writer ) { +// Make selected cell rectangular by splitting the cells that stand out from a rectangular selection. +function makeSelectedCellsRectangular( selectedTableCells, writer ) { const table = findAncestor( 'table', selectedTableCells[ 0 ] ); - const { first: firstRow, last: lastRow } = getRowIndexes( selectedTableCells ); - const { first: firstColumn, last: lastColumn } = getColumnIndexes( selectedTableCells ); + const rowIndexes = getRowIndexes( selectedTableCells ); + const columnIndexes = getColumnIndexes( selectedTableCells ); + const { first: firstRow, last: lastRow } = rowIndexes; + const { first: firstColumn, last: lastColumn } = columnIndexes; - if ( firstColumn > 0 ) { - cutCellsVerticallyAt( table, firstColumn, 0, writer, { firstRow, firstColumn: 0, lastRow, lastColumn: 1000 } ); + doVerticalSplit( table, firstColumn, rowIndexes, writer ); // TODO: Could use startColumn = 0. + doVerticalSplit( table, lastColumn + 1, rowIndexes, writer ); // TODO: Could use startColumn = firstColumn. + + doHorizontalSplit( table, firstRow, columnIndexes, writer, 0 ); + doHorizontalSplit( table, lastRow + 1, columnIndexes, writer, firstRow ); +} + +function doHorizontalSplit( table, splitRow, columnIndexes, writer, startRow ) { + if ( splitRow < 1 ) { + return; } - cutCellsVerticallyAt( table, lastColumn + 1, firstColumn, writer, { firstRow, firstColumn: 0, lastRow, lastColumn: 1000 } ); + const overlappingCells = getHorizontallyOverlappingCells( table, splitRow, startRow ); + + const { first, last } = columnIndexes; + const cellsToSplit = overlappingCells.filter( ( { column, colspan } ) => isBetweenColumns( column, colspan, first, last ) ); - if ( firstRow > 0 ) { - cutCellsHorizontallyAt( table, firstRow, 0, writer, { firstRow: 0, firstColumn, lastRow: 1000, lastColumn } ); + for ( const { cell } of cellsToSplit ) { + splitHorizontally( cell, splitRow, writer ); } +} + +function doVerticalSplit( table, splitColumn, rowIndexes, writer ) { + if ( splitColumn < 1 ) { + return; + } + + const overlappingCells = getVerticallyOverlappingCells( table, splitColumn ); + + const { first, last } = rowIndexes; + const cellsToSplit = overlappingCells.filter( ( { row, rowspan } ) => isBetweenRows( row, rowspan, first, last ) ); + + for ( const { cell, column } of cellsToSplit ) { + splitVertically( cell, column, splitColumn, writer ); + } +} + +function isBetweenRows( row, rowspan, first, last ) { + const endRowOfCell = row + rowspan - 1; + return first <= endRowOfCell && endRowOfCell <= last; +} + +function isBetweenColumns( column, colspan, first, last ) { + const endColumnOfCell = column + colspan - 1; - cutCellsHorizontallyAt( table, lastRow + 1, firstRow, writer, { firstRow: 0, firstColumn, lastRow: 1000, lastColumn } ); + return first <= endColumnOfCell && endColumnOfCell <= last; } diff --git a/packages/ckeditor5-table/src/utils.js b/packages/ckeditor5-table/src/utils.js index 9075cd2c6a0..0b8e15c73bf 100644 --- a/packages/ckeditor5-table/src/utils.js +++ b/packages/ckeditor5-table/src/utils.js @@ -188,7 +188,7 @@ export function getColumnIndexes( tableCells ) { * │ a │ b │ c │ d │ * ├───┴───┼───┤ │ * │ e │ f │ │ - * ├ ├───┼───┤ + * │ ├───┼───┤ * │ │ g │ h │ * └───────┴───┴───┘ * @@ -247,88 +247,63 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) { return areaOfValidSelection == areaOfSelectedCells; } -// TODO: refactor it to a better, general util. -export function cutCellsHorizontallyAt( table, headingRowsToSet, currentHeadingRows, writer, boundingBox ) { - const overlappingCells = getHorizontallyOverlappingCells( table, headingRowsToSet, currentHeadingRows ); - - let cellsToSplit; - - if ( boundingBox === undefined ) { - cellsToSplit = overlappingCells; - } else { - cellsToSplit = overlappingCells.filter( filterToBoundingBox( boundingBox ) ); - } - - for ( const { cell } of cellsToSplit ) { - splitHorizontally( cell, headingRowsToSet, writer ); - } -} - -// TODO: refactor it to a better, general util. -export function cutCellsVerticallyAt( table, headingColumnsToSet, currentHeadingColumns, writer, boundingBox ) { - const overlappingCells = getVerticallyOverlappingCells( table, headingColumnsToSet, currentHeadingColumns ); - - let cellsToSplit; - - if ( boundingBox === undefined ) { - cellsToSplit = overlappingCells; - } else { - cellsToSplit = overlappingCells.filter( filterToBoundingBox( boundingBox ) ); - } - - for ( const { cell, column } of cellsToSplit ) { - splitVertically( cell, column, headingColumnsToSet, writer ); - } -} - -// TODO: better fit to bounding box to match criteria.. should check also spans because sometimes we need to split them. -function filterToBoundingBox( boundingBox ) { - const { firstRow, firstColumn, lastRow, lastColumn } = boundingBox; - - return ( { row, column, colspan, rowspan } ) => { - return ( ( firstRow <= row + rowspan - 1 ) && ( row + rowspan - 1 <= lastRow ) ) && - ( firstColumn <= column + colspan - 1 && column + colspan - 1 <= lastColumn ); - }; -} - -// Returns cells that span beyond the new heading section. -// -// @param {module:engine/model/element~Element} table The table to check. -// @param {Number} headingRowsToSet New heading rows attribute. -// @param {Number} currentHeadingRows Current heading rows attribute. -// @returns {Array.} -function getHorizontallyOverlappingCells( table, headingRowsToSet, currentHeadingRows ) { +/** + * Returns cells that starts below and overlaps a given row. + * + * In a table below, passing `overlapRow = 3` + * + * ┌───┬───┬───┬───┬───┐ + * 0 │ a │ b │ c │ d │ e │ + * │ ├───┼───┼───┼───┤ + * 1 │ │ f │ g │ h │ i │ + * ├───┤ ├───┼───┤ │ + * 2 │ j │ │ k │ l │ │ + * │ │ │ ├───┼───┤ + * 3 │ │ │ │ m │ n │ <- overlap row to check + * ├───┼───┤ │ ├───│ + * 4 │ o │ p │ │ │ q │ + * └───┴───┴───┴───┴───┘ + * + * will return cells: "j", "f", "k". + * + * @param {module:engine/model/element~Element} table The table to check. + * @param {Number} overlapRow + * @param {Number} [startRow=0] A row to start analysis if it is known where. + * @returns {Array.} + */ +export function getHorizontallyOverlappingCells( table, overlapRow, startRow = 0 ) { const cellsToSplit = []; - const startAnalysisRow = headingRowsToSet > currentHeadingRows ? currentHeadingRows : 0; // We're analyzing only when headingRowsToSet > 0. - const endAnalysisRow = headingRowsToSet - 1; + const endAnalysisRow = overlapRow - 1; - const tableWalker = new TableWalker( table, { startRow: startAnalysisRow, endRow: endAnalysisRow } ); + const tableWalker = new TableWalker( table, { startRow, endRow: endAnalysisRow } ); - for ( const twv of tableWalker ) { - const { row, rowspan } = twv; + for ( const slotInfo of tableWalker ) { + const { row, rowspan } = slotInfo; - if ( rowspan > 1 && row + rowspan > headingRowsToSet ) { - cellsToSplit.push( twv ); + if ( rowspan > 1 && row + rowspan > overlapRow ) { + cellsToSplit.push( slotInfo ); } } return cellsToSplit; } -// Splits the table cell horizontally. -// -// @param {module:engine/model/element~Element} tableCell -// @param {Number} headingRows -// @param {module:engine/model/writer~Writer} writer -function splitHorizontally( tableCell, headingRows, writer ) { +/** + * Splits the table cell horizontally. + * + * @param {module:engine/model/element~Element} tableCell + * @param {Number} splitRow + * @param {module:engine/model/writer~Writer} writer + */ +export function splitHorizontally( tableCell, splitRow, writer ) { const tableRow = tableCell.parent; const table = tableRow.parent; const rowIndex = tableRow.index; const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) ); - const newRowspan = headingRows - rowIndex; + const newRowspan = splitRow - rowIndex; const attributes = {}; @@ -367,45 +342,59 @@ function splitHorizontally( tableCell, headingRows, writer ) { updateNumericAttribute( 'rowspan', newRowspan, tableCell, writer ); } -// Returns cells that span beyond the new heading section. -// -// @param {module:engine/model/element~Element} table The table to check. -// @param {Number} headingColumnsToSet New heading columns attribute. -// @param {Number} currentHeadingColumns Current heading columns attribute. -// @returns {Array.} -function getVerticallyOverlappingCells( table, headingColumnsToSet, currentHeadingColumns ) { +/** + * Returns cells that starts before and overlaps a given column. + * + * In a table below, passing `overlapColumn = 3` + * + * 0 1 2 3 4 + * ┌───────┬───────┬───┐ + * │ a │ b │ c │ + * │───┬───┴───────┼───┤ + * │ d │ e │ f │ + * ├───┼───┬───────┴───┤ + * │ g │ h │ i │ + * ├───┼───┼───┬───────┤ + * │ j │ k │ l │ m │ + * ├───┼───┴───┼───┬───┤ + * │ n │ o │ p │ q │ + * └───┴───────┴───┴───┘ + * ^ + * Overlap column to check + * + * will return cells: "b", "e", "i". + * + * @param {module:engine/model/element~Element} table The table to check. + * @param {Number} overlapColumn New heading columns attribute. + * @returns {Array.} + */ +export function getVerticallyOverlappingCells( table, overlapColumn ) { const cellsToSplit = []; - const startAnalysisColumn = headingColumnsToSet > currentHeadingColumns ? currentHeadingColumns : 0; - // We're analyzing only when headingColumnsToSet > 0. - const endAnalysisColumn = headingColumnsToSet - 1; - - // todo: end/start column const tableWalker = new TableWalker( table ); - for ( const twv of tableWalker ) { - const { column, colspan } = twv; - // Skip slots outside the cropped area. - // Could use startColumn, endColumn. See: https://github.com/ckeditor/ckeditor5/issues/6785. - if ( startAnalysisColumn > column || column > endAnalysisColumn ) { - continue; - } - if ( colspan > 1 && column + colspan > headingColumnsToSet ) { - cellsToSplit.push( twv ); + for ( const slotInfo of tableWalker ) { + const { column, colspan } = slotInfo; + const endColumn = column + colspan; + + if ( column < overlapColumn && overlapColumn < endColumn ) { + cellsToSplit.push( slotInfo ); } } return cellsToSplit; } -// Splits the table cell vertically. -// -// @param {module:engine/model/element~Element} tableCell -// @param {Number} headingColumns -// @param {module:engine/model/writer~Writer} writer -function splitVertically( tableCell, columnIndex, headingColumns, writer ) { +/** + * Splits the table cell vertically. + * + * @param {module:engine/model/element~Element} tableCell + * @param {Number} splitColumn + * @param {module:engine/model/writer~Writer} writer + */ +export function splitVertically( tableCell, columnIndex, splitColumn, writer ) { const colspan = parseInt( tableCell.getAttribute( 'colspan' ) ); - const newColspan = headingColumns - columnIndex; + const newColspan = splitColumn - columnIndex; const attributes = {}; From ed3ca18375755336c14206b70a81f57320a63d07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 18 May 2020 13:21:52 +0200 Subject: [PATCH 11/39] Add docs to trimCellsToRectangularSelection(). --- .../ckeditor5-table/src/tableclipboard.js | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index f836a0da8cc..cb3e332d056 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -141,7 +141,7 @@ export default class TableClipboard extends Plugin { // Currently not handled. The selected table content should be trimmed to a rectangular selection. // See: https://github.com/ckeditor/ckeditor5/issues/6122. if ( !isSelectionRectangular( selectedTableCells, tableUtils ) ) { - makeSelectedCellsRectangular( selectedTableCells, writer ); + trimCellsToRectangularSelection( selectedTableCells, writer ); } const { last: lastColumnOfSelection, first: firstColumnOfSelection } = getColumnIndexes( selectedTableCells ); @@ -307,7 +307,43 @@ function createLocationMap( table, width, height ) { } // Make selected cell rectangular by splitting the cells that stand out from a rectangular selection. -function makeSelectedCellsRectangular( selectedTableCells, writer ) { +// +// In the table below a selection is shown with "::" and slots with anchor cells are named. +// +// +----+----+----+----+----+ +----+----+----+----+----+ +// | 00 | 01 | 02 | 03 | | 00 | 01 | 02 | 03 | +// + +----+ +----+----+ | ::::::::::::::::----+ +// | | 11 | | 13 | 14 | | ::11 | | 13:: 14 | <- first row +// +----+----+ + +----+ +----::---| | ::----+ +// | 20 | 21 | | | 24 | select cells: | 20 ::21 | | :: 24 | +// +----+----+ +----+----+ 11 -> 33 +----::---| |---::----+ +// | 30 | | 33 | 34 | | 30 :: | | 33:: 34 | <- last row +// + + +----+ + | :::::::::::::::: + +// | | | 43 | | | | | 43 | | +// +----+----+----+----+----+ +----+----+----+----+----+ +// ^ ^ +// first & last columns +// +// In th example above: +// - Cell "02" which have `rowspan = 4` must be trimmed at first and at after last row. +// - Cell "03" which have `rowspan = 2` and `colspan = 2` must be trimmed at first column and after last row. +// - Cells "00", "03" & "30" which cannot be cut by this algorithm as they are outside the trimmed area. +// - Cell "13" cannot be cut as it is inside the trimmed area. +// +// Will update table to: +// +// +----+----+----+----+----+ +// | 00 | 01 | 02 | 03 | +// + +----+----+----+----+ +// | | 11 | | 13 | 14 | +// +----+----+ + +----+ +// | 20 | 21 | | | 24 | +// +----+----+ +----+----+ +// | 30 | | | 33 | 34 | +// + +----+----+----+ + +// | | | | 43 | | +// +----+----+----+----+----+ +function trimCellsToRectangularSelection( selectedTableCells, writer ) { const table = findAncestor( 'table', selectedTableCells[ 0 ] ); const rowIndexes = getRowIndexes( selectedTableCells ); @@ -315,9 +351,11 @@ function makeSelectedCellsRectangular( selectedTableCells, writer ) { const { first: firstRow, last: lastRow } = rowIndexes; const { first: firstColumn, last: lastColumn } = columnIndexes; + // 1. Split cells vertically in two steps as first step might create cells that needs to split again. doVerticalSplit( table, firstColumn, rowIndexes, writer ); // TODO: Could use startColumn = 0. doVerticalSplit( table, lastColumn + 1, rowIndexes, writer ); // TODO: Could use startColumn = firstColumn. + // 2. Split cells horizontally in two steps as first step might create cells that needs to split again. doHorizontalSplit( table, firstRow, columnIndexes, writer, 0 ); doHorizontalSplit( table, lastRow + 1, columnIndexes, writer, firstRow ); } From 199a199353969fbec158ecb2549a6f4dffdb4e4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 18 May 2020 14:48:04 +0200 Subject: [PATCH 12/39] Add corner cases to the tests. --- .../tests/tableclipboard-paste.js | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 15bc0e0d355..123d23d349e 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -1375,6 +1375,84 @@ describe( 'table clipboard', () => { ] ) ); } ); + it( 'should split cells anchored outside selection rectangle that overlaps selection (above selection)', () => { + // +----+----+----+ + // | 00 | 02 | + // + +----+ + // | | 12 | + // +----+----+----+ + // | 20 | 21 | 22 | + // +----+----+----+ + setModelData( model, modelTable( [ + [ { contents: '00', colspan: 2, rowspan: 2 }, '02' ], + [ '12' ], + [ '20', '21', '22' ] + ] ) ); + + // Select 21 -> 12 + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 2, 1 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', 'bb' ] + ] ); + + // +----+----+----+ + // | 00 | | 02 | + // + +----+----+ + // | | aa | ab | + // +----+----+----+ + // | 20 | ba | bb | + // +----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ { contents: '00', rowspan: 2 }, '', '02' ], + [ 'aa', 'ab' ], + [ '20', 'ba', 'bb' ] + ] ) ); + } ); + + it( 'should split cells anchored outside selection rectangle that overlaps selection (below selection)', () => { + // +----+----+----+ + // | 00 | 01 | 02 | + // +----+----+----+ + // | 10 | 12 | + // + +----+ + // | | 22 | + // +----+----+----+ + setModelData( model, modelTable( [ + [ '00', '01', '02' ], + [ { contents: '10', colspan: 2, rowspan: 2 }, '12' ], + [ '22' ] + ] ) ); + + // Select 01 -> 12 + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 1 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', 'bb' ] + ] ); + + // +----+----+----+ + // | 00 | aa | ab | + // +----+----+----+ + // | 10 | ba | bb | + // + +----+----+ + // | | | 22 | + // +----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', 'aa', 'ab' ], + [ { contents: '10', rowspan: 2 }, 'ba', 'bb' ], + [ '', '22' ] + ] ) ); + } ); + it( 'should properly handle complex case', () => { // +----+----+----+----+----+----+----+ // | 00 | 03 | 04 | From 27a73b7247b193d81a0b33830a4df170536ba2a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 18 May 2020 14:53:02 +0200 Subject: [PATCH 13/39] Refactor the logic of splitting cells to a rectangular selection. --- .../ckeditor5-table/src/tableclipboard.js | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index cb3e332d056..e15308da9c6 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -13,8 +13,8 @@ import TableSelection from './tableselection'; import TableWalker from './tablewalker'; import { getColumnIndexes, - getRowIndexes, getHorizontallyOverlappingCells, + getRowIndexes, getVerticallyOverlappingCells, isSelectionRectangular, splitHorizontally, @@ -352,51 +352,56 @@ function trimCellsToRectangularSelection( selectedTableCells, writer ) { const { first: firstColumn, last: lastColumn } = columnIndexes; // 1. Split cells vertically in two steps as first step might create cells that needs to split again. - doVerticalSplit( table, firstColumn, rowIndexes, writer ); // TODO: Could use startColumn = 0. - doVerticalSplit( table, lastColumn + 1, rowIndexes, writer ); // TODO: Could use startColumn = firstColumn. + doVerticalSplit( table, firstColumn, rowIndexes, writer ); + doVerticalSplit( table, lastColumn + 1, rowIndexes, writer ); // 2. Split cells horizontally in two steps as first step might create cells that needs to split again. doHorizontalSplit( table, firstRow, columnIndexes, writer, 0 ); doHorizontalSplit( table, lastRow + 1, columnIndexes, writer, firstRow ); } -function doHorizontalSplit( table, splitRow, columnIndexes, writer, startRow ) { +function doHorizontalSplit( table, splitRow, limitColumns, writer, startRow ) { + // If selection starts at first row then no split is needed. if ( splitRow < 1 ) { return; } const overlappingCells = getHorizontallyOverlappingCells( table, splitRow, startRow ); - const { first, last } = columnIndexes; - const cellsToSplit = overlappingCells.filter( ( { column, colspan } ) => isBetweenColumns( column, colspan, first, last ) ); + // Filter out cells that are not touching insides of the rectangular selection. + const { first, last } = limitColumns; + const cellsToSplit = overlappingCells.filter( ( { column, colspan } ) => isAffectedBySelection( column, colspan, first, last ) ); for ( const { cell } of cellsToSplit ) { splitHorizontally( cell, splitRow, writer ); } } -function doVerticalSplit( table, splitColumn, rowIndexes, writer ) { +function doVerticalSplit( table, splitColumn, limitRows, writer ) { + // If selection starts at first column then no split is needed. if ( splitColumn < 1 ) { return; } const overlappingCells = getVerticallyOverlappingCells( table, splitColumn ); - const { first, last } = rowIndexes; - const cellsToSplit = overlappingCells.filter( ( { row, rowspan } ) => isBetweenRows( row, rowspan, first, last ) ); + // Filter out cells that are not touching insides of the rectangular selection. + const { first, last } = limitRows; + const cellsToSplit = overlappingCells.filter( ( { row, rowspan } ) => isAffectedBySelection( row, rowspan, first, last ) ); for ( const { cell, column } of cellsToSplit ) { splitVertically( cell, column, splitColumn, writer ); } } -function isBetweenRows( row, rowspan, first, last ) { - const endRowOfCell = row + rowspan - 1; - return first <= endRowOfCell && endRowOfCell <= last; -} +// Checks if cell at given row (column) is affected by a rectangular selection defined by first/last column (row). +// +// The same check is used for row as for column. +function isAffectedBySelection( rowOrColumn, rowOrColumnSpan, first, last ) { + const endIndexOfCell = rowOrColumn + rowOrColumnSpan - 1; -function isBetweenColumns( column, colspan, first, last ) { - const endColumnOfCell = column + colspan - 1; + const isInsideSelection = rowOrColumn >= first && rowOrColumn <= last; + const overlapsSelectionFromOutside = rowOrColumn < first && endIndexOfCell >= first; - return first <= endColumnOfCell && endColumnOfCell <= last; + return isInsideSelection || overlapsSelectionFromOutside; } From e28fea1a89515a7bfee7a134a712b2b8886a93b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 18 May 2020 14:55:19 +0200 Subject: [PATCH 14/39] Change order of jsdoc samples. --- .../ckeditor5-table/src/tableclipboard.js | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index e15308da9c6..134d0711793 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -324,25 +324,25 @@ function createLocationMap( table, width, height ) { // ^ ^ // first & last columns // +// Will update table to: +// +// +----+----+----+----+----+ +// | 00 | 01 | 02 | 03 | +// + +----+----+----+----+ +// | | 11 | | 13 | 14 | +// +----+----+ + +----+ +// | 20 | 21 | | | 24 | +// +----+----+ +----+----+ +// | 30 | | | 33 | 34 | +// + +----+----+----+ + +// | | | | 43 | | +// +----+----+----+----+----+ +// // In th example above: // - Cell "02" which have `rowspan = 4` must be trimmed at first and at after last row. // - Cell "03" which have `rowspan = 2` and `colspan = 2` must be trimmed at first column and after last row. // - Cells "00", "03" & "30" which cannot be cut by this algorithm as they are outside the trimmed area. // - Cell "13" cannot be cut as it is inside the trimmed area. -// -// Will update table to: -// -// +----+----+----+----+----+ -// | 00 | 01 | 02 | 03 | -// + +----+----+----+----+ -// | | 11 | | 13 | 14 | -// +----+----+ + +----+ -// | 20 | 21 | | | 24 | -// +----+----+ +----+----+ -// | 30 | | | 33 | 34 | -// + +----+----+----+ + -// | | | | 43 | | -// +----+----+----+----+----+ function trimCellsToRectangularSelection( selectedTableCells, writer ) { const table = findAncestor( 'table', selectedTableCells[ 0 ] ); From 57bb1f64db38bc84f22962c95b2f1b068653c8cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 18 May 2020 14:59:44 +0200 Subject: [PATCH 15/39] Fix tests case. --- packages/ckeditor5-table/tests/tableclipboard-paste.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 123d23d349e..209bc7dd999 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -1392,7 +1392,7 @@ describe( 'table clipboard', () => { // Select 21 -> 12 tableSelection.setCellSelection( modelRoot.getNodeByPath( [ 0, 2, 1 ] ), - modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + modelRoot.getNodeByPath( [ 0, 1, 0 ] ) ); pasteTable( [ From 8cfca04918c9621aba6f93b68f2b4b1bb539720e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 18 May 2020 15:46:58 +0200 Subject: [PATCH 16/39] Working corner case fix - last row/column in selection has all spanned cells. --- .../ckeditor5-table/src/tableclipboard.js | 57 +++++++-- .../tests/tableclipboard-paste.js | 117 ++++++++++++++++-- 2 files changed, 152 insertions(+), 22 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 134d0711793..4f2d59c175e 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -138,17 +138,53 @@ export default class TableClipboard extends Plugin { const model = this.editor.model; model.change( writer => { - // Currently not handled. The selected table content should be trimmed to a rectangular selection. - // See: https://github.com/ckeditor/ckeditor5/issues/6122. + // Content table to which we insert a pasted table. + const selectedTable = findAncestor( 'table', selectedTableCells[ 0 ] ); + const { last: lastColumnOfSelection, first: firstColumnOfSelection } = getColumnIndexes( selectedTableCells ); + const { first: firstRowOfSelection, last: lastRowOfSelection } = getRowIndexes( selectedTableCells ); + + let lastRowOfAffectedSelection = lastRowOfSelection; + let lastColumnOfAffectedSelection = lastColumnOfSelection; + + let selectionHeight = lastRowOfAffectedSelection - firstRowOfSelection + 1; + let selectionWidth = lastColumnOfAffectedSelection - firstColumnOfSelection + 1; + if ( !isSelectionRectangular( selectedTableCells, tableUtils ) ) { trimCellsToRectangularSelection( selectedTableCells, writer ); - } + } else { + // Corner case... + let rs; + + const ahaRow = Array.from( new TableWalker( selectedTable, { + startRow: lastRowOfAffectedSelection, + endRow: lastRowOfAffectedSelection + } ) ).every( ( { rowspan } ) => { + rs = rowspan; + return rowspan === 1; + } ); + + if ( !ahaRow ) { + selectionHeight += rs - 1; + lastRowOfAffectedSelection = firstRowOfSelection + selectionHeight - 1; + } - const { last: lastColumnOfSelection, first: firstColumnOfSelection } = getColumnIndexes( selectedTableCells ); - const { first: firstRowOfSelection, last: lastRowOfSelection } = getRowIndexes( selectedTableCells ); + let cs; + + const ahaColumn = Array.from( new TableWalker( selectedTable, { + startRow: firstRowOfSelection, + endRow: lastRowOfAffectedSelection, + column: lastColumnOfAffectedSelection + } ) ).every( ( { colspan } ) => { + cs = colspan; - const selectionHeight = lastRowOfSelection - firstRowOfSelection + 1; - const selectionWidth = lastColumnOfSelection - firstColumnOfSelection + 1; + return colspan === 1; + } ); + + if ( !ahaColumn ) { + selectionWidth += cs - 1; + lastColumnOfAffectedSelection = firstColumnOfSelection + selectionWidth - 1; + } + } const pasteHeight = tableUtils.getRows( pastedTable ); const pasteWidth = tableUtils.getColumns( pastedTable ); @@ -176,12 +212,9 @@ export default class TableClipboard extends Plugin { // Holds two-dimensional array that is addressed by [ row ][ column ] that stores cells anchored at given location. const pastedTableLocationMap = createLocationMap( pastedTable, selectionWidth, selectionHeight ); - // Content table to which we insert a pasted table. - const selectedTable = findAncestor( 'table', selectedTableCells[ 0 ] ); - const selectedTableMap = [ ...new TableWalker( selectedTable, { startRow: firstRowOfSelection, - endRow: lastRowOfSelection, + endRow: firstRowOfSelection + selectionHeight - 1, includeSpanned: true } ) ]; @@ -203,7 +236,7 @@ export default class TableClipboard extends Plugin { } // Could use startColumn, endColumn. See: https://github.com/ckeditor/ckeditor5/issues/6785. - if ( column < firstColumnOfSelection || column > lastColumnOfSelection ) { + if ( column < firstColumnOfSelection || column > lastColumnOfAffectedSelection ) { // Only update the previousCellInRow for non-spanned slots. if ( !isSpanned ) { previousCellInRow = cell; diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 209bc7dd999..385d7b849bf 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -853,8 +853,7 @@ describe( 'table clipboard', () => { /* eslint-enable no-multi-spaces */ } ); - // TODO: Skipped case - should allow pasting but no tools to compare areas (like in MergeCellsCommand). - it.skip( 'handles pasting table that has cell with colspan (last row in selection is spanned)', () => { + it( 'handles pasting table that has cell with colspan (last row in selection is spanned)', () => { // +----+----+----+----+ // | 00 | 01 | 02 | 03 | // +----+----+----+----+ @@ -871,6 +870,7 @@ describe( 'table clipboard', () => { [ '30', '31', '32', '33' ] ] ) ); + // Select 02 -> 10 (selection 3x3) tableSelection.setCellSelection( modelRoot.getNodeByPath( [ 0, 0, 2 ] ), modelRoot.getNodeByPath( [ 0, 1, 0 ] ) @@ -896,6 +896,50 @@ describe( 'table clipboard', () => { [ 0, 0, 0, 0 ] ] ); } ); + + it( 'handles pasting table that has cell with colspan (last column in selection is spanned)', () => { + // +----+----+----+----+ + // | 00 | 01 | 03 | + // +----+----+----+----+ + // | 10 | 11 | 13 | + // +----+ +----+ + // | 20 | | 23 | + // +----+----+----+----+ + // | 30 | 31 | 32 | 33 | + // +----+----+----+----+ + setModelData( model, modelTable( [ + [ '00', { contents: '01', colspan: 2 }, '03' ], + [ '10', { contents: '11', colspan: 2, rowspan: 2 }, '13' ], + [ '20', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + // Select 20 -> 01 (selection 3x3) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 2, 0 ] ), + modelRoot.getNodeByPath( [ 0, 0, 1 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab', 'ac' ], + [ 'ba', 'bb', 'bc' ], + [ 'ca', 'cb', 'cc' ] + ] ); + + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ 'aa', 'ab', 'ac', '03' ], + [ 'ba', 'bb', 'bc', '13' ], + [ 'ca', 'cb', 'cc', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + assertSelectedCells( model, [ + [ 1, 1, 1, 0 ], + [ 1, 1, 1, 0 ], + [ 1, 1, 1, 0 ], + [ 0, 0, 0, 0 ] + ] ); + } ); } ); describe( 'content and paste tables have spans', () => { @@ -1047,8 +1091,7 @@ describe( 'table clipboard', () => { /* eslint-enable no-multi-spaces */ } ); - // TODO: Skipped case - should allow pasting but no tools to compare areas (like in MergeCellsCommand). - it.skip( 'handles pasting table that has cell with colspan (last row in selection is spanned)', () => { + it( 'handles pasting table that has cell with colspan (last row in selection is spanned)', () => { // +----+----+----+----+ // | 00 | 01 | 02 | 03 | // +----+----+----+----+ @@ -1065,6 +1108,7 @@ describe( 'table clipboard', () => { [ '30', '31', '32', '33' ] ] ) ); + // Select 02 -> 10 (selection 3x3) tableSelection.setCellSelection( modelRoot.getNodeByPath( [ 0, 0, 2 ] ), modelRoot.getNodeByPath( [ 0, 1, 0 ] ) @@ -1084,17 +1128,70 @@ describe( 'table clipboard', () => { ] ); assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ - [ '00', '01', '02', '03' ], - [ '10', { colspan: 2, contents: 'aa' }, '13' ], - [ '20', 'ba', 'bb', '23' ], + [ 'aa', 'ab', 'ac', '03' ], + [ { contents: 'ba', colspan: 2, rowspan: 2 }, 'bc', '13' ], + [ 'cc', '23' ], [ '30', '31', '32', '33' ] ] ) ); /* eslint-disable no-multi-spaces */ assertSelectedCells( model, [ - [ 0, 0, 0, 0 ], - [ 0, 1, 0 ], - [ 0, 1, 1, 0 ], + [ 1, 1, 1, 0 ], + [ 1, 1, 0 ], + [ 1, 0 ], + [ 0, 0, 0, 0 ] + ] ); + /* eslint-enable no-multi-spaces */ + } ); + + it( 'handles pasting table that has cell with colspan (last column in selection is spanned)', () => { + // +----+----+----+----+ + // | 00 | 01 | 03 | + // +----+----+----+----+ + // | 10 | 11 | 13 | + // +----+ +----+ + // | 20 | | 23 | + // +----+----+----+----+ + // | 30 | 31 | 32 | 33 | + // +----+----+----+----+ + setModelData( model, modelTable( [ + [ '00', { contents: '01', colspan: 2 }, '03' ], + [ '10', { contents: '11', colspan: 2, rowspan: 2 }, '13' ], + [ '20', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + // Select 20 -> 01 (selection 3x3) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 2, 0 ] ), + modelRoot.getNodeByPath( [ 0, 0, 1 ] ) + ); + + // +----+----+----+ + // | aa | ab | ac | + // +----+----+----+ + // | ba | bc | + // + +----+ + // | | cc | + // +----+----+----+ + pasteTable( [ + [ 'aa', 'ab', 'ac' ], + [ { contents: 'ba', colspan: 2, rowspan: 2 }, 'bc' ], + [ 'cc' ] + ] ); + + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ 'aa', 'ab', 'ac', '03' ], + [ { contents: 'ba', colspan: 2, rowspan: 2 }, 'bc', '13' ], + [ 'cc', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 1, 1, 1, 0 ], + [ 1, 1, 0 ], + [ 1, 0 ], [ 0, 0, 0, 0 ] ] ); /* eslint-enable no-multi-spaces */ From 55ef3437d995fc0a861fa926c63d50c98f5cf3db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 18 May 2020 17:00:49 +0200 Subject: [PATCH 17/39] Refactor last row/column adjustments. --- .../ckeditor5-table/src/tableclipboard.js | 86 ++++++++++--------- 1 file changed, 46 insertions(+), 40 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 4f2d59c175e..03b4b5a9b3b 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -140,52 +140,24 @@ export default class TableClipboard extends Plugin { model.change( writer => { // Content table to which we insert a pasted table. const selectedTable = findAncestor( 'table', selectedTableCells[ 0 ] ); - const { last: lastColumnOfSelection, first: firstColumnOfSelection } = getColumnIndexes( selectedTableCells ); - const { first: firstRowOfSelection, last: lastRowOfSelection } = getRowIndexes( selectedTableCells ); + const columnIndexes = getColumnIndexes( selectedTableCells ); + const { last: lastColumnOfSelection, first: firstColumnOfSelection } = columnIndexes; + const rowIndexes = getRowIndexes( selectedTableCells ); + const { first: firstRowOfSelection, last: lastRowOfSelection } = rowIndexes; - let lastRowOfAffectedSelection = lastRowOfSelection; - let lastColumnOfAffectedSelection = lastColumnOfSelection; - - let selectionHeight = lastRowOfAffectedSelection - firstRowOfSelection + 1; - let selectionWidth = lastColumnOfAffectedSelection - firstColumnOfSelection + 1; + let lastRowOfSelectionArea = lastRowOfSelection; + let lastColumnOfSelectionArea = lastColumnOfSelection; if ( !isSelectionRectangular( selectedTableCells, tableUtils ) ) { trimCellsToRectangularSelection( selectedTableCells, writer ); } else { - // Corner case... - let rs; - - const ahaRow = Array.from( new TableWalker( selectedTable, { - startRow: lastRowOfAffectedSelection, - endRow: lastRowOfAffectedSelection - } ) ).every( ( { rowspan } ) => { - rs = rowspan; - return rowspan === 1; - } ); - - if ( !ahaRow ) { - selectionHeight += rs - 1; - lastRowOfAffectedSelection = firstRowOfSelection + selectionHeight - 1; - } - - let cs; - - const ahaColumn = Array.from( new TableWalker( selectedTable, { - startRow: firstRowOfSelection, - endRow: lastRowOfAffectedSelection, - column: lastColumnOfAffectedSelection - } ) ).every( ( { colspan } ) => { - cs = colspan; - - return colspan === 1; - } ); - - if ( !ahaColumn ) { - selectionWidth += cs - 1; - lastColumnOfAffectedSelection = firstColumnOfSelection + selectionWidth - 1; - } + lastRowOfSelectionArea = adjustLastRowOfSelection( selectedTable, rowIndexes, columnIndexes ); + lastColumnOfSelectionArea = adjustLastColumnOfSelection( selectedTable, rowIndexes, columnIndexes ); } + const selectionHeight = lastRowOfSelectionArea - firstRowOfSelection + 1; + const selectionWidth = lastColumnOfSelectionArea - firstColumnOfSelection + 1; + const pasteHeight = tableUtils.getRows( pastedTable ); const pasteWidth = tableUtils.getColumns( pastedTable ); @@ -236,7 +208,7 @@ export default class TableClipboard extends Plugin { } // Could use startColumn, endColumn. See: https://github.com/ckeditor/ckeditor5/issues/6785. - if ( column < firstColumnOfSelection || column > lastColumnOfAffectedSelection ) { + if ( column < firstColumnOfSelection || column > lastColumnOfSelectionArea ) { // Only update the previousCellInRow for non-spanned slots. if ( !isSpanned ) { previousCellInRow = cell; @@ -438,3 +410,37 @@ function isAffectedBySelection( rowOrColumn, rowOrColumnSpan, first, last ) { return isInsideSelection || overlapsSelectionFromOutside; } + +function adjustLastRowOfSelection( selectedTable, rowIndexes, columnIndexes ) { + // Corner case... + const lastRowMap = Array.from( new TableWalker( selectedTable, { + startRow: rowIndexes.last, + endRow: rowIndexes.last + } ) ).filter( ( { column } ) => columnIndexes.first <= column && column <= columnIndexes.last ); + + const everyCellHasSingleRowspan = lastRowMap.every( ( { rowspan } ) => rowspan === 1 ); + + if ( !everyCellHasSingleRowspan ) { + const rowspanAdjustment = lastRowMap.pop().rowspan - 1; + return rowIndexes.last + rowspanAdjustment; + } + + return rowIndexes.last; +} + +function adjustLastColumnOfSelection( selectedTable, rowIndexes, columnIndexes ) { + const lastColumnMap = Array.from( new TableWalker( selectedTable, { + startRow: rowIndexes.first, + endRow: rowIndexes.last, + column: columnIndexes.last + } ) ); + + const everyCellHasSingleColspan = lastColumnMap.every( ( { colspan } ) => colspan === 1 ); + + if ( !everyCellHasSingleColspan ) { + const colspanAdjustment = lastColumnMap.pop().colspan - 1; + return columnIndexes.last + colspanAdjustment; + } + + return columnIndexes.last; +} From 4c2aeb55a058517278f7949761b5f158b89742ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 09:33:06 +0200 Subject: [PATCH 18/39] Add default values to internal methods. --- packages/ckeditor5-table/src/tableclipboard.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 03b4b5a9b3b..3b2615360ce 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -361,11 +361,11 @@ function trimCellsToRectangularSelection( selectedTableCells, writer ) { doVerticalSplit( table, lastColumn + 1, rowIndexes, writer ); // 2. Split cells horizontally in two steps as first step might create cells that needs to split again. - doHorizontalSplit( table, firstRow, columnIndexes, writer, 0 ); + doHorizontalSplit( table, firstRow, columnIndexes, writer ); doHorizontalSplit( table, lastRow + 1, columnIndexes, writer, firstRow ); } -function doHorizontalSplit( table, splitRow, limitColumns, writer, startRow ) { +function doHorizontalSplit( table, splitRow, limitColumns, writer, startRow = 0 ) { // If selection starts at first row then no split is needed. if ( splitRow < 1 ) { return; From 1f3c682147d87c3a82955ce89f1b8bb5f0573a6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 09:33:38 +0200 Subject: [PATCH 19/39] Add test cases for fixing broken table layout. --- .../tests/tableclipboard-paste.js | 142 ++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 385d7b849bf..95025b907ac 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -2003,6 +2003,148 @@ describe( 'table clipboard', () => { ] ); } ); } ); + + describe( 'fixing pasted table broken layout', () => { + it( 'should trim pasted cells\' width if they exceeds table width established by the first row', () => { + // Select 00 -> 22 (selection 2x3 - equal to expected fixed table) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 2, 1 ] ) + ); + + // +----+----+ + // | aa | ab | <- First row establish table width=2. + // +----+----+----+----+ + // | ba | bb | <- Cell "bb" sticks out by 2 slots. + // +----+----+----+----+ + // | ca | <- Cell "ca" sticks out by 1 slot. + // +----+----+----+ + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', { colspan: 3, contents: 'bb' } ], + [ { colspan: 3, contents: 'ca' } ] + ] ); + + // +----+----+----+----+ + // | aa | ab | 02 | 03 | + // +----+----+----+----+ + // | ba | bb | 12 | 13 | + // +----+----+----+----+ + // | ca | 22 | 23 | + // +----+----+----+----+ + // | 30 | 31 | 32 | 33 | + // +----+----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ 'aa', 'ab', '02', '03' ], + [ 'ba', 'bb', '12', '13' ], + [ { contents: 'ca', colspan: 2 }, '22', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 1, 1, 0, 0 ], + [ 1, 1, 0, 0 ], + [ 1, 0, 0 ], + [ 0, 0, 0, 0 ] + ] ); + /* eslint-enable no-multi-spaces */ + } ); + + it( 'should trim pasted cells\' height if they exceeds table height established by the last row', () => { + // Select 00 -> 12 (selection 3x2 - equal to expected fixed table) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 2 ] ) + ); + + // +----+----+----+ + // | aa | ab | ac | + // +----+----+ + + // | ba | bb | | <- Last row establish table height=2. + // +----+ + + + // | | | <- Cell "ac" sticks out by 1 slot. + // + +----+ + // | | <- Cell "bb" sticks out by 2 slots. + // +----+ + pasteTable( [ + [ + [ 'aa', 'ab', { contents: 'ac', rowspan: 3 } ], + [ 'ba', { contents: 'bb', rowspan: 3 } ] + ] + ] ); + + // +----+----+----+----+ + // | aa | ab | ac | 03 | + // +----+----+ +----+ + // | ba | bb | | 13 | + // +----+----+----+----+ + // | 20 | 21 | 22 | 23 | + // +----+----+----+----+ + // | 30 | 31 | 32 | 33 | + // +----+----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ 'aa', 'ab', '02', '03' ], + [ 'ba', 'bb', '12', '13' ], + [ { contents: 'ca', colspan: 2 }, '22', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 1, 1, 1, 0 ], + [ 1, 1, 0 ], + [ 0, 0, 0, 0 ], + [ 0, 0, 0, 0 ] + ] ); + /* eslint-enable no-multi-spaces */ + } ); + + it( 'should trim pasted cells\' height and width if they exceeds table height and width', () => { + // Select 00 -> 11 (selection 2x2 - equal to expected fixed table) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + // +----+----+ + // | aa | ab | + // +----+----+----+----+ + // | ba | bb | <- Cell "bb" sticks out by 2 slots in width and by 1 slot in height. + // +----+ + + // | | + // +----+----+----+ + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', { contents: 'bb', colspan: 3, rowspan: 2 } ] + ] ); + + // +----+----+----+----+ + // | aa | ab | 02 | 03 | + // +----+----+----+----+ + // | ba | bb | 12 | 13 | + // +----+----+----+----+ + // | 20 | 21 | 22 | 23 | + // +----+----+----+----+ + // | 30 | 31 | 32 | 33 | + // +----+----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ 'aa', 'ab', '02', '03' ], + [ 'ba', 'bb', '12', '13' ], + [ '20', '21', '22', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 1, 1, 0, 0 ], + [ 1, 1, 0, 0 ], + [ 0, 0, 0, 0 ], + [ 0, 0, 0, 0 ] + ] ); + /* eslint-enable no-multi-spaces */ + } ); + } ); } ); describe( 'Clipboard integration - paste (content scenarios)', () => { From ae90c7421d76f0a5febf02e5a74cbe4a8e8263c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 09:58:25 +0200 Subject: [PATCH 20/39] Fix rowspan trim test case. --- .../ckeditor5-table/tests/tableclipboard-paste.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 95025b907ac..c32828fda00 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -2068,10 +2068,8 @@ describe( 'table clipboard', () => { // | | <- Cell "bb" sticks out by 2 slots. // +----+ pasteTable( [ - [ - [ 'aa', 'ab', { contents: 'ac', rowspan: 3 } ], - [ 'ba', { contents: 'bb', rowspan: 3 } ] - ] + [ 'aa', 'ab', { contents: 'ac', rowspan: 3 } ], + [ 'ba', { contents: 'bb', rowspan: 3 } ] ] ); // +----+----+----+----+ @@ -2084,9 +2082,9 @@ describe( 'table clipboard', () => { // | 30 | 31 | 32 | 33 | // +----+----+----+----+ assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ - [ 'aa', 'ab', '02', '03' ], - [ 'ba', 'bb', '12', '13' ], - [ { contents: 'ca', colspan: 2 }, '22', '23' ], + [ 'aa', 'ab', { rowspan: 2, contents: 'ac' }, '03' ], + [ 'ba', 'bb', '13' ], + [ '20', '21', '22', '23' ], [ '30', '31', '32', '33' ] ] ) ); From 486c16d6f97fe2bd1111671b97835a7953941240 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 09:58:53 +0200 Subject: [PATCH 21/39] Crop pasted table to its established dimensions. --- packages/ckeditor5-table/src/tableclipboard.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 3b2615360ce..22737ad70bd 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -179,6 +179,13 @@ export default class TableClipboard extends Plugin { }; pastedTable = cropTableToDimensions( pastedTable, cropDimensions, writer, tableUtils ); + } else { + pastedTable = cropTableToDimensions( pastedTable, { + startRow: 0, + endRow: pasteHeight - 1, + startColumn: 0, + endColumn: pasteWidth - 1 + }, writer, tableUtils ); } // Holds two-dimensional array that is addressed by [ row ][ column ] that stores cells anchored at given location. From 6f7357e96ca5baf3e816231274979abab75a62d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 10:16:42 +0200 Subject: [PATCH 22/39] Fixing pasted table layout should also occur when selection dimensions are smaller than pasted table. --- .../ckeditor5-table/src/tableclipboard.js | 14 +- .../tests/tableclipboard-paste.js | 148 ++++++++++++++++++ 2 files changed, 155 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 22737ad70bd..860e9de5597 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -169,6 +169,13 @@ export default class TableClipboard extends Plugin { return; } + pastedTable = cropTableToDimensions( pastedTable, { + startRow: 0, + endRow: pasteHeight - 1, + startColumn: 0, + endColumn: pasteWidth - 1 + }, writer, tableUtils ); + // Crop pasted table if it extends selection area. if ( selectionHeight < pasteHeight || selectionWidth < pasteWidth ) { const cropDimensions = { @@ -179,13 +186,6 @@ export default class TableClipboard extends Plugin { }; pastedTable = cropTableToDimensions( pastedTable, cropDimensions, writer, tableUtils ); - } else { - pastedTable = cropTableToDimensions( pastedTable, { - startRow: 0, - endRow: pasteHeight - 1, - startColumn: 0, - endColumn: pasteWidth - 1 - }, writer, tableUtils ); } // Holds two-dimensional array that is addressed by [ row ][ column ] that stores cells anchored at given location. diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index c32828fda00..7fd4aeb6e7c 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -2142,6 +2142,154 @@ describe( 'table clipboard', () => { ] ); /* eslint-enable no-multi-spaces */ } ); + + it( + 'should trim pasted cells\' width if they exceeds pasted table width (pasted height is bigger then selection height)', + () => { + // Select 00 -> 11 (selection 2x2 - smaller by height than the expected fixed table) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + // +----+----+ + // | aa | ab | <- First row establish table width=2. + // +----+----+----+----+ + // | ba | bb | <- Cell "bb" sticks out by 2 slots. + // +----+----+----+----+ + // | ca | <- Cell "ca" sticks out by 1 slot. + // +----+----+----+ + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', { colspan: 3, contents: 'bb' } ], + [ { colspan: 3, contents: 'ca' } ] + ] ); + + // +----+----+----+----+ + // | aa | ab | 02 | 03 | + // +----+----+----+----+ + // | ba | bb | 12 | 13 | + // +----+----+----+----+ + // | 20 | 21 | 22 | 23 | + // +----+----+----+----+ + // | 30 | 31 | 32 | 33 | + // +----+----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ 'aa', 'ab', '02', '03' ], + [ 'ba', 'bb', '12', '13' ], + [ '20', '21', '22', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + assertSelectedCells( model, [ + [ 1, 1, 0, 0 ], + [ 1, 1, 0, 0 ], + [ 0, 0, 0, 0 ], + [ 0, 0, 0, 0 ] + ] ); + } + ); + + it( + 'should trim pasted cells\' height if they exceeds pasted table height (pasted width is bigger then selection width)', + () => { + // Select 00 -> 11 (selection 2x2 - smaller by width than the expected fixed table) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + // +----+----+----+ + // | aa | ab | ac | + // +----+----+ + + // | ba | bb | | <- Last row establish table height=2. + // +----+ + + + // | | | <- Cell "ac" sticks out by 1 slot. + // + +----+ + // | | <- Cell "bb" sticks out by 2 slots. + // +----+ + pasteTable( [ + [ 'aa', 'ab', { contents: 'ac', rowspan: 3 } ], + [ 'ba', { contents: 'bb', rowspan: 3 } ] + ] ); + + // +----+----+----+----+ + // | aa | ab | 02 | 03 | + // +----+----+----+----+ + // | ba | bb | 12 | 13 | + // +----+----+----+----+ + // | 20 | 21 | 22 | 23 | + // +----+----+----+----+ + // | 30 | 31 | 32 | 33 | + // +----+----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ 'aa', 'ab', '02', '03' ], + [ 'ba', 'bb', '12', '13' ], + [ '20', '21', '22', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 1, 1, 0, 0 ], + [ 1, 1, 0, 0 ], + [ 0, 0, 0, 0 ], + [ 0, 0, 0, 0 ] + ] ); + /* eslint-enable no-multi-spaces */ + } + ); + + it( + `should trim pasted pasted cells' height and width if they exceeds pasted table dimensions + (pasted table is bigger then selection width)`, + () => { + // Select 00 -> 11 (selection 2x2 - smaller than the expected fixed table) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + // +----+----+----+ + // | aa | ab | ac | + // +----+----+----+----+ + // | ba | bb | <- Cell "bb" sticks out by 1 slots in width and by 1 slot in height. + // +----+ + + // | ca | | + // +----+ + + // | | + // +----+----+----+ + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', { contents: 'bb', colspan: 3, rowspan: 2 } ] + ] ); + + // +----+----+----+----+ + // | aa | ab | 02 | 03 | + // +----+----+----+----+ + // | ba | bb | 12 | 13 | + // +----+----+----+----+ + // | 20 | 21 | 22 | 23 | + // +----+----+----+----+ + // | 30 | 31 | 32 | 33 | + // +----+----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ 'aa', 'ab', '02', '03' ], + [ 'ba', 'bb', '12', '13' ], + [ '20', '21', '22', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 1, 1, 0, 0 ], + [ 1, 1, 0, 0 ], + [ 0, 0, 0, 0 ], + [ 0, 0, 0, 0 ] + ] ); + /* eslint-enable no-multi-spaces */ + } + ); } ); } ); From 081d461870c1594b4e857dd653eaf3fd006a5131 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 10:21:28 +0200 Subject: [PATCH 23/39] Use single crop pass for fixing pasted table layout and cropping pasted table to selection area. --- .../ckeditor5-table/src/tableclipboard.js | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 860e9de5597..6fbb17c77f9 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -169,24 +169,17 @@ export default class TableClipboard extends Plugin { return; } - pastedTable = cropTableToDimensions( pastedTable, { + // Crop pasted table if: + // - Pasted table dimensions exceeds selection area. + // - Pasted table has broken layout (ie some cells sticks out by the table dimensions established by the first and last row). + const cropDimensions = { startRow: 0, - endRow: pasteHeight - 1, startColumn: 0, - endColumn: pasteWidth - 1 - }, writer, tableUtils ); - - // Crop pasted table if it extends selection area. - if ( selectionHeight < pasteHeight || selectionWidth < pasteWidth ) { - const cropDimensions = { - startRow: 0, - startColumn: 0, - endRow: selectionHeight - 1, - endColumn: selectionWidth - 1 - }; - - pastedTable = cropTableToDimensions( pastedTable, cropDimensions, writer, tableUtils ); - } + endRow: Math.min( selectionHeight - 1, pasteHeight - 1 ), + endColumn: Math.min( selectionWidth - 1, pasteWidth - 1 ) + }; + + pastedTable = cropTableToDimensions( pastedTable, cropDimensions, writer, tableUtils ); // Holds two-dimensional array that is addressed by [ row ][ column ] that stores cells anchored at given location. const pastedTableLocationMap = createLocationMap( pastedTable, selectionWidth, selectionHeight ); From 63576938d2e53a3650beb0deefce6796744d6973 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 10:40:26 +0200 Subject: [PATCH 24/39] Update internal comments for pasting table scenarios. --- .../ckeditor5-table/src/tableclipboard.js | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 6fbb17c77f9..9cfe73d3330 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -140,17 +140,28 @@ export default class TableClipboard extends Plugin { model.change( writer => { // Content table to which we insert a pasted table. const selectedTable = findAncestor( 'table', selectedTableCells[ 0 ] ); + const columnIndexes = getColumnIndexes( selectedTableCells ); - const { last: lastColumnOfSelection, first: firstColumnOfSelection } = columnIndexes; const rowIndexes = getRowIndexes( selectedTableCells ); + + const { last: lastColumnOfSelection, first: firstColumnOfSelection } = columnIndexes; const { first: firstRowOfSelection, last: lastRowOfSelection } = rowIndexes; let lastRowOfSelectionArea = lastRowOfSelection; let lastColumnOfSelectionArea = lastColumnOfSelection; + // For a non- rectangular selection (ie in which some cells sticks out from a virtual selection rectangle) we need to create + // a table layout that has a rectangular selection. This will split cells so the selection become rectangular. + // Beyond this point we will operate on fixed content table. if ( !isSelectionRectangular( selectedTableCells, tableUtils ) ) { - trimCellsToRectangularSelection( selectedTableCells, writer ); - } else { + splitCellsToRectangularSelection( selectedTableCells, writer ); + } + // However a rectangular selection might consist an invalid sub-table (if the selected cell would be moved outside the table). + // This happens in a table which has: + // - last row has empty slots (so are covered by cells from rows above) and/or + // - last column has empty slots (are covered by cells from previous columns) + // This case needs only adjusting the selection dimension as the rest of the algorithm operates on empty slots also. + else { lastRowOfSelectionArea = adjustLastRowOfSelection( selectedTable, rowIndexes, columnIndexes ); lastColumnOfSelectionArea = adjustLastColumnOfSelection( selectedTable, rowIndexes, columnIndexes ); } @@ -172,6 +183,10 @@ export default class TableClipboard extends Plugin { // Crop pasted table if: // - Pasted table dimensions exceeds selection area. // - Pasted table has broken layout (ie some cells sticks out by the table dimensions established by the first and last row). + // + // Note: The table dimensions are established by the width of the first row and the total number of rows. + // It is possible to programmatically create a table that has rows which would have cells anchored beyond first row width but + // such table will not be created by other editing solutions. const cropDimensions = { startRow: 0, startColumn: 0, @@ -181,7 +196,7 @@ export default class TableClipboard extends Plugin { pastedTable = cropTableToDimensions( pastedTable, cropDimensions, writer, tableUtils ); - // Holds two-dimensional array that is addressed by [ row ][ column ] that stores cells anchored at given location. + // Holds a two-dimensional array that is addressed by [ row ][ column ] that stores cells anchored at given location. const pastedTableLocationMap = createLocationMap( pastedTable, selectionWidth, selectionHeight ); const selectedTableMap = [ ...new TableWalker( selectedTable, { @@ -348,7 +363,7 @@ function createLocationMap( table, width, height ) { // - Cell "03" which have `rowspan = 2` and `colspan = 2` must be trimmed at first column and after last row. // - Cells "00", "03" & "30" which cannot be cut by this algorithm as they are outside the trimmed area. // - Cell "13" cannot be cut as it is inside the trimmed area. -function trimCellsToRectangularSelection( selectedTableCells, writer ) { +function splitCellsToRectangularSelection( selectedTableCells, writer ) { const table = findAncestor( 'table', selectedTableCells[ 0 ] ); const rowIndexes = getRowIndexes( selectedTableCells ); From 2c1d1a4abed918ce60e9574e7337504a6413cb0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 10:52:37 +0200 Subject: [PATCH 25/39] Add test cases for updating selection dimension for multiple empty rows/columns. --- .../tests/tableclipboard-paste.js | 90 ++++++++++++++++++- 1 file changed, 86 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 7fd4aeb6e7c..1acd24a7039 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -604,10 +604,10 @@ describe( 'table clipboard', () => { /* eslint-disable no-multi-spaces */ assertSelectedCells( model, [ - [ 1, 1, 1, 1, 0 ], - [ 1, 1, 0 ], - [ 1, 1, 0 ], - [ 1, 1, 1, 0 ], + [ 1, 1, 1, 1, 0 ], + [ 1, 1, 0 ], + [ 1, 1, 0 ], + [ 1, 1, 1, 0 ], [ 0, 0, 0, 0, 0, 0 ] ] ); /* eslint-enable no-multi-spaces */ @@ -897,6 +897,50 @@ describe( 'table clipboard', () => { ] ); } ); + it( 'handles pasting table that has cell with colspan (multiple ending rows in the selection are spanned)', () => { + // +----+----+----+ + // | 00 | 01 | 02 | + // +----+ + + + // | 10 | | | + // +----+ + + + // | 20 | | | + // +----+----+----+ + // | 30 | 31 | 32 | + // +----+----+----+ + setModelData( model, modelTable( [ + [ '00', { contents: '01', rowspan: 3 }, { contents: '02', rowspan: 3 } ], + [ '10' ], + [ '20' ], + [ '30', '31', '32' ] + ] ) ); + + // Select 01 -> 02 (selection 2x2) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 1 ] ), + modelRoot.getNodeByPath( [ 0, 0, 2 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', 'bb' ], + [ 'ca', 'cb' ] + ] ); + + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', 'aa', 'ab' ], + [ '10', 'ba', 'bb' ], + [ '20', 'ca', 'cb' ], + [ '30', '31', '32' ] + ] ) ); + + assertSelectedCells( model, [ + [ 0, 1, 1 ], + [ 0, 1, 1 ], + [ 0, 1, 1 ], + [ 0, 0, 0 ] + ] ); + } ); + it( 'handles pasting table that has cell with colspan (last column in selection is spanned)', () => { // +----+----+----+----+ // | 00 | 01 | 03 | @@ -940,6 +984,44 @@ describe( 'table clipboard', () => { [ 0, 0, 0, 0 ] ] ); } ); + + it( 'handles pasting table that has cell with colspan (multiple ending columns in the selection are spanned)', () => { + // +----+----+----+----+ + // | 00 | 01 | 02 | 03 | + // +----+----+----+----+ + // | 10 | 13 | + // +----+----+----+----+ + // | 20 | 23 | + // +----+----+----+----+ + setModelData( model, modelTable( [ + [ '00', '01', '02', '03' ], + [ { contents: '10', colspan: 3 }, '13' ], + [ { contents: '20', colspan: 3 }, '23' ] + ] ) ); + + // Select 10 -> 20 (selection 3x2) + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 1, 0 ] ), + modelRoot.getNodeByPath( [ 0, 2, 0 ] ) + ); + + pasteTable( [ + [ 'aa', 'ab', 'ac' ], + [ 'ba', 'bb', 'bc' ] + ] ); + + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01', '02', '03' ], + [ 'aa', 'ab', 'ac', '13' ], + [ 'ba', 'bb', 'bc', '23' ] + ] ) ); + + assertSelectedCells( model, [ + [ 0, 0, 0, 0 ], + [ 1, 1, 1, 0 ], + [ 1, 1, 1, 0 ] + ] ); + } ); } ); describe( 'content and paste tables have spans', () => { From 40c45ad968e76bfd07d62a624258f80f38fde9ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 11:20:34 +0200 Subject: [PATCH 26/39] Set table heading columns now splits overlapping cells vertically. --- .../src/commands/setheadercolumncommand.js | 15 +++- .../src/commands/setheaderrowcommand.js | 2 +- .../tests/commands/setheadercolumncommand.js | 90 +++++++++++++++++++ 3 files changed, 103 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/setheadercolumncommand.js b/packages/ckeditor5-table/src/commands/setheadercolumncommand.js index 69c9d1256ef..16cdfb37cf8 100644 --- a/packages/ckeditor5-table/src/commands/setheadercolumncommand.js +++ b/packages/ckeditor5-table/src/commands/setheadercolumncommand.js @@ -10,7 +10,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; import { findAncestor, isHeadingColumnCell, updateNumericAttribute } from './utils'; -import { getColumnIndexes, getSelectionAffectedTableCells } from '../utils'; +import { getColumnIndexes, getSelectionAffectedTableCells, getVerticallyOverlappingCells, splitVertically } from '../utils'; /** * The header column command. @@ -69,12 +69,21 @@ export default class SetHeaderColumnCommand extends Command { const model = this.editor.model; const selectedCells = getSelectionAffectedTableCells( model.document.selection ); - const { first, last } = getColumnIndexes( selectedCells ); + const table = findAncestor( 'table', selectedCells[ 0 ] ); + const { first, last } = getColumnIndexes( selectedCells ); const headingColumnsToSet = this.value ? first : last + 1; model.change( writer => { - const table = findAncestor( 'table', selectedCells[ 0 ] ); + if ( headingColumnsToSet ) { + // Changing heading columns requires to check if any of a heading cell is overlapping vertically the table head. + // Any table cell that has a colspan attribute > 1 will not exceed the table head so we need to fix it in columns before. + const overlappingCells = getVerticallyOverlappingCells( table, headingColumnsToSet ); + + for ( const { cell, column } of overlappingCells ) { + splitVertically( cell, column, headingColumnsToSet, writer ); + } + } updateNumericAttribute( 'headingColumns', headingColumnsToSet, table, writer, 0 ); } ); diff --git a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js index 9c92dd26d1f..b049ea68d92 100644 --- a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js +++ b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js @@ -74,7 +74,7 @@ export default class SetHeaderRowCommand extends Command { model.change( writer => { if ( headingRowsToSet ) { - // Changing heading rows requires to check if any of a heading cell is overlapping vertically the table head. + // Changing heading rows requires to check if any of a heading cell is overlapping horizontally the table head. // Any table cell that has a rowspan attribute > 1 will not exceed the table head so we need to fix it in rows below. const startRow = headingRowsToSet > currentHeadingRows ? currentHeadingRows : 0; const overlappingCells = getHorizontallyOverlappingCells( table, headingRowsToSet, startRow ); diff --git a/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js b/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js index e87d5c36cda..48c6a285cf3 100644 --- a/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js +++ b/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js @@ -427,5 +427,95 @@ describe( 'SetHeaderColumnCommand', () => { [ '00', '01[]', '02', '03' ] ], { headingColumns: 1 } ) ); } ); + + it( 'should fix col-spanned cells on the edge of an table heading columns section', () => { + // +----+----+----+ + // | 00 | 01 | + // +----+ + + // | 10 | | + // +----+----+----+ + // | 20 | 21 | 22 | + // +----+----+----+ + // ^-- heading columns + setData( model, modelTable( [ + [ '00', { contents: '[]01', colspan: 2, rowspan: 2 } ], + [ '10' ], + [ '20', '21', '22' ] + ], { headingColumns: 1 } ) ); + + command.execute(); + + // +----+----+----+ + // | 00 | 01 | | + // +----+ + + + // | 10 | | | + // +----+----+----+ + // | 20 | 21 | 22 | + // +----+----+----+ + // ^-- heading columns + assertEqualMarkup( getData( model ), modelTable( [ + [ '00', { contents: '[]01', rowspan: 2 }, { contents: '', rowspan: 2 } ], + [ '10' ], + [ '20', '21', '22' ] + ], { headingColumns: 2 } ) ); + } ); + + it( 'should split to at most 2 table cells when fixing col-spanned cells on the edge of an table heading columns section', () => { + // +----+----+----+----+----+----+ + // | 00 | 01 | + // +----+ + + // | 10 | | + // +----+----+----+----+----+----+ + // | 20 | 21 | 22 | 23 | 24 | 25 | + // +----+----+----+----+----+----+ + // ^-- heading columns + setData( model, modelTable( [ + [ '00', { contents: '01', colspan: 5, rowspan: 2 } ], + [ '10' ], + [ '20', '21', '22[]', '23', '24', '25' ] + ], { headingColumns: 1 } ) ); + + command.execute(); + + // +----+----+----+----+----+----+ + // | 00 | 01 | | + // +----+ + + + // | 10 | | | + // +----+----+----+----+----+----+ + // | 20 | 21 | 22 | 23 | 24 | 25 | + // +----+----+----+----+----+----+ + // ^-- heading columns + assertEqualMarkup( getData( model ), modelTable( [ + [ '00', { contents: '01', colspan: 2, rowspan: 2 }, { contents: '', colspan: 3, rowspan: 2 } ], + [ '10' ], + [ '20', '21', '22[]', '23', '24', '25' ] + ], { headingColumns: 3 } ) ); + } ); + + it( 'should fix col-spanned cells on the edge of an table heading columns section when creating section', () => { + // +----+----+ + // | 00 | + // +----+----+ + // | 10 | 11 | + // +----+----+ + // ^-- heading columns + setData( model, modelTable( [ + [ { contents: '00', colspan: 2 } ], + [ '10', '[]11' ] + ], { headingColumns: 2 } ) ); + + command.execute(); + + // +----+----+ + // | 00 | | + // +----+----+ + // | 10 | 11 | + // +----+----+ + // ^-- heading columns + assertEqualMarkup( getData( model ), modelTable( [ + [ '00', '' ], + [ '10', '[]11' ] + ], { headingColumns: 1 } ) ); + } ); } ); } ); From 0ff5a240150f828e5352c961ab81a53fb71e7041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 19 May 2020 13:09:20 +0200 Subject: [PATCH 27/39] Add internal docs comments. --- .../ckeditor5-table/src/tableclipboard.js | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index c724d9779b8..e7acd3e7031 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -119,9 +119,6 @@ export default class TableClipboard extends Plugin { return; } - // Content table to which we insert a pasted table. - const selectedTable = findAncestor( 'table', selectedTableCells[ 0 ] ); - // We might need to crop table before inserting so reference might change. let pastedTable = getTableIfOnlyTableInContent( content ); @@ -142,7 +139,10 @@ export default class TableClipboard extends Plugin { const pasteHeight = tableUtils.getRows( pastedTable ); const pasteWidth = tableUtils.getColumns( pastedTable ); - if ( selectedTableCells.length == 1 ) { + // Content table to which we insert a pasted table. + const selectedTable = findAncestor( 'table', selectedTableCells[ 0 ] ); + + if ( selectedTableCells.length === 1 ) { lastRowOfSelection += pasteHeight - 1; lastColumnOfSelection += pasteWidth - 1; @@ -172,6 +172,7 @@ export default class TableClipboard extends Plugin { const selectionWidth = lastColumnOfSelectionArea - firstColumnOfSelection + 1; // The if below is temporal and will be removed when handling this case. + // This if to be removed as handling of replicating cells should be done in replaceSelectedCellsWithPasted(). // See: https://github.com/ckeditor/ckeditor5/issues/6769. if ( selectionHeight > pasteHeight || selectionWidth > pasteWidth ) { // @if CK_DEBUG // console.log( 'NOT IMPLEMENTED YET: Pasted table is smaller than selection area.' ); @@ -223,8 +224,9 @@ export default class TableClipboard extends Plugin { // @param {module:engine/model/writer~Writer} writer function replaceSelectedCellsWithPasted( pastedTable, selectedTable, selectionDimensions, writer ) { const { - firstColumnOfSelection, lastColumnOfSelection, selectionWidth, - firstRowOfSelection, lastRowOfSelection, selectionHeight + firstColumnOfSelection, lastColumnOfSelection, + selectionWidth, selectionHeight, + firstRowOfSelection, lastRowOfSelection } = selectionDimensions; // Holds two-dimensional array that is addressed by [ row ][ column ] that stores cells anchored at given location. @@ -377,7 +379,7 @@ function createLocationMap( table, width, height ) { return map; } -// Make selected cell rectangular by splitting the cells that stand out from a rectangular selection. +// Make selected cells rectangular by splitting the cells that stand out from a rectangular selection. // // In the table below a selection is shown with "::" and slots with anchor cells are named. // @@ -477,12 +479,15 @@ function isAffectedBySelection( rowOrColumn, rowOrColumnSpan, first, last ) { return isInsideSelection || overlapsSelectionFromOutside; } +// Returns adjusted last row index if selection covers part of a row with empty slots (spanned by other cells). function adjustLastRowOfSelection( selectedTable, rowIndexes, columnIndexes ) { - // Corner case... const lastRowMap = Array.from( new TableWalker( selectedTable, { startRow: rowIndexes.last, endRow: rowIndexes.last - } ) ).filter( ( { column } ) => columnIndexes.first <= column && column <= columnIndexes.last ); + } ) ).filter( ( { column } ) => { + // Could use startColumn, endColumn. See: https://github.com/ckeditor/ckeditor5/issues/6785. + return columnIndexes.first <= column && column <= columnIndexes.last; + } ); const everyCellHasSingleRowspan = lastRowMap.every( ( { rowspan } ) => rowspan === 1 ); @@ -491,9 +496,11 @@ function adjustLastRowOfSelection( selectedTable, rowIndexes, columnIndexes ) { return rowIndexes.last + rowspanAdjustment; } + // Default to the last row index. return rowIndexes.last; } +// Returns adjusted last column index if selection covers part of a column with empty slots (spanned by other cells). function adjustLastColumnOfSelection( selectedTable, rowIndexes, columnIndexes ) { const lastColumnMap = Array.from( new TableWalker( selectedTable, { startRow: rowIndexes.first, @@ -508,5 +515,6 @@ function adjustLastColumnOfSelection( selectedTable, rowIndexes, columnIndexes ) return columnIndexes.last + colspanAdjustment; } + // Default to the last column index. return columnIndexes.last; } From d94af12438e7ed28b1fc1746d65837d02cc6f2d6 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 19 May 2020 21:45:56 +0200 Subject: [PATCH 28/39] Some changes made during review to understand how it works. --- .../src/commands/setheadercolumncommand.js | 6 +- .../src/commands/setheaderrowcommand.js | 6 +- .../ckeditor5-table/src/tableclipboard.js | 102 ++++++++---------- packages/ckeditor5-table/src/utils.js | 56 +++++----- 4 files changed, 77 insertions(+), 93 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/setheadercolumncommand.js b/packages/ckeditor5-table/src/commands/setheadercolumncommand.js index 16cdfb37cf8..078fc199e67 100644 --- a/packages/ckeditor5-table/src/commands/setheadercolumncommand.js +++ b/packages/ckeditor5-table/src/commands/setheadercolumncommand.js @@ -10,7 +10,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; import { findAncestor, isHeadingColumnCell, updateNumericAttribute } from './utils'; -import { getColumnIndexes, getSelectionAffectedTableCells, getVerticallyOverlappingCells, splitVertically } from '../utils'; +import { getColumnIndexes, getSelectionAffectedTableCells, getHorizontallyOverlappingCells, splitVertically } from '../utils'; /** * The header column command. @@ -76,9 +76,9 @@ export default class SetHeaderColumnCommand extends Command { model.change( writer => { if ( headingColumnsToSet ) { - // Changing heading columns requires to check if any of a heading cell is overlapping vertically the table head. + // Changing heading columns requires to check if any of a heading cell is overlapping horizontally the table head. // Any table cell that has a colspan attribute > 1 will not exceed the table head so we need to fix it in columns before. - const overlappingCells = getVerticallyOverlappingCells( table, headingColumnsToSet ); + const overlappingCells = getHorizontallyOverlappingCells( table, headingColumnsToSet ); for ( const { cell, column } of overlappingCells ) { splitVertically( cell, column, headingColumnsToSet, writer ); diff --git a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js index b049ea68d92..d6b6c256558 100644 --- a/packages/ckeditor5-table/src/commands/setheaderrowcommand.js +++ b/packages/ckeditor5-table/src/commands/setheaderrowcommand.js @@ -10,7 +10,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; import { findAncestor, updateNumericAttribute } from './utils'; -import { getHorizontallyOverlappingCells, getRowIndexes, getSelectionAffectedTableCells, splitHorizontally } from '../utils'; +import { getVerticallyOverlappingCells, getRowIndexes, getSelectionAffectedTableCells, splitHorizontally } from '../utils'; /** * The header row command. @@ -74,10 +74,10 @@ export default class SetHeaderRowCommand extends Command { model.change( writer => { if ( headingRowsToSet ) { - // Changing heading rows requires to check if any of a heading cell is overlapping horizontally the table head. + // Changing heading rows requires to check if any of a heading cell is overlapping vertically the table head. // Any table cell that has a rowspan attribute > 1 will not exceed the table head so we need to fix it in rows below. const startRow = headingRowsToSet > currentHeadingRows ? currentHeadingRows : 0; - const overlappingCells = getHorizontallyOverlappingCells( table, headingRowsToSet, startRow ); + const overlappingCells = getVerticallyOverlappingCells( table, headingRowsToSet, startRow ); for ( const { cell } of overlappingCells ) { splitHorizontally( cell, headingRowsToSet, writer ); diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index e7acd3e7031..f233201c4e6 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -13,10 +13,10 @@ import TableSelection from './tableselection'; import TableWalker from './tablewalker'; import { getColumnIndexes, - getHorizontallyOverlappingCells, + getVerticallyOverlappingCells, getRowIndexes, getSelectionAffectedTableCells, - getVerticallyOverlappingCells, + getHorizontallyOverlappingCells, isSelectionRectangular, splitHorizontally, splitVertically @@ -133,7 +133,7 @@ export default class TableClipboard extends Plugin { const columnIndexes = getColumnIndexes( selectedTableCells ); const rowIndexes = getRowIndexes( selectedTableCells ); - let { last: lastColumnOfSelection, first: firstColumnOfSelection } = columnIndexes; + let { first: firstColumnOfSelection, last: lastColumnOfSelection } = columnIndexes; let { first: firstRowOfSelection, last: lastRowOfSelection } = rowIndexes; const pasteHeight = tableUtils.getRows( pastedTable ); @@ -149,14 +149,19 @@ export default class TableClipboard extends Plugin { expandTableSize( selectedTable, lastRowOfSelection + 1, lastColumnOfSelection + 1, writer, tableUtils ); } - let lastRowOfSelectionArea = lastRowOfSelection; - let lastColumnOfSelectionArea = lastColumnOfSelection; - - // For a non- rectangular selection (ie in which some cells sticks out from a virtual selection rectangle) we need to create - // a table layout that has a rectangular selection. This will split cells so the selection become rectangular. // Beyond this point we will operate on fixed content table. - if ( !isSelectionRectangular( selectedTableCells, tableUtils ) ) { - splitCellsToRectangularSelection( selectedTableCells, writer ); + if ( selectedTableCells.length === 1 || !isSelectionRectangular( selectedTableCells, tableUtils ) ) { + const splitDimensions = { + firstRow: firstRowOfSelection, + lastRow: lastRowOfSelection, + firstColumn: firstColumnOfSelection, + lastColumn: lastColumnOfSelection + }; + + // For a non-rectangular selection (ie in which some cells sticks out from a virtual selection rectangle) we need to create + // a table layout that has a rectangular selection. This will split cells so the selection become rectangular. + // Beyond this point we will operate on fixed content table. + splitCellsToRectangularSelection( selectedTable, splitDimensions, writer ); } // However a rectangular selection might consist an invalid sub-table (if the selected cell would be moved outside the table). // This happens in a table which has: @@ -164,12 +169,12 @@ export default class TableClipboard extends Plugin { // - last column has empty slots (are covered by cells from previous columns) // This case needs only adjusting the selection dimension as the rest of the algorithm operates on empty slots also. else { - lastRowOfSelectionArea = adjustLastRowOfSelection( selectedTable, rowIndexes, columnIndexes ); - lastColumnOfSelectionArea = adjustLastColumnOfSelection( selectedTable, rowIndexes, columnIndexes ); + lastRowOfSelection = adjustLastRowIndex( selectedTable, rowIndexes, columnIndexes ); + lastColumnOfSelection = adjustLastColumnIndex( selectedTable, rowIndexes, columnIndexes ); } - const selectionHeight = lastRowOfSelectionArea - firstRowOfSelection + 1; - const selectionWidth = lastColumnOfSelectionArea - firstColumnOfSelection + 1; + const selectionHeight = lastRowOfSelection - firstRowOfSelection + 1; + const selectionWidth = lastColumnOfSelection - firstColumnOfSelection + 1; // The if below is temporal and will be removed when handling this case. // This if to be removed as handling of replicating cells should be done in replaceSelectedCellsWithPasted(). @@ -199,8 +204,8 @@ export default class TableClipboard extends Plugin { const selectionDimensions = { firstColumnOfSelection, firstRowOfSelection, - lastColumnOfSelection: lastColumnOfSelectionArea, - lastRowOfSelection: lastRowOfSelectionArea, + lastColumnOfSelection, + lastRowOfSelection, selectionHeight, selectionWidth }; @@ -225,8 +230,8 @@ export default class TableClipboard extends Plugin { function replaceSelectedCellsWithPasted( pastedTable, selectedTable, selectionDimensions, writer ) { const { firstColumnOfSelection, lastColumnOfSelection, - selectionWidth, selectionHeight, - firstRowOfSelection, lastRowOfSelection + firstRowOfSelection, lastRowOfSelection, + selectionWidth, selectionHeight } = selectionDimensions; // Holds two-dimensional array that is addressed by [ row ][ column ] that stores cells anchored at given location. @@ -416,13 +421,11 @@ function createLocationMap( table, width, height ) { // - Cell "03" which have `rowspan = 2` and `colspan = 2` must be trimmed at first column and after last row. // - Cells "00", "03" & "30" which cannot be cut by this algorithm as they are outside the trimmed area. // - Cell "13" cannot be cut as it is inside the trimmed area. -function splitCellsToRectangularSelection( selectedTableCells, writer ) { - const table = findAncestor( 'table', selectedTableCells[ 0 ] ); +function splitCellsToRectangularSelection( table, dimensions, writer ) { + const { firstRow, lastRow, firstColumn, lastColumn } = dimensions; - const rowIndexes = getRowIndexes( selectedTableCells ); - const columnIndexes = getColumnIndexes( selectedTableCells ); - const { first: firstRow, last: lastRow } = rowIndexes; - const { first: firstColumn, last: lastColumn } = columnIndexes; + const rowIndexes = { first: firstRow, last: lastRow }; + const columnIndexes = { first: firstColumn, last: lastColumn }; // 1. Split cells vertically in two steps as first step might create cells that needs to split again. doVerticalSplit( table, firstColumn, rowIndexes, writer ); @@ -439,11 +442,10 @@ function doHorizontalSplit( table, splitRow, limitColumns, writer, startRow = 0 return; } - const overlappingCells = getHorizontallyOverlappingCells( table, splitRow, startRow ); + const overlappingCells = getVerticallyOverlappingCells( table, splitRow, startRow ); // Filter out cells that are not touching insides of the rectangular selection. - const { first, last } = limitColumns; - const cellsToSplit = overlappingCells.filter( ( { column, colspan } ) => isAffectedBySelection( column, colspan, first, last ) ); + const cellsToSplit = overlappingCells.filter( ( { column, colspan } ) => isAffectedBySelection( column, colspan, limitColumns ) ); for ( const { cell } of cellsToSplit ) { splitHorizontally( cell, splitRow, writer ); @@ -456,11 +458,10 @@ function doVerticalSplit( table, splitColumn, limitRows, writer ) { return; } - const overlappingCells = getVerticallyOverlappingCells( table, splitColumn ); + const overlappingCells = getHorizontallyOverlappingCells( table, splitColumn ); // Filter out cells that are not touching insides of the rectangular selection. - const { first, last } = limitRows; - const cellsToSplit = overlappingCells.filter( ( { row, rowspan } ) => isAffectedBySelection( row, rowspan, first, last ) ); + const cellsToSplit = overlappingCells.filter( ( { row, rowspan } ) => isAffectedBySelection( row, rowspan, limitRows ) ); for ( const { cell, column } of cellsToSplit ) { splitVertically( cell, column, splitColumn, writer ); @@ -470,51 +471,38 @@ function doVerticalSplit( table, splitColumn, limitRows, writer ) { // Checks if cell at given row (column) is affected by a rectangular selection defined by first/last column (row). // // The same check is used for row as for column. -function isAffectedBySelection( rowOrColumn, rowOrColumnSpan, first, last ) { - const endIndexOfCell = rowOrColumn + rowOrColumnSpan - 1; +function isAffectedBySelection( index, span, limit ) { + const endIndex = index + span - 1; + const { first, last } = limit; - const isInsideSelection = rowOrColumn >= first && rowOrColumn <= last; - const overlapsSelectionFromOutside = rowOrColumn < first && endIndexOfCell >= first; + const isInsideSelection = index >= first && index <= last; + const overlapsSelectionFromOutside = index < first && endIndex >= first; return isInsideSelection || overlapsSelectionFromOutside; } // Returns adjusted last row index if selection covers part of a row with empty slots (spanned by other cells). -function adjustLastRowOfSelection( selectedTable, rowIndexes, columnIndexes ) { - const lastRowMap = Array.from( new TableWalker( selectedTable, { +function adjustLastRowIndex( table, rowIndexes, columnIndexes ) { + const tableIterator = new TableWalker( table, { startRow: rowIndexes.last, endRow: rowIndexes.last - } ) ).filter( ( { column } ) => { + } ); + + const lastRowMap = Array.from( tableIterator ).filter( ( { column } ) => { // Could use startColumn, endColumn. See: https://github.com/ckeditor/ckeditor5/issues/6785. return columnIndexes.first <= column && column <= columnIndexes.last; } ); - const everyCellHasSingleRowspan = lastRowMap.every( ( { rowspan } ) => rowspan === 1 ); - - if ( !everyCellHasSingleRowspan ) { - const rowspanAdjustment = lastRowMap.pop().rowspan - 1; - return rowIndexes.last + rowspanAdjustment; - } - - // Default to the last row index. - return rowIndexes.last; + return rowIndexes.last + Math.max( 1, ...lastRowMap.map( ( { rowspan } ) => rowspan ) ) - 1; } // Returns adjusted last column index if selection covers part of a column with empty slots (spanned by other cells). -function adjustLastColumnOfSelection( selectedTable, rowIndexes, columnIndexes ) { - const lastColumnMap = Array.from( new TableWalker( selectedTable, { +function adjustLastColumnIndex( table, rowIndexes, columnIndexes ) { + const lastColumnMap = Array.from( new TableWalker( table, { startRow: rowIndexes.first, endRow: rowIndexes.last, column: columnIndexes.last } ) ); - const everyCellHasSingleColspan = lastColumnMap.every( ( { colspan } ) => colspan === 1 ); - - if ( !everyCellHasSingleColspan ) { - const colspanAdjustment = lastColumnMap.pop().colspan - 1; - return columnIndexes.last + colspanAdjustment; - } - - // Default to the last column index. - return columnIndexes.last; + return columnIndexes.last + Math.max( 1, ...lastColumnMap.map( ( { colspan } ) => colspan ) ) - 1; } diff --git a/packages/ckeditor5-table/src/utils.js b/packages/ckeditor5-table/src/utils.js index 0b8e15c73bf..1268d56317b 100644 --- a/packages/ckeditor5-table/src/utils.js +++ b/packages/ckeditor5-table/src/utils.js @@ -248,7 +248,7 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) { } /** - * Returns cells that starts below and overlaps a given row. + * Returns cells that starts above and overlaps a given row. * * In a table below, passing `overlapRow = 3` * @@ -267,27 +267,24 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) { * will return cells: "j", "f", "k". * * @param {module:engine/model/element~Element} table The table to check. - * @param {Number} overlapRow + * @param {Number} overlapRow The index of the row to check. * @param {Number} [startRow=0] A row to start analysis if it is known where. * @returns {Array.} */ -export function getHorizontallyOverlappingCells( table, overlapRow, startRow = 0 ) { - const cellsToSplit = []; - - // We're analyzing only when headingRowsToSet > 0. - const endAnalysisRow = overlapRow - 1; +export function getVerticallyOverlappingCells( table, overlapRow, startRow = 0 ) { + const cells = []; - const tableWalker = new TableWalker( table, { startRow, endRow: endAnalysisRow } ); + const tableWalker = new TableWalker( table, { startRow, endRow: overlapRow - 1 } ); for ( const slotInfo of tableWalker ) { const { row, rowspan } = slotInfo; if ( rowspan > 1 && row + rowspan > overlapRow ) { - cellsToSplit.push( slotInfo ); + cells.push( slotInfo ); } } - return cellsToSplit; + return cells; } /** @@ -305,21 +302,20 @@ export function splitHorizontally( tableCell, splitRow, writer ) { const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) ); const newRowspan = splitRow - rowIndex; - const attributes = {}; + const newCellAttributes = {}; + const newCellRowSpan = rowspan - newRowspan; - const spanToSet = rowspan - newRowspan; - - if ( spanToSet > 1 ) { - attributes.rowspan = spanToSet; + if ( newCellRowSpan > 1 ) { + newCellAttributes.rowspan = newCellRowSpan; } const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 ); if ( colspan > 1 ) { - attributes.colspan = colspan; + newCellAttributes.colspan = colspan; } - const startRow = table.getChildIndex( tableRow ); + const startRow = rowIndex; const endRow = startRow + newRowspan; const tableMap = [ ...new TableWalker( table, { startRow, endRow, includeSpanned: true } ) ]; @@ -334,7 +330,7 @@ export function splitHorizontally( tableCell, splitRow, writer ) { const tableRow = table.getChild( row ); const tableCellPosition = writer.createPositionAt( tableRow, cellIndex ); - createEmptyTableCell( writer, tableCellPosition, attributes ); + createEmptyTableCell( writer, tableCellPosition, newCellAttributes ); } } @@ -365,19 +361,19 @@ export function splitHorizontally( tableCell, splitRow, writer ) { * will return cells: "b", "e", "i". * * @param {module:engine/model/element~Element} table The table to check. - * @param {Number} overlapColumn New heading columns attribute. + * @param {Number} overlapColumn The index of the column to check. * @returns {Array.} */ -export function getVerticallyOverlappingCells( table, overlapColumn ) { +export function getHorizontallyOverlappingCells( table, overlapColumn ) { const cellsToSplit = []; const tableWalker = new TableWalker( table ); for ( const slotInfo of tableWalker ) { const { column, colspan } = slotInfo; - const endColumn = column + colspan; + const endColumn = column + colspan - 1; - if ( column < overlapColumn && overlapColumn < endColumn ) { + if ( column < overlapColumn && overlapColumn <= endColumn ) { cellsToSplit.push( slotInfo ); } } @@ -389,28 +385,28 @@ export function getVerticallyOverlappingCells( table, overlapColumn ) { * Splits the table cell vertically. * * @param {module:engine/model/element~Element} tableCell - * @param {Number} splitColumn + * @param {Number} columnIndex The table cell column index. + * @param {Number} splitColumn The index of column to split cell on. * @param {module:engine/model/writer~Writer} writer */ export function splitVertically( tableCell, columnIndex, splitColumn, writer ) { const colspan = parseInt( tableCell.getAttribute( 'colspan' ) ); const newColspan = splitColumn - columnIndex; - const attributes = {}; - - const spanToSet = colspan - newColspan; + const newCellAttributes = {}; + const newCellColSpan = colspan - newColspan; - if ( spanToSet > 1 ) { - attributes.colspan = spanToSet; + if ( newCellColSpan > 1 ) { + newCellAttributes.colspan = newCellColSpan; } const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) || 1 ); if ( rowspan > 1 ) { - attributes.rowspan = rowspan; + newCellAttributes.rowspan = rowspan; } - createEmptyTableCell( writer, writer.createPositionAfter( tableCell ), attributes ); + createEmptyTableCell( writer, writer.createPositionAfter( tableCell ), newCellAttributes ); // Update the colspan attribute after updating table. updateNumericAttribute( 'colspan', newColspan, tableCell, writer ); } From 6cfab6a525285caadc5bac3bfc4340194e9de7e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 09:16:08 +0200 Subject: [PATCH 29/39] Use different wording for expandTableSize() params. --- packages/ckeditor5-table/src/tableclipboard.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index f233201c4e6..78e010b15d9 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -149,7 +149,7 @@ export default class TableClipboard extends Plugin { expandTableSize( selectedTable, lastRowOfSelection + 1, lastColumnOfSelection + 1, writer, tableUtils ); } - // Beyond this point we will operate on fixed content table. + // Beyond this point we will operate on a fixed content table. if ( selectedTableCells.length === 1 || !isSelectionRectangular( selectedTableCells, tableUtils ) ) { const splitDimensions = { firstRow: firstRowOfSelection, @@ -306,24 +306,24 @@ function replaceSelectedCellsWithPasted( pastedTable, selectedTable, selectionDi writer.setSelection( cellsToSelect.map( cell => writer.createRangeOn( cell ) ) ); } -// Expand table (in place) to expected size (rows and columns). -function expandTableSize( table, rows, columns, writer, tableUtils ) { +// Expand table (in place) to expected size. +function expandTableSize( table, expectedHeight, expectedWidth, writer, tableUtils ) { const tableWidth = tableUtils.getColumns( table ); const tableHeight = tableUtils.getRows( table ); - if ( columns > tableWidth ) { + if ( expectedWidth > tableWidth ) { tableUtils.insertColumns( table, { batch: writer.batch, at: tableWidth, - columns: columns - tableWidth + columns: expectedWidth - tableWidth } ); } - if ( rows > tableHeight ) { + if ( expectedHeight > tableHeight ) { tableUtils.insertRows( table, { batch: writer.batch, at: tableHeight, - rows: rows - tableHeight + rows: expectedHeight - tableHeight } ); } } From ef95552c0e89e3a865229bc03f913fccabf96fe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 09:16:54 +0200 Subject: [PATCH 30/39] Revert row/column adjust algorithm to the previous form. Add more docs. --- .../ckeditor5-table/src/tableclipboard.js | 51 ++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 78e010b15d9..7a1e5cc6219 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -482,6 +482,19 @@ function isAffectedBySelection( index, span, limit ) { } // Returns adjusted last row index if selection covers part of a row with empty slots (spanned by other cells). +// The rowIndexes.last is equal to last row index but selection might be bigger. +// +// This happens *only* on rectangular selection so we analyze a case like this: +// +// +---+---+---+---+ +// 0 | a | b | c | d | +// + + +---+---+ +// 1 | | e | f | g | +// + +---+ +---+ +// 2 | | h | | i | <- last row, each cell has rowspan = 2, +// + + + + + so we need to return 3, not 2 +// 3 | | | | | +// +---+---+---+---+ function adjustLastRowIndex( table, rowIndexes, columnIndexes ) { const tableIterator = new TableWalker( table, { startRow: rowIndexes.last, @@ -493,10 +506,35 @@ function adjustLastRowIndex( table, rowIndexes, columnIndexes ) { return columnIndexes.first <= column && column <= columnIndexes.last; } ); - return rowIndexes.last + Math.max( 1, ...lastRowMap.map( ( { rowspan } ) => rowspan ) ) - 1; + const everyCellHasSingleRowspan = lastRowMap.every( ( { rowspan } ) => rowspan === 1 ); + + // It is a "flat" row, so the last row index is OK. + if ( everyCellHasSingleRowspan ) { + return rowIndexes.last; + } + + // Otherwise get any cell's rowspan and adjust the last row index. + const rowspanAdjustment = lastRowMap.pop().rowspan - 1; + return rowIndexes.last + rowspanAdjustment; } // Returns adjusted last column index if selection covers part of a column with empty slots (spanned by other cells). +// The columnIndexes.last is equal to last column index but selection might be bigger. +// +// This happens *only* on rectangular selection so we analyze a case like this: +// +// 0 1 2 3 +// +---+---+---+---+ +// | a | +// +---+---+---+---+ +// | b | c | d | +// +---+---+---+---+ +// | e | f | +// +---+---+---+---+ +// | g | h | +// +---+---+---+---+ +// ^ +// last column, each cell has colspan = 2, so we need to return 3, not 2 function adjustLastColumnIndex( table, rowIndexes, columnIndexes ) { const lastColumnMap = Array.from( new TableWalker( table, { startRow: rowIndexes.first, @@ -504,5 +542,14 @@ function adjustLastColumnIndex( table, rowIndexes, columnIndexes ) { column: columnIndexes.last } ) ); - return columnIndexes.last + Math.max( 1, ...lastColumnMap.map( ( { colspan } ) => colspan ) ) - 1; + const everyCellHasSingleColspan = lastColumnMap.every( ( { colspan } ) => colspan === 1 ); + + // It is a "flat" column, so the last column index is OK. + if ( everyCellHasSingleColspan ) { + return columnIndexes.last; + } + + // Otherwise get any cell's colspan and adjust the last column index. + const colspanAdjustment = lastColumnMap.pop().colspan - 1; + return columnIndexes.last + colspanAdjustment; } From 12e386f2d7b44bed2f169ae6340ef647be87dd01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 09:57:18 +0200 Subject: [PATCH 31/39] Add internal docs. --- packages/ckeditor5-table/src/tableclipboard.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 7a1e5cc6219..159cc0630f4 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -142,15 +142,19 @@ export default class TableClipboard extends Plugin { // Content table to which we insert a pasted table. const selectedTable = findAncestor( 'table', selectedTableCells[ 0 ] ); - if ( selectedTableCells.length === 1 ) { + // Single cell selected - expand selection to pasted table dimensions. + const shouldExpandSelection = selectedTableCells.length === 1; + + if ( shouldExpandSelection ) { lastRowOfSelection += pasteHeight - 1; lastColumnOfSelection += pasteWidth - 1; expandTableSize( selectedTable, lastRowOfSelection + 1, lastColumnOfSelection + 1, writer, tableUtils ); } - // Beyond this point we will operate on a fixed content table. - if ( selectedTableCells.length === 1 || !isSelectionRectangular( selectedTableCells, tableUtils ) ) { + // In case of expanding selection we do not reset the selection so in this case we will always try to fix selection + // like in the case of a non-rectangular area. This might be fixed by re-setting selected cells array but this shortcut is safe. + if ( shouldExpandSelection || !isSelectionRectangular( selectedTableCells, tableUtils ) ) { const splitDimensions = { firstRow: firstRowOfSelection, lastRow: lastRowOfSelection, @@ -173,6 +177,8 @@ export default class TableClipboard extends Plugin { lastColumnOfSelection = adjustLastColumnIndex( selectedTable, rowIndexes, columnIndexes ); } + // Beyond this point we operate on a fixed content table with rectangular selection and proper last row/column values. + const selectionHeight = lastRowOfSelection - firstRowOfSelection + 1; const selectionWidth = lastColumnOfSelection - firstColumnOfSelection + 1; From eb0105485580a9ca33d40d8f367d20b61d716a91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 10:09:06 +0200 Subject: [PATCH 32/39] Add test for splitting table cells in one table cell selected. --- .../tests/tableclipboard-paste.js | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index df7cfbe8061..4dff6f479b1 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -765,6 +765,39 @@ describe( 'table clipboard', () => { [ '', '', '', 'ea', 'eb', 'ec', 'ed' ] ] ) ); } ); + + it( 'should fix non-rectangular are on matched table fragment', () => { + // +----+----+----+ + // | 00 | 01 | 02 | + // +----+----+ + + // | 10 | 11 | | + // +----+----+----+ + // | 20 | 22 | + // +----+----+----+ + setModelData( model, modelTable( [ + [ '00', '01', { contents: '02', rowspan: 2 } ], + [ '10', '11[]' ], + [ { contents: '20', colspan: 2 }, '22' ] + ] ) ); + + pasteTable( [ + [ 'aa', 'ab' ], + [ 'ba', 'bb' ] + ] ); + + // +----+----+----+ + // | 00 | 01 | 02 | + // +----+----+----+ + // | 10 | aa | ab | + // +----+----+----+ + // | 20 | ba | bb | + // +----+----+----+ + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01', '02' ], + [ '10', 'aa', 'ab' ], + [ '20', 'ba', 'bb' ] + ] ) ); + } ); } ); } ); From 9c1a0fd56323918cacb36e6990e2cb271bb63094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 10:12:19 +0200 Subject: [PATCH 33/39] Remove #1,#2 from tests descriptions. --- .../ckeditor5-table/tests/commands/setheadercolumncommand.js | 4 ++-- .../ckeditor5-table/tests/commands/setheaderrowcommand.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js b/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js index 48c6a285cf3..dd80ef7600c 100644 --- a/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js +++ b/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js @@ -404,7 +404,7 @@ describe( 'SetHeaderColumnCommand', () => { ], { headingColumns: 2 } ) ); } ); - it( 'should respect forceValue parameter #1', () => { + it( 'should respect forceValue parameter (forceValue=true)', () => { setData( model, modelTable( [ [ '00', '01[]', '02', '03' ] ], { headingColumns: 3 } ) ); @@ -416,7 +416,7 @@ describe( 'SetHeaderColumnCommand', () => { ], { headingColumns: 3 } ) ); } ); - it( 'should respect forceValue parameter #2', () => { + it( 'should respect forceValue parameter (forceValue=false)', () => { setData( model, modelTable( [ [ '00', '01[]', '02', '03' ] ], { headingColumns: 1 } ) ); diff --git a/packages/ckeditor5-table/tests/commands/setheaderrowcommand.js b/packages/ckeditor5-table/tests/commands/setheaderrowcommand.js index 67abee2d143..aacad2f9c7b 100644 --- a/packages/ckeditor5-table/tests/commands/setheaderrowcommand.js +++ b/packages/ckeditor5-table/tests/commands/setheaderrowcommand.js @@ -551,7 +551,7 @@ describe( 'SetHeaderRowCommand', () => { } ); } ); - it( 'should respect forceValue parameter #1', () => { + it( 'should respect forceValue parameter (forceValue=true)', () => { setData( model, modelTable( [ [ '00' ], [ '[]10' ], @@ -569,7 +569,7 @@ describe( 'SetHeaderRowCommand', () => { ], { headingRows: 3 } ) ); } ); - it( 'should respect forceValue parameter #2', () => { + it( 'should respect forceValue parameter (forceValue=false)', () => { setData( model, modelTable( [ [ '00' ], [ '[]10' ], From fc32f404955c89f63751728ec030c33effeed90d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 10:16:16 +0200 Subject: [PATCH 34/39] Add missing test for removing "headingColumns" attribute. --- .../tests/commands/setheadercolumncommand.js | 12 ++++++++++++ .../tests/commands/setheaderrowcommand.js | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js b/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js index dd80ef7600c..3d0904663b0 100644 --- a/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js +++ b/packages/ckeditor5-table/tests/commands/setheadercolumncommand.js @@ -198,6 +198,18 @@ describe( 'SetHeaderColumnCommand', () => { ], { headingColumns: 1 } ) ); } ); + it( 'should remove "headingColumns" attribute from table if no value was given', () => { + setData( model, modelTable( [ + [ '[]00', '01', '02', '03' ] + ], { headingColumns: 3 } ) ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ '[]00', '01', '02', '03' ] + ] ) ); + } ); + describe( 'multi-cell selection', () => { describe( 'setting header', () => { it( 'should set it correctly in a middle of single-row, multiple cell selection', () => { diff --git a/packages/ckeditor5-table/tests/commands/setheaderrowcommand.js b/packages/ckeditor5-table/tests/commands/setheaderrowcommand.js index aacad2f9c7b..85e53937d77 100644 --- a/packages/ckeditor5-table/tests/commands/setheaderrowcommand.js +++ b/packages/ckeditor5-table/tests/commands/setheaderrowcommand.js @@ -225,7 +225,7 @@ describe( 'SetHeaderRowCommand', () => { ], { headingRows: 1 } ) ); } ); - it( 'should unsetset heading rows attribute', () => { + it( 'should remove "headingRows" attribute from table if no value was given', () => { setData( model, modelTable( [ [ '[]00' ], [ '10' ], From 8176e23a4592fde7825d641ce2e908a129270b0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 10:46:16 +0200 Subject: [PATCH 35/39] Add tests for getVerticallyOverlappingCells() and getHorizontallyOverlappingCells(). --- packages/ckeditor5-table/src/utils.js | 17 ++--- packages/ckeditor5-table/tests/utils.js | 90 ++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 9 deletions(-) diff --git a/packages/ckeditor5-table/src/utils.js b/packages/ckeditor5-table/src/utils.js index 1268d56317b..47c03683025 100644 --- a/packages/ckeditor5-table/src/utils.js +++ b/packages/ckeditor5-table/src/utils.js @@ -248,7 +248,7 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) { } /** - * Returns cells that starts above and overlaps a given row. + * Returns slot info of cells that starts above and overlaps a given row. * * In a table below, passing `overlapRow = 3` * @@ -264,11 +264,11 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) { * 4 │ o │ p │ │ │ q │ * └───┴───┴───┴───┴───┘ * - * will return cells: "j", "f", "k". + * will return slot info for cells: "j", "f", "k". * * @param {module:engine/model/element~Element} table The table to check. * @param {Number} overlapRow The index of the row to check. - * @param {Number} [startRow=0] A row to start analysis if it is known where. + * @param {Number} [startRow=0] A row to start analysis. Use it when it is known that cells below that row will not overlap. * @returns {Array.} */ export function getVerticallyOverlappingCells( table, overlapRow, startRow = 0 ) { @@ -278,8 +278,9 @@ export function getVerticallyOverlappingCells( table, overlapRow, startRow = 0 ) for ( const slotInfo of tableWalker ) { const { row, rowspan } = slotInfo; + const slotEndRow = row + rowspan - 1; - if ( rowspan > 1 && row + rowspan > overlapRow ) { + if ( row < overlapRow && overlapRow <= slotEndRow ) { cells.push( slotInfo ); } } @@ -339,7 +340,7 @@ export function splitHorizontally( tableCell, splitRow, writer ) { } /** - * Returns cells that starts before and overlaps a given column. + * Returns slot info of cells that starts before and overlaps a given column. * * In a table below, passing `overlapColumn = 3` * @@ -358,7 +359,7 @@ export function splitHorizontally( tableCell, splitRow, writer ) { * ^ * Overlap column to check * - * will return cells: "b", "e", "i". + * will return slot info for cells: "b", "e", "i". * * @param {module:engine/model/element~Element} table The table to check. * @param {Number} overlapColumn The index of the column to check. @@ -371,9 +372,9 @@ export function getHorizontallyOverlappingCells( table, overlapColumn ) { for ( const slotInfo of tableWalker ) { const { column, colspan } = slotInfo; - const endColumn = column + colspan - 1; + const slotEndColumn = column + colspan - 1; - if ( column < overlapColumn && overlapColumn <= endColumn ) { + if ( column < overlapColumn && overlapColumn <= slotEndColumn ) { cellsToSplit.push( slotInfo ); } } diff --git a/packages/ckeditor5-table/tests/utils.js b/packages/ckeditor5-table/tests/utils.js index 7815e5d19d1..14fe46bbab2 100644 --- a/packages/ckeditor5-table/tests/utils.js +++ b/packages/ckeditor5-table/tests/utils.js @@ -13,7 +13,7 @@ import { modelTable } from './_utils/utils'; import { getSelectedTableCells, getTableCellsContainingSelection, - getSelectionAffectedTableCells + getSelectionAffectedTableCells, getVerticallyOverlappingCells, getHorizontallyOverlappingCells } from '../src/utils'; describe( 'table utils', () => { @@ -359,4 +359,92 @@ describe( 'table utils', () => { expect( getSelectionAffectedTableCells( selection ) ).to.be.empty; } ); } ); + + describe( 'getVerticallyOverlappingCells()', () => { + let table; + + beforeEach( () => { + // +----+----+----+----+----+ + // | 00 | 01 | 02 | 03 | 04 | + // + + +----+ +----+ + // | | | 12 | | 14 | + // + + + +----+----+ + // | | | | 23 | 24 | + // + +----+ + +----+ + // | | 31 | | | 34 | + // + + +----+----+----+ + // | | | 42 | 43 | 44 | + // +----+----+----+----+----+ + setModelData( model, modelTable( [ + [ { contents: '00', rowspan: 5 }, { contents: '01', rowspan: 3 }, '02', { contents: '03', rowspan: 2 }, '04' ], + [ { contents: '12', rowspan: 3 }, '14' ], + [ { contents: '23', rowspan: 2 }, '24' ], + [ { contents: '31', rowspan: 2 }, '34' ], + [ '42', '43', '44' ] + ] ) ); + + table = modelRoot.getChild( 0 ); + } ); + + it( 'should return empty array for no overlapping cells', () => { + const cellsInfo = getVerticallyOverlappingCells( table, 0 ); + + expect( cellsInfo ).to.be.empty; + } ); + + it( 'should return overlapping cells info for given overlapRow', () => { + const cellsInfo = getVerticallyOverlappingCells( table, 2 ); + + expect( cellsInfo[ 0 ].cell ).to.equal( modelRoot.getNodeByPath( [ 0, 0, 0 ] ) ); // Cell 00 + expect( cellsInfo[ 1 ].cell ).to.equal( modelRoot.getNodeByPath( [ 0, 0, 1 ] ) ); // Cell 01 + expect( cellsInfo[ 2 ].cell ).to.equal( modelRoot.getNodeByPath( [ 0, 1, 0 ] ) ); // Cell 12 + } ); + + it( 'should ignore rows below startRow', () => { + const cellsInfo = getVerticallyOverlappingCells( table, 2, 1 ); + + expect( cellsInfo[ 0 ].cell ).to.equal( modelRoot.getNodeByPath( [ 0, 1, 0 ] ) ); // Cell 12 + } ); + } ); + + describe( 'getHorizontallyOverlappingCells()', () => { + let table; + + beforeEach( () => { + // +----+----+----+----+----+ + // | 00 | + // +----+----+----+----+----+ + // | 10 | 13 | + // +----+----+----+----+----+ + // | 20 | 21 | 24 | + // +----+----+----+----+----+ + // | 30 | 32 | 34 | + // +----+----+----+----+----+ + // | 40 | 41 | 42 | 43 | 44 | + // +----+----+----+----+----+ + setModelData( model, modelTable( [ + [ { contents: '00', colspan: 5 } ], + [ { contents: '10', colspan: 3 }, { contents: '13', colspan: 2 } ], + [ '20', { contents: '21', colspan: 3 }, '24' ], + [ { contents: '30', colspan: 2 }, { contents: '32', colspan: 2 }, '34' ], + [ '40', '41', '42', '43', '44' ] + ] ) ); + + table = modelRoot.getChild( 0 ); + } ); + + it( 'should return empty array for no overlapping cells', () => { + const cellsInfo = getHorizontallyOverlappingCells( table, 0 ); + + expect( cellsInfo ).to.be.empty; + } ); + + it( 'should return overlapping cells info for given overlapColumn', () => { + const cellsInfo = getHorizontallyOverlappingCells( table, 2 ); + + expect( cellsInfo[ 0 ].cell ).to.equal( modelRoot.getNodeByPath( [ 0, 0, 0 ] ) ); // Cell 00 + expect( cellsInfo[ 1 ].cell ).to.equal( modelRoot.getNodeByPath( [ 0, 1, 0 ] ) ); // Cell 10 + expect( cellsInfo[ 2 ].cell ).to.equal( modelRoot.getNodeByPath( [ 0, 2, 1 ] ) ); // Cell 21 + } ); + } ); } ); From aa9a854d2c77708043d7b1fd3746d085c1a49059 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 11:14:46 +0200 Subject: [PATCH 36/39] Add better comment about fixing selection dimensions. --- packages/ckeditor5-table/src/tableclipboard.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 159cc0630f4..774ff70e89c 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -167,10 +167,19 @@ export default class TableClipboard extends Plugin { // Beyond this point we will operate on fixed content table. splitCellsToRectangularSelection( selectedTable, splitDimensions, writer ); } - // However a rectangular selection might consist an invalid sub-table (if the selected cell would be moved outside the table). - // This happens in a table which has: - // - last row has empty slots (so are covered by cells from rows above) and/or - // - last column has empty slots (are covered by cells from previous columns) + // However a selected table fragment might be invalid if examined alone. Ie such table fragment: + // + // +---+---+---+---+ + // 0 | a | b | c | d | + // + + +---+---+ + // 1 | | e | f | g | + // + +---+ +---+ + // 2 | | h | | i | <- last row, each cell has rowspan = 2, + // + + + + + so we need to return 3, not 2 + // 3 | | | | | + // +---+---+---+---+ + // + // is invalid as the cells "h" and "i" have rowspans. // This case needs only adjusting the selection dimension as the rest of the algorithm operates on empty slots also. else { lastRowOfSelection = adjustLastRowIndex( selectedTable, rowIndexes, columnIndexes ); From 7385fcb656b90b351173562d208a42aeafa4e696 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 11:25:06 +0200 Subject: [PATCH 37/39] Minor refactor. --- packages/ckeditor5-table/src/tableclipboard.js | 4 ++-- packages/ckeditor5-table/src/utils.js | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 774ff70e89c..91170bfb9bb 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -529,7 +529,7 @@ function adjustLastRowIndex( table, rowIndexes, columnIndexes ) { } // Otherwise get any cell's rowspan and adjust the last row index. - const rowspanAdjustment = lastRowMap.pop().rowspan - 1; + const rowspanAdjustment = lastRowMap[ 0 ].rowspan - 1; return rowIndexes.last + rowspanAdjustment; } @@ -565,6 +565,6 @@ function adjustLastColumnIndex( table, rowIndexes, columnIndexes ) { } // Otherwise get any cell's colspan and adjust the last column index. - const colspanAdjustment = lastColumnMap.pop().colspan - 1; + const colspanAdjustment = lastColumnMap[ 0 ].colspan - 1; return columnIndexes.last + colspanAdjustment; } diff --git a/packages/ckeditor5-table/src/utils.js b/packages/ckeditor5-table/src/utils.js index 47c03683025..6df34db94ea 100644 --- a/packages/ckeditor5-table/src/utils.js +++ b/packages/ckeditor5-table/src/utils.js @@ -278,9 +278,9 @@ export function getVerticallyOverlappingCells( table, overlapRow, startRow = 0 ) for ( const slotInfo of tableWalker ) { const { row, rowspan } = slotInfo; - const slotEndRow = row + rowspan - 1; + const cellEndRow = row + rowspan - 1; - if ( row < overlapRow && overlapRow <= slotEndRow ) { + if ( row < overlapRow && overlapRow <= cellEndRow ) { cells.push( slotInfo ); } } @@ -372,9 +372,9 @@ export function getHorizontallyOverlappingCells( table, overlapColumn ) { for ( const slotInfo of tableWalker ) { const { column, colspan } = slotInfo; - const slotEndColumn = column + colspan - 1; + const cellEndColumn = column + colspan - 1; - if ( column < overlapColumn && overlapColumn <= slotEndColumn ) { + if ( column < overlapColumn && overlapColumn <= cellEndColumn ) { cellsToSplit.push( slotInfo ); } } From e50d90872edeb5874e69f0f7013d1f6784219178 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 20 May 2020 11:31:26 +0200 Subject: [PATCH 38/39] Fix code style in tests. --- packages/ckeditor5-table/tests/tableclipboard-paste.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 4dff6f479b1..edd54218310 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -1128,10 +1128,10 @@ describe( 'table clipboard', () => { /* eslint-disable no-multi-spaces */ assertSelectedCells( model, [ - [ 1, 1, 1, 1, 0 ], - [ 1, 1, 0 ], - [ 1, 1, 0 ], - [ 1, 1, 1, 0 ], + [ 1, 1, 1, 1, 0 ], + [ 1, 1, 0 ], + [ 1, 1, 0 ], + [ 1, 1, 1, 0 ], [ 0, 0, 0, 0, 0, 0 ] ] ); /* eslint-enable no-multi-spaces */ From d6720f7af3cdd908a718d98fe5bc4d83079eb077 Mon Sep 17 00:00:00 2001 From: Maciej Date: Wed, 20 May 2020 11:42:06 +0200 Subject: [PATCH 39/39] Fix above/blow wording in packages/ckeditor5-table/src/utils.js. Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com> --- packages/ckeditor5-table/src/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-table/src/utils.js b/packages/ckeditor5-table/src/utils.js index 6df34db94ea..f4d9f39eefd 100644 --- a/packages/ckeditor5-table/src/utils.js +++ b/packages/ckeditor5-table/src/utils.js @@ -268,7 +268,7 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) { * * @param {module:engine/model/element~Element} table The table to check. * @param {Number} overlapRow The index of the row to check. - * @param {Number} [startRow=0] A row to start analysis. Use it when it is known that cells below that row will not overlap. + * @param {Number} [startRow=0] A row to start analysis. Use it when it is known that the cells above that row will not overlap. * @returns {Array.} */ export function getVerticallyOverlappingCells( table, overlapRow, startRow = 0 ) {