Skip to content

Commit

Permalink
Merge pull request #7362 from ckeditor/i/6609
Browse files Browse the repository at this point in the history 
Fix (table): Removing empty rows will no longer produce an invalid table model in certain scenarios. Closes #6609.
  • Loading branch information
@jodator
jodator authored Jun 4, 2020
2 parents a87b364 + fa54c0c commit 11d69fc
Show file tree
Hide file tree
Showing 9 changed files with 298 additions and 64 deletions.
11 changes: 5 additions & 6 deletions packages/ckeditor5-table/src/commands/mergecellcommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command';
import TableWalker from '../tablewalker';
import { getTableCellsContainingSelection } from '../utils/selection';
import { findAncestor, isHeadingColumnCell } from '../utils/common';
import { removeEmptyRowsColumns } from '../utils/structure';

/**
* The merge cell command.
Expand Down Expand Up @@ -104,13 +105,11 @@ export default class MergeCellCommand extends Command {
writer.setAttribute( spanAttribute, cellSpan + cellToMergeSpan, cellToExpand );
writer.setSelection( writer.createRangeIn( cellToExpand ) );

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

tableUtils.removeRows( table, { at: removedTableCellRow.index, batch: writer.batch } );
}
// Remove empty rows and columns after merging.
removeEmptyRowsColumns( table, tableUtils, writer.batch );
} );
}

Expand Down
16 changes: 4 additions & 12 deletions packages/ckeditor5-table/src/commands/mergecellscommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command';
import TableUtils from '../tableutils';
import { getSelectedTableCells, isSelectionRectangular } from '../utils/selection';
import { findAncestor, updateNumericAttribute } from '../utils/common';
import { removeEmptyRowsColumns } from '../utils/structure';

/**
* The merge cells command.
Expand Down Expand Up @@ -57,23 +58,14 @@ 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 );

if ( !tableRow.childCount ) {
emptyRowsIndexes.push( tableRow.index );
}
}

if ( emptyRowsIndexes.length ) {
const table = findAncestor( 'table', firstTableCell );
const table = findAncestor( 'table', firstTableCell );

emptyRowsIndexes.reverse().forEach( row => tableUtils.removeRows( table, { at: row, batch: writer.batch } ) );
}
// Remove rows and columns that become empty (have no anchored cells).
removeEmptyRowsColumns( table, tableUtils, writer.batch );

writer.setSelection( firstTableCell, 'in' );
} );
Expand Down
19 changes: 7 additions & 12 deletions packages/ckeditor5-table/src/tableutils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin';

import TableWalker from './tablewalker';
import { createEmptyTableCell, updateNumericAttribute } from './utils/common';
import { removeEmptyColumns, removeEmptyRows } from './utils/structure';

/**
* The table utilities plugin.
Expand Down Expand Up @@ -336,7 +337,10 @@ export default class TableUtils extends Plugin {
updateNumericAttribute( 'rowspan', rowspan, cell, writer );
}

// 2d. Adjust heading rows if removed rows were in a heading section.
// 2d. Remove empty columns (without anchored cells) if there are any.
removeEmptyColumns( table, this );

// 2e. Adjust heading rows if removed rows were in a heading section.
updateHeadingRows( table, first, last, model, batch );
} );
}
Expand Down Expand Up @@ -379,29 +383,20 @@ export default class TableUtils extends Plugin {
model.change( writer => {
adjustHeadingColumns( table, { first, last }, writer );

const emptyRowsIndexes = [];

for ( let removedColumnIndex = last; removedColumnIndex >= first; removedColumnIndex-- ) {
for ( const { cell, column, cellWidth } of [ ...new TableWalker( table ) ] ) {
// If colspaned cell overlaps removed column decrease its span.
if ( column <= removedColumnIndex && cellWidth > 1 && column + cellWidth > removedColumnIndex ) {
updateNumericAttribute( 'colspan', cellWidth - 1, cell, writer );
} else if ( column === removedColumnIndex ) {
const cellRow = cell.parent;

// The cell in removed column has colspan of 1.
writer.remove( cell );

// If the cell was the last one in the row, get rid of the entire row.
// https://github.com/ckeditor/ckeditor5/issues/6429
if ( !cellRow.childCount ) {
emptyRowsIndexes.push( cellRow.index );
}
}
}
}

emptyRowsIndexes.reverse().forEach( row => this.removeRows( table, { at: row, batch: writer.batch } ) );
// Remove empty rows that could appear after removing columns.
removeEmptyRows( table, this, writer.batch );
} );
}

Expand Down
133 changes: 133 additions & 0 deletions packages/ckeditor5-table/src/utils/structure.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,3 +305,136 @@ function addHeadingsToCroppedTable( croppedTable, sourceTable, startRow, startCo
updateNumericAttribute( 'headingColumns', headingColumnsInCrop, croppedTable, writer, 0 );
}
}

/**
* Removes columns that have no cells anchored.
*
* In table below:
*
* +----+----+----+----+----+----+----+
* | 00 | 01 | 03 | 04 | 06 |
* +----+----+----+----+ +----+
* | 10 | 11 | 13 | | 16 |
* +----+----+----+----+----+----+----+
* | 20 | 21 | 23 | 24 | 26 |
* +----+----+----+----+----+----+----+
* ^--- empty ---^
*
* Will remove columns 2 and 5.
*
* **Note:** This is a low-level helper method for clearing invalid model state when doing table modifications.
* To remove a column from a table use {@link module:table/tableutils~TableUtils#removeColumns `TableUtils.removeColumns()`}.
*
* @protected
* @param {module:engine/model/element~Element} table
* @param {module:table/tableutils~TableUtils} tableUtils
* @return {Boolean} True if removed some columns.
*/
export function removeEmptyColumns( table, tableUtils ) {
const width = tableUtils.getColumns( table );
const columnsMap = new Array( width ).fill( 0 );

for ( const { column } of new TableWalker( table ) ) {
columnsMap[ column ]++;
}

const emptyColumns = columnsMap.reduce( ( result, cellsCount, column ) => {
return cellsCount ? result : [ ...result, column ];
}, [] );

// @if CK_DEBUG_TABLE // emptyColumns.length > 0 && console.log( `Removing empty columns: ${ emptyColumns.join( ', ' ) }.` );

emptyColumns.reverse().forEach( column => {
tableUtils.removeColumns( table, { at: column } );
} );

return emptyColumns.length > 0;
}

/**
* Removes rows that have no cells anchored.
*
* In table below:
*
* +----+----+----+
* | 00 | 01 | 02 |
* +----+----+----+
* | 10 | 11 | 12 |
* + + + +
* | | | | <-- empty
* +----+----+----+
* | 30 | 31 | 32 |
* +----+----+----+
* | 40 | 42 |
* + + +
* | | | <-- empty
* +----+----+----+
* | 60 | 61 | 62 |
* +----+----+----+
*
* Will remove rows 2 and 5.
*
* **Note:** This is a low-level helper method for clearing invalid model state when doing table modifications.
* To remove a row from a table use {@link module:table/tableutils~TableUtils#removeRows `TableUtils.removeRows()`}.
*
* @protected
* @param {module:engine/model/element~Element} table
* @param {module:table/tableutils~TableUtils} tableUtils
* @param {module:engine/model/batch~Batch|null} [batch] Batch that should be used for removing empty rows.
* @return {Boolean} True if removed some rows.
*/
export function removeEmptyRows( table, tableUtils, batch ) {
const emptyRows = [];

for ( let rowIndex = 0; rowIndex < table.childCount; rowIndex++ ) {
const tableRow = table.getChild( rowIndex );

if ( tableRow.isEmpty ) {
emptyRows.push( rowIndex );
}
}

// @if CK_DEBUG_TABLE // emptyRows.length > 0 && console.log( `Removing empty rows: ${ emptyRows.join( ', ' ) }.` );

emptyRows.reverse().forEach( row => {
tableUtils.removeRows( table, { at: row, batch } );
} );

return emptyRows.length > 0;
}

/**
* Removes rows and columns that have no cells anchored.
*
* In table below:
*
* +----+----+----+----+
* | 00 | 02 |
* +----+----+ +
* | 10 | |
* +----+----+----+----+
* | 20 | 22 | 23 |
* + + + +
* | | | | <-- empty row
* +----+----+----+----+
* ^--- empty column
*
* Will remove row 3 and column 1.
*
* **Note:** This is a low-level helper method for clearing invalid model state when doing table modifications.
* To remove a rows from a table use {@link module:table/tableutils~TableUtils#removeRows `TableUtils.removeRows()`} and
* {@link module:table/tableutils~TableUtils#removeColumns `TableUtils.removeColumns()`} to remove a column.
*
* @protected
* @param {module:engine/model/element~Element} table
* @param {module:table/tableutils~TableUtils} tableUtils
* @param {module:engine/model/batch~Batch|null} [batch] Batch that should be used for removing empty rows.
*/
export function removeEmptyRowsColumns( table, tableUtils, batch ) {
const removedColumns = removeEmptyColumns( table, tableUtils );

// If there was some columns removed then cleaning empty rows was already triggered.
if ( !removedColumns ) {
removeEmptyRows( table, tableUtils, batch );
}
}
Loading

0 comments on commit 11d69fc

Please sign in to comment.