From 331c6d09680005aa60d040064f0225182064c805 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 8 Jul 2020 18:43:53 +0200 Subject: [PATCH 1/4] Cells that overlap from headings after table paste should be splitted. --- .../ckeditor5-table/src/tableclipboard.js | 32 +++++++-- packages/ckeditor5-table/src/utils/common.js | 4 ++ .../ckeditor5-table/src/utils/structure.js | 12 +++- .../tests/manual/tablemocking.html | 15 +++- .../tests/manual/tablemocking.js | 4 ++ .../tests/manual/tablemocking.md | 2 - .../tests/tableclipboard-paste.js | 69 +++++++++++++++++++ 7 files changed, 125 insertions(+), 13 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 84ff5e5b65e..0f2914d3109 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -316,7 +316,29 @@ function replaceSelectedCellsWithPasted( pastedTable, pastedDimensions, selected insertPosition = writer.createPositionAfter( cellToInsert ); } - writer.setSelection( cellsToSelect.map( cell => writer.createRangeOn( cell ) ) ); + // If there are any headings, all the cells that overlap from heading must be splitted. + const headingRows = parseInt( selectedTable.getAttribute( 'headingRows' ) || 0 ); + const headingColumns = parseInt( selectedTable.getAttribute( 'headingColumns' ) || 0 ); + + if ( selection.firstRow < headingRows && headingRows <= selection.lastRow ) { + const columnsLimit = { first: selection.firstColumn, last: selection.lastColumn }; + const newCells = doHorizontalSplit( selectedTable, headingRows, columnsLimit, writer, selection.firstRow ); + + cellsToSelect.push( ...newCells ); + } + + if ( selection.firstColumn < headingColumns && headingColumns <= selection.lastColumn ) { + const rowsLimit = { first: selection.firstRow, last: selection.lastRow }; + const newCells = doVerticalSplit( selectedTable, headingColumns, rowsLimit, writer ); + + cellsToSelect.push( ...newCells ); + } + + const selectionRanges = cellsToSelect + .map( cell => writer.createRangeOn( cell ) ) + .sort( ( a, b ) => a.start.isBefore( b.start ) ? -1 : 1 ); + + writer.setSelection( selectionRanges ); } // Expand table (in place) to expected size. @@ -489,9 +511,7 @@ function doHorizontalSplit( table, splitRow, limitColumns, writer, startRow = 0 // Filter out cells that are not touching insides of the rectangular selection. const cellsToSplit = overlappingCells.filter( ( { column, cellWidth } ) => isAffectedBySelection( column, cellWidth, limitColumns ) ); - for ( const { cell } of cellsToSplit ) { - splitHorizontally( cell, splitRow, writer ); - } + return cellsToSplit.map( ( { cell } ) => splitHorizontally( cell, splitRow, writer ) ); } function doVerticalSplit( table, splitColumn, limitRows, writer ) { @@ -505,9 +525,7 @@ function doVerticalSplit( table, splitColumn, limitRows, writer ) { // Filter out cells that are not touching insides of the rectangular selection. const cellsToSplit = overlappingCells.filter( ( { row, cellHeight } ) => isAffectedBySelection( row, cellHeight, limitRows ) ); - for ( const { cell, column } of cellsToSplit ) { - splitVertically( cell, column, splitColumn, writer ); - } + return cellsToSplit.map( ( { cell, column } ) => splitVertically( cell, column, splitColumn, writer ) ); } // Checks if cell at given row (column) is affected by a rectangular selection defined by first/last column (row). diff --git a/packages/ckeditor5-table/src/utils/common.js b/packages/ckeditor5-table/src/utils/common.js index bef021eaea2..173d49b7bee 100644 --- a/packages/ckeditor5-table/src/utils/common.js +++ b/packages/ckeditor5-table/src/utils/common.js @@ -50,11 +50,15 @@ export function updateNumericAttribute( key, value, item, writer, defaultValue = * @param {module:engine/model/writer~Writer} writer The model writer. * @param {module:engine/model/position~Position} insertPosition The position at which the table cell should be inserted. * @param {Object} attributes The element attributes. + * @returns {module:engine/model/element~Element} Created table cell. */ export function createEmptyTableCell( writer, insertPosition, attributes = {} ) { const tableCell = writer.createElement( 'tableCell', attributes ); + writer.insertElement( 'paragraph', tableCell ); writer.insert( tableCell, insertPosition ); + + return tableCell; } /** diff --git a/packages/ckeditor5-table/src/utils/structure.js b/packages/ckeditor5-table/src/utils/structure.js index 41fc71a496c..003eaed7d05 100644 --- a/packages/ckeditor5-table/src/utils/structure.js +++ b/packages/ckeditor5-table/src/utils/structure.js @@ -138,6 +138,7 @@ export function getVerticallyOverlappingCells( table, overlapRow, startRow = 0 ) * @param {module:engine/model/element~Element} tableCell * @param {Number} splitRow * @param {module:engine/model/writer~Writer} writer + * @returns {module:engine/model/element~Element} Created table cell. */ export function splitHorizontally( tableCell, splitRow, writer ) { const tableRow = tableCell.parent; @@ -164,6 +165,7 @@ export function splitHorizontally( tableCell, splitRow, writer ) { const endRow = startRow + newRowspan; const tableMap = [ ...new TableWalker( table, { startRow, endRow, includeAllSlots: true } ) ]; + let newCell = null; let columnIndex; for ( const tableSlot of tableMap ) { @@ -174,12 +176,14 @@ export function splitHorizontally( tableCell, splitRow, writer ) { } if ( columnIndex !== undefined && columnIndex === column && row === endRow ) { - createEmptyTableCell( writer, tableSlot.getPositionBefore(), newCellAttributes ); + newCell = createEmptyTableCell( writer, tableSlot.getPositionBefore(), newCellAttributes ); } } // Update the rowspan attribute after updating table. updateNumericAttribute( 'rowspan', newRowspan, tableCell, writer ); + + return newCell; } /** @@ -232,6 +236,7 @@ export function getHorizontallyOverlappingCells( table, overlapColumn ) { * @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 + * @returns {module:engine/model/element~Element} Created table cell. */ export function splitVertically( tableCell, columnIndex, splitColumn, writer ) { const colspan = parseInt( tableCell.getAttribute( 'colspan' ) ); @@ -250,9 +255,12 @@ export function splitVertically( tableCell, columnIndex, splitColumn, writer ) { newCellAttributes.rowspan = rowspan; } - createEmptyTableCell( writer, writer.createPositionAfter( tableCell ), newCellAttributes ); + const newCell = createEmptyTableCell( writer, writer.createPositionAfter( tableCell ), newCellAttributes ); + // Update the colspan attribute after updating table. updateNumericAttribute( 'colspan', newColspan, tableCell, writer ); + + return newCell; } /** diff --git a/packages/ckeditor5-table/tests/manual/tablemocking.html b/packages/ckeditor5-table/tests/manual/tablemocking.html index 8569e04e56e..a3987980de5 100644 --- a/packages/ckeditor5-table/tests/manual/tablemocking.html +++ b/packages/ckeditor5-table/tests/manual/tablemocking.html @@ -12,10 +12,16 @@ box-sizing: border-box; margin: 10px 0; } - pre,code { + pre, code { font-size: 11px; font-family: Menlo, Consolas, Lucida Console, Courier New, dejavu sans mono, monospace; } + #table-tools pre { + background: hsl( 0, 0%, 95% ); + max-height:300px; + overflow: auto; + padding: 10px; + } .diff-add { color: hsl( 120, 70%, 35% ); } @@ -61,4 +67,9 @@
-

+
+

+
+	

Clipboard preview:

+

+
diff --git a/packages/ckeditor5-table/tests/manual/tablemocking.js b/packages/ckeditor5-table/tests/manual/tablemocking.js index 1f67b8f81e0..83c2719248f 100644 --- a/packages/ckeditor5-table/tests/manual/tablemocking.js +++ b/packages/ckeditor5-table/tests/manual/tablemocking.js @@ -33,6 +33,10 @@ ClassicEditor const asciiOut = document.getElementById( 'ascii-art' ); const modelData = document.getElementById( 'model-data' ); + editor.editing.view.document.on( 'paste', ( evt, data ) => { + document.getElementById( 'clipboard' ).innerText = data.dataTransfer.getData( 'text/html' ).replace( />(?=<)/g, '>\n' ); + } ); + document.getElementById( 'clear-content' ).addEventListener( 'click', () => { editor.setData( '' ); } ); diff --git a/packages/ckeditor5-table/tests/manual/tablemocking.md b/packages/ckeditor5-table/tests/manual/tablemocking.md index ae951b3d4d2..21915562eae 100644 --- a/packages/ckeditor5-table/tests/manual/tablemocking.md +++ b/packages/ckeditor5-table/tests/manual/tablemocking.md @@ -22,5 +22,3 @@ setModelData( model, modelTable( [ [ '40', '41', '42', '43', '44' ] ] ) ); ``` - -**Note:** Cell content is ignored while generating ASCII-art and `modelTableData`. diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index e7eac1472f4..147621ba61a 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -3385,6 +3385,75 @@ describe( 'table clipboard', () => { } ); } ); + + describe( 'headings overlapping selected area', () => { + // TODO more tests + + it( 'should split cells that overlap from headings', () => { + setModelData( model, modelTable( [ + [ '00[]', '01', '02', '03', '04' ], + [ '10', '11', '12', '13', '14' ], + [ '20', '21', '22', '23', '24' ], + [ '30', '31', '32', '33', '34' ], + [ '40', '41', '42', '43', '44' ] + ], { headingRows: 2, headingColumns: 2 } ) ); + + // +----+----+----+----+ + // | aa | ad | + // + +----+ + // | | bd | + // + +----+ + // | | cd | + // +----+----+----+----+ + // | da | db | dc | dd | + // +----+----+----+----+ + pasteTable( [ + [ { contents: 'aa', colspan: 3, rowspan: 3 }, 'ad' ], + [ 'bd' ], + [ 'cd' ], + [ 'da', 'db', 'dc', 'dd' ] + ] ); + + // +----+----+----+----+----+ + // | aa | | ad | 04 | + // + + +----+----+ + // | | | bd | 14 | + // +----+----+----+----+----+ <-- heading rows + // | | | cd | 24 | + // +----+----+----+----+----+ + // | da | db | dc | dd | 34 | + // +----+----+----+----+----+ + // | 40 | 41 | 42 | 43 | 44 | + // +----+----+----+----+----+ + // ^-- heading columns + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ { contents: 'aa', colspan: 2, rowspan: 2 }, { contents: '', rowspan: 2 }, 'ad', '04' ], + [ 'bd', '14' ], + [ { contents: '', colspan: 2 }, '', 'cd', '24' ], + [ 'da', 'db', 'dc', 'dd', '34' ], + [ '40', '41', '42', '43', '44' ] + ], { headingRows: 2, headingColumns: 2 } ) ); + + const selectionRanges = Array.from( model.document.selection.getRanges() ); + const selectedCellsPaths = selectionRanges.map( ( { start } ) => start.path ); + + // Note that order of ranges is also important. + expect( selectionRanges.length ).to.be.equal( 11 ); + expect( selectedCellsPaths ).to.deep.equal( [ + [ 0, 0, 0 ], + [ 0, 0, 1 ], + [ 0, 0, 2 ], + [ 0, 1, 0 ], + [ 0, 2, 0 ], + [ 0, 2, 1 ], + [ 0, 2, 2 ], + [ 0, 3, 0 ], + [ 0, 3, 1 ], + [ 0, 3, 2 ], + [ 0, 3, 3 ] + ] ); + } ); + } ); } ); describe( 'Clipboard integration - paste (content scenarios)', () => { From 04670185a3cafabf2794c0017b30194261ada421 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 8 Jul 2020 19:06:54 +0200 Subject: [PATCH 2/4] Updated table selection assertion. --- .../tests/tableclipboard-paste.js | 37 ++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 147621ba61a..398cf872c8e 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -3434,25 +3434,28 @@ describe( 'table clipboard', () => { [ '40', '41', '42', '43', '44' ] ], { headingRows: 2, headingColumns: 2 } ) ); - const selectionRanges = Array.from( model.document.selection.getRanges() ); - const selectedCellsPaths = selectionRanges.map( ( { start } ) => start.path ); - - // Note that order of ranges is also important. - expect( selectionRanges.length ).to.be.equal( 11 ); - expect( selectedCellsPaths ).to.deep.equal( [ - [ 0, 0, 0 ], - [ 0, 0, 1 ], - [ 0, 0, 2 ], - [ 0, 1, 0 ], - [ 0, 2, 0 ], - [ 0, 2, 1 ], - [ 0, 2, 2 ], - [ 0, 3, 0 ], - [ 0, 3, 1 ], - [ 0, 3, 2 ], - [ 0, 3, 3 ] + assertSelectionRangesSorted(); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 1, 1, 1, 0 ], + [ 1, 0 ], + [ 1, 1, 1, 0 ], + [ 1, 1, 1, 1, 0 ], + [ 0, 0, 0, 0, 0 ] ] ); + /* eslint-enable no-multi-spaces */ } ); + + function assertSelectionRangesSorted() { + const selectionRanges = Array.from( model.document.selection.getRanges() ); + const selectionRangesSorted = selectionRanges.slice().sort( ( a, b ) => a.start.isBefore( b.start ) ? -1 : 1 ); + + const selectionPaths = selectionRanges.map( ( { start } ) => start.path ); + const sortedPaths = selectionRangesSorted.map( ( { start } ) => start.path ); + + expect( selectionPaths ).to.deep.equal( sortedPaths ); + } } ); } ); From 3a7176c77ac90528e92373b56f35b8d7825f7f73 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 9 Jul 2020 15:10:07 +0200 Subject: [PATCH 3/4] Added tests for table paste mixed selection scenarios. --- .../tests/tableclipboard-paste.js | 303 ++++++++++++++++-- 1 file changed, 277 insertions(+), 26 deletions(-) diff --git a/packages/ckeditor5-table/tests/tableclipboard-paste.js b/packages/ckeditor5-table/tests/tableclipboard-paste.js index 398cf872c8e..8810dce2da4 100644 --- a/packages/ckeditor5-table/tests/tableclipboard-paste.js +++ b/packages/ckeditor5-table/tests/tableclipboard-paste.js @@ -3387,16 +3387,140 @@ describe( 'table clipboard', () => { } ); describe( 'headings overlapping selected area', () => { - // TODO more tests + beforeEach( () => { + setModelData( model, modelTable( [ + [ '00', '01', '02', '03', '04', '05' ], + [ '10', '11', '12', '13', '14', '15' ], + [ '20', '21', '22', '23', '24', '25' ], + [ '30', '31', '32', '33', '34', '35' ], + [ '40', '41', '42', '43', '44', '45' ], + [ '50', '51', '52', '53', '54', '55' ] + ], { headingRows: 3, headingColumns: 3 } ) ); + } ); + + it( 'should not split cells if they are not overlapping from headings', () => { + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 0, 0 ] ) + ); + + // +----+----+----+----+ + // | aa | ad | + // + +----+ + // | | bd | + // + +----+ + // | | cd | + // +----+----+----+----+ + // | da | db | dc | dd | + // +----+----+----+----+ + pasteTable( [ + [ { contents: 'aa', colspan: 3, rowspan: 3 }, 'ad' ], + [ 'bd' ], + [ 'cd' ], + [ 'da', 'db', 'dc', 'dd' ] + ] ); + + // +----+----+----+----+----+----+ + // | aa | ad | 04 | 05 | + // + +----+----+----+ + // | | bd | 14 | 15 | + // + +----+----+----+ + // | | cd | 24 | 25 | + // +----+----+----+----+----+----+ <-- heading rows + // | da | db | dc | dd | 34 | 35 | + // +----+----+----+----+----+----+ + // | 40 | 41 | 42 | 43 | 44 | 45 | + // +----+----+----+----+----+----+ + // | 50 | 51 | 52 | 53 | 54 | 55 | + // +----+----+----+----+----+----+ + // ^-- heading columns + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ { contents: 'aa', colspan: 3, rowspan: 3 }, 'ad', '04', '05' ], + [ 'bd', '14', '15' ], + [ 'cd', '24', '25' ], + [ 'da', 'db', 'dc', 'dd', '34', '35' ], + [ '40', '41', '42', '43', '44', '45' ], + [ '50', '51', '52', '53', '54', '55' ] + ], { headingRows: 3, headingColumns: 3 } ) ); + + assertSelectionRangesSorted(); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 1, 1, 0, 0 ], + [ 1, 0, 0 ], + [ 1, 0, 0 ], + [ 1, 1, 1, 1, 0, 0 ], + [ 0, 0, 0, 0, 0, 0 ], + [ 0, 0, 0, 0, 0, 0 ] + ] ); + /* eslint-enable no-multi-spaces */ + } ); it( 'should split cells that overlap from headings', () => { - setModelData( model, modelTable( [ - [ '00[]', '01', '02', '03', '04' ], - [ '10', '11', '12', '13', '14' ], - [ '20', '21', '22', '23', '24' ], - [ '30', '31', '32', '33', '34' ], - [ '40', '41', '42', '43', '44' ] - ], { headingRows: 2, headingColumns: 2 } ) ); + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 1, 1 ] ), + modelRoot.getNodeByPath( [ 0, 1, 1 ] ) + ); + + // +----+----+----+----+ + // | aa | ad | + // + +----+ + // | | bd | + // + +----+ + // | | cd | + // +----+----+----+----+ + // | da | db | dc | dd | + // +----+----+----+----+ + pasteTable( [ + [ { contents: 'aa', colspan: 3, rowspan: 3 }, 'ad' ], + [ 'bd' ], + [ 'cd' ], + [ 'da', 'db', 'dc', 'dd' ] + ] ); + + // +----+----+----+----+----+----+ + // | 00 | 01 | 02 | 03 | 04 | 05 | + // +----+----+----+----+----+----+ + // | 10 | aa | | ad | 15 | + // +----+ + +----+----+ + // | 20 | | | bd | 25 | + // +----+----+----+----+----+----+ <-- heading rows + // | 30 | | | cd | 35 | + // +----+----+----+----+----+----+ + // | 40 | da | db | dc | dd | 45 | + // +----+----+----+----+----+----+ + // | 50 | 51 | 52 | 53 | 54 | 55 | + // +----+----+----+----+----+----+ + // ^-- heading columns + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01', '02', '03', '04', '05' ], + [ '10', { contents: 'aa', colspan: 2, rowspan: 2 }, { contents: '', rowspan: 2 }, 'ad', '15' ], + [ '20', 'bd', '25' ], + [ '30', { contents: '', colspan: 2 }, '', 'cd', '35' ], + [ '40', 'da', 'db', 'dc', 'dd', '45' ], + [ '50', '51', '52', '53', '54', '55' ] + ], { headingRows: 3, headingColumns: 3 } ) ); + + assertSelectionRangesSorted(); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 0, 0, 0, 0, 0, 0 ], + [ 0, 1, 1, 1, 0 ], + [ 0, 1, 0 ], + [ 0, 1, 1, 1, 0 ], + [ 0, 1, 1, 1, 1, 0 ], + [ 0, 0, 0, 0, 0, 0 ] + ] ); + /* eslint-enable no-multi-spaces */ + } ); + + it( 'should split cells that overlap from heading rows', () => { + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 2, 3 ] ), + modelRoot.getNodeByPath( [ 0, 2, 3 ] ) + ); // +----+----+----+----+ // | aa | ad | @@ -3414,35 +3538,162 @@ describe( 'table clipboard', () => { [ 'da', 'db', 'dc', 'dd' ] ] ); + // +----+----+----+----+----+----+----+ + // | 00 | 01 | 02 | 03 | 04 | 05 | | + // +----+----+----+----+----+----+----+ + // | 10 | 11 | 12 | 13 | 14 | 15 | | + // +----+----+----+----+----+----+----+ + // | 20 | 21 | 22 | aa | ad | + // +----+----+----+----+----+----+----+ <-- heading rows + // | 30 | 31 | 32 | | bd | + // +----+----+----+ +----+ + // | 40 | 41 | 42 | | cd | + // +----+----+----+----+----+----+----+ + // | 50 | 51 | 52 | da | db | dc | dd | + // +----+----+----+----+----+----+----+ + // ^-- heading columns + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01', '02', '03', '04', '05', '' ], + [ '10', '11', '12', '13', '14', '15', '' ], + [ '20', '21', '22', { contents: 'aa', colspan: 3 }, 'ad' ], + [ '30', '31', '32', { contents: '', colspan: 3, rowspan: 2 }, 'bd' ], + [ '40', '41', '42', 'cd' ], + [ '50', '51', '52', 'da', 'db', 'dc', 'dd' ] + ], { headingRows: 3, headingColumns: 3 } ) ); + + assertSelectionRangesSorted(); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 0, 0, 0, 0, 0, 0, 0 ], + [ 0, 0, 0, 0, 0, 0, 0 ], + [ 0, 0, 0, 1, 1 ], + [ 0, 0, 0, 1, 1 ], + [ 0, 0, 0, 1 ], + [ 0, 0, 0, 1, 1, 1, 1 ] + ] ); + /* eslint-enable no-multi-spaces */ + } ); + + it( 'should split cells that overlap from heading columns', () => { + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 3, 2 ] ), + modelRoot.getNodeByPath( [ 0, 3, 2 ] ) + ); + + // +----+----+----+----+ + // | aa | ad | + // + +----+ + // | | bd | + // + +----+ + // | | cd | + // +----+----+----+----+ + // | da | db | dc | dd | + // +----+----+----+----+ + pasteTable( [ + [ { contents: 'aa', colspan: 3, rowspan: 3 }, 'ad' ], + [ 'bd' ], + [ 'cd' ], + [ 'da', 'db', 'dc', 'dd' ] + ] ); + + // +----+----+----+----+----+----+ + // | 00 | 01 | 02 | 03 | 04 | 05 | + // +----+----+----+----+----+----+ + // | 10 | 11 | 12 | 13 | 14 | 15 | + // +----+----+----+----+----+----+ + // | 20 | 21 | 22 | 23 | 24 | 25 | + // +----+----+----+----+----+----+ <-- heading rows + // | 30 | 31 | aa | | ad | + // +----+----+ + +----+ + // | 40 | 41 | | | bd | + // +----+----+ + +----+ + // | 50 | 51 | | | cd | + // +----+----+----+----+----+----+ + // | | | da | db | dc | dd | + // +----+----+----+----+----+----+ + // ^-- heading columns + assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ + [ '00', '01', '02', '03', '04', '05' ], + [ '10', '11', '12', '13', '14', '15' ], + [ '20', '21', '22', '23', '24', '25' ], + [ '30', '31', { contents: 'aa', rowspan: 3 }, { contents: '', colspan: 2, rowspan: 3 }, 'ad' ], + [ '40', '41', 'bd' ], + [ '50', '51', 'cd' ], + [ '', '', 'da', 'db', 'dc', 'dd' ] + ], { headingRows: 3, headingColumns: 3 } ) ); + + assertSelectionRangesSorted(); + + /* eslint-disable no-multi-spaces */ + assertSelectedCells( model, [ + [ 0, 0, 0, 0, 0, 0 ], + [ 0, 0, 0, 0, 0, 0 ], + [ 0, 0, 0, 0, 0, 0 ], + [ 0, 0, 1, 1, 1 ], + [ 0, 0, 1 ], + [ 0, 0, 1 ], + [ 0, 0, 1, 1, 1, 1 ] + ] ); + /* eslint-enable no-multi-spaces */ + } ); + + it( 'should split cells that overlap from headings (repeated pasted table)', () => { + setModelData( model, modelTable( [ + [ '00', '01', '02', '03', '04' ], + [ '10', '11', '12', '13', '14' ], + [ '20', '21', '22', '23', '24' ], + [ '30', '31', '32', '33', '34' ], + [ '40', '41', '42', '43', '44' ] + ], { headingRows: 1, headingColumns: 1 } ) ); + + tableSelection.setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 4, 4 ] ) + ); + + // +----+----+----+ + // | aa | ac | + // + +----+ + // | | bc | + // +----+----+----+ + // | ca | cb | cc | + // +----+----+----+ + pasteTable( [ + [ { contents: 'aa', colspan: 2, rowspan: 2 }, 'ac' ], + [ 'bc' ], + [ 'ca', 'cb', 'cc' ] + ] ); + // +----+----+----+----+----+ - // | aa | | ad | 04 | - // + + +----+----+ - // | | | bd | 14 | + // | aa | | ac | aa | // +----+----+----+----+----+ <-- heading rows - // | | | cd | 24 | + // | | | bc | | // +----+----+----+----+----+ - // | da | db | dc | dd | 34 | + // | ca | cb | cc | ca | cb | // +----+----+----+----+----+ - // | 40 | 41 | 42 | 43 | 44 | + // | aa | | ac | aa | + // + + +----+ + + // | | | bc | | // +----+----+----+----+----+ - // ^-- heading columns + // ^-- heading columns assertEqualMarkup( getModelData( model, { withoutSelection: true } ), modelTable( [ - [ { contents: 'aa', colspan: 2, rowspan: 2 }, { contents: '', rowspan: 2 }, 'ad', '04' ], - [ 'bd', '14' ], - [ { contents: '', colspan: 2 }, '', 'cd', '24' ], - [ 'da', 'db', 'dc', 'dd', '34' ], - [ '40', '41', '42', '43', '44' ] - ], { headingRows: 2, headingColumns: 2 } ) ); + [ 'aa', '', 'ac', { contents: 'aa', colspan: 2 } ], + [ '', '', 'bc', { contents: '', colspan: 2 } ], + [ 'ca', 'cb', 'cc', 'ca', 'cb' ], + [ { contents: 'aa', rowspan: 2 }, { contents: '', rowspan: 2 }, 'ac', { contents: 'aa', colspan: 2, rowspan: 2 } ], + [ 'bc' ] + ], { headingRows: 1, headingColumns: 1 } ) ); assertSelectionRangesSorted(); /* eslint-disable no-multi-spaces */ assertSelectedCells( model, [ - [ 1, 1, 1, 0 ], - [ 1, 0 ], - [ 1, 1, 1, 0 ], - [ 1, 1, 1, 1, 0 ], - [ 0, 0, 0, 0, 0 ] + [ 1, 1, 1, 1 ], + [ 1, 1, 1, 1 ], + [ 1, 1, 1, 1, 1 ], + [ 1, 1, 1, 1 ], + [ 1 ] ] ); /* eslint-enable no-multi-spaces */ } ); From b4eaa69b1a14ea1090b7128b406a9e018feec391 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Fri, 10 Jul 2020 16:37:48 +0200 Subject: [PATCH 4/4] Used sortRanges() helper from utils. Some code readability tuning. --- packages/ckeditor5-table/src/tableclipboard.js | 15 +++++++++------ packages/ckeditor5-table/src/utils/selection.js | 14 ++++++++++---- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index bedadc68a47..c2611b8a403 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -15,7 +15,7 @@ import { findAncestor } from './utils/common'; import TableUtils from './tableutils'; -import { getColumnIndexes, getRowIndexes, getSelectionAffectedTableCells, isSelectionRectangular } from './utils/selection'; +import { getColumnIndexes, getRowIndexes, getSelectionAffectedTableCells, isSelectionRectangular, sortRanges } from './utils/selection'; import { cropTableToDimensions, getHorizontallyOverlappingCells, @@ -320,23 +320,26 @@ function replaceSelectedCellsWithPasted( pastedTable, pastedDimensions, selected const headingRows = parseInt( selectedTable.getAttribute( 'headingRows' ) || 0 ); const headingColumns = parseInt( selectedTable.getAttribute( 'headingColumns' ) || 0 ); - if ( selection.firstRow < headingRows && headingRows <= selection.lastRow ) { + const areHeadingRowsIntersectingSelection = selection.firstRow < headingRows && headingRows <= selection.lastRow; + const areHeadingColumnsIntersectingSelection = selection.firstColumn < headingColumns && headingColumns <= selection.lastColumn; + + if ( areHeadingRowsIntersectingSelection ) { const columnsLimit = { first: selection.firstColumn, last: selection.lastColumn }; const newCells = doHorizontalSplit( selectedTable, headingRows, columnsLimit, writer, selection.firstRow ); cellsToSelect.push( ...newCells ); } - if ( selection.firstColumn < headingColumns && headingColumns <= selection.lastColumn ) { + if ( areHeadingColumnsIntersectingSelection ) { const rowsLimit = { first: selection.firstRow, last: selection.lastRow }; const newCells = doVerticalSplit( selectedTable, headingColumns, rowsLimit, writer ); cellsToSelect.push( ...newCells ); } - const selectionRanges = cellsToSelect - .map( cell => writer.createRangeOn( cell ) ) - .sort( ( a, b ) => a.start.isBefore( b.start ) ? -1 : 1 ); + // Selection ranges must be sorted because the first and last selection ranges are considered + // as anchor/focus cell ranges for multi-cell selection. + const selectionRanges = sortRanges( cellsToSelect.map( cell => writer.createRangeOn( cell ) ) ); writer.setSelection( selectionRanges ); } diff --git a/packages/ckeditor5-table/src/utils/selection.js b/packages/ckeditor5-table/src/utils/selection.js index fae446ea0f9..b59ac41e656 100644 --- a/packages/ckeditor5-table/src/utils/selection.js +++ b/packages/ckeditor5-table/src/utils/selection.js @@ -188,6 +188,16 @@ export function isSelectionRectangular( selectedTableCells, tableUtils ) { return areaOfValidSelection == areaOfSelectedCells; } +/** + * Returns array of sorted ranges. + * + * @param {Iterable.} ranges + * @return {Array.} + */ +export function sortRanges( ranges ) { + return Array.from( ranges ).sort( compareRangeOrder ); +} + // 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 ); @@ -198,10 +208,6 @@ function getFirstLastIndexesObject( indexes ) { return { first, last }; } -function sortRanges( rangesIterator ) { - return Array.from( rangesIterator ).sort( compareRangeOrder ); -} - function compareRangeOrder( rangeA, rangeB ) { // Since table cell ranges are disjoint, it's enough to check their start positions. const posA = rangeA.start;