From 60e4ca71dde99fc1b79504c8696908c00eacd04a Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 7 Jul 2020 16:44:55 +0200 Subject: [PATCH 1/7] Table headingRows attribute change triggers downcasting of the whole table. --- .../src/commands/removerowcommand.js | 10 +- .../src/converters/downcast.js | 100 ------------------ packages/ckeditor5-table/src/tableediting.js | 28 ++++- packages/ckeditor5-table/src/tableutils.js | 16 +-- .../tests/converters/downcast.js | 30 ------ 5 files changed, 31 insertions(+), 153 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/removerowcommand.js b/packages/ckeditor5-table/src/commands/removerowcommand.js index 7df37cdc74d..2bed009af8e 100644 --- a/packages/ckeditor5-table/src/commands/removerowcommand.js +++ b/packages/ckeditor5-table/src/commands/removerowcommand.js @@ -60,20 +60,14 @@ export default class RemoveRowCommand extends Command { const columnIndexToFocus = this.editor.plugins.get( 'TableUtils' ).getCellLocation( firstCell ).column; - // Use single batch to modify table in steps but in one undo step. - const batch = model.createBatch(); - - model.enqueueChange( batch, () => { + model.change( writer => { const rowsToRemove = removedRowIndexes.last - removedRowIndexes.first + 1; this.editor.plugins.get( 'TableUtils' ).removeRows( table, { at: removedRowIndexes.first, - rows: rowsToRemove, - batch + rows: rowsToRemove } ); - } ); - model.enqueueChange( batch, writer => { const cellToFocus = getCellToFocus( table, removedRowIndexes.first, columnIndexToFocus ); writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) ); diff --git a/packages/ckeditor5-table/src/converters/downcast.js b/packages/ckeditor5-table/src/converters/downcast.js index f5339aaa5cd..ffdf6e83a5e 100644 --- a/packages/ckeditor5-table/src/converters/downcast.js +++ b/packages/ckeditor5-table/src/converters/downcast.js @@ -173,79 +173,6 @@ export function downcastInsertCell() { } ); } -/** - * Conversion helper that acts on heading row table attribute change. - * - * This converter will: - * - * * Rename `` to `` elements or vice versa depending on headings. - * * Create `` or `` elements if needed. - * * Remove empty `` or `` if needed. - * - * @returns {Function} Conversion helper. - */ -export function downcastTableHeadingRowsChange() { - return dispatcher => dispatcher.on( 'attribute:headingRows:table', ( evt, data, conversionApi ) => { - const table = data.item; - - if ( !conversionApi.consumable.consume( data.item, evt.name ) ) { - return; - } - - const figureElement = conversionApi.mapper.toViewElement( table ); - const viewTable = getViewTable( figureElement ); - - const oldRows = data.attributeOldValue; - const newRows = data.attributeNewValue; - - // The head section has grown so move rows from to . - if ( newRows > oldRows ) { - // Filter out only those rows that are in wrong section. - const rowsToMove = Array.from( table.getChildren() ).filter( ( { index } ) => isBetween( index, oldRows - 1, newRows ) ); - - const viewTableHead = getOrCreateTableSection( 'thead', viewTable, conversionApi ); - moveViewRowsToTableSection( rowsToMove, viewTableHead, conversionApi, 'end' ); - - // Rename all table cells from moved rows to 'th' as they lands in . - for ( const tableRow of rowsToMove ) { - for ( const tableCell of tableRow.getChildren() ) { - renameViewTableCell( tableCell, 'th', conversionApi ); - } - } - } - // The head section has shrunk so move rows from to . - else { - // Filter out only those rows that are in wrong section. - const rowsToMove = Array.from( table.getChildren() ) - .filter( ( { index } ) => isBetween( index, newRows - 1, oldRows ) ) - .reverse(); // The rows will be moved from to in reverse order at the beginning of a . - - const viewTableBody = getOrCreateTableSection( 'tbody', viewTable, conversionApi ); - moveViewRowsToTableSection( rowsToMove, viewTableBody, conversionApi, 0 ); - - // Check if cells moved from to requires renaming to as this depends on current heading columns attribute. - const tableWalker = new TableWalker( table, { startRow: newRows ? newRows - 1 : newRows, endRow: oldRows - 1 } ); - - const tableAttributes = { - headingRows: table.getAttribute( 'headingRows' ) || 0, - headingColumns: table.getAttribute( 'headingColumns' ) || 0 - }; - - for ( const tableSlot of tableWalker ) { - renameViewTableCellIfRequired( tableSlot, tableAttributes, conversionApi ); - } - } - - // Cleanup: Ensure that thead & tbody sections are removed if left empty after moving rows. See #6437, #6391. - removeTableSectionIfEmpty( 'thead', viewTable, conversionApi ); - removeTableSectionIfEmpty( 'tbody', viewTable, conversionApi ); - - function isBetween( index, lower, upper ) { - return index > lower && index < upper; - } - } ); -} - /** * Conversion helper that acts on heading column table attribute change. * @@ -333,11 +260,6 @@ function renameViewTableCell( tableCell, desiredCellElementName, conversionApi ) const viewWriter = conversionApi.writer; const viewCell = conversionApi.mapper.toViewElement( tableCell ); - // View cell might be not yet converted - skip it as it will be properly created by cell converter later on. - if ( !viewCell ) { - return; - } - const editable = viewWriter.createEditableElement( desiredCellElementName, viewCell.getAttributes() ); const renamedCell = toWidgetEditable( editable, viewWriter ); @@ -545,28 +467,6 @@ function removeTableSectionIfEmpty( sectionName, tableElement, conversionApi ) { } } -// Moves view table rows associated with passed model rows to the provided table section element. -// -// **Note**: This method will skip not converted table rows. -// -// @param {Array.} rowsToMove -// @param {module:engine/view/element~Element} viewTableSection -// @param {module:engine/conversion/downcastdispatcher~DowncastConversionApi} conversionApi -// @param {Number|'end'|'before'|'after'} offset Offset or one of the flags. -function moveViewRowsToTableSection( rowsToMove, viewTableSection, conversionApi, offset ) { - for ( const tableRow of rowsToMove ) { - const viewTableRow = conversionApi.mapper.toViewElement( tableRow ); - - // View table row might be not yet converted - skip it as it will be properly created by cell converter later on. - if ( viewTableRow ) { - conversionApi.writer.move( - conversionApi.writer.createRangeOn( viewTableRow ), - conversionApi.writer.createPositionAt( viewTableSection, offset ) - ); - } - } -} - // Finds a '' element inside the `
` widget. // // @param {module:engine/view/element~Element} viewFigure diff --git a/packages/ckeditor5-table/src/tableediting.js b/packages/ckeditor5-table/src/tableediting.js index bdaf9fcda70..4fa0e0ed89b 100644 --- a/packages/ckeditor5-table/src/tableediting.js +++ b/packages/ckeditor5-table/src/tableediting.js @@ -15,8 +15,7 @@ import { downcastInsertRow, downcastInsertTable, downcastRemoveRow, - downcastTableHeadingColumnsChange, - downcastTableHeadingRowsChange + downcastTableHeadingColumnsChange } from './converters/downcast'; import InsertTableCommand from './commands/inserttablecommand'; @@ -113,9 +112,11 @@ export default class TableEditing extends Plugin { conversion.attributeToAttribute( { model: 'colspan', view: 'colspan' } ); conversion.attributeToAttribute( { model: 'rowspan', view: 'rowspan' } ); - // Table heading rows and columns conversion. + // Table heading columns conversion (change of heading rows requires reconversion of the whole table). conversion.for( 'editingDowncast' ).add( downcastTableHeadingColumnsChange() ); - conversion.for( 'editingDowncast' ).add( downcastTableHeadingRowsChange() ); + + // Table heading rows change requires reconversion of the whole table. + this.listenTo( model, 'applyOperation', headingRowsAttributeChangeHandler( model ) ); // Define all the commands. editor.commands.add( 'insertTable', new InsertTableCommand( editor ) ); @@ -155,3 +156,22 @@ export default class TableEditing extends Plugin { return [ TableUtils ]; } } + +// Model#applyOperation handler for headingRows attribute changes. +function headingRowsAttributeChangeHandler( model ) { + return ( event, [ operation ] ) => { + if ( !operation.isDocumentOperation ) { + return; + } + + if ( operation.type != 'addAttribute' && operation.type != 'removeAttribute' && operation.type != 'changeAttribute' ) { + return; + } + + const element = operation.range.getContainedElement(); + + if ( element && element.is( 'table' ) && operation.key == 'headingRows' ) { + model.document.differ.refreshItem( element ); + } + }; +} diff --git a/packages/ckeditor5-table/src/tableutils.js b/packages/ckeditor5-table/src/tableutils.js index 47a8b2d1c48..86413b72b8c 100644 --- a/packages/ckeditor5-table/src/tableutils.js +++ b/packages/ckeditor5-table/src/tableutils.js @@ -136,7 +136,7 @@ export default class TableUtils extends Plugin { // Inserting rows inside heading section requires to update `headingRows` attribute as the heading section will grow. if ( headingRows > insertAt ) { - writer.setAttribute( 'headingRows', headingRows + rowsToInsert, table ); + updateNumericAttribute( 'headingRows', headingRows + rowsToInsert, table, writer, 0 ); } // Inserting at the end or at the beginning of a table doesn't require to calculate anything special. @@ -309,9 +309,8 @@ export default class TableUtils extends Plugin { const rowsToRemove = options.rows || 1; const first = options.at; const last = first + rowsToRemove - 1; - const batch = options.batch || 'default'; - model.enqueueChange( batch, writer => { + model.change( 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. @@ -341,7 +340,7 @@ export default class TableUtils extends Plugin { removeEmptyColumns( table, this ); // 2e. Adjust heading rows if removed rows were in a heading section. - updateHeadingRows( table, first, last, model, batch ); + updateHeadingRows( table, first, last, model ); } ); } @@ -776,13 +775,8 @@ function adjustHeadingColumns( table, removedColumnIndexes, writer ) { } // Calculates a new heading rows value for removing rows from heading section. -function updateHeadingRows( table, first, last, model, batch ) { - // 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 => { +function updateHeadingRows( table, first, last, model ) { + model.change( writer => { const headingRows = table.getAttribute( 'headingRows' ) || 0; if ( first < headingRows ) { diff --git a/packages/ckeditor5-table/tests/converters/downcast.js b/packages/ckeditor5-table/tests/converters/downcast.js index e138160f42b..db5d41d6afa 100644 --- a/packages/ckeditor5-table/tests/converters/downcast.js +++ b/packages/ckeditor5-table/tests/converters/downcast.js @@ -941,36 +941,6 @@ describe( 'downcast converters', () => { ], { headingRows: 2, asWidget: true } ) ); } ); - it( 'should be possible to overwrite', () => { - editor.conversion.attributeToAttribute( { - model: 'headingRows', - view: 'headingRows', - converterPriority: 'high' - } ); - setModelData( model, modelTable( [ [ '00' ] ] ) ); - - const table = root.getChild( 0 ); - - model.change( writer => { - writer.setAttribute( 'headingRows', 1, table ); - } ); - - assertEqualMarkup( getViewData( view, { withoutSelection: true } ), - '
' + - '
' + - '
' + - '' + - '' + - '' + - '' + - '' + - '
' + - '00' + - '
' + - '' - ); - } ); - it( 'should work with adding table rows at the beginning of a table', () => { setModelData( model, modelTable( [ [ '00', '01' ], From e7492641bad8c9476cff8bdbbab33bbf0aeacf0a Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 7 Jul 2020 16:54:23 +0200 Subject: [PATCH 2/7] Added test for #7454 issue. --- packages/ckeditor5-table/tests/tableutils.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/ckeditor5-table/tests/tableutils.js b/packages/ckeditor5-table/tests/tableutils.js index ef6d498c9a0..cba404dacd1 100644 --- a/packages/ckeditor5-table/tests/tableutils.js +++ b/packages/ckeditor5-table/tests/tableutils.js @@ -959,6 +959,21 @@ describe( 'TableUtils', () => { ], { headingRows: 1 } ) ); } ); + it( 'should change heading rows if removing a heading row (and cell below is row-spanned)', () => { + setData( model, modelTable( [ + [ '00', '01' ], + [ '10', { contents: '11', rowspan: 2 } ], + [ '20' ] + ], { headingRows: 1 } ) ); + + tableUtils.removeRows( root.getChild( 0 ), { at: 0 } ); + + assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [ + [ '10', { contents: '11', rowspan: 2 } ], + [ '20' ] + ] ) ); + } ); + it( 'should decrease rowspan of table cells from previous rows', () => { // +----+----+----+----+----+ // | 00 | 01 | 02 | 03 | 04 | From 66ebfe46ff30baccb6933dc2666460750e1f1f81 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 7 Jul 2020 17:53:23 +0200 Subject: [PATCH 3/7] Fixed removing empty rows and columns when multiple rows and columns should be removed. --- .../src/commands/mergecellcommand.js | 2 +- .../src/commands/mergecellscommand.js | 2 +- .../ckeditor5-table/src/tableclipboard.js | 6 +-- packages/ckeditor5-table/src/tableutils.js | 18 +++++-- .../ckeditor5-table/src/utils/structure.js | 36 ++++++++------ .../tests/commands/mergecellscommand.js | 47 +++++++++++++++++++ 6 files changed, 85 insertions(+), 26 deletions(-) diff --git a/packages/ckeditor5-table/src/commands/mergecellcommand.js b/packages/ckeditor5-table/src/commands/mergecellcommand.js index 7a59f9af2ab..dbd90871f85 100644 --- a/packages/ckeditor5-table/src/commands/mergecellcommand.js +++ b/packages/ckeditor5-table/src/commands/mergecellcommand.js @@ -109,7 +109,7 @@ export default class MergeCellCommand extends Command { const table = findAncestor( 'table', removedTableCellRow ); // Remove empty rows and columns after merging. - removeEmptyRowsColumns( table, tableUtils, writer.batch ); + removeEmptyRowsColumns( table, tableUtils ); } ); } diff --git a/packages/ckeditor5-table/src/commands/mergecellscommand.js b/packages/ckeditor5-table/src/commands/mergecellscommand.js index f737e58d94b..3f278c34894 100644 --- a/packages/ckeditor5-table/src/commands/mergecellscommand.js +++ b/packages/ckeditor5-table/src/commands/mergecellscommand.js @@ -60,7 +60,7 @@ export default class MergeCellsCommand extends Command { const table = findAncestor( 'table', firstTableCell ); // Remove rows and columns that become empty (have no anchored cells). - removeEmptyRowsColumns( table, tableUtils, writer.batch ); + removeEmptyRowsColumns( table, tableUtils ); writer.setSelection( firstTableCell, 'in' ); } ); diff --git a/packages/ckeditor5-table/src/tableclipboard.js b/packages/ckeditor5-table/src/tableclipboard.js index 84ff5e5b65e..7bfe8a84b0c 100644 --- a/packages/ckeditor5-table/src/tableclipboard.js +++ b/packages/ckeditor5-table/src/tableclipboard.js @@ -205,7 +205,7 @@ function prepareTableForPasting( selectedTableCells, pastedDimensions, writer, t selection.lastRow += pastedDimensions.height - 1; selection.lastColumn += pastedDimensions.width - 1; - expandTableSize( selectedTable, selection.lastRow + 1, selection.lastColumn + 1, writer, tableUtils ); + expandTableSize( selectedTable, selection.lastRow + 1, selection.lastColumn + 1, tableUtils ); } // In case of expanding selection we do not reset the selection so in this case we will always try to fix selection @@ -320,13 +320,12 @@ function replaceSelectedCellsWithPasted( pastedTable, pastedDimensions, selected } // Expand table (in place) to expected size. -function expandTableSize( table, expectedHeight, expectedWidth, writer, tableUtils ) { +function expandTableSize( table, expectedHeight, expectedWidth, tableUtils ) { const tableWidth = tableUtils.getColumns( table ); const tableHeight = tableUtils.getRows( table ); if ( expectedWidth > tableWidth ) { tableUtils.insertColumns( table, { - batch: writer.batch, at: tableWidth, columns: expectedWidth - tableWidth } ); @@ -334,7 +333,6 @@ function expandTableSize( table, expectedHeight, expectedWidth, writer, tableUti if ( expectedHeight > tableHeight ) { tableUtils.insertRows( table, { - batch: writer.batch, at: tableHeight, rows: expectedHeight - tableHeight } ); diff --git a/packages/ckeditor5-table/src/tableutils.js b/packages/ckeditor5-table/src/tableutils.js index 86413b72b8c..fba6e2558e8 100644 --- a/packages/ckeditor5-table/src/tableutils.js +++ b/packages/ckeditor5-table/src/tableutils.js @@ -336,11 +336,15 @@ export default class TableUtils extends Plugin { updateNumericAttribute( 'rowspan', rowspan, cell, writer ); } - // 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. + // 2d. Adjust heading rows if removed rows were in a heading section. updateHeadingRows( table, first, last, model ); + + // 2e. Remove empty columns (without anchored cells) if there are any. + if ( !removeEmptyColumns( table, this ) ) { + // If there wasn't any empty columns then we still need to check if this wasn't called + // because of cleaning empty rows and we only removed one of them. + removeEmptyRows( table, this ); + } } ); } @@ -395,7 +399,11 @@ export default class TableUtils extends Plugin { } // Remove empty rows that could appear after removing columns. - removeEmptyRows( table, this, writer.batch ); + if ( !removeEmptyRows( table, this ) ) { + // If there wasn't any empty rows then we still need to check if this wasn't called + // because of cleaning empty columns and we only removed one of them. + removeEmptyColumns( table, this ); + } } ); } diff --git a/packages/ckeditor5-table/src/utils/structure.js b/packages/ckeditor5-table/src/utils/structure.js index 41fc71a496c..b8bb6f4023d 100644 --- a/packages/ckeditor5-table/src/utils/structure.js +++ b/packages/ckeditor5-table/src/utils/structure.js @@ -342,13 +342,17 @@ export function removeEmptyColumns( table, tableUtils ) { return cellsCount ? result : [ ...result, column ]; }, [] ); - // @if CK_DEBUG_TABLE // emptyColumns.length > 0 && console.log( `Removing empty columns: ${ emptyColumns.join( ', ' ) }.` ); + if ( emptyColumns.length > 0 ) { + // Remove only last empty column because it will recurrently trigger removing empty rows. + const emptyColumn = emptyColumns[ emptyColumns.length - 1 ]; - emptyColumns.reverse().forEach( column => { - tableUtils.removeColumns( table, { at: column } ); - } ); + // @if CK_DEBUG_TABLE // console.log( `Removing empty column: ${ emptyColumn }.` ); + tableUtils.removeColumns( table, { at: emptyColumn } ); - return emptyColumns.length > 0; + return true; + } + + return false; } /** @@ -380,10 +384,9 @@ export function removeEmptyColumns( table, tableUtils ) { * @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. * @returns {Boolean} True if removed some rows. */ -export function removeEmptyRows( table, tableUtils, batch ) { +export function removeEmptyRows( table, tableUtils ) { const emptyRows = []; for ( let rowIndex = 0; rowIndex < table.childCount; rowIndex++ ) { @@ -394,13 +397,17 @@ export function removeEmptyRows( table, tableUtils, batch ) { } } - // @if CK_DEBUG_TABLE // emptyRows.length > 0 && console.log( `Removing empty rows: ${ emptyRows.join( ', ' ) }.` ); + if ( emptyRows.length > 0 ) { + // Remove only last empty row because it will recurrently trigger removing empty columns. + const emptyRow = emptyRows[ emptyRows.length - 1 ]; - emptyRows.reverse().forEach( row => { - tableUtils.removeRows( table, { at: row, batch } ); - } ); + // @if CK_DEBUG_TABLE // console.log( `Removing empty row: ${ emptyRow }.` ); + tableUtils.removeRows( table, { at: emptyRow } ); + + return true; + } - return emptyRows.length > 0; + return false; } /** @@ -428,14 +435,13 @@ export function removeEmptyRows( table, tableUtils, batch ) { * @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 ) { +export function removeEmptyRowsColumns( table, tableUtils ) { const removedColumns = removeEmptyColumns( table, tableUtils ); // If there was some columns removed then cleaning empty rows was already triggered. if ( !removedColumns ) { - removeEmptyRows( table, tableUtils, batch ); + removeEmptyRows( table, tableUtils ); } } diff --git a/packages/ckeditor5-table/tests/commands/mergecellscommand.js b/packages/ckeditor5-table/tests/commands/mergecellscommand.js index 272e3b5b585..1ef468844ab 100644 --- a/packages/ckeditor5-table/tests/commands/mergecellscommand.js +++ b/packages/ckeditor5-table/tests/commands/mergecellscommand.js @@ -706,6 +706,53 @@ describe( 'MergeCellsCommand', () => { ] ] ) ); } ); + + it( 'should remove all empty rows and columns', () => { + setData( model, modelTable( [ + [ '00', '01', '02' ], + [ '10', '11', '12' ], + [ '20', '21', '22' ] + ] ) ); + + tableSelection.setCellSelection( + root.getNodeByPath( [ 0, 0, 0 ] ), + root.getNodeByPath( [ 0, 2, 2 ] ) + ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ + '[000102' + + '101112' + + '202122]' + ] + ] ) ); + } ); + + it( 'should remove all empty rows and columns (asymmetrical case)', () => { + setData( model, modelTable( [ + [ '00', '01', { contents: '02', rowspan: 3 } ], + [ '10', '11' ], + [ '20', '21' ] + ] ) ); + + tableSelection.setCellSelection( + root.getNodeByPath( [ 0, 0, 0 ] ), + root.getNodeByPath( [ 0, 2, 1 ] ) + ); + + command.execute(); + + assertEqualMarkup( getData( model ), modelTable( [ + [ + '[0001' + + '1011' + + '2021]', + '02' + ] + ] ) ); + } ); } ); } ); From 2eb2a1114bf6cd80c5cdb5e9eee024bcd4ae5b0b Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 8 Jul 2020 10:40:36 +0200 Subject: [PATCH 4/7] Removed redundant model.change block. --- packages/ckeditor5-table/src/tableutils.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/ckeditor5-table/src/tableutils.js b/packages/ckeditor5-table/src/tableutils.js index fba6e2558e8..5a9202d8cc4 100644 --- a/packages/ckeditor5-table/src/tableutils.js +++ b/packages/ckeditor5-table/src/tableutils.js @@ -337,7 +337,7 @@ export default class TableUtils extends Plugin { } // 2d. Adjust heading rows if removed rows were in a heading section. - updateHeadingRows( table, first, last, model ); + updateHeadingRows( table, first, last, writer ); // 2e. Remove empty columns (without anchored cells) if there are any. if ( !removeEmptyColumns( table, this ) ) { @@ -783,16 +783,14 @@ function adjustHeadingColumns( table, removedColumnIndexes, writer ) { } // Calculates a new heading rows value for removing rows from heading section. -function updateHeadingRows( table, first, last, model ) { - model.change( writer => { - const headingRows = table.getAttribute( 'headingRows' ) || 0; +function updateHeadingRows( table, first, last, 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; - updateNumericAttribute( 'headingRows', newRows, table, writer, 0 ); - } - } ); + updateNumericAttribute( 'headingRows', newRows, table, writer, 0 ); + } } // Finds cells that will be: From 17a7242fb920269116c5f7e56470ac66b0481bd4 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 8 Jul 2020 12:15:35 +0200 Subject: [PATCH 5/7] Added test for handling headingRows attribute in undo. --- .../tests/converters/downcast.js | 35 +++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-table/tests/converters/downcast.js b/packages/ckeditor5-table/tests/converters/downcast.js index db5d41d6afa..0243a76076b 100644 --- a/packages/ckeditor5-table/tests/converters/downcast.js +++ b/packages/ckeditor5-table/tests/converters/downcast.js @@ -5,6 +5,7 @@ import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; +import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; @@ -19,7 +20,7 @@ describe( 'downcast converters', () => { testUtils.createSinonSandbox(); beforeEach( async () => { - editor = await VirtualTestEditor.create( { plugins: [ Paragraph, TableEditing ] } ); + editor = await VirtualTestEditor.create( { plugins: [ Paragraph, TableEditing, UndoEditing ] } ); model = editor.model; root = model.document.getRoot( 'main' ); @@ -840,8 +841,10 @@ describe( 'downcast converters', () => { } ); } ); - describe( 'downcastTableHeadingRowsChange()', () => { + describe( 'downcastTableHeadingRowsChange', () => { // The heading rows change downcast conversion is not executed in data pipeline. + // Note that headingRows table attribute triggers whole table downcast. + describe( 'editing pipeline', () => { it( 'should work for adding heading rows', () => { setModelData( model, modelTable( [ @@ -1017,6 +1020,34 @@ describe( 'downcast converters', () => { '' ); } ); + + it( 'should properly integrate with undo', () => { + setModelData( model, modelTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ], { headingRows: 1 } ) ); + + const table = root.getChild( 0 ); + + model.change( writer => { + writer.setAttribute( 'headingRows', 2, table ); + } ); + + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ], { headingRows: 2, asWidget: true } ) ); + + editor.execute( 'undo' ); + + assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [ + [ '00', '01' ], + [ '10', '11' ], + [ '20', '21' ] + ], { headingRows: 1, asWidget: true } ) ); + } ); } ); } ); From 70b5134324f8682c15dd90140bfa7081b087b000 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Wed, 8 Jul 2020 14:13:07 +0200 Subject: [PATCH 6/7] Added table post-fixer that marks table to refresh on headingRows attribute change. --- .../table-heading-rows-refresh-post-fixer.js | 53 +++++++++++++++++++ packages/ckeditor5-table/src/tableediting.js | 24 +-------- 2 files changed, 55 insertions(+), 22 deletions(-) create mode 100644 packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js diff --git a/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js b/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js new file mode 100644 index 00000000000..77be279de6d --- /dev/null +++ b/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js @@ -0,0 +1,53 @@ +/** + * @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/converters/table-heading-rows-refresh-post-fixer + */ + +/** + * Injects a table post-fixer into the model which marks the table in the differ to have it re-rendered. + * + * Table heading rows are represented in model just by `headingRows` attribute but in the view it's rendered as separate + * sections of the table (`thead`, `tbody`) so any change of that attribute would trigger a lot changes in view. + * + * When table `headingRows` attribute changes, entire table is re-rendered. + * + * @param {module:engine/model/model~Model} model + */ +export default function injectTableHeadingRowsRefreshPostFixer( model ) { + model.document.registerPostFixer( () => tableHeadingRowsRefreshPostFixer( model ) ); +} + +function tableHeadingRowsRefreshPostFixer( model ) { + const differ = model.document.differ; + + // Stores tables to be refreshed so the table will be refreshed once for multiple changes. + const tablesToRefresh = new Set(); + + for ( const change of differ.getChanges() ) { + if ( change.type != 'attribute' ) { + continue; + } + + const element = change.range.start.nodeAfter; + + if ( element && element.is( 'table' ) && change.attributeKey == 'headingRows' ) { + tablesToRefresh.add( element ); + } + } + + if ( tablesToRefresh.size ) { + // @if CK_DEBUG_TABLE // console.log( `Post-fixing table: refreshing heading rows (${ tablesToRefresh.size }).` ); + + for ( const table of tablesToRefresh.values() ) { + differ.refreshItem( table ); + } + + return true; + } + + return false; +} diff --git a/packages/ckeditor5-table/src/tableediting.js b/packages/ckeditor5-table/src/tableediting.js index 4fa0e0ed89b..f46c8e27d36 100644 --- a/packages/ckeditor5-table/src/tableediting.js +++ b/packages/ckeditor5-table/src/tableediting.js @@ -35,6 +35,7 @@ import TableUtils from '../src/tableutils'; import injectTableLayoutPostFixer from './converters/table-layout-post-fixer'; import injectTableCellParagraphPostFixer from './converters/table-cell-paragraph-post-fixer'; import injectTableCellRefreshPostFixer from './converters/table-cell-refresh-post-fixer'; +import injectTableHeadingRowsRefreshPostFixer from './converters/table-heading-rows-refresh-post-fixer'; import '../theme/tableediting.css'; @@ -115,9 +116,6 @@ export default class TableEditing extends Plugin { // Table heading columns conversion (change of heading rows requires reconversion of the whole table). conversion.for( 'editingDowncast' ).add( downcastTableHeadingColumnsChange() ); - // Table heading rows change requires reconversion of the whole table. - this.listenTo( model, 'applyOperation', headingRowsAttributeChangeHandler( model ) ); - // Define all the commands. editor.commands.add( 'insertTable', new InsertTableCommand( editor ) ); editor.commands.add( 'insertTableRowAbove', new InsertRowCommand( editor, { order: 'above' } ) ); @@ -144,6 +142,7 @@ export default class TableEditing extends Plugin { editor.commands.add( 'selectTableRow', new SelectRowCommand( editor ) ); editor.commands.add( 'selectTableColumn', new SelectColumnCommand( editor ) ); + injectTableHeadingRowsRefreshPostFixer( model ); injectTableLayoutPostFixer( model ); injectTableCellRefreshPostFixer( model ); injectTableCellParagraphPostFixer( model ); @@ -156,22 +155,3 @@ export default class TableEditing extends Plugin { return [ TableUtils ]; } } - -// Model#applyOperation handler for headingRows attribute changes. -function headingRowsAttributeChangeHandler( model ) { - return ( event, [ operation ] ) => { - if ( !operation.isDocumentOperation ) { - return; - } - - if ( operation.type != 'addAttribute' && operation.type != 'removeAttribute' && operation.type != 'changeAttribute' ) { - return; - } - - const element = operation.range.getContainedElement(); - - if ( element && element.is( 'table' ) && operation.key == 'headingRows' ) { - model.document.differ.refreshItem( element ); - } - }; -} From 13c9fd8122cea943656849cecbdf7ff93a09e422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Fri, 10 Jul 2020 10:09:45 +0200 Subject: [PATCH 7/7] Update injectTableHeadingRowsRefreshPostFixer() documentation. --- .../converters/table-heading-rows-refresh-post-fixer.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js b/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js index 77be279de6d..b1f0b6bc8d6 100644 --- a/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js +++ b/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js @@ -10,10 +10,11 @@ /** * Injects a table post-fixer into the model which marks the table in the differ to have it re-rendered. * - * Table heading rows are represented in model just by `headingRows` attribute but in the view it's rendered as separate - * sections of the table (`thead`, `tbody`) so any change of that attribute would trigger a lot changes in view. + * Table heading rows are represented in the model by a `headingRows` attribute. However, in the view, it's represented as separate + * sections of the table (`` or ``) and changing `headingRows` attribute requires moving table rows between two sections. + * This causes problems with structural changes in a table (like adding and removing rows) thus atomic converters cannot be used. * - * When table `headingRows` attribute changes, entire table is re-rendered. + * When table `headingRows` attribute changes, the entire table is re-rendered. * * @param {module:engine/model/model~Model} model */