Skip to content

Commit

Permalink
Merge pull request #7451 from ckeditor/i/7245
Browse files Browse the repository at this point in the history 
Fix (table): Copied and pasted table fragment should maintain the proper structure when the fragment contains merged table cells. Closes #7245.
  • Loading branch information
@jodator
jodator authored Jun 18, 2020
2 parents 9700131 + 1feadb9 commit 17d7bd7
Show file tree
Hide file tree
Showing 8 changed files with 313 additions and 100 deletions.
36 changes: 24 additions & 12 deletions packages/ckeditor5-table/src/converters/downcast.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,8 @@ export function downcastInsertTable( options = {} ) {
for ( const tableSlot of tableWalker ) {
const { row, cell } = tableSlot;

const tableSection = getOrCreateTableSection( getSectionName( row, tableAttributes ), tableElement, conversionApi );
const tableRow = table.getChild( row );

const trElement = viewRows.get( row ) || createTr( tableRow, row, tableSection, conversionApi );
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.
Expand All @@ -70,6 +68,16 @@ export function downcastInsertTable( options = {} ) {
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 ) );
}
}

const viewPosition = conversionApi.mapper.toViewPosition( data.range.start );

conversionApi.mapper.bindElements( table, asWidget ? tableWidget : figureElement );
Expand Down Expand Up @@ -110,9 +118,7 @@ export function downcastInsertRow() {
const viewRows = new Map();

for ( const tableSlot of tableWalker ) {
const tableSection = getOrCreateTableSection( getSectionName( row, tableAttributes ), tableElement, conversionApi );

const trElement = viewRows.get( row ) || createTr( tableRow, row, tableSection, conversionApi );
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.
Expand Down Expand Up @@ -425,22 +431,28 @@ function createViewTableCellElement( tableSlot, tableAttributes, insertPosition,

// Creates a `<tr>` view element.
//
// @param {module:engine/view/element~Element} tableRow
// @param {module:engine/view/element~Element} tableElement
// @param {module:engine/model/element~Element} tableRow
// @param {Number} rowIndex
// @param {module:engine/view/element~Element} tableSection
// @param {{headingColumns, headingRows}} tableAttributes
// @param {module:engine/conversion/downcastdispatcher~DowncastConversionApi} conversionApi
// @returns {module:engine/view/element~Element}
function createTr( tableRow, rowIndex, tableSection, conversionApi ) {
function createTr( tableElement, tableRow, rowIndex, tableAttributes, conversionApi ) {
// Will always consume since we're converting <tableRow> element from a parent <table>.
conversionApi.consumable.consume( tableRow, 'insert' );

const trElement = conversionApi.writer.createContainerElement( 'tr' );
const trElement = tableRow.isEmpty ?
conversionApi.writer.createEmptyElement( 'tr' ) :
conversionApi.writer.createContainerElement( 'tr' );

conversionApi.mapper.bindElements( tableRow, trElement );

const headingRows = tableRow.parent.getAttribute( 'headingRows' ) || 0;
const offset = headingRows > 0 && rowIndex >= headingRows ? rowIndex - headingRows : rowIndex;
const headingRows = tableAttributes.headingRows;
const tableSection = getOrCreateTableSection( getSectionName( rowIndex, tableAttributes ), tableElement, conversionApi );

const offset = headingRows > 0 && rowIndex >= headingRows ? rowIndex - headingRows : rowIndex;
const position = conversionApi.writer.createPositionAt( tableSection, offset );

conversionApi.writer.insert( position, trElement );

return trElement;
Expand Down
10 changes: 7 additions & 3 deletions packages/ckeditor5-table/src/converters/upcasttable.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,20 @@ export default function upcastTable() {
}

/**
* Conversion helper that skips empty <tr> from upcasting.
* Conversion helper that skips empty <tr> from upcasting at the beginning of the table.
*
* Empty row is considered a table model error.
* Empty row is considered a table model error but when handling clipboard data there could be rows that contain only row-spanned cells
* and empty TR-s are used to maintain table structure (also {@link module:table/tablewalker~TableWalker} assumes that there are only rows
* that have related tableRow elements).
*
* *Note:* Only first empty rows are removed because those have no meaning and solves issue of improper table with all empty rows.
*
* @returns {Function} Conversion helper.
*/
export function skipEmptyTableRow() {
return dispatcher => {
dispatcher.on( 'element:tr', ( evt, data ) => {
if ( data.viewItem.isEmpty ) {
if ( data.viewItem.isEmpty && data.modelCursor.index == 0 ) {
evt.stop();
}
}, { priority: 'high' } );
Expand Down
86 changes: 11 additions & 75 deletions packages/ckeditor5-table/src/tableclipboard.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,12 @@ import {
cropTableToDimensions,
getHorizontallyOverlappingCells,
getVerticallyOverlappingCells,
removeEmptyRowsColumns,
splitHorizontally,
splitVertically,
trimTableCellIfNeeded
trimTableCellIfNeeded,
adjustLastRowIndex,
adjustLastColumnIndex
} from './utils/structure';

/**
Expand Down Expand Up @@ -113,16 +116,18 @@ export default class TableClipboard extends Plugin {
const model = this.editor.model;
const tableUtils = this.editor.plugins.get( TableUtils );

const selectedTableCells = getSelectionAffectedTableCells( model.document.selection );
// We might need to crop table before inserting so reference might change.
let pastedTable = getTableIfOnlyTableInContent( content, model );

if ( !selectedTableCells.length ) {
if ( !pastedTable ) {
return;
}

// We might need to crop table before inserting so reference might change.
let pastedTable = getTableIfOnlyTableInContent( content, model );
const selectedTableCells = getSelectionAffectedTableCells( model.document.selection );

if ( !selectedTableCells.length ) {
removeEmptyRowsColumns( pastedTable, tableUtils );

if ( !pastedTable ) {
return;
}

Expand Down Expand Up @@ -513,72 +518,3 @@ function isAffectedBySelection( index, span, limit ) {

return isInsideSelection || overlapsSelectionFromOutside;
}

// Returns adjusted last row index if selection covers part of a row with empty slots (spanned by other cells).
// The rowIndexes.last is equal to last row index but selection might be bigger.
//
// This happens *only* on rectangular selection so we analyze a case like this:
//
// +---+---+---+---+
// 0 | a | b | c | d |
// + + +---+---+
// 1 | | e | f | g |
// + +---+ +---+
// 2 | | h | | i | <- last row, each cell has rowspan = 2,
// + + + + + so we need to return 3, not 2
// 3 | | | | |
// +---+---+---+---+
function adjustLastRowIndex( table, dimensions ) {
const lastRowMap = Array.from( new TableWalker( table, {
startColumn: dimensions.firstColumn,
endColumn: dimensions.lastColumn,
row: dimensions.lastRow
} ) );

const everyCellHasSingleRowspan = lastRowMap.every( ( { cellHeight } ) => cellHeight === 1 );

// It is a "flat" row, so the last row index is OK.
if ( everyCellHasSingleRowspan ) {
return dimensions.lastRow;
}

// Otherwise get any cell's rowspan and adjust the last row index.
const rowspanAdjustment = lastRowMap[ 0 ].cellHeight - 1;
return dimensions.lastRow + rowspanAdjustment;
}

// Returns adjusted last column index if selection covers part of a column with empty slots (spanned by other cells).
// The columnIndexes.last is equal to last column index but selection might be bigger.
//
// This happens *only* on rectangular selection so we analyze a case like this:
//
// 0 1 2 3
// +---+---+---+---+
// | a |
// +---+---+---+---+
// | b | c | d |
// +---+---+---+---+
// | e | f |
// +---+---+---+---+
// | g | h |
// +---+---+---+---+
// ^
// last column, each cell has colspan = 2, so we need to return 3, not 2
function adjustLastColumnIndex( table, dimensions ) {
const lastColumnMap = Array.from( new TableWalker( table, {
startRow: dimensions.firstRow,
endRow: dimensions.lastRow,
column: dimensions.lastColumn
} ) );

const everyCellHasSingleColspan = lastColumnMap.every( ( { cellWidth } ) => cellWidth === 1 );

// It is a "flat" column, so the last column index is OK.
if ( everyCellHasSingleColspan ) {
return dimensions.lastColumn;
}

// Otherwise get any cell's colspan and adjust the last column index.
const colspanAdjustment = lastColumnMap[ 0 ].cellWidth - 1;
return dimensions.lastColumn + colspanAdjustment;
}
34 changes: 26 additions & 8 deletions packages/ckeditor5-table/src/tableselection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import TableWalker from './tablewalker';
import TableUtils from './tableutils';

import { findAncestor } from './utils/common';
import { cropTableToDimensions } from './utils/structure';
import { getColumnIndexes, getRowIndexes, getSelectedTableCells } from './utils/selection';
import { cropTableToDimensions, adjustLastRowIndex, adjustLastColumnIndex } from './utils/structure';
import { getColumnIndexes, getRowIndexes, getSelectedTableCells, isSelectionRectangular } from './utils/selection';

import '../theme/tableselection.css';

Expand Down Expand Up @@ -90,17 +90,35 @@ export default class TableSelection extends Plugin {

return this.editor.model.change( writer => {
const documentFragment = writer.createDocumentFragment();
const tableUtils = this.editor.plugins.get( 'TableUtils' );

const { first: startColumn, last: endColumn } = getColumnIndexes( selectedCells );
const { first: startRow, last: endRow } = getRowIndexes( selectedCells );
const { first: firstColumn, last: lastColumn } = getColumnIndexes( selectedCells );
const { first: firstRow, last: lastRow } = getRowIndexes( selectedCells );

const sourceTable = findAncestor( 'table', selectedCells[ 0 ] );

let adjustedLastRow = lastRow;
let adjustedLastColumn = lastColumn;

// If the selection is rectangular there could be a case of all cells in the last row/column spanned over
// next row/column so the real lastRow/lastColumn should be updated.
if ( isSelectionRectangular( selectedCells, tableUtils ) ) {
const dimensions = {
firstColumn,
lastColumn,
firstRow,
lastRow
};

adjustedLastRow = adjustLastRowIndex( sourceTable, dimensions );
adjustedLastColumn = adjustLastColumnIndex( sourceTable, dimensions );
}

const cropDimensions = {
startRow,
startColumn,
endRow,
endColumn
startRow: firstRow,
startColumn: firstColumn,
endRow: adjustedLastRow,
endColumn: adjustedLastColumn
};

const table = cropTableToDimensions( sourceTable, cropDimensions, writer );
Expand Down
93 changes: 91 additions & 2 deletions packages/ckeditor5-table/src/utils/structure.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ function addHeadingsToCroppedTable( croppedTable, sourceTable, startRow, startCo
* @protected
* @param {module:engine/model/element~Element} table
* @param {module:table/tableutils~TableUtils} tableUtils
* @return {Boolean} True if removed some columns.
* @returns {Boolean} True if removed some columns.
*/
export function removeEmptyColumns( table, tableUtils ) {
const width = tableUtils.getColumns( table );
Expand Down Expand Up @@ -381,7 +381,7 @@ export function removeEmptyColumns( table, tableUtils ) {
* @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.
* @returns {Boolean} True if removed some rows.
*/
export function removeEmptyRows( table, tableUtils, batch ) {
const emptyRows = [];
Expand Down Expand Up @@ -438,3 +438,92 @@ export function removeEmptyRowsColumns( table, tableUtils, batch ) {
removeEmptyRows( table, tableUtils, batch );
}
}

/**
* Returns adjusted last row index if selection covers part of a row with empty slots (spanned by other cells).
* The `dimensions.lastRow` is equal to last row index but selection might be bigger.
*
* This happens *only* on rectangular selection so we analyze a case like this:
*
* +---+---+---+---+
* 0 | a | b | c | d |
* + + +---+---+
* 1 | | e | f | g |
* + +---+ +---+
* 2 | | h | | i | <- last row, each cell has rowspan = 2,
* + + + + + so we need to return 3, not 2
* 3 | | | | |
* +---+---+---+---+
*
* @param {module:engine/model/element~Element} table
* @param {Object} dimensions
* @param {Number} dimensions.firstRow
* @param {Number} dimensions.firstColumn
* @param {Number} dimensions.lastRow
* @param {Number} dimensions.lastColumn
* @returns {Number} Adjusted last row index.
*/
export function adjustLastRowIndex( table, dimensions ) {
const lastRowMap = Array.from( new TableWalker( table, {
startColumn: dimensions.firstColumn,
endColumn: dimensions.lastColumn,
row: dimensions.lastRow
} ) );

const everyCellHasSingleRowspan = lastRowMap.every( ( { cellHeight } ) => cellHeight === 1 );

// It is a "flat" row, so the last row index is OK.
if ( everyCellHasSingleRowspan ) {
return dimensions.lastRow;
}

// Otherwise get any cell's rowspan and adjust the last row index.
const rowspanAdjustment = lastRowMap[ 0 ].cellHeight - 1;
return dimensions.lastRow + rowspanAdjustment;
}

/**
* Returns adjusted last column index if selection covers part of a column with empty slots (spanned by other cells).
* The `dimensions.lastColumn` is equal to last column index but selection might be bigger.
*
* This happens *only* on rectangular selection so we analyze a case like this:
*
* 0 1 2 3
* +---+---+---+---+
* | a |
* +---+---+---+---+
* | b | c | d |
* +---+---+---+---+
* | e | f |
* +---+---+---+---+
* | g | h |
* +---+---+---+---+
* ^
* last column, each cell has colspan = 2, so we need to return 3, not 2
*
* @param {module:engine/model/element~Element} table
* @param {Object} dimensions
* @param {Number} dimensions.firstRow
* @param {Number} dimensions.firstColumn
* @param {Number} dimensions.lastRow
* @param {Number} dimensions.lastColumn
* @returns {Number} Adjusted last column index.
*/
export function adjustLastColumnIndex( table, dimensions ) {
const lastColumnMap = Array.from( new TableWalker( table, {
startRow: dimensions.firstRow,
endRow: dimensions.lastRow,
column: dimensions.lastColumn
} ) );

const everyCellHasSingleColspan = lastColumnMap.every( ( { cellWidth } ) => cellWidth === 1 );

// It is a "flat" column, so the last column index is OK.
if ( everyCellHasSingleColspan ) {
return dimensions.lastColumn;
}

// Otherwise get any cell's colspan and adjust the last column index.
const colspanAdjustment = lastColumnMap[ 0 ].cellWidth - 1;
return dimensions.lastColumn + colspanAdjustment;
}
Loading

0 comments on commit 17d7bd7

Please sign in to comment.