From b6b94e4b926ccf2ad01af115b92865d85992ba04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 20 Apr 2020 13:19:47 +0200 Subject: [PATCH 1/9] Add test cases for merging cells from heading columns and other cells. --- tests/commands/mergecellscommand.js | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js index 7200beda..1b86b0f6 100644 --- a/tests/commands/mergecellscommand.js +++ b/tests/commands/mergecellscommand.js @@ -237,6 +237,34 @@ describe( 'MergeCellsCommand', () => { expect( command.isEnabled ).to.be.false; } ); + + it( 'should be false if selection has cells from column headers and other cells - rows in body section', () => { + setData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ], { headingColumns: 1 } ) ); + + tableSelection._setCellSelection( + root.getNodeByPath( [ 0, 0, 0 ] ), + root.getNodeByPath( [ 0, 0, 1 ] ) + ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be true if selection has cells from column headers and other cells - rows in header section', () => { + setData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ], { headingColumns: 1, headingRows: 1 } ) ); + + tableSelection._setCellSelection( + root.getNodeByPath( [ 0, 0, 0 ] ), + root.getNodeByPath( [ 0, 0, 1 ] ) + ); + + expect( command.isEnabled ).to.be.true; + } ); } ); describe( 'execute()', () => { From d5f2e2be38c45fc681f0741f622814c4f036ca74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 20 Apr 2020 13:29:10 +0200 Subject: [PATCH 2/9] Make merge table cells command check also cells from heading columns for mergeability. --- src/commands/mergecellscommand.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 37e1d09a..658f7e42 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -11,7 +11,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; import TableWalker from '../tablewalker'; import { findAncestor, updateNumericAttribute } from './utils'; import TableUtils from '../tableutils'; -import { getRowIndexes, getSelectedTableCells } from '../utils'; +import { getColumnIndexes, getRowIndexes, getSelectedTableCells } from '../utils'; /** * The merge cells command. @@ -208,7 +208,17 @@ function areCellInTheSameTableSection( tableCells ) { const firstCellIsInBody = rowIndexes.first > headingRows - 1; const lastCellIsInBody = rowIndexes.last > headingRows - 1; - return firstCellIsInBody === lastCellIsInBody; + const cellsInSameHeadingRows = firstCellIsInBody === lastCellIsInBody; + + const headingColumns = parseInt( table.getAttribute( 'headingColumns' ) || 0 ); + const columnIndexes = getColumnIndexes( tableCells ); + + const firstCellIsInColumnBody = columnIndexes.first > headingColumns - 1; + const lastCellIsInColumnBody = columnIndexes.last > headingColumns - 1; + + const cellsInSameHeadingColumns = firstCellIsInColumnBody && lastCellIsInColumnBody; + + return cellsInSameHeadingRows && ( !firstCellIsInBody || cellsInSameHeadingColumns ); } function getMergeDimensions( firstTableCell, selectedTableCells, tableUtils ) { From d08cc8b91d90ee35701b62a912c7c5e45d18c43f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 20 Apr 2020 14:19:57 +0200 Subject: [PATCH 3/9] Add more cases to MergeCellsCommand#isEnabled checks. --- tests/commands/mergecellscommand.js | 44 +++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js index 1b86b0f6..2a157358 100644 --- a/tests/commands/mergecellscommand.js +++ b/tests/commands/mergecellscommand.js @@ -238,25 +238,39 @@ describe( 'MergeCellsCommand', () => { expect( command.isEnabled ).to.be.false; } ); + it( 'should be true if selection has cells only from column headers - rows in body section', () => { + setData( model, modelTable( [ + [ '00[]', '01', '02', '03' ], + [ '10', '11', '12', '13' ] + ], { headingColumns: 2 } ) ); + + tableSelection._setCellSelection( + root.getNodeByPath( [ 0, 0, 0 ] ), + root.getNodeByPath( [ 0, 1, 1 ] ) + ); + + expect( command.isEnabled ).to.be.true; + } ); + it( 'should be false if selection has cells from column headers and other cells - rows in body section', () => { setData( model, modelTable( [ - [ '00[]', '01' ], - [ '10', '11' ] - ], { headingColumns: 1 } ) ); + [ '00[]', '01', '02', '03' ], + [ '10', '11', '12', '13' ] + ], { headingColumns: 2 } ) ); tableSelection._setCellSelection( root.getNodeByPath( [ 0, 0, 0 ] ), - root.getNodeByPath( [ 0, 0, 1 ] ) + root.getNodeByPath( [ 0, 1, 2 ] ) ); expect( command.isEnabled ).to.be.false; } ); - it( 'should be true if selection has cells from column headers and other cells - rows in header section', () => { + it( 'should be true if selection has cells only from column headers - rows in header section', () => { setData( model, modelTable( [ - [ '00[]', '01' ], - [ '10', '11' ] - ], { headingColumns: 1, headingRows: 1 } ) ); + [ '00[]', '01', '02', '03' ], + [ '10', '11', '12', '13' ] + ], { headingColumns: 2, headingRows: 1 } ) ); tableSelection._setCellSelection( root.getNodeByPath( [ 0, 0, 0 ] ), @@ -265,6 +279,20 @@ describe( 'MergeCellsCommand', () => { expect( command.isEnabled ).to.be.true; } ); + + it( 'should be false if selection has cells only from column headers and other cells - rows in header section', () => { + setData( model, modelTable( [ + [ '00[]', '01', '02', '03' ], + [ '10', '11', '12', '13' ] + ], { headingColumns: 2, headingRows: 1 } ) ); + + tableSelection._setCellSelection( + root.getNodeByPath( [ 0, 0, 0 ] ), + root.getNodeByPath( [ 0, 0, 2 ] ) + ); + + expect( command.isEnabled ).to.be.false; + } ); } ); describe( 'execute()', () => { From de3ab8ed9481b5d25045e3a26503eb99d6a7a681 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 20 Apr 2020 14:20:22 +0200 Subject: [PATCH 4/9] Refactor MergeCellsCommand#isEnabled checks. --- src/commands/mergecellscommand.js | 35 ++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 658f7e42..42f689df 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -199,26 +199,45 @@ function getBiggestRectangleArea( rows, columns ) { return ( lastRow - firstRow + 1 ) * ( lastColumn - firstColumn + 1 ); } +// Checks if selection does not mix header (column or row) with non-header cells. +// +// In the below table a valid selection is such that consist cells with the same letter. +// So, a-a (same heading row and column) or d-d (body cells) are valid while c-d or a-b are not. +// +// header columns +// ↓ ↓ +// ┌───┬───┬───┬───┐ +// │ a │ a │ b │ b │ ← header row +// ├───┼───┼───┼───│ +// │ c │ c │ d │ d │ +// ├───┼───┼───┼───│ +// │ c │ c │ d │ d │ +// └───┴───┴───┴───┘ +// function areCellInTheSameTableSection( tableCells ) { const table = findAncestor( 'table', tableCells[ 0 ] ); const rowIndexes = getRowIndexes( tableCells ); const headingRows = parseInt( table.getAttribute( 'headingRows' ) || 0 ); - const firstCellIsInBody = rowIndexes.first > headingRows - 1; - const lastCellIsInBody = rowIndexes.last > headingRows - 1; - - const cellsInSameHeadingRows = firstCellIsInBody === lastCellIsInBody; + // Calculating row indexes is a bit cheaper so if this check fails we can't merge. + if ( !areIndexesInSameSection( rowIndexes, headingRows ) ) { + return false; + } const headingColumns = parseInt( table.getAttribute( 'headingColumns' ) || 0 ); const columnIndexes = getColumnIndexes( tableCells ); - const firstCellIsInColumnBody = columnIndexes.first > headingColumns - 1; - const lastCellIsInColumnBody = columnIndexes.last > headingColumns - 1; + // Similarly cells must be in same column section. + return areIndexesInSameSection( columnIndexes, headingColumns ); +} - const cellsInSameHeadingColumns = firstCellIsInColumnBody && lastCellIsInColumnBody; +// Unified check if table rows/columns indexes are in the same heading/body section. +function areIndexesInSameSection( { first, last }, headingSectionSize ) { + const firstCellIsInHeading = first < headingSectionSize; + const lastCellIsInHeading = last < headingSectionSize; - return cellsInSameHeadingRows && ( !firstCellIsInBody || cellsInSameHeadingColumns ); + return firstCellIsInHeading === lastCellIsInHeading; } function getMergeDimensions( firstTableCell, selectedTableCells, tableUtils ) { From 23c2895e80a704116f8b7e07fcb0425432a4f720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 20 Apr 2020 14:27:04 +0200 Subject: [PATCH 5/9] Update docs in areCellInTheSameTableSection(). --- src/commands/mergecellscommand.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 42f689df..41846b43 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -199,7 +199,7 @@ function getBiggestRectangleArea( rows, columns ) { return ( lastRow - firstRow + 1 ) * ( lastColumn - firstColumn + 1 ); } -// Checks if selection does not mix header (column or row) with non-header cells. +// Checks if selection does not mix header (column or row) with other cells. // // In the below table a valid selection is such that consist cells with the same letter. // So, a-a (same heading row and column) or d-d (body cells) are valid while c-d or a-b are not. From 161498841903b87b382ba8c02838329a589e5299 Mon Sep 17 00:00:00 2001 From: Maciej Date: Wed, 22 Apr 2020 07:20:07 +0200 Subject: [PATCH 6/9] Fix table sample in internal docs. Co-Authored-By: Kuba Niegowski <1232187+niegowski@users.noreply.github.com> --- src/commands/mergecellscommand.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 41846b43..f17ece2c 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -208,9 +208,9 @@ function getBiggestRectangleArea( rows, columns ) { // ↓ ↓ // ┌───┬───┬───┬───┐ // │ a │ a │ b │ b │ ← header row -// ├───┼───┼───┼───│ +// ├───┼───┼───┼───┤ // │ c │ c │ d │ d │ -// ├───┼───┼───┼───│ +// ├───┼───┼───┼───┤ // │ c │ c │ d │ d │ // └───┴───┴───┴───┘ // From 6c3ad1d971121ef2acc3532a1c1758ea73ceb677 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 22 Apr 2020 07:26:54 +0200 Subject: [PATCH 7/9] Add a test case for selecting all types of cells. --- tests/commands/mergecellscommand.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js index 2a157358..137d3913 100644 --- a/tests/commands/mergecellscommand.js +++ b/tests/commands/mergecellscommand.js @@ -293,6 +293,20 @@ describe( 'MergeCellsCommand', () => { expect( command.isEnabled ).to.be.false; } ); + + it( 'should be false if selection has cells from column headers, row headers and body sections', () => { + setData( model, modelTable( [ + [ '00[]', '01', '02', '03' ], + [ '10', '11', '12', '13' ] + ], { headingColumns: 2, headingRows: 1 } ) ); + + tableSelection._setCellSelection( + root.getNodeByPath( [ 0, 0, 0 ] ), + root.getNodeByPath( [ 0, 1, 2 ] ) + ); + + expect( command.isEnabled ).to.be.false; + } ); } ); describe( 'execute()', () => { From 4891b52c4fc5a00bef019482ac44468cddcecbba Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 22 Apr 2020 17:00:34 +0200 Subject: [PATCH 8/9] Update src/commands/mergecellscommand.js --- src/commands/mergecellscommand.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index f17ece2c..998af86e 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -199,7 +199,7 @@ function getBiggestRectangleArea( rows, columns ) { return ( lastRow - firstRow + 1 ) * ( lastColumn - firstColumn + 1 ); } -// Checks if selection does not mix header (column or row) with other cells. +// Checks if the selection does not mix header (column or row) with other cells. // // In the below table a valid selection is such that consist cells with the same letter. // So, a-a (same heading row and column) or d-d (body cells) are valid while c-d or a-b are not. From fa958a1a858a1ccaf801406a6923aea8be43745e Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 22 Apr 2020 17:00:42 +0200 Subject: [PATCH 9/9] Update src/commands/mergecellscommand.js --- src/commands/mergecellscommand.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 998af86e..ea0a6f95 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -201,7 +201,7 @@ function getBiggestRectangleArea( rows, columns ) { // Checks if the selection does not mix header (column or row) with other cells. // -// In the below table a valid selection is such that consist cells with the same letter. +// For instance, in the table below valid selections consist of cells with the same letter only. // So, a-a (same heading row and column) or d-d (body cells) are valid while c-d or a-b are not. // // header columns