` 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/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..b1f0b6bc8d6
--- /dev/null
+++ b/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js
@@ -0,0 +1,54 @@
+/**
+ * @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 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, the 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/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/tableediting.js b/packages/ckeditor5-table/src/tableediting.js
index bdaf9fcda70..f46c8e27d36 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';
@@ -36,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';
@@ -113,9 +113,8 @@ 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() );
// Define all the commands.
editor.commands.add( 'insertTable', new InsertTableCommand( editor ) );
@@ -143,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 );
diff --git a/packages/ckeditor5-table/src/tableutils.js b/packages/ckeditor5-table/src/tableutils.js
index 47a8b2d1c48..5a9202d8cc4 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.
@@ -337,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 );
+ // 2d. Adjust heading rows if removed rows were in a heading section.
+ updateHeadingRows( table, first, last, writer );
- // 2e. Adjust heading rows if removed rows were in a heading section.
- updateHeadingRows( table, first, last, model, batch );
+ // 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 );
+ }
} );
}
@@ -396,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 );
+ }
} );
}
@@ -776,21 +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, 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 => {
- const headingRows = table.getAttribute( 'headingRows' ) || 0;
-
- if ( first < headingRows ) {
- const newRows = last < headingRows ? headingRows - ( last - first + 1 ) : first;
-
- updateNumericAttribute( 'headingRows', newRows, table, writer, 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;
+
+ updateNumericAttribute( 'headingRows', newRows, table, writer, 0 );
+ }
}
// Finds cells that will be:
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'
+ ]
+ ] ) );
+ } );
} );
} );
diff --git a/packages/ckeditor5-table/tests/converters/downcast.js b/packages/ckeditor5-table/tests/converters/downcast.js
index e138160f42b..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( [
@@ -941,36 +944,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 } ),
- '