Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #303 from ckeditor/i/6502
Browse files Browse the repository at this point in the history 
Fix: Removing rows in complex tables should properly move cells from removed rows. Closes ckeditor/ckeditor5#6502.
  • Loading branch information
@oleq
oleq authored Apr 17, 2020
2 parents 7213f93 + b04996d commit c8d8d32
Show file tree
Hide file tree
Showing 2 changed files with 415 additions and 94 deletions.
197 changes: 134 additions & 63 deletions src/tableutils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,13 +267,13 @@ export default class TableUtils extends Plugin {
* ┌───┬───┬───┐ `at` = 1 ┌───┬───┬───┐
* 0 │ a │ b │ c │ `rows` = 2 │ a │ b │ c │ 0
* │ ├───┼───┤ │ ├───┼───┤
* 1 │ │ d │ e │ <-- remove from here │ │ h │ i │ 1
* │ ├───┼───┤ will give: ├───┼───┼───┤
* 2 │ │ f │ g │ │ j │ k │ l │ 2
* │ ├───┼───┤ └───┴───┴───┘
* 3 │ │ h │ i │
* 1 │ │ d │ e │ <-- remove from here │ │ d │ g │ 1
* │ │ ├───┤ will give: ├───┼───┼───┤
* 2 │ │ │ f │ │ h │ i │ j │ 2
* │ │ ├───┤ └───┴───┴───┘
* 3 │ │ │ g │
* ├───┼───┼───┤
* 4 │ j │ k │ l │
* 4 │ h │ i │ j │
* └───┴───┴───┘
*
* @param {module:engine/model/element~Element} table
Expand All @@ -283,27 +283,35 @@ export default class TableUtils extends Plugin {
*/
removeRows( table, options ) {
const model = this.editor.model;
const first = options.at;
const rowsToRemove = options.rows || 1;

const rowsToRemove = options.rows || 1;
const first = options.at;
const last = first + rowsToRemove - 1;

// Removing rows from table requires most calculations to be done prior to changing table structure.

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

// 2. Execution
model.change( 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 );

// 2b. Remove all required rows.
for ( let i = last; i >= first; i-- ) {
removeRow( table, i, writer );
writer.remove( table.getChild( i ) );
}

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

if ( headingRows && first < headingRows ) {
const newRows = getNewHeadingRowsValue( first, last, headingRows );

// 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( writer.batch, writer => {
updateNumericAttribute( 'headingRows', newRows, table, writer, 0 );
} );
// 2c. Update cells from rows above that overlap removed section. Similar to step 2 but does not involve moving cells.
for ( const { rowspan, cell } of cellsToTrim ) {
updateNumericAttribute( 'rowspan', rowspan, cell, writer );
}

// 2d. Adjust heading rows if removed rows were in a heading section.
updateHeadingRows( table, first, last, model, writer.batch );
} );
}

Expand Down Expand Up @@ -730,60 +738,123 @@ function breakSpanEvenly( span, numberOfCells ) {
return { newCellsSpan, updatedSpan };
}

function removeRow( table, rowIndex, writer ) {
const cellsToMove = new Map();
const tableRow = table.getChild( rowIndex );
const tableMap = [ ...new TableWalker( table, { endRow: rowIndex } ) ];

// Get cells from removed row that are spanned over multiple rows.
tableMap
.filter( ( { row, rowspan } ) => row === rowIndex && rowspan > 1 )
.forEach( ( { column, cell, rowspan } ) => cellsToMove.set( column, { cell, rowspanToSet: rowspan - 1 } ) );

// Reduce rowspan on cells that are above removed row and overlaps removed row.
tableMap
.filter( ( { row, rowspan } ) => row <= rowIndex - 1 && row + rowspan > rowIndex )
.forEach( ( { cell, rowspan } ) => updateNumericAttribute( 'rowspan', rowspan - 1, cell, writer ) );

// Move cells to another row.
const targetRow = rowIndex + 1;
const tableWalker = new TableWalker( table, { includeSpanned: true, startRow: targetRow, endRow: targetRow } );
let previousCell;
// Updates heading columns attribute if removing a row from head section.
function adjustHeadingColumns( table, removedColumnIndexes, writer ) {
const headingColumns = table.getAttribute( 'headingColumns' ) || 0;

for ( const { row, column, cell } of [ ...tableWalker ] ) {
if ( cellsToMove.has( column ) ) {
const { cell: cellToMove, rowspanToSet } = cellsToMove.get( column );
const targetPosition = previousCell ?
writer.createPositionAfter( previousCell ) :
writer.createPositionAt( table.getChild( row ), 0 );
writer.move( writer.createRangeOn( cellToMove ), targetPosition );
updateNumericAttribute( 'rowspan', rowspanToSet, cellToMove, writer );
previousCell = cellToMove;
} else {
previousCell = cell;
}
}
if ( headingColumns && removedColumnIndexes.first < headingColumns ) {
const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) -
removedColumnIndexes.first + 1;

writer.remove( tableRow );
writer.setAttribute( 'headingColumns', headingColumns - headingsRemoved, table );
}
}

// Calculates a new heading rows value for removing rows from heading section.
function getNewHeadingRowsValue( first, last, headingRows ) {
if ( last < headingRows ) {
return headingRows - ( last - first + 1 );
function updateHeadingRows( table, first, last, model, batch ) {
const headingRows = table.getAttribute( 'headingRows' ) || 0;

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:
// - trimmed - Cells that are "above" removed rows sections and overlap the removed section - their rowspan must be trimmed.
// - moved - Cells from removed rows section might stick out of. These cells are moved to the next row after a removed section.
//
// Sample table with overlapping & sticking out cells:
//
// +----+----+----+----+----+
// | 00 | 01 | 02 | 03 | 04 |
// +----+ + + + +
// | 10 | | | | |
// +----+----+ + + +
// | 20 | 21 | | | | <-- removed row
// + + +----+ + +
// | | | 32 | | | <-- removed row
// +----+ + +----+ +
// | 40 | | | 43 | |
// +----+----+----+----+----+
//
// In a table above:
// - cells to trim: '02', '03' & '04'.
// - cells to move: '21' & '32'.
function getCellsToMoveAndTrimOnRemoveRow( table, first, last ) {
const cellsToMove = new Map();
const cellsToTrim = [];

for ( const { row, column, rowspan, cell } of new TableWalker( table, { endRow: last } ) ) {
const lastRowOfCell = row + rowspan - 1;

const isCellStickingOutFromRemovedRows = row >= first && row <= last && lastRowOfCell > last;

if ( isCellStickingOutFromRemovedRows ) {
const rowspanInRemovedSection = last - row + 1;
const rowSpanToSet = rowspan - rowspanInRemovedSection;

cellsToMove.set( column, {
cell,
rowspan: rowSpanToSet
} );
}

return first;
const isCellOverlappingRemovedRows = row < first && lastRowOfCell >= first;

if ( isCellOverlappingRemovedRows ) {
let rowspanAdjustment;

// Cell fully covers removed section - trim it by removed rows count.
if ( lastRowOfCell >= last ) {
rowspanAdjustment = last - first + 1;
}
// Cell partially overlaps removed section - calculate cell's span that is in removed section.
else {
rowspanAdjustment = lastRowOfCell - first + 1;
}

cellsToTrim.push( {
cell,
rowspan: rowspan - rowspanAdjustment
} );
}
}
return { cellsToMove, cellsToTrim };
}

// Updates heading columns attribute if removing a row from head section.
function adjustHeadingColumns( table, removedColumnIndexes, writer ) {
const headingColumns = table.getAttribute( 'headingColumns' ) || 0;
function moveCellsToRow( table, targetRowIndex, cellsToMove, writer ) {
const tableWalker = new TableWalker( table, {
includeSpanned: true,
startRow: targetRowIndex,
endRow: targetRowIndex
} );

if ( headingColumns && removedColumnIndexes.first < headingColumns ) {
const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) -
removedColumnIndexes.first + 1;
const tableRowMap = [ ...tableWalker ];
const row = table.getChild( targetRowIndex );

writer.setAttribute( 'headingColumns', headingColumns - headingsRemoved, table );
let previousCell;

for ( const { column, cell, isSpanned } of tableRowMap ) {
if ( cellsToMove.has( column ) ) {
const { cell: cellToMove, rowspan } = cellsToMove.get( column );

const targetPosition = previousCell ?
writer.createPositionAfter( previousCell ) :
writer.createPositionAt( row, 0 );

writer.move( writer.createRangeOn( cellToMove ), targetPosition );
updateNumericAttribute( 'rowspan', rowspan, cellToMove, writer );

previousCell = cellToMove;
} else if ( !isSpanned ) {
// If cell is spanned then `cell` holds reference to overlapping cell. See ckeditor/ckeditor5#6502.
previousCell = cell;
}
}
}
Loading

0 comments on commit c8d8d32

Please sign in to comment.