Skip to content

Commit

Permalink
Always reconvert table on table rows change.
Browse files Browse the repository at this point in the history
  • Loading branch information
@jodator
jodator committed Oct 15, 2020
1 parent 77c8115 commit f6032d5
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 171 deletions.
4 changes: 3 additions & 1 deletion packages/ckeditor5-engine/src/conversion/downcastdispatcher.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,9 @@ export default class DowncastDispatcher {
const itemsToReconvert = new Set();
const updated = [];

for ( const entry of differ.getChanges() ) {
const changes = [ ...differ.getChanges() ];

for ( const entry of changes ) {
const position = entry.position || entry.range.start;
// Cached parent - just in case. See https://github.com/ckeditor/ckeditor5/issues/6579.
const positionParent = position.parent;
Expand Down
22 changes: 5 additions & 17 deletions packages/ckeditor5-engine/src/conversion/downcasthelpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,25 +545,13 @@ export function insertText() {
*/
export function remove() {
return ( evt, data, conversionApi ) => {
if ( !conversionApi.consumable.consume( data.item, 'remove' ) ) {
return;
}

let viewRange;

const viewItem = conversionApi.mapper.toViewElement( data.item );
// Find view range start position by mapping model position at which the remove happened.
const viewStart = conversionApi.mapper.toViewPosition( data.position );

if ( !viewItem ) {
// Find view range start position by mapping model position at which the remove happened.
const viewStart = conversionApi.mapper.toViewPosition( data.position );
const modelEnd = data.position.getShiftedBy( data.length );
const viewEnd = conversionApi.mapper.toViewPosition( modelEnd, { isPhantom: true } );

const modelEnd = data.position.getShiftedBy( data.length );
const viewEnd = conversionApi.mapper.toViewPosition( modelEnd, { isPhantom: true } );

viewRange = conversionApi.writer.createRange( viewStart, viewEnd );
} else {
viewRange = conversionApi.writer.createRangeOn( viewItem );
}
const viewRange = conversionApi.writer.createRange( viewStart, viewEnd );

// Trim the range to remove in case some UI elements are on the view range boundaries.
const removed = conversionApi.writer.remove( viewRange.getTrimmed() );
Expand Down
148 changes: 5 additions & 143 deletions packages/ckeditor5-table/src/converters/downcast.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,97 +37,25 @@ export function downcastInsertTable( options = {} ) {
const tableElement = conversionApi.writer.createContainerElement( 'table' );
conversionApi.writer.insert( conversionApi.writer.createPositionAt( figureElement, 0 ), tableElement );

let tableWidget;

if ( asWidget ) {
tableWidget = toTableWidget( figureElement, conversionApi.writer );
toTableWidget( figureElement, conversionApi.writer );
}

const tableWalker = new TableWalker( table );

const tableAttributes = {
headingRows: table.getAttribute( 'headingRows' ) || 0,
headingColumns: table.getAttribute( 'headingColumns' ) || 0
};

// Cache for created table rows.
const viewRows = new Map();

for ( const tableSlot of tableWalker ) {
const { row, cell } = tableSlot;

const tableRow = table.getChild( row );
const trElement = viewRows.get( row ) || createTr( tableElement, tableRow, row, tableAttributes, conversionApi );
viewRows.set( row, trElement );

// Consume table cell - it will be always consumed as we convert whole table at once.
conversionApi.consumable.consume( cell, 'insert' );

const insertPosition = conversionApi.writer.createPositionAt( trElement, 'end' );

createViewTableCellElement( tableSlot, tableAttributes, insertPosition, conversionApi, options );
}

// Insert empty TR elements if there are any rows without anchored cells. Since the model is always normalized
// this can happen only in the document fragment that only part of the table is down-casted.
for ( const tableRow of table.getChildren() ) {
const rowIndex = tableRow.index;

if ( !viewRows.has( rowIndex ) ) {
viewRows.set( rowIndex, createTr( tableElement, tableRow, rowIndex, tableAttributes, conversionApi ) );
}
createTr( tableElement, tableRow, tableRow.index, tableAttributes, conversionApi );
}

return asWidget ? tableWidget : figureElement;
return figureElement;
};
}

/**
* Model row element to view `<tr>` element conversion helper.
*
* This conversion helper creates the whole `<tr>` element with child elements.
*
* @returns {Function} Conversion helper.
*/
export function downcastInsertRow() {
return dispatcher => dispatcher.on( 'insert:tableRow', ( evt, data, conversionApi ) => {
const tableRow = data.item;

if ( !conversionApi.consumable.consume( tableRow, 'insert' ) ) {
return;
}

const table = tableRow.parent;

const figureElement = conversionApi.mapper.toViewElement( table );
const tableElement = getViewTable( figureElement );

const row = table.getChildIndex( tableRow );

const tableWalker = new TableWalker( table, { row } );

const tableAttributes = {
headingRows: table.getAttribute( 'headingRows' ) || 0,
headingColumns: table.getAttribute( 'headingColumns' ) || 0
};

// Cache for created table rows.
const viewRows = new Map();

for ( const tableSlot of tableWalker ) {
const trElement = viewRows.get( row ) || createTr( tableElement, tableRow, row, tableAttributes, conversionApi );
viewRows.set( row, trElement );

// Consume table cell - it will be always consumed as we convert whole row at once.
conversionApi.consumable.consume( tableSlot.cell, 'insert' );

const insertPosition = conversionApi.writer.createPositionAt( trElement, 'end' );

createViewTableCellElement( tableSlot, tableAttributes, insertPosition, conversionApi, { asWidget: true } );
}
} );
}

/**
* Model table cell element to view `<td>` or `<th>` element conversion helper.
*
Expand All @@ -136,7 +64,7 @@ export function downcastInsertRow() {
*
* @returns {Function} Conversion helper.
*/
export function downcastInsertCell() {
export function downcastInsertCell( options ) {
return dispatcher => dispatcher.on( 'insert:tableCell', ( evt, data, conversionApi ) => {
const tableCell = data.item;

Expand All @@ -161,7 +89,7 @@ export function downcastInsertCell() {
const trElement = conversionApi.mapper.toViewElement( tableRow );
const insertPosition = conversionApi.writer.createPositionAt( trElement, tableRow.getChildIndex( tableCell ) );

createViewTableCellElement( tableSlot, tableAttributes, insertPosition, conversionApi, { asWidget: true } );
createViewTableCellElement( tableSlot, tableAttributes, insertPosition, conversionApi, options );

// No need to iterate further.
return;
Expand Down Expand Up @@ -201,48 +129,6 @@ export function downcastTableHeadingColumnsChange() {
} );
}

/**
* Conversion helper that acts on a removed row.
*
* @returns {Function} Conversion helper.
*/
export function downcastRemoveRow() {
return dispatcher => dispatcher.on( 'remove:tableRow', ( evt, data, conversionApi ) => {
// Prevent default remove converter.
// evt.stop();

const removedRow = data.item;

const removedView = conversionApi.mapper.toViewElement( removedRow );

if ( !removedView ) {
return;
}

conversionApi.consumable.consume( removedRow, 'remove' );

const viewWriter = conversionApi.writer;
const mapper = conversionApi.mapper;

// const viewStart = mapper.toViewPosition( data.position ).getLastMatchingPosition( value => !value.item.is( 'element', 'tr' ) );
const viewItem = removedView;
const tableSection = viewItem.parent;
const viewTable = tableSection.parent;

// Remove associated <tr> from the view.
const removeRange = viewWriter.createRangeOn( viewItem );
const removed = viewWriter.remove( removeRange );

for ( const child of viewWriter.createRangeIn( removed ).getItems() ) {
mapper.unbindViewElement( child );
}

// Cleanup: Ensure that thead & tbody sections are removed if left empty after removing rows. See #6437, #6391.
removeTableSectionIfEmpty( 'thead', viewTable, conversionApi );
removeTableSectionIfEmpty( 'tbody', viewTable, conversionApi );
}, { priority: 'higher' } );
}

/**
* Overrides paragraph inside table cell conversion.
*
Expand Down Expand Up @@ -498,30 +384,6 @@ function createTableSection( sectionName, tableElement, conversionApi ) {
return tableChildElement;
}

// Removes an existing `<tbody>` or `<thead>` element if it is empty.
//
// @param {String} sectionName
// @param {module:engine/view/element~Element} tableElement
// @param {module:engine/conversion/downcastdispatcher~DowncastConversionApi} conversionApi
function removeTableSectionIfEmpty( sectionName, tableElement, conversionApi ) {
const tableSection = getExistingTableSectionElement( sectionName, tableElement );

if ( tableSection && tableSection.childCount === 0 ) {
conversionApi.writer.remove( conversionApi.writer.createRangeOn( tableSection ) );
}
}

// Finds a '<table>' element inside the `<figure>` widget.
//
// @param {module:engine/view/element~Element} viewFigure
function getViewTable( viewFigure ) {
for ( const child of viewFigure.getChildren() ) {
if ( child.name === 'table' ) {
return child;
}
}
}

// Checks if an element has any attributes set.
//
// @param {module:engine/model/element~Element element
Expand Down
12 changes: 6 additions & 6 deletions packages/ckeditor5-table/src/tableediting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ import upcastTable, { skipEmptyTableRow } from './converters/upcasttable';
import {
convertParagraphInTableCell,
downcastInsertCell,
downcastInsertRow,
downcastInsertTable,
downcastRemoveRow,
downcastTableHeadingColumnsChange
} from './converters/downcast';

Expand Down Expand Up @@ -97,7 +95,8 @@ export default class TableEditing extends Plugin {
model: 'table',
view: downcastInsertTable( { asWidget: true } ),
triggerBy: {
attributes: [ 'headingRows' ]
attributes: [ 'headingRows' ],
children: [ 'tableRow' ]
}
} );
conversion.for( 'dataDowncast' ).elementToElement( {
Expand All @@ -109,14 +108,15 @@ export default class TableEditing extends Plugin {
conversion.for( 'upcast' ).elementToElement( { model: 'tableRow', view: 'tr' } );
conversion.for( 'upcast' ).add( skipEmptyTableRow() );

conversion.for( 'editingDowncast' ).add( downcastInsertRow() );
conversion.for( 'editingDowncast' ).add( downcastRemoveRow() );
// conversion.for( 'editingDowncast' ).add( downcastInsertRow() );
// conversion.for( 'editingDowncast' ).add( downcastRemoveRow() );

// Table cell conversion.
conversion.for( 'upcast' ).elementToElement( { model: 'tableCell', view: 'td' } );
conversion.for( 'upcast' ).elementToElement( { model: 'tableCell', view: 'th' } );

conversion.for( 'editingDowncast' ).add( downcastInsertCell() );
conversion.for( 'dataDowncast' ).add( downcastInsertCell() );
conversion.for( 'editingDowncast' ).add( downcastInsertCell( { asWidget: true } ) );

// Duplicates code - needed to properly refresh paragraph inside table cell.
editor.conversion.for( 'editingDowncast' ).elementToElement( {
Expand Down
9 changes: 5 additions & 4 deletions packages/ckeditor5-table/tests/converters/downcast.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ describe( 'downcast converters', () => {
);
} );

it( 'should re-create table on reinsert', () => {
it( 'should re-create table on move', () => {
model.schema.register( 'wrapper', {
allowWhere: '$block',
allowContentOf: '$root'
Expand All @@ -205,7 +205,7 @@ describe( 'downcast converters', () => {

model.change( writer => {
const table = model.document.getRoot().getChild( 0 );
const range = writer.createRange( writer.createPositionBefore( table ), writer.createPositionAfter( table ) );
const range = writer.createRangeOn( table );
const wrapper = writer.createElement( 'wrapper' );

writer.wrap( range, wrapper );
Expand Down Expand Up @@ -401,7 +401,7 @@ describe( 'downcast converters', () => {
], { asWidget: true } ) );
} );

it( 'should insert row on proper index when table has heading rows defined - insert in body', () => {
it( 'should insert row at a proper index when table has heading rows defined - insert in body', () => {
setModelData( model, modelTable( [
[ '00', '01' ],
[ '21', '22' ],
Expand All @@ -427,7 +427,7 @@ describe( 'downcast converters', () => {
], { headingRows: 1, asWidget: true } ) );
} );

it( 'should insert row on proper index when table has heading rows defined - insert in heading', () => {
it( 'should insert row at a proper index when table has heading rows defined - insert in heading', () => {
setModelData( model, modelTable( [
[ '00', '01' ],
[ '21', '22' ],
Expand All @@ -440,6 +440,7 @@ describe( 'downcast converters', () => {
const row = writer.createElement( 'tableRow' );

writer.insert( row, table, 1 );
writer.setAttribute( 'headingRows', 3, table );

writer.insertElement( 'tableCell', row, 'end' );
writer.insertElement( 'tableCell', row, 'end' );
Expand Down
Loading

0 comments on commit f6032d5

Please sign in to comment.