From 07aaab870e4a43d6d1f0a149de289a36d03ab054 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 29 Jan 2020 12:49:18 +0100 Subject: [PATCH 01/13] Re-introduce MergeCellsCommand. --- src/commands/mergecellscommand.js | 260 +++++++++++++++++ src/tableediting.js | 3 + src/tableui.js | 25 +- tests/commands/mergecellscommand.js | 420 ++++++++++++++++++++++++++++ 4 files changed, 685 insertions(+), 23 deletions(-) create mode 100644 src/commands/mergecellscommand.js create mode 100644 tests/commands/mergecellscommand.js diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js new file mode 100644 index 00000000..6bba1863 --- /dev/null +++ b/src/commands/mergecellscommand.js @@ -0,0 +1,260 @@ +/** + * @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +/** + * @module table/commands/mergecellscommand + */ + +import Command from '@ckeditor/ckeditor5-core/src/command'; +import TableWalker from '../tablewalker'; +import { findAncestor, updateNumericAttribute } from './utils'; +import TableUtils from '../tableutils'; + +/** + * The merge cells command. + * + * The command is registered by {@link module:table/tableediting~TableEditing} as `'mergeTableCellRight'`, `'mergeTableCellLeft'`, + * `'mergeTableCellUp'` and `'mergeTableCellDown'` editor commands. + * + * To merge a table cell at the current selection with another cell, execute the command corresponding with the preferred direction. + * + * For example, to merge with a cell to the right: + * + * editor.execute( 'mergeTableCellRight' ); + * + * **Note**: If a table cell has a different [`rowspan`](https://www.w3.org/TR/html50/tabular-data.html#attr-tdth-rowspan) + * (for `'mergeTableCellRight'` and `'mergeTableCellLeft'`) or [`colspan`](https://www.w3.org/TR/html50/tabular-data.html#attr-tdth-colspan) + * (for `'mergeTableCellUp'` and `'mergeTableCellDown'`), the command will be disabled. + * + * @extends module:core/command~Command + */ +export default class MergeCellsCommand extends Command { + /** + * @inheritDoc + */ + refresh() { + this.isEnabled = canMergeCells( this.editor.model.document.selection, this.editor.plugins.get( TableUtils ) ); + } + + /** + * Executes the command. + * + * Depending on the command's {@link #direction} value, it will merge the cell that is to the `'left'`, `'right'`, `'up'` or `'down'`. + * + * @fires execute + */ + execute() { + const model = this.editor.model; + + const tableUtils = this.editor.plugins.get( TableUtils ); + + model.change( writer => { + const selectedTableCells = [ ... this.editor.model.document.selection.getRanges() ].map( range => range.start.nodeAfter ); + + const firstTableCell = selectedTableCells.shift(); + + // TODO: this shouldn't be necessary (right now the selection could overlap existing. + writer.setSelection( firstTableCell, 'in' ); + + const { row, column } = tableUtils.getCellLocation( firstTableCell ); + + const colspan = parseInt( firstTableCell.getAttribute( 'colspan' ) || 1 ); + const rowspan = parseInt( firstTableCell.getAttribute( 'rowspan' ) || 1 ); + + let rightMax = column + colspan; + let bottomMax = row + rowspan; + + const rowsToCheck = new Set(); + + for ( const tableCell of selectedTableCells ) { + const { row, column } = tableUtils.getCellLocation( tableCell ); + + const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 ); + const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) || 1 ); + + if ( column + colspan > rightMax ) { + rightMax = column + colspan; + } + + if ( row + rowspan > bottomMax ) { + bottomMax = row + rowspan; + } + } + + for ( const tableCell of selectedTableCells ) { + rowsToCheck.add( tableCell.parent ); + mergeTableCells( tableCell, firstTableCell, writer ); + } + + // Update table cell span attribute and merge set selection on merged contents. + updateNumericAttribute( 'colspan', rightMax - column, firstTableCell, writer ); + updateNumericAttribute( 'rowspan', bottomMax - row, firstTableCell, writer ); + + writer.setSelection( firstTableCell, 'in' ); + + // Remove empty rows after merging table cells. + for ( const row of rowsToCheck ) { + if ( !row.childCount ) { + removeEmptyRow( row, writer ); + } + } + } ); + } +} + +// Properly removes empty row from a table. Will update `rowspan` attribute of cells that overlaps removed row. +// +// @param {module:engine/model/element~Element} removedTableCellRow +// @param {module:engine/model/writer~Writer} writer +function removeEmptyRow( removedTableCellRow, writer ) { + const table = removedTableCellRow.parent; + + const removedRowIndex = table.getChildIndex( removedTableCellRow ); + + for ( const { cell, row, rowspan } of new TableWalker( table, { endRow: removedRowIndex } ) ) { + const overlapsRemovedRow = row + rowspan - 1 >= removedRowIndex; + + if ( overlapsRemovedRow ) { + updateNumericAttribute( 'rowspan', rowspan - 1, cell, writer ); + } + } + + writer.remove( removedTableCellRow ); +} + +// Merges two table cells - will ensure that after merging cells with empty paragraph the result table cell will only have one paragraph. +// If one of the merged table cell is empty the merged table cell will have contents of the non-empty table cell. +// If both are empty the merged table cell will have only one empty paragraph. +// +// @param {module:engine/model/element~Element} cellToRemove +// @param {module:engine/model/element~Element} cellToExpand +// @param {module:engine/model/writer~Writer} writer +function mergeTableCells( cellToRemove, cellToExpand, writer ) { + if ( !isEmpty( cellToRemove ) ) { + if ( isEmpty( cellToExpand ) ) { + writer.remove( writer.createRangeIn( cellToExpand ) ); + } + + writer.move( writer.createRangeIn( cellToRemove ), writer.createPositionAt( cellToExpand, 'end' ) ); + } + + // Remove merged table cell. + writer.remove( cellToRemove ); +} + +// Checks if passed table cell contains empty paragraph. +// +// @param {module:engine/model/element~Element} tableCell +// @returns {Boolean} +function isEmpty( tableCell ) { + return tableCell.childCount == 1 && tableCell.getChild( 0 ).is( 'paragraph' ) && tableCell.getChild( 0 ).isEmpty; +} + +// Check if selection contains mergeable cells. +// +// In a table below: +// +// +---+---+---+---+ +// | a | b | c | d | +// +---+---+---+ + +// | e | f | | +// + +---+---+ +// | | g | h | +// +---+---+---+---+ +// +// Valid selections are those which creates a solid rectangle (without gaps), such as: +// - a, b (two horizontal cells) +// - c, f (two vertical cells) +// - a, b, e (cell "e" spans over four cells) +// - c, d, f (cell d spans over cell in row below) +// +// While invalid selection would be: +// - a, c (cell "b" not selected creates a gap) +// - f, g, h (cell "d" spans over a cell from row of "f" cell - thus creates a gap) +// +// @param {module:engine/model/selection~Selection} selection +// @param {module:table/tableUtils~TableUtils} tableUtils +// @returns {boolean} +function canMergeCells( selection, tableUtils ) { + // Collapsed selection or selection only one range can't contain mergeable table cells. + if ( selection.isCollapsed || selection.rangeCount < 2 ) { + return false; + } + + // All cells must be inside the same table. + let firstRangeTable; + + const tableCells = []; + + for ( const range of selection.getRanges() ) { + // Selection ranges must be set on whole element. + if ( range.isCollapsed || !range.isFlat || !range.start.nodeAfter.is( 'tableCell' ) ) { + return false; + } + + const parentTable = findAncestor( 'table', range.start ); + + if ( !firstRangeTable ) { + firstRangeTable = parentTable; + } else if ( firstRangeTable !== parentTable ) { + return false; + } + + tableCells.push( range.start.nodeAfter ); + } + + // At this point selection contains ranges over table cells in the same table. + // The valid selection is a fully occupied rectangle composed of table cells. + // Below we calculate area of selected cells and the area of valid selection. + // The area of valid selection is defined by top-left and bottom-right cells. + const rows = new Set(); + const columns = new Set(); + + let areaOfSelectedCells = 0; + + for ( const tableCell of tableCells ) { + const { row, column } = tableUtils.getCellLocation( tableCell ); + const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) || 1 ); + const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 ); + + // Record row & column indexes of current cell. + rows.add( row ); + columns.add( column ); + + // For cells that spans over multiple rows add also the last row that this cell spans over. + if ( rowspan > 1 ) { + rows.add( row + rowspan - 1 ); + } + + // For cells that spans over multiple columns add also the last column that this cell spans over. + if ( colspan > 1 ) { + columns.add( column + colspan - 1 ); + } + + areaOfSelectedCells += ( rowspan * colspan ); + } + + // We can only merge table cells that are in adjacent rows... + const areaOfValidSelection = getBiggestRectangleArea( rows, columns ); + + return areaOfValidSelection == areaOfSelectedCells; +} + +// Calculates the area of a maximum rectangle that can span over provided row & column indexes. +// +// @param {Array.} rows +// @param {Array.} columns +// @returns {Number} +function getBiggestRectangleArea( rows, columns ) { + const rowsIndexes = Array.from( rows.values() ); + const columnIndexes = Array.from( columns.values() ); + + const lastRow = Math.max( ...rowsIndexes ); + const firstRow = Math.min( ...rowsIndexes ); + const lastColumn = Math.max( ...columnIndexes ); + const firstColumn = Math.min( ...columnIndexes ); + + return ( lastRow - firstRow + 1 ) * ( lastColumn - firstColumn + 1 ); +} diff --git a/src/tableediting.js b/src/tableediting.js index 2a716646..1e9aaba4 100644 --- a/src/tableediting.js +++ b/src/tableediting.js @@ -28,6 +28,7 @@ import RemoveRowCommand from './commands/removerowcommand'; import RemoveColumnCommand from './commands/removecolumncommand'; import SetHeaderRowCommand from './commands/setheaderrowcommand'; import SetHeaderColumnCommand from './commands/setheadercolumncommand'; +import MergeCellsCommand from './commands/mergecellscommand'; import { findAncestor } from './commands/utils'; import TableUtils from '../src/tableutils'; @@ -131,6 +132,8 @@ export default class TableEditing extends Plugin { editor.commands.add( 'splitTableCellVertically', new SplitCellCommand( editor, { direction: 'vertically' } ) ); editor.commands.add( 'splitTableCellHorizontally', new SplitCellCommand( editor, { direction: 'horizontally' } ) ); + editor.commands.add( 'mergeTableCells', new MergeCellsCommand( editor ) ); + editor.commands.add( 'mergeTableCellRight', new MergeCellCommand( editor, { direction: 'right' } ) ); editor.commands.add( 'mergeTableCellLeft', new MergeCellCommand( editor, { direction: 'left' } ) ); editor.commands.add( 'mergeTableCellDown', new MergeCellCommand( editor, { direction: 'down' } ) ); diff --git a/src/tableui.js b/src/tableui.js index 34646f1a..8b5a1873 100644 --- a/src/tableui.js +++ b/src/tableui.js @@ -153,29 +153,8 @@ export default class TableUI extends Plugin { { type: 'button', model: { - commandName: 'mergeTableCellUp', - label: t( 'Merge cell up' ) - } - }, - { - type: 'button', - model: { - commandName: isContentLtr ? 'mergeTableCellRight' : 'mergeTableCellLeft', - label: t( 'Merge cell right' ) - } - }, - { - type: 'button', - model: { - commandName: 'mergeTableCellDown', - label: t( 'Merge cell down' ) - } - }, - { - type: 'button', - model: { - commandName: isContentLtr ? 'mergeTableCellLeft' : 'mergeTableCellRight', - label: t( 'Merge cell left' ) + commandName: 'mergeTableCells', + label: t( 'Merge cells' ) } }, { type: 'separator' }, diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js new file mode 100644 index 00000000..bba07847 --- /dev/null +++ b/tests/commands/mergecellscommand.js @@ -0,0 +1,420 @@ +/** + * @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor'; +import { getData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; + +import MergeCellsCommand from '../../src/commands/mergecellscommand'; +import { defaultConversion, defaultSchema, modelTable } from '../_utils/utils'; +import TableUtils from '../../src/tableutils'; +import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; + +describe( 'MergeCellsCommand', () => { + let editor, model, command, root; + + beforeEach( () => { + return ModelTestEditor + .create( { + plugins: [ TableUtils ] + } ) + .then( newEditor => { + editor = newEditor; + model = editor.model; + root = model.document.getRoot( 'main' ); + + command = new MergeCellsCommand( editor ); + + defaultSchema( model.schema ); + defaultConversion( editor.conversion ); + } ); + } ); + + afterEach( () => { + return editor.destroy(); + } ); + + describe( 'isEnabled', () => { + it( 'should be false if collapsed selection in table cell', () => { + setData( model, modelTable( [ + [ '00[]', '01' ] + ] ) ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be false if only one table cell is selected', () => { + setData( model, modelTable( [ + [ '00', '01' ] + ] ) ); + + selectNodes( [ [ 0, 0, 0 ] ] ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be true if at least two adjacent table cells are selected', () => { + setData( model, modelTable( [ + [ '00', '01' ] + ] ) ); + + selectNodes( [ [ 0, 0, 0 ], [ 0, 0, 1 ] ] ); + + expect( command.isEnabled ).to.be.true; + } ); + + it( 'should be true if many table cells are selected', () => { + setData( model, modelTable( [ + [ '00', '01', '02', '03' ], + [ '10', '11', '12', '13' ], + [ '20', '21', '22', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 1 ], [ 0, 0, 2 ], + [ 0, 1, 1 ], [ 0, 1, 2 ], + [ 0, 2, 1 ], [ 0, 2, 2 ], + [ 0, 3, 1 ], [ 0, 3, 2 ] + ] ); + + expect( command.isEnabled ).to.be.true; + } ); + + it( 'should be false if at least one table cell is not selected from an area', () => { + setData( model, modelTable( [ + [ '00', '01', '02', '03' ], + [ '10', '11', '12', '13' ], + [ '20', '21', '22', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 1 ], [ 0, 0, 2 ], + [ 0, 1, 2 ], // one table cell not selected from this row + [ 0, 2, 1 ], [ 0, 2, 2 ], + [ 0, 3, 1 ], [ 0, 3, 2 ] + ] ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be false if table cells are not in adjacent rows', () => { + setData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ] + ] ) ); + + selectNodes( [ + [ 0, 1, 0 ], + [ 0, 0, 1 ] + ] ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be false if table cells are not in adjacent columns', () => { + setData( model, modelTable( [ + [ '00', '01', '02' ] + ] ) ); + + selectNodes( [ [ 0, 0, 0 ], [ 0, 0, 2 ] ] ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be false if any range is collapsed in selection', () => { + setData( model, modelTable( [ + [ '00', '01', '02' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 0, 0, 0 ], // The "00" text node + [ 0, 0, 1 ] + ] ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be false if any ranges are on different tables', () => { + setData( model, + modelTable( [ [ '00', '01' ] ] ) + + modelTable( [ [ 'aa', 'ab' ] ] ) + ); + + selectNodes( [ + [ 0, 0, 0 ], // first table + [ 1, 0, 1 ] // second table + ] ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be false if any table cell with colspan attribute extends over selection area', () => { + setData( model, modelTable( [ + [ '00', { colspan: 2, contents: '01' } ], + [ '10', '11', '12' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 0 ], [ 0, 0, 1 ], + [ 0, 1, 0 ], [ 0, 1, 1 ] + ] ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be true if none table cell with colspan attribute extends over selection area', () => { + setData( model, modelTable( [ + [ '00', { colspan: 2, contents: '01' } ], + [ '10', '11', '12' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 0 ], [ 0, 0, 1 ], + [ 0, 1, 0 ], [ 0, 1, 1 ], + [ 0, 1, 2 ] + ] ); + + expect( command.isEnabled ).to.be.true; + } ); + + it( 'should be true if first table cell is inside selection area', () => { + setData( model, modelTable( [ + [ { colspan: 2, rowspan: 2, contents: '00' }, '02', '03' ], + [ '12', '13' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 0 ], [ 0, 0, 1 ], + [ 0, 1, 0 ] + ] ); + + expect( command.isEnabled ).to.be.true; + } ); + + it( 'should be false if any table cell with rowspan attribute extends over selection area', () => { + setData( model, modelTable( [ + [ '00', { rowspan: 2, contents: '01' } ], + [ '10' ] + ] ) ); + + selectNodes( [ [ 0, 0, 0 ], [ 0, 0, 1 ] ] ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be true if none table cell with rowspan attribute extends over selection area', () => { + setData( model, modelTable( [ + [ '00', { rowspan: 2, contents: '01' } ], + [ '10' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 0 ], [ 0, 0, 1 ], + [ 0, 1, 0 ] + ] ); + + expect( command.isEnabled ).to.be.true; + } ); + + it( 'should be false if not in a cell', () => { + setData( model, '11[]' ); + + expect( command.isEnabled ).to.be.false; + } ); + } ); + + describe( 'execute()', () => { + it( 'should merge table cells', () => { + setData( model, modelTable( [ + [ '[]00', '01' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 0 ], [ 0, 0, 1 ] + ] ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ { colspan: 2, contents: '[0001]' } ] + ] ) ); + } ); + + it( 'should merge table cells - extend colspan attribute', () => { + setData( model, modelTable( [ + [ { colspan: 2, contents: '00' }, '02', '03' ], + [ '10', '11', '12', '13' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 0 ], [ 0, 0, 1 ], + [ 0, 1, 0 ], [ 0, 1, 1 ], [ 0, 1, 2 ] + ] ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ { + colspan: 3, + rowspan: 2, + contents: '[00' + + '02' + + '10' + + '11' + + '12]' + }, '03' ], + [ '13' ] + ] ) ); + } ); + + it( 'should merge to a single paragraph - every cell is empty', () => { + setData( model, modelTable( [ + [ '[]', '' ] + ] ) ); + + selectNodes( [ [ 0, 0, 0 ], [ 0, 0, 1 ] ] ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ { colspan: 2, contents: '[]' } ] + ] ) ); + } ); + + it( 'should merge to a single paragraph - merged cell is empty', () => { + setData( model, modelTable( [ + [ 'foo', '' ] + ] ) ); + + selectNodes( [ [ 0, 0, 0 ], [ 0, 0, 1 ] ] ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ { colspan: 2, contents: '[foo]' } ] + ] ) ); + } ); + + it( 'should merge to a single paragraph - cell to which others are merged is empty', () => { + setData( model, modelTable( [ + [ '', 'foo' ] + ] ) ); + + selectNodes( [ [ 0, 0, 0 ], [ 0, 0, 1 ] ] ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ { colspan: 2, contents: '[foo]' } ] + ] ) ); + } ); + + it( 'should not merge empty blocks other then to a single block', () => { + model.schema.register( 'block', { + allowWhere: '$block', + allowContentOf: '$block', + isBlock: true + } ); + + setData( model, modelTable( [ + [ '[]', '' ] + ] ) ); + + selectNodes( [ [ 0, 0, 0 ], [ 0, 0, 1 ] ] ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ { colspan: 2, contents: '[]' } ] + ] ) ); + } ); + + describe( 'removing empty row', () => { + it( 'should remove empty row if merging all table cells from that row', () => { + setData( model, modelTable( [ + [ '00' ], + [ '10' ], + [ '20' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 0 ], + [ 0, 1, 0 ], + [ 0, 2, 0 ] + ] ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ + '[001020]' + ] + ] ) ); + } ); + + it( 'should decrease rowspan if cell overlaps removed row', () => { + setData( model, modelTable( [ + [ '00', { rowspan: 2, contents: '01' }, { rowspan: 3, contents: '02' } ], + [ '10' ], + [ '20', '21' ] + ] ) ); + + selectNodes( [ + [ 0, 0, 0 ], + [ 0, 1, 0 ], + [ 0, 2, 0 ] + ] ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ + { rowspan: 2, contents: '[001020]' }, + '01', + { rowspan: 2, contents: '02' } + ], + [ '21' ] + ] ) ); + } ); + + it( 'should not decrease rowspan if cell from previous row does not overlaps removed row', () => { + setData( model, modelTable( [ + [ '00', { rowspan: 2, contents: '01' } ], + [ '10' ], + [ '20', '21' ], + [ '30', '31' ] + ] ) ); + + selectNodes( [ + [ 0, 2, 0 ], [ 0, 2, 1 ], + [ 0, 3, 0 ], [ 0, 3, 1 ] + ] ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ '00', { rowspan: 2, contents: '01' } ], + [ '10' ], + [ + { + colspan: 2, + contents: '[2021' + + '3031]' + } + ] + ] ) ); + } ); + } ); + } ); + + function selectNodes( paths ) { + model.change( writer => { + const ranges = paths.map( path => writer.createRangeOn( root.getNodeByPath( path ) ) ); + + writer.setSelection( ranges ); + } ); + } +} ); From a6a237a6c7e974df8e56f0fa848cf3f77590ff7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 26 Mar 2020 15:47:36 +0100 Subject: [PATCH 02/13] Temporally enable all merge cell buttons. --- src/tableui.js | 29 +++++++++++++++++++++++++++++ tests/tableui.js | 43 ++++++++++++++++++++++++++++++------------- 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/src/tableui.js b/src/tableui.js index 2fc9638b..0ab19ee6 100644 --- a/src/tableui.js +++ b/src/tableui.js @@ -158,6 +158,35 @@ export default class TableUI extends Plugin { editor.ui.componentFactory.add( 'mergeTableCells', locale => { const options = [ + { + type: 'button', + model: { + commandName: 'mergeTableCellUp', + label: t( 'Merge cell up' ) + } + }, + { + type: 'button', + model: { + commandName: isContentLtr ? 'mergeTableCellRight' : 'mergeTableCellLeft', + label: t( 'Merge cell right' ) + } + }, + { + type: 'button', + model: { + commandName: 'mergeTableCellDown', + label: t( 'Merge cell down' ) + } + }, + { + type: 'button', + model: { + commandName: isContentLtr ? 'mergeTableCellLeft' : 'mergeTableCellRight', + label: t( 'Merge cell left' ) + } + }, + { type: 'separator' }, { type: 'button', model: { diff --git a/tests/tableui.js b/tests/tableui.js index b02a05b2..c554e83d 100644 --- a/tests/tableui.js +++ b/tests/tableui.js @@ -372,6 +372,8 @@ describe( 'TableUI', () => { 'Merge cell down', 'Merge cell left', '|', + 'Merge cells', + '|', 'Split cell vertically', 'Split cell horizontally' ] ); @@ -384,6 +386,7 @@ describe( 'TableUI', () => { const mergeCellRightCommand = editor.commands.get( 'mergeTableCellRight' ); const mergeCellDownCommand = editor.commands.get( 'mergeTableCellDown' ); const mergeCellLeftCommand = editor.commands.get( 'mergeTableCellLeft' ); + const mergeCellsCommand = editor.commands.get( 'mergeTableCells' ); const splitCellVerticallyCommand = editor.commands.get( 'splitTableCellVertically' ); const splitCellHorizontallyCommand = editor.commands.get( 'splitTableCellHorizontally' ); @@ -391,38 +394,52 @@ describe( 'TableUI', () => { mergeCellRightCommand.isEnabled = true; mergeCellDownCommand.isEnabled = true; mergeCellLeftCommand.isEnabled = true; + mergeCellsCommand.isEnabled = true; splitCellVerticallyCommand.isEnabled = true; splitCellHorizontallyCommand.isEnabled = true; - expect( items.first.children.first.isEnabled ).to.be.true; - expect( items.get( 1 ).children.first.isEnabled ).to.be.true; - expect( items.get( 2 ).children.first.isEnabled ).to.be.true; - expect( items.get( 3 ).children.first.isEnabled ).to.be.true; - expect( items.get( 5 ).children.first.isEnabled ).to.be.true; - expect( items.get( 6 ).children.first.isEnabled ).to.be.true; + const mergeCellUpButton = items.first; + const mergeCellRightButton = items.get( 1 ); + const mergeCellDownButton = items.get( 2 ); + const mergeCellLeftButton = items.get( 3 ); + // separator + const mergeCellsButton = items.get( 5 ); + // separator + const splitVerticallyButton = items.get( 7 ); + const splitHorizontallyButton = items.get( 8 ); + + expect( mergeCellUpButton.children.first.isEnabled ).to.be.true; + expect( mergeCellRightButton.children.first.isEnabled ).to.be.true; + expect( mergeCellDownButton.children.first.isEnabled ).to.be.true; + expect( mergeCellLeftButton.children.first.isEnabled ).to.be.true; + expect( mergeCellsButton.children.first.isEnabled ).to.be.true; + expect( splitVerticallyButton.children.first.isEnabled ).to.be.true; + expect( splitHorizontallyButton.children.first.isEnabled ).to.be.true; expect( dropdown.buttonView.isEnabled ).to.be.true; mergeCellUpCommand.isEnabled = false; - - expect( items.first.children.first.isEnabled ).to.be.false; + expect( mergeCellUpButton.children.first.isEnabled ).to.be.false; expect( dropdown.buttonView.isEnabled ).to.be.true; mergeCellRightCommand.isEnabled = false; - expect( items.get( 1 ).children.first.isEnabled ).to.be.false; + expect( mergeCellRightButton.children.first.isEnabled ).to.be.false; expect( dropdown.buttonView.isEnabled ).to.be.true; mergeCellDownCommand.isEnabled = false; - expect( items.get( 2 ).children.first.isEnabled ).to.be.false; + expect( mergeCellDownButton.children.first.isEnabled ).to.be.false; mergeCellLeftCommand.isEnabled = false; - expect( items.get( 3 ).children.first.isEnabled ).to.be.false; + expect( mergeCellLeftButton.children.first.isEnabled ).to.be.false; + + mergeCellsCommand.isEnabled = false; + expect( mergeCellsButton.children.first.isEnabled ).to.be.false; splitCellVerticallyCommand.isEnabled = false; - expect( items.get( 5 ).children.first.isEnabled ).to.be.false; + expect( splitVerticallyButton.children.first.isEnabled ).to.be.false; splitCellHorizontallyCommand.isEnabled = false; - expect( items.get( 6 ).children.first.isEnabled ).to.be.false; + expect( splitHorizontallyButton.children.first.isEnabled ).to.be.false; expect( dropdown.buttonView.isEnabled ).to.be.false; } ); From e2783aac12a23c060a25bfb0ffdc9363870e76a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 27 Mar 2020 13:00:51 +0100 Subject: [PATCH 03/13] Use existing util methods for obtaining selected table cells. --- src/commands/mergecellscommand.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 6bba1863..f43e2346 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -11,6 +11,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; import TableWalker from '../tablewalker'; import { findAncestor, updateNumericAttribute } from './utils'; import TableUtils from '../tableutils'; +import { getSelectionAffectedTableCells } from '../utils'; /** * The merge cells command. @@ -186,8 +187,6 @@ function canMergeCells( selection, tableUtils ) { // All cells must be inside the same table. let firstRangeTable; - const tableCells = []; - for ( const range of selection.getRanges() ) { // Selection ranges must be set on whole element. if ( range.isCollapsed || !range.isFlat || !range.start.nodeAfter.is( 'tableCell' ) ) { @@ -201,10 +200,10 @@ function canMergeCells( selection, tableUtils ) { } else if ( firstRangeTable !== parentTable ) { return false; } - - tableCells.push( range.start.nodeAfter ); } + const selectedTableCells = getSelectionAffectedTableCells( selection ); + // At this point selection contains ranges over table cells in the same table. // The valid selection is a fully occupied rectangle composed of table cells. // Below we calculate area of selected cells and the area of valid selection. @@ -214,7 +213,7 @@ function canMergeCells( selection, tableUtils ) { let areaOfSelectedCells = 0; - for ( const tableCell of tableCells ) { + for ( const tableCell of selectedTableCells ) { const { row, column } = tableUtils.getCellLocation( tableCell ); const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) || 1 ); const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 ); From 61fc962c4703169a631b362d68d72f0240185bd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 27 Mar 2020 13:22:47 +0100 Subject: [PATCH 04/13] Disallow merge cells command execution on selection extending from heading to body. --- src/commands/mergecellscommand.js | 16 +++++++++++++++- src/commands/removerowcommand.js | 12 +----------- src/utils.js | 10 ++++++++++ tests/commands/mergecellscommand.js | 20 +++++++++++++++++++- 4 files changed, 45 insertions(+), 13 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index f43e2346..eabd105c 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 { getSelectionAffectedTableCells } from '../utils'; +import { getRowIndexes, getSelectionAffectedTableCells } from '../utils'; /** * The merge cells command. @@ -204,6 +204,10 @@ function canMergeCells( selection, tableUtils ) { const selectedTableCells = getSelectionAffectedTableCells( selection ); + if ( !areCellInTheSameTableSection( selectedTableCells, firstRangeTable ) ) { + return false; + } + // At this point selection contains ranges over table cells in the same table. // The valid selection is a fully occupied rectangle composed of table cells. // Below we calculate area of selected cells and the area of valid selection. @@ -257,3 +261,13 @@ function getBiggestRectangleArea( rows, columns ) { return ( lastRow - firstRow + 1 ) * ( lastColumn - firstColumn + 1 ); } + +function areCellInTheSameTableSection( tableCells, table ) { + const rowIndexes = getRowIndexes( tableCells ); + const headingRows = parseInt( table.getAttribute( 'headingRows' ) || 0 ); + + const firstCellIsInBody = rowIndexes.first > headingRows - 1; + const lastCellIsInBody = rowIndexes.last > headingRows - 1; + + return firstCellIsInBody === lastCellIsInBody; +} diff --git a/src/commands/removerowcommand.js b/src/commands/removerowcommand.js index c1162a10..7e716a5a 100644 --- a/src/commands/removerowcommand.js +++ b/src/commands/removerowcommand.js @@ -11,7 +11,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; import TableWalker from '../tablewalker'; import { findAncestor, updateNumericAttribute } from './utils'; -import { getSelectionAffectedTableCells } from '../utils'; +import { getRowIndexes, getSelectionAffectedTableCells } from '../utils'; /** * The remove row command. @@ -138,16 +138,6 @@ export default class RemoveRowCommand extends Command { } } -// Returns a helper object with first and last row index contained in given `referenceCells`. -function getRowIndexes( referenceCells ) { - const allIndexesSorted = referenceCells.map( cell => cell.parent.index ).sort(); - - return { - first: allIndexesSorted[ 0 ], - last: allIndexesSorted[ allIndexesSorted.length - 1 ] - }; -} - // Returns a cell that should be focused before removing the row, belonging to the same column as the currently focused cell. // * If the row was not the last one, the cell to focus will be in the row that followed it (before removal). // * If the row was the last one, the cell to focus will be in the row that preceded it (before removal). diff --git a/src/utils.js b/src/utils.js index 408884bd..9a9dfcfd 100644 --- a/src/utils.js +++ b/src/utils.js @@ -137,6 +137,16 @@ export function getSelectionAffectedTableCells( selection ) { return getTableCellsContainingSelection( selection ); } +// Returns a helper object with first and last row index contained in given `referenceCells`. +export function getRowIndexes( referenceCells ) { + const allIndexesSorted = referenceCells.map( cell => cell.parent.index ).sort(); + + return { + first: allIndexesSorted[ 0 ], + last: allIndexesSorted[ allIndexesSorted.length - 1 ] + }; +} + function sortRanges( rangesIterator ) { return Array.from( rangesIterator ).sort( compareRangeOrder ); } diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js index bba07847..7d1c09f1 100644 --- a/tests/commands/mergecellscommand.js +++ b/tests/commands/mergecellscommand.js @@ -10,6 +10,7 @@ import MergeCellsCommand from '../../src/commands/mergecellscommand'; import { defaultConversion, defaultSchema, modelTable } from '../_utils/utils'; import TableUtils from '../../src/tableutils'; import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; +import TableSelection from '../../src/tableselection'; describe( 'MergeCellsCommand', () => { let editor, model, command, root; @@ -17,7 +18,7 @@ describe( 'MergeCellsCommand', () => { beforeEach( () => { return ModelTestEditor .create( { - plugins: [ TableUtils ] + plugins: [ TableUtils, TableSelection ] } ) .then( newEditor => { editor = newEditor; @@ -224,6 +225,23 @@ describe( 'MergeCellsCommand', () => { expect( command.isEnabled ).to.be.false; } ); + + it( 'should be false if selection has cells from header and body sections', () => { + setData( model, modelTable( [ + [ '00[]', '01' ], + [ '10', '11' ] + ], { headingRows: 1 } ) ); + + const tableSelection = editor.plugins.get( TableSelection ); + const modelRoot = model.document.getRoot(); + + tableSelection._setCellSelection( + modelRoot.getNodeByPath( [ 0, 0, 0 ] ), + modelRoot.getNodeByPath( [ 0, 1, 0 ] ) + ); + + expect( command.isEnabled ).to.be.false; + } ); } ); describe( 'execute()', () => { From 7908f68889cc1fa7e27e536c93d2ffb4bdc85092 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 27 Mar 2020 13:59:24 +0100 Subject: [PATCH 05/13] Use table editing for merge cells command tests. --- tests/commands/mergecellscommand.js | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js index 7d1c09f1..cd903e2a 100644 --- a/tests/commands/mergecellscommand.js +++ b/tests/commands/mergecellscommand.js @@ -7,29 +7,24 @@ import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltestedit import { getData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import MergeCellsCommand from '../../src/commands/mergecellscommand'; -import { defaultConversion, defaultSchema, modelTable } from '../_utils/utils'; -import TableUtils from '../../src/tableutils'; +import { modelTable } from '../_utils/utils'; import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; import TableSelection from '../../src/tableselection'; +import TableEditing from '../../src/tableediting'; +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; describe( 'MergeCellsCommand', () => { let editor, model, command, root; - beforeEach( () => { - return ModelTestEditor - .create( { - plugins: [ TableUtils, TableSelection ] - } ) - .then( newEditor => { - editor = newEditor; - model = editor.model; - root = model.document.getRoot( 'main' ); + beforeEach( async () => { + editor = await ModelTestEditor.create( { + plugins: [ Paragraph, TableEditing, TableSelection ] + } ); - command = new MergeCellsCommand( editor ); + model = editor.model; + root = model.document.getRoot( 'main' ); - defaultSchema( model.schema ); - defaultConversion( editor.conversion ); - } ); + command = new MergeCellsCommand( editor ); } ); afterEach( () => { @@ -245,7 +240,7 @@ describe( 'MergeCellsCommand', () => { } ); describe( 'execute()', () => { - it( 'should merge table cells', () => { + it( 'should merge simple table cell selection', () => { setData( model, modelTable( [ [ '[]00', '01' ] ] ) ); From 061b0bf4d1a0174041b00a0c815c1de548f96a5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 27 Mar 2020 14:28:03 +0100 Subject: [PATCH 06/13] MergeCellsCommand should provide the same output if selection is reversed. --- src/commands/mergecellscommand.js | 4 +- tests/commands/mergecellscommand.js | 65 +++++++++++++++++++++++++---- 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index eabd105c..77494509 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -52,11 +52,11 @@ export default class MergeCellsCommand extends Command { const tableUtils = this.editor.plugins.get( TableUtils ); model.change( writer => { - const selectedTableCells = [ ... this.editor.model.document.selection.getRanges() ].map( range => range.start.nodeAfter ); + const selectedTableCells = getSelectionAffectedTableCells( model.document.selection ); const firstTableCell = selectedTableCells.shift(); - // TODO: this shouldn't be necessary (right now the selection could overlap existing. + // This prevents the "model-selection-range-intersects" error, caused by removing row selected cells. writer.setSelection( firstTableCell, 'in' ); const { row, column } = tableUtils.getCellLocation( firstTableCell ); diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js index cd903e2a..783ef4ae 100644 --- a/tests/commands/mergecellscommand.js +++ b/tests/commands/mergecellscommand.js @@ -14,7 +14,7 @@ import TableEditing from '../../src/tableediting'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; describe( 'MergeCellsCommand', () => { - let editor, model, command, root; + let editor, model, command, root, tableSelection; beforeEach( async () => { editor = await ModelTestEditor.create( { @@ -23,6 +23,7 @@ describe( 'MergeCellsCommand', () => { model = editor.model; root = model.document.getRoot( 'main' ); + tableSelection = editor.plugins.get( TableSelection ); command = new MergeCellsCommand( editor ); } ); @@ -227,12 +228,9 @@ describe( 'MergeCellsCommand', () => { [ '10', '11' ] ], { headingRows: 1 } ) ); - const tableSelection = editor.plugins.get( TableSelection ); - const modelRoot = model.document.getRoot(); - tableSelection._setCellSelection( - modelRoot.getNodeByPath( [ 0, 0, 0 ] ), - modelRoot.getNodeByPath( [ 0, 1, 0 ] ) + root.getNodeByPath( [ 0, 0, 0 ] ), + root.getNodeByPath( [ 0, 1, 0 ] ) ); expect( command.isEnabled ).to.be.false; @@ -245,9 +243,10 @@ describe( 'MergeCellsCommand', () => { [ '[]00', '01' ] ] ) ); - selectNodes( [ - [ 0, 0, 0 ], [ 0, 0, 1 ] - ] ); + tableSelection._setCellSelection( + root.getNodeByPath( [ 0, 0, 0 ] ), + root.getNodeByPath( [ 0, 0, 1 ] ) + ); command.execute(); @@ -256,6 +255,54 @@ describe( 'MergeCellsCommand', () => { ] ) ); } ); + it( 'should merge selection with a cell with rowspan in the selection', () => { + setData( model, modelTable( [ + [ '[]00', '01', '02' ], + [ '10', { contents: '11', rowspan: 2 }, '12' ], + [ '20', '22' ] + ] ) ); + + tableSelection._setCellSelection( + root.getNodeByPath( [ 0, 1, 0 ] ), + root.getNodeByPath( [ 0, 2, 1 ] ) + ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ '00', '01', '02' ], + [ { + colspan: 3, + contents: '[101112' + + '2022]' + } ] + ] ) ); + } ); + + it( 'should merge selection with a cell with rowspan in the selection (reverse selection)', () => { + setData( model, modelTable( [ + [ '[]00', '01', '02' ], + [ '10', { contents: '11', rowspan: 2 }, '12' ], + [ '20', '22' ] + ] ) ); + + tableSelection._setCellSelection( + root.getNodeByPath( [ 0, 2, 1 ] ), + root.getNodeByPath( [ 0, 1, 0 ] ) + ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ '00', '01', '02' ], + [ { + colspan: 3, + contents: '[101112' + + '2022]' + } ] + ] ) ); + } ); + it( 'should merge table cells - extend colspan attribute', () => { setData( model, modelTable( [ [ { colspan: 2, contents: '00' }, '02', '03' ], From 8dc8f0d3ce45b4be638992fea182ecab6b3f3ca8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 27 Mar 2020 15:33:38 +0100 Subject: [PATCH 07/13] Refactor MergeCellsCommand and update its docs. --- src/commands/mergecellscommand.js | 115 +++++++++++++--------------- tests/commands/mergecellscommand.js | 27 +++++++ 2 files changed, 80 insertions(+), 62 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 77494509..63d1a2aa 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -16,18 +16,11 @@ import { getRowIndexes, getSelectionAffectedTableCells } from '../utils'; /** * The merge cells command. * - * The command is registered by {@link module:table/tableediting~TableEditing} as `'mergeTableCellRight'`, `'mergeTableCellLeft'`, - * `'mergeTableCellUp'` and `'mergeTableCellDown'` editor commands. + * The command is registered by {@link module:table/tableediting~TableEditing} as `'mergeTableCells'` editor command. * - * To merge a table cell at the current selection with another cell, execute the command corresponding with the preferred direction. + * For example, to merge selected table cells: * - * For example, to merge with a cell to the right: - * - * editor.execute( 'mergeTableCellRight' ); - * - * **Note**: If a table cell has a different [`rowspan`](https://www.w3.org/TR/html50/tabular-data.html#attr-tdth-rowspan) - * (for `'mergeTableCellRight'` and `'mergeTableCellLeft'`) or [`colspan`](https://www.w3.org/TR/html50/tabular-data.html#attr-tdth-colspan) - * (for `'mergeTableCellUp'` and `'mergeTableCellDown'`), the command will be disabled. + * editor.execute( 'mergeTableCells' ); * * @extends module:core/command~Command */ @@ -42,8 +35,6 @@ export default class MergeCellsCommand extends Command { /** * Executes the command. * - * Depending on the command's {@link #direction} value, it will merge the cell that is to the `'left'`, `'right'`, `'up'` or `'down'`. - * * @fires execute */ execute() { @@ -54,65 +45,39 @@ export default class MergeCellsCommand extends Command { model.change( writer => { const selectedTableCells = getSelectionAffectedTableCells( model.document.selection ); + // All cells will be merge into the first one. const firstTableCell = selectedTableCells.shift(); // This prevents the "model-selection-range-intersects" error, caused by removing row selected cells. - writer.setSelection( firstTableCell, 'in' ); + writer.setSelection( firstTableCell, 'on' ); - const { row, column } = tableUtils.getCellLocation( firstTableCell ); - - const colspan = parseInt( firstTableCell.getAttribute( 'colspan' ) || 1 ); - const rowspan = parseInt( firstTableCell.getAttribute( 'rowspan' ) || 1 ); - - let rightMax = column + colspan; - let bottomMax = row + rowspan; - - const rowsToCheck = new Set(); + // Update target cell dimensions. + const { mergeWidth, mergeHeight } = getMergeDimensions( firstTableCell, selectedTableCells, tableUtils ); + updateNumericAttribute( 'colspan', mergeWidth, firstTableCell, writer ); + updateNumericAttribute( 'rowspan', mergeHeight, firstTableCell, writer ); for ( const tableCell of selectedTableCells ) { - const { row, column } = tableUtils.getCellLocation( tableCell ); - - const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 ); - const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) || 1 ); - - if ( column + colspan > rightMax ) { - rightMax = column + colspan; - } - - if ( row + rowspan > bottomMax ) { - bottomMax = row + rowspan; - } - } - - for ( const tableCell of selectedTableCells ) { - rowsToCheck.add( tableCell.parent ); + const tableRow = tableCell.parent; mergeTableCells( tableCell, firstTableCell, writer ); + removeRowIfEmpty( tableRow, writer ); } - // Update table cell span attribute and merge set selection on merged contents. - updateNumericAttribute( 'colspan', rightMax - column, firstTableCell, writer ); - updateNumericAttribute( 'rowspan', bottomMax - row, firstTableCell, writer ); - writer.setSelection( firstTableCell, 'in' ); - - // Remove empty rows after merging table cells. - for ( const row of rowsToCheck ) { - if ( !row.childCount ) { - removeEmptyRow( row, writer ); - } - } } ); } } // Properly removes empty row from a table. Will update `rowspan` attribute of cells that overlaps removed row. // -// @param {module:engine/model/element~Element} removedTableCellRow +// @param {module:engine/model/element~Element} row // @param {module:engine/model/writer~Writer} writer -function removeEmptyRow( removedTableCellRow, writer ) { - const table = removedTableCellRow.parent; +function removeRowIfEmpty( row, writer ) { + if ( row.childCount ) { + return; + } - const removedRowIndex = table.getChildIndex( removedTableCellRow ); + const table = row.parent; + const removedRowIndex = table.getChildIndex( row ); for ( const { cell, row, rowspan } of new TableWalker( table, { endRow: removedRowIndex } ) ) { const overlapsRemovedRow = row + rowspan - 1 >= removedRowIndex; @@ -122,27 +87,27 @@ function removeEmptyRow( removedTableCellRow, writer ) { } } - writer.remove( removedTableCellRow ); + writer.remove( row ); } // Merges two table cells - will ensure that after merging cells with empty paragraph the result table cell will only have one paragraph. // If one of the merged table cell is empty the merged table cell will have contents of the non-empty table cell. // If both are empty the merged table cell will have only one empty paragraph. // -// @param {module:engine/model/element~Element} cellToRemove -// @param {module:engine/model/element~Element} cellToExpand +// @param {module:engine/model/element~Element} cellBeingMerged +// @param {module:engine/model/element~Element} targetCell // @param {module:engine/model/writer~Writer} writer -function mergeTableCells( cellToRemove, cellToExpand, writer ) { - if ( !isEmpty( cellToRemove ) ) { - if ( isEmpty( cellToExpand ) ) { - writer.remove( writer.createRangeIn( cellToExpand ) ); +function mergeTableCells( cellBeingMerged, targetCell, writer ) { + if ( !isEmpty( cellBeingMerged ) ) { + if ( isEmpty( targetCell ) ) { + writer.remove( writer.createRangeIn( targetCell ) ); } - writer.move( writer.createRangeIn( cellToRemove ), writer.createPositionAt( cellToExpand, 'end' ) ); + writer.move( writer.createRangeIn( cellBeingMerged ), writer.createPositionAt( targetCell, 'end' ) ); } // Remove merged table cell. - writer.remove( cellToRemove ); + writer.remove( cellBeingMerged ); } // Checks if passed table cell contains empty paragraph. @@ -271,3 +236,29 @@ function areCellInTheSameTableSection( tableCells, table ) { return firstCellIsInBody === lastCellIsInBody; } + +function getMergeDimensions( firstTableCell, selectedTableCells, tableUtils ) { + let maxWidthOffset = 0; + let maxHeightOffset = 0; + + for ( const tableCell of selectedTableCells ) { + const { row, column } = tableUtils.getCellLocation( tableCell ); + + maxWidthOffset = getMaxOffset( tableCell, column, maxWidthOffset, 'colspan' ); + maxHeightOffset = getMaxOffset( tableCell, row, maxHeightOffset, 'rowspan' ); + } + + // Update table cell span attribute and merge set selection on a merged contents. + const { row: firstCellRow, column: firstCellColumn } = tableUtils.getCellLocation( firstTableCell ); + + const mergeWidth = maxWidthOffset - firstCellColumn; + const mergeHeight = maxHeightOffset - firstCellRow; + + return { mergeWidth, mergeHeight }; +} + +function getMaxOffset( tableCell, start, currentMaxOffset, which ) { + const dimensionValue = parseInt( tableCell.getAttribute( which ) || 1 ); + + return Math.max( currentMaxOffset, start + dimensionValue ); +} diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js index 783ef4ae..e5deec84 100644 --- a/tests/commands/mergecellscommand.js +++ b/tests/commands/mergecellscommand.js @@ -303,6 +303,33 @@ describe( 'MergeCellsCommand', () => { ] ) ); } ); + it( 'should merge selection inside a table (properly calculate target rowspan/colspan)', () => { + setData( model, modelTable( [ + [ '[]00', '01', '02', '03' ], + [ '10', '11', { contents: '12', rowspan: 2 }, '13' ], + [ '20', '21', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + + tableSelection._setCellSelection( + root.getNodeByPath( [ 0, 2, 1 ] ), + root.getNodeByPath( [ 0, 1, 2 ] ) + ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ '00', '01', '02', '03' ], + [ '10', { + colspan: 2, + rowspan: 2, + contents: '[111221]' + }, '13' ], + [ '20', '23' ], + [ '30', '31', '32', '33' ] + ] ) ); + } ); + it( 'should merge table cells - extend colspan attribute', () => { setData( model, modelTable( [ [ { colspan: 2, contents: '00' }, '02', '03' ], From 85317b1d23e2de999cb328606eb3dfb42adc0011 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 27 Mar 2020 15:34:47 +0100 Subject: [PATCH 08/13] Use proper indentation in jsdoc. --- 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 63d1a2aa..881fac7a 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -20,7 +20,7 @@ import { getRowIndexes, getSelectionAffectedTableCells } from '../utils'; * * For example, to merge selected table cells: * - * editor.execute( 'mergeTableCells' ); + * editor.execute( 'mergeTableCells' ); * * @extends module:core/command~Command */ From c0ea88273358f38312312373bc17560411293362 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 27 Mar 2020 15:40:29 +0100 Subject: [PATCH 09/13] Remove redundant checks. --- src/commands/mergecellscommand.js | 32 +++++++---------------------- tests/commands/mergecellscommand.js | 27 ------------------------ 2 files changed, 7 insertions(+), 52 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 881fac7a..6476712f 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -39,7 +39,6 @@ export default class MergeCellsCommand extends Command { */ execute() { const model = this.editor.model; - const tableUtils = this.editor.plugins.get( TableUtils ); model.change( writer => { @@ -149,34 +148,15 @@ function canMergeCells( selection, tableUtils ) { return false; } - // All cells must be inside the same table. - let firstRangeTable; - - for ( const range of selection.getRanges() ) { - // Selection ranges must be set on whole element. - if ( range.isCollapsed || !range.isFlat || !range.start.nodeAfter.is( 'tableCell' ) ) { - return false; - } - - const parentTable = findAncestor( 'table', range.start ); - - if ( !firstRangeTable ) { - firstRangeTable = parentTable; - } else if ( firstRangeTable !== parentTable ) { - return false; - } - } - const selectedTableCells = getSelectionAffectedTableCells( selection ); - if ( !areCellInTheSameTableSection( selectedTableCells, firstRangeTable ) ) { + if ( !areCellInTheSameTableSection( selectedTableCells ) ) { return false; } - // At this point selection contains ranges over table cells in the same table. - // The valid selection is a fully occupied rectangle composed of table cells. - // Below we calculate area of selected cells and the area of valid selection. - // The area of valid selection is defined by top-left and bottom-right cells. + // A valid selection is a fully occupied rectangle composed of table cells. + // Below we will calculate the area of a selected table cells and the area of valid selection. + // The area of a valid selection is defined by top-left and bottom-right cells. const rows = new Set(); const columns = new Set(); @@ -227,7 +207,9 @@ function getBiggestRectangleArea( rows, columns ) { return ( lastRow - firstRow + 1 ) * ( lastColumn - firstColumn + 1 ); } -function areCellInTheSameTableSection( tableCells, table ) { +function areCellInTheSameTableSection( tableCells ) { + const table = findAncestor( 'table', tableCells[ 0 ] ); + const rowIndexes = getRowIndexes( tableCells ); const headingRows = parseInt( table.getAttribute( 'headingRows' ) || 0 ); diff --git a/tests/commands/mergecellscommand.js b/tests/commands/mergecellscommand.js index e5deec84..d0689ece 100644 --- a/tests/commands/mergecellscommand.js +++ b/tests/commands/mergecellscommand.js @@ -121,33 +121,6 @@ describe( 'MergeCellsCommand', () => { expect( command.isEnabled ).to.be.false; } ); - it( 'should be false if any range is collapsed in selection', () => { - setData( model, modelTable( [ - [ '00', '01', '02' ] - ] ) ); - - selectNodes( [ - [ 0, 0, 0, 0, 0 ], // The "00" text node - [ 0, 0, 1 ] - ] ); - - expect( command.isEnabled ).to.be.false; - } ); - - it( 'should be false if any ranges are on different tables', () => { - setData( model, - modelTable( [ [ '00', '01' ] ] ) + - modelTable( [ [ 'aa', 'ab' ] ] ) - ); - - selectNodes( [ - [ 0, 0, 0 ], // first table - [ 1, 0, 1 ] // second table - ] ); - - expect( command.isEnabled ).to.be.false; - } ); - it( 'should be false if any table cell with colspan attribute extends over selection area', () => { setData( model, modelTable( [ [ '00', { colspan: 2, contents: '01' } ], From a8b4d897bc56cfa5c0a780dcc78e8b29961d11fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 30 Mar 2020 09:50:51 +0200 Subject: [PATCH 10/13] Add docs to getRowIndexes() helper function. --- src/utils.js | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/utils.js b/src/utils.js index 9a9dfcfd..a4f066cf 100644 --- a/src/utils.js +++ b/src/utils.js @@ -137,9 +137,20 @@ export function getSelectionAffectedTableCells( selection ) { return getTableCellsContainingSelection( selection ); } -// Returns a helper object with first and last row index contained in given `referenceCells`. -export function getRowIndexes( referenceCells ) { - const allIndexesSorted = referenceCells.map( cell => cell.parent.index ).sort(); +/** + * Returns a helper object with `first` and `last` row index contained in given `tableCells`. + * + * const selectedTableCells = getSelectedTableCells( editor.model.document.selection ); + * + * const { first, last } = getRowIndexes( selectedTableCells ); + * + * console.log( `Selected rows ${ first } to ${ last }` ); + * + * @package {Array.} + * @returns {Object} Returns an object with `first` and `last` table row indexes. + */ +export function getRowIndexes( tableCells ) { + const allIndexesSorted = tableCells.map( cell => cell.parent.index ).sort(); return { first: allIndexesSorted[ 0 ], From ba9080d5cb113634fda31efe97de149af3b116d3 Mon Sep 17 00:00:00 2001 From: Maciej Date: Wed, 1 Apr 2020 11:17:14 +0200 Subject: [PATCH 11/13] Apply suggestions from code review. Co-Authored-By: Aleksander Nowodzinski --- src/commands/mergecellscommand.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 6476712f..0be4ae8f 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -16,7 +16,7 @@ import { getRowIndexes, getSelectionAffectedTableCells } from '../utils'; /** * The merge cells command. * - * The command is registered by {@link module:table/tableediting~TableEditing} as `'mergeTableCells'` editor command. + * The command is registered by the {@link module:table/tableediting~TableEditing} as `'mergeTableCells'` editor command. * * For example, to merge selected table cells: * @@ -66,7 +66,7 @@ export default class MergeCellsCommand extends Command { } } -// Properly removes empty row from a table. Will update `rowspan` attribute of cells that overlaps removed row. +// Properly removes the empty row from a table. Updates the `rowspan` attribute of cells that overlap the removed row. // // @param {module:engine/model/element~Element} row // @param {module:engine/model/writer~Writer} writer @@ -89,9 +89,9 @@ function removeRowIfEmpty( row, writer ) { writer.remove( row ); } -// Merges two table cells - will ensure that after merging cells with empty paragraph the result table cell will only have one paragraph. -// If one of the merged table cell is empty the merged table cell will have contents of the non-empty table cell. -// If both are empty the merged table cell will have only one empty paragraph. +// Merges two table cells - will ensure that after merging cells with empty paragraphs the result table cell will only have one paragraph. +// If one of the merged table cells is empty, the merged table cell will have contents of the non-empty table cell. +// If both are empty, the merged table cell will have only one empty paragraph. // // @param {module:engine/model/element~Element} cellBeingMerged // @param {module:engine/model/element~Element} targetCell @@ -109,7 +109,7 @@ function mergeTableCells( cellBeingMerged, targetCell, writer ) { writer.remove( cellBeingMerged ); } -// Checks if passed table cell contains empty paragraph. +// Checks if the passed table cell contains an empty paragraph. // // @param {module:engine/model/element~Element} tableCell // @returns {Boolean} @@ -117,7 +117,7 @@ function isEmpty( tableCell ) { return tableCell.childCount == 1 && tableCell.getChild( 0 ).is( 'paragraph' ) && tableCell.getChild( 0 ).isEmpty; } -// Check if selection contains mergeable cells. +// Checks if the selection contains mergeable cells. // // In a table below: // @@ -129,13 +129,13 @@ function isEmpty( tableCell ) { // | | g | h | // +---+---+---+---+ // -// Valid selections are those which creates a solid rectangle (without gaps), such as: +// Valid selections are these which create a solid rectangle (without gaps), such as: // - a, b (two horizontal cells) // - c, f (two vertical cells) // - a, b, e (cell "e" spans over four cells) -// - c, d, f (cell d spans over cell in row below) +// - c, d, f (cell d spans over a cell in the row below) // -// While invalid selection would be: +// While an invalid selection would be: // - a, c (cell "b" not selected creates a gap) // - f, g, h (cell "d" spans over a cell from row of "f" cell - thus creates a gap) // From 0f1661129598bd3c0b6bc1ea21f65268fd6c59a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 1 Apr 2020 11:25:05 +0200 Subject: [PATCH 12/13] Use box-drawing characters for table samples. --- src/commands/mergecellscommand.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 0be4ae8f..187af1b6 100644 --- a/src/commands/mergecellscommand.js +++ b/src/commands/mergecellscommand.js @@ -121,13 +121,13 @@ function isEmpty( tableCell ) { // // In a table below: // -// +---+---+---+---+ -// | a | b | c | d | -// +---+---+---+ + -// | e | f | | -// + +---+---+ -// | | g | h | -// +---+---+---+---+ +// ┌───┬───┬───┬───┐ +// │ a │ b │ c │ d │ +// ├───┴───┼───┤ │ +// │ e │ f │ │ +// ├ ├───┼───┤ +// │ │ g │ h │ +// └───────┴───┴───┘ // // Valid selections are these which create a solid rectangle (without gaps), such as: // - a, b (two horizontal cells) From e5cb4b7f428e1eb8d08865293a3e1a2b03b37f6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 1 Apr 2020 12:48:04 +0200 Subject: [PATCH 13/13] Simplify check for table cells mergeability. --- src/commands/mergecellscommand.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/commands/mergecellscommand.js b/src/commands/mergecellscommand.js index 187af1b6..c0fc30d0 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, getSelectionAffectedTableCells } from '../utils'; +import { getRowIndexes, getSelectedTableCells } from '../utils'; /** * The merge cells command. @@ -42,7 +42,7 @@ export default class MergeCellsCommand extends Command { const tableUtils = this.editor.plugins.get( TableUtils ); model.change( writer => { - const selectedTableCells = getSelectionAffectedTableCells( model.document.selection ); + const selectedTableCells = getSelectedTableCells( model.document.selection ); // All cells will be merge into the first one. const firstTableCell = selectedTableCells.shift(); @@ -143,14 +143,9 @@ function isEmpty( tableCell ) { // @param {module:table/tableUtils~TableUtils} tableUtils // @returns {boolean} function canMergeCells( selection, tableUtils ) { - // Collapsed selection or selection only one range can't contain mergeable table cells. - if ( selection.isCollapsed || selection.rangeCount < 2 ) { - return false; - } - - const selectedTableCells = getSelectionAffectedTableCells( selection ); + const selectedTableCells = getSelectedTableCells( selection ); - if ( !areCellInTheSameTableSection( selectedTableCells ) ) { + if ( selectedTableCells.length < 2 || !areCellInTheSameTableSection( selectedTableCells ) ) { return false; }