Merge pull request #202 from ckeditor/cf/tc-tables

Feature: `TableWalker` will now return `cell` value also for spanned cells when traversing a table with `includeSpanned` option set to `true`. Additionally, `isSpanned` property was introduced in returned values.
Internal: Improvements in table plugin allowing for integration with track changes.

BREAKING CHANGE: `TableWalker` will not return `undefined` as `cell` value for spanned cells anymore. Use `isSpanned` instead.

Author: jodator

src/commands/setheadercolumncommand.js

@@ -61,22 +61,27 @@ export default class SetHeaderColumnCommand extends Command {
 	 * When the selection is already in a header column, it will set `headingColumns` so the heading section will end before that column.
 	 *
 	 * @fires execute
+	 * @params {Boolean} [forceValue] If set, the command will set (`true`) or unset (`false`) header columns according to `forceValue`
+	 * parameter instead of the current model state.
 	 */
-	execute() {
+	execute( forceValue = null ) {
 		const model = this.editor.model;
 		const doc = model.document;
 		const selection = doc.selection;
 		const tableUtils = this.editor.plugins.get( 'TableUtils' );
 
 		const position = selection.getFirstPosition();
-		const tableCell = findAncestor( 'tableCell', position.parent );
+		const tableCell = findAncestor( 'tableCell', position );
 		const tableRow = tableCell.parent;
 		const table = tableRow.parent;
 
-		const currentHeadingColumns = parseInt( table.getAttribute( 'headingColumns' ) || 0 );
 		const { column: selectionColumn } = tableUtils.getCellLocation( tableCell );
 
-		const headingColumnsToSet = currentHeadingColumns > selectionColumn ? selectionColumn : selectionColumn + 1;
+		if ( forceValue === this.value ) {
+			return;
+		}
+
+		const headingColumnsToSet = this.value ? selectionColumn : selectionColumn + 1;
 
 		model.change( writer => {
 			updateNumericAttribute( 'headingColumns', headingColumnsToSet, table, writer, 0 );

src/commands/setheaderrowcommand.js

@@ -60,8 +60,10 @@ export default class SetHeaderRowCommand extends Command {
 	 * When the selection is already in a header row, it will set `headingRows` so the heading section will end before that row.
 	 *
 	 * @fires execute
+	 * @params {Boolean} [forceValue] If set, the command will set (`true`) or unset (`false`) header rows according to `forceValue`
+	 * parameter instead of the current model state.
 	 */
-	execute() {
+	execute( forceValue = null ) {
 		const model = this.editor.model;
 		const doc = model.document;
 		const selection = doc.selection;
@@ -74,7 +76,11 @@ export default class SetHeaderRowCommand extends Command {
 		const currentHeadingRows = table.getAttribute( 'headingRows' ) || 0;
 		const selectionRow = tableRow.index;
 
-		const headingRowsToSet = currentHeadingRows > selectionRow ? selectionRow : selectionRow + 1;
+		if ( forceValue === this.value ) {
+			return;
+		}
+
+		const headingRowsToSet = this.value ? selectionRow : selectionRow + 1;
 
 		model.change( writer => {
 			if ( headingRowsToSet ) {
@@ -151,19 +157,21 @@ function splitHorizontally( tableCell, headingRows, writer ) {
 		attributes.rowspan = spanToSet;
 	}
 
+	const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 );
+
+	if ( colspan > 1 ) {
+		attributes.colspan = colspan;
+	}
+
 	const startRow = table.getChildIndex( tableRow );
 	const endRow = startRow + newRowspan;
 	const tableMap = [ ...new TableWalker( table, { startRow, endRow, includeSpanned: true } ) ];
 
 	let columnIndex;
 
-	for ( const { row, column, cell, colspan, cellIndex } of tableMap ) {
-		if ( cell === tableCell ) {
+	for ( const { row, column, cell, cellIndex } of tableMap ) {
+		if ( cell === tableCell && columnIndex === undefined ) {
 			columnIndex = column;
-
-			if ( colspan > 1 ) {
-				attributes.colspan = colspan;
-			}
 		}
 
 		if ( columnIndex !== undefined && columnIndex === column && row === endRow ) {

src/converters/downcast.js

@@ -345,6 +345,7 @@ function renameViewTableCell( tableCell, desiredCellElementName, conversionApi,
 		renamedCell = viewWriter.rename( desiredCellElementName, viewCell );
 	}
 
+	conversionApi.mapper.unbindViewElement( viewCell );
 	conversionApi.mapper.bindElements( tableCell, renamedCell );
 }
 

src/tableutils.js

@@ -180,7 +180,7 @@ export default class TableUtils extends Plugin {
 	 *		|       | c |                   |               | c |
 	 *		+---+---+---+     will give:    +---+---+---+---+---+
 	 *		| d | e | f |                   | d |   |   | e | f |
-	 *		+---+   +---+                   +---+---+---+  +---+
+	 *		+---+   +---+                   +---+---+---+   +---+
 	 *		| g |   | h |                   | g |   |   |   | h |
 	 *		+---+---+---+                   +---+---+---+---+---+
 	 *		| i         |                   | i                 |
@@ -219,13 +219,16 @@ export default class TableUtils extends Plugin {
 
 			const tableWalker = new TableWalker( table, { column: insertAt, includeSpanned: true } );
 
-			for ( const { row, column, cell, colspan, rowspan, cellIndex } of tableWalker ) {
+			for ( const { row, cell, cellIndex } of tableWalker ) {
 				// When iterating over column the table walker outputs either:
 				// - cells at given column index (cell "e" from method docs),
 				// - spanned columns (spanned cell from row between cells "g" and "h" - spanned by "e", only if `includeSpanned: true`),
 				// - or a cell from the same row which spans over this column (cell "a").
 
-				if ( column !== insertAt ) {
+				const rowspan = parseInt( cell.getAttribute( 'rowspan' ) || 1 );
+				const colspan = parseInt( cell.getAttribute( 'colspan' ) || 1 );
+
+				if ( cell.index !== insertAt && colspan > 1 ) {
 					// If column is different than `insertAt`, it is a cell that spans over an inserted column (cell "a" & "i").
 					// For such cells expand them by a number of columns inserted.
 					writer.setAttribute( 'colspan', colspan + columnsToInsert, cell );