Skip to content

Commit

Permalink
Merge pull request #6764 from ckeditor/i/6667
Browse files Browse the repository at this point in the history 
Fix (table): Table heading rows should be properly updated after removing rows as a side effect of merging cells. Closes #6667.
  • Loading branch information
@jodator
jodator authored May 13, 2020
2 parents d25ebfc + 5aa707d commit 72f6491
Show file tree
Hide file tree
Showing 8 changed files with 251 additions and 67 deletions.
3 changes: 2 additions & 1 deletion packages/ckeditor5-table/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
"@ckeditor/ckeditor5-typing": "^19.0.0",
"@ckeditor/ckeditor5-undo": "^19.0.0",
"@ckeditor/ckeditor5-utils": "^19.0.0",
"json-diff": "^0.5.4"
"json-diff": "^0.5.4",
"lodash-es": "^4.17.10"
},
"engines": {
"node": ">=8.0.0",
Expand Down
31 changes: 6 additions & 25 deletions packages/ckeditor5-table/src/commands/mergecellcommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,7 @@

import Command from '@ckeditor/ckeditor5-core/src/command';
import TableWalker from '../tablewalker';
import {
updateNumericAttribute,
isHeadingColumnCell
} from './utils';
import { isHeadingColumnCell, findAncestor } from './utils';
import { getTableCellsContainingSelection } from '../utils';

/**
Expand Down Expand Up @@ -83,6 +80,7 @@ export default class MergeCellCommand extends Command {
const model = this.editor.model;
const doc = model.document;
const tableCell = getTableCellsContainingSelection( doc.selection )[ 0 ];

const cellToMerge = this.value;
const direction = this.direction;

Expand All @@ -108,7 +106,10 @@ export default class MergeCellCommand extends Command {

// Remove empty row after merging.
if ( !removedTableCellRow.childCount ) {
removeEmptyRow( removedTableCellRow, writer );
const tableUtils = this.editor.plugins.get( 'TableUtils' );
const table = findAncestor( 'table', removedTableCellRow );

tableUtils.removeRows( table, { at: removedTableCellRow.index, batch: writer.batch } );
}
} );
}
Expand Down Expand Up @@ -243,26 +244,6 @@ function getVerticalCell( tableCell, direction ) {
return cellToMergeData && cellToMergeData.cell;
}

// Properly removes an empty row from a table. It will update the `rowspan` attribute of cells that overlap the 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. It will ensure that after merging cells with an empty paragraph, the resulting table cell will only have one
// paragraph. If one of the merged table cells is empty, the merged table cell will have the contents of the non-empty table cell.
// If both are empty, the merged table cell will have only one empty paragraph.
Expand Down
38 changes: 13 additions & 25 deletions packages/ckeditor5-table/src/commands/mergecellscommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

import Command from '@ckeditor/ckeditor5-core/src/command';
import TableWalker from '../tablewalker';
import { findAncestor, updateNumericAttribute } from './utils';
import TableUtils from '../tableutils';
import { getColumnIndexes, getRowIndexes, getSelectedTableCells } from '../utils';
Expand Down Expand Up @@ -57,38 +56,27 @@ export default class MergeCellsCommand extends Command {
updateNumericAttribute( 'colspan', mergeWidth, firstTableCell, writer );
updateNumericAttribute( 'rowspan', mergeHeight, firstTableCell, writer );

const emptyRowsIndexes = [];

for ( const tableCell of selectedTableCells ) {
const tableRow = tableCell.parent;
mergeTableCells( tableCell, firstTableCell, writer );
removeRowIfEmpty( tableRow, writer );
}

writer.setSelection( firstTableCell, 'in' );
} );
}
}
mergeTableCells( tableCell, firstTableCell, writer );

// Properly removes an 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
function removeRowIfEmpty( row, writer ) {
if ( row.childCount ) {
return;
}
if ( !tableRow.childCount ) {
emptyRowsIndexes.push( tableRow.index );
}
}

const table = row.parent;
const removedRowIndex = table.getChildIndex( row );
if ( emptyRowsIndexes.length ) {
const table = findAncestor( 'table', firstTableCell );

for ( const { cell, row, rowspan } of new TableWalker( table, { endRow: removedRowIndex } ) ) {
const overlapsRemovedRow = row + rowspan - 1 >= removedRowIndex;
emptyRowsIndexes.reverse().forEach( row => tableUtils.removeRows( table, { at: row, batch: writer.batch } ) );
}

if ( overlapsRemovedRow ) {
updateNumericAttribute( 'rowspan', rowspan - 1, cell, writer );
}
writer.setSelection( firstTableCell, 'in' );
} );
}

writer.remove( row );
}

// Merges two table cells. It will ensure that after merging cells with empty paragraphs the resulting table cell will only have one
Expand Down
37 changes: 22 additions & 15 deletions packages/ckeditor5-table/src/tableutils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,17 +289,21 @@ export default class TableUtils extends Plugin {
const last = first + rowsToRemove - 1;
const batch = options.batch || 'default';

// Removing rows from table requires most calculations to be done prior to changing table structure.
model.enqueueChange( batch, writer => {
// Removing rows from the table require that most calculations to be done prior to changing table structure.
// Preparations must be done in the same enqueueChange callback to use the current table structure.

// 1. Preparation - get row-spanned cells that have to be modified after removing rows.
const { cellsToMove, cellsToTrim } = getCellsToMoveAndTrimOnRemoveRow( table, first, last );
// 1. Preparation - get row-spanned cells that have to be modified after removing rows.
const { cellsToMove, cellsToTrim } = getCellsToMoveAndTrimOnRemoveRow( table, first, last );

// 2. Execution

// 2. Execution
model.enqueueChange( batch, writer => {
// 2a. Move cells from removed rows that extends over a removed section - must be done before removing rows.
// This will fill any gaps in a rows below that previously were empty because of row-spanned cells.
const rowAfterRemovedSection = last + 1;
moveCellsToRow( table, rowAfterRemovedSection, cellsToMove, writer );
if ( cellsToMove.size ) {
const rowAfterRemovedSection = last + 1;
moveCellsToRow( table, rowAfterRemovedSection, cellsToMove, writer );
}

// 2b. Remove all required rows.
for ( let i = last; i >= first; i-- ) {
Expand Down Expand Up @@ -753,17 +757,20 @@ function adjustHeadingColumns( table, removedColumnIndexes, writer ) {

// Calculates a new heading rows value for removing rows from heading section.
function updateHeadingRows( table, first, last, model, batch ) {
const headingRows = table.getAttribute( 'headingRows' ) || 0;
// Must be done after the changes in table structure (removing rows).
// Otherwise the downcast converter for headingRows attribute will fail.
// See https://github.com/ckeditor/ckeditor5/issues/6391.
//
// Must be completely wrapped in enqueueChange to get the current table state (after applying other enqueued changes).
model.enqueueChange( batch, writer => {
const headingRows = table.getAttribute( 'headingRows' ) || 0;

if ( first < headingRows ) {
const newRows = last < headingRows ? headingRows - ( last - first + 1 ) : first;
if ( first < headingRows ) {
const newRows = last < headingRows ? headingRows - ( last - first + 1 ) : first;

// Must be done after the changes in table structure (removing rows).
// Otherwise the downcast converter for headingRows attribute will fail. ckeditor/ckeditor5#6391.
model.enqueueChange( batch, writer => {
updateNumericAttribute( 'headingRows', newRows, table, writer, 0 );
} );
}
}
} );
}

// Finds cells that will be:
Expand Down
15 changes: 15 additions & 0 deletions packages/ckeditor5-table/tests/_utils/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,9 @@ export function createTableAsciiArt( model, table ) {
const { row: lastRow, column: lastColumn } = tableMap[ tableMap.length - 1 ];
const columns = lastColumn + 1;

const headingRows = parseInt( table.getAttribute( 'headingRows' ) ) || 0;
const headingColumns = parseInt( table.getAttribute( 'headingColumns' ) ) || 0;

let result = '';

for ( let row = 0; row <= lastRow; row++ ) {
Expand Down Expand Up @@ -539,13 +542,25 @@ export function createTableAsciiArt( model, table ) {
if ( column == lastColumn ) {
gridLine += '+';
contentLine += '|';

if ( headingRows && row == headingRows ) {
gridLine += ' <-- heading rows';
}
}
}
result += gridLine + '\n';
result += contentLine + '\n';

if ( row == lastRow ) {
result += `+${ '----+'.repeat( columns ) }`;

if ( headingRows && row == headingRows - 1 ) {
result += ' <-- heading rows';
}

if ( headingColumns > 0 ) {
result += `\n${ ' '.repeat( headingColumns ) }^-- heading columns`;
}
}
}

Expand Down
94 changes: 94 additions & 0 deletions packages/ckeditor5-table/tests/commands/mergecellcommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,53 @@ describe( 'MergeCellCommand', () => {
[ '40', '41' ]
] ) );
} );

it( 'should adjust heading rows if empty row was removed ', () => {
// +----+----+
// | 00 | 01 |
// + +----+
// | | 11 |
// +----+----+ <-- heading rows
// | 20 | 21 |
// +----+----+
setData( model, modelTable( [
[ { contents: '00', rowspan: 2 }, '[]01' ],
[ '11' ],
[ '20', '21' ]
], { headingRows: 2 } ) );

command.execute();

assertEqualMarkup( getData( model ), modelTable( [
[ '00', '<paragraph>[01</paragraph><paragraph>11]</paragraph>' ],
[ '20', '21' ]
], { headingRows: 1 } ) );
} );

it( 'should create one undo step (1 batch)', () => {
// +----+----+
// | 00 | 01 |
// + +----+
// | | 11 |
// +----+----+ <-- heading rows
// | 20 | 21 |
// +----+----+
setData( model, modelTable( [
[ { contents: '00', rowspan: 2 }, '[]01' ],
[ '11' ],
[ '20', '21' ]
], { headingRows: 2 } ) );

const createdBatches = new Set();

model.on( 'applyOperation', ( evt, [ operation ] ) => {
createdBatches.add( operation.batch );
} );

command.execute();

expect( createdBatches.size ).to.equal( 1 );
} );
} );
} );

Expand Down Expand Up @@ -959,6 +1006,53 @@ describe( 'MergeCellCommand', () => {
[ '40', '41' ]
] ) );
} );

it( 'should adjust heading rows if empty row was removed ', () => {
// +----+----+
// | 00 | 01 |
// + +----+
// | | 11 |
// +----+----+ <-- heading rows
// | 20 | 21 |
// +----+----+
setData( model, modelTable( [
[ { contents: '00', rowspan: 2 }, '01' ],
[ '[]11' ],
[ '20', '21' ]
], { headingRows: 2 } ) );

command.execute();

assertEqualMarkup( getData( model ), modelTable( [
[ '00', '<paragraph>[01</paragraph><paragraph>11]</paragraph>' ],
[ '20', '21' ]
], { headingRows: 1 } ) );
} );

it( 'should create one undo step (1 batch)', () => {
// +----+----+
// | 00 | 01 |
// + +----+
// | | 11 |
// +----+----+ <-- heading rows
// | 20 | 21 |
// +----+----+
setData( model, modelTable( [
[ { contents: '00', rowspan: 2 }, '01' ],
[ '[]11' ],
[ '20', '21' ]
], { headingRows: 2 } ) );

const createdBatches = new Set();

model.on( 'applyOperation', ( evt, [ operation ] ) => {
createdBatches.add( operation.batch );
} );

command.execute();

expect( createdBatches.size ).to.equal( 1 );
} );
} );
} );
} );
Loading

0 comments on commit 72f6491

Please sign in to comment.