Răsfoiți Sursa

Changed: Refactor table converters and make CellSpans class private.

Maciej Gołaszewski 8 ani în urmă
părinte
comite
474db8375d

+ 125 - 52
packages/ckeditor5-table/src/converters.js

@@ -73,27 +73,20 @@ export function downcastTable() {
 			return;
 		}
 
+		let tHead, tBody;
+
 		const tableElement = conversionApi.writer.createContainerElement( 'table' );
-		const headingRowsCount = table.getAttribute( 'headingRows' ) || 0;
+		const headingRowsCount = parseInt( table.getAttribute( 'headingRows' ) ) || 0;
 		const tableRows = Array.from( table.getChildren() );
 		const cellSpans = new CellSpans();
 
-		const tHead = headingRowsCount ? _createTableSection( 'thead', tableElement, conversionApi ) : undefined;
-		const tBody = headingRowsCount < tableRows.length ? _createTableSection( 'tbody', tableElement, conversionApi ) : undefined;
-
-		let parent;
-
-		for ( let rowIndex = 0; rowIndex < tableRows.length; rowIndex++ ) {
-			if ( headingRowsCount && rowIndex < headingRowsCount ) {
-				parent = tHead;
-			} else {
-				parent = tBody;
-			}
-
-			const row = tableRows[ rowIndex ];
+		for ( const row of tableRows ) {
+			const rowIndex = tableRows.indexOf( row );
+			const parent = getParent( rowIndex, headingRowsCount, tableElement, conversionApi );
 
 			_downcastTableRow( row, rowIndex, cellSpans, parent, conversionApi );
 
+			// Drop table cell spans information for downcasted row.
 			cellSpans.drop( rowIndex );
 		}
 
@@ -101,6 +94,23 @@ export function downcastTable() {
 
 		conversionApi.mapper.bindElements( table, tableElement );
 		conversionApi.writer.insert( viewPosition, tableElement );
+
+		// Creates if not existing and returns <tbody> or <thead> element for given rowIndex.
+		function getParent( rowIndex, headingRowsCount, tableElement, conversionApi ) {
+			if ( headingRowsCount && rowIndex < headingRowsCount ) {
+				if ( !tHead ) {
+					tHead = _createTableSection( 'thead', tableElement, conversionApi );
+				}
+
+				return tHead;
+			}
+
+			if ( !tBody ) {
+				tBody = _createTableSection( 'tbody', tableElement, conversionApi );
+			}
+
+			return tBody;
+		}
 	}, { priority: 'normal' } );
 }
 
@@ -117,44 +127,26 @@ function _downcastTableRow( tableRow, rowIndex, cellSpans, parent, conversionApi
 	const headingColumns = tableRow.parent.getAttribute( 'headingColumns' ) || 0;
 
 	for ( const tableCell of Array.from( tableRow.getChildren() ) ) {
-		let shift = cellSpans.check( rowIndex, cellIndex ) || 0;
-
-		while ( shift ) {
-			cellIndex += shift;
-			shift = cellSpans.check( rowIndex, cellIndex );
-		}
+		cellIndex = cellSpans.getColumnWithSpan( rowIndex, cellIndex );
 
 		const cellWidth = tableCell.hasAttribute( 'colspan' ) ? parseInt( tableCell.getAttribute( 'colspan' ) ) : 1;
 		const cellHeight = tableCell.hasAttribute( 'rowspan' ) ? parseInt( tableCell.getAttribute( 'rowspan' ) ) : 1;
 
-		if ( cellHeight > 1 ) {
-			cellSpans.update( rowIndex, cellIndex, cellHeight, cellWidth );
-		}
+		cellSpans.updateSpans( rowIndex, cellIndex, cellHeight, cellWidth );
 
 		// Will always consume since we're converting <tableRow> element from a parent <table>.
 		conversionApi.consumable.consume( tableCell, 'insert' );
 
-		const isHead = _isHead( tableCell, cellIndex, headingColumns );
-
-		const tableCellElement = conversionApi.writer.createContainerElement( isHead ? 'th' : 'td' );
-		const viewPosition = Position.createAt( trElement, 'end' );
+		const isHead = _isHead( tableCell, rowIndex, cellIndex, headingColumns );
+		const cellElement = conversionApi.writer.createContainerElement( isHead ? 'th' : 'td' );
 
-		conversionApi.mapper.bindElements( tableCell, tableCellElement );
-		conversionApi.writer.insert( viewPosition, tableCellElement );
+		conversionApi.mapper.bindElements( tableCell, cellElement );
+		conversionApi.writer.insert( Position.createAt( trElement, 'end' ), cellElement );
 
 		cellIndex += cellWidth;
 	}
 }
 
-function _isHead( tableCell, cellIndex, columnHeadings ) {
-	const row = tableCell.parent;
-	const table = row.parent;
-	const rowIndex = table.getChildIndex( row );
-	const headingRows = table.getAttribute( 'headingRows' ) || 0;
-
-	return ( !!headingRows && headingRows > rowIndex ) || ( !!columnHeadings && columnHeadings > cellIndex );
-}
-
 function _createTableSection( elementName, tableElement, conversionApi ) {
 	const tableChildElement = conversionApi.writer.createContainerElement( elementName );
 
@@ -163,6 +155,14 @@ function _createTableSection( elementName, tableElement, conversionApi ) {
 	return tableChildElement;
 }
 
+function _isHead( tableCell, rowIndex, cellIndex, columnHeadings ) {
+	const row = tableCell.parent;
+	const table = row.parent;
+	const headingRows = table.getAttribute( 'headingRows' ) || 0;
+
+	return ( !!headingRows && headingRows > rowIndex ) || ( !!columnHeadings && columnHeadings > cellIndex );
+}
+
 function _upcastTableRows( viewTable, modelTable, conversionApi ) {
 	const { rows, headingRows, headingColumns } = _scanTable( viewTable );
 
@@ -271,36 +271,109 @@ function _scanRow( tr, tableMeta, firstThead ) {
 	}
 }
 
-export class CellSpans {
+/**
+ * Holds information about spanned table cells.
+ *
+ * @private
+ */
+class CellSpans {
+	/**
+	 * Creates CellSpans instance.
+	 */
 	constructor() {
+		/**
+		 * Holds table cell spans mapping.
+		 *
+		 * @type {Map<Number, Number>}
+		 * @private
+		 */
 		this._spans = new Map();
 	}
 
-	check( row, column ) {
-		if ( !this._spans.has( row ) ) {
-			return false;
+	/**
+	 * Returns proper column index if current cell have span.
+	 *
+	 * @param {Number} row
+	 * @param {Number} column
+	 * @return {Number} Returns current column or updated column index.
+	 */
+	getColumnWithSpan( row, column ) {
+		let span = this._check( row, column ) || 0;
+
+		// Offset current table cell columnIndex by spanning cells from rows above.
+		while ( span ) {
+			column += span;
+			span = this._check( row, column );
 		}
 
-		const rowSpans = this._spans.get( row );
-
-		return rowSpans.has( column ) ? rowSpans.get( column ) : false;
+		return column;
 	}
 
-	update( row, column, height, width ) {
-		if ( height > 1 ) {
-			for ( let nextRow = row + 1; nextRow < row + height; nextRow++ ) {
-				const rowSpans = this._spans.has( nextRow ) ? this._spans.get( nextRow ) : new Map();
+	/**
+	 * Updates spans based on current table cell height & width.
+	 *
+	 * For instance if a table cell at row 0 and column 0 has height of 3 and width of 2 we're setting spans:
+	 *
+	 *        0 1 2
+	 *     0:
+	 *     1: 2
+	 *     2: 2
+	 *     3:
+	 *
+	 * Adding another spans for a table cell at row 2 and column 1 that has height of 2 and width of 4 will update above to:
+	 *
+	 *        0 1 2
+	 *     0:
+	 *     1: 2
+	 *     2: 2
+	 *     3:   4
+	 *
+	 * @param {Number} row
+	 * @param {Number} column
+	 * @param {Number} height
+	 * @param {Number} width
+	 */
+	updateSpans( row, column, height, width ) {
+		// Omit horizontal spans as those are handled during current row conversion.
+
+		// This will update all rows below up to row height with value of span width.
+		for ( let nextRow = row + 1; nextRow < row + height; nextRow++ ) {
+			if ( !this._spans.has( nextRow ) ) {
+				this._spans.set( nextRow, new Map() );
+			}
 
-				rowSpans.set( column, width );
+			const rowSpans = this._spans.get( nextRow );
 
-				this._spans.set( nextRow, rowSpans );
-			}
+			rowSpans.set( column, width );
 		}
 	}
 
+	/**
+	 * Drops already downcasted row.
+	 *
+	 * @param {Number} row
+	 */
 	drop( row ) {
 		if ( this._spans.has( row ) ) {
 			this._spans.delete( row );
 		}
 	}
+
+	/**
+	 * Checks if given table cell is spanned by other.
+	 *
+	 * @param {Number} row
+	 * @param {Number} column
+	 * @return {Boolean|Number} Returns false or width of a span.
+	 * @private
+	 */
+	_check( row, column ) {
+		if ( !this._spans.has( row ) ) {
+			return false;
+		}
+
+		const rowSpans = this._spans.get( row );
+
+		return rowSpans.has( column ) ? rowSpans.get( column ) : false;
+	}
 }

+ 10 - 42
packages/ckeditor5-table/tests/converters.js

@@ -8,7 +8,7 @@ import { upcastElementToElement } from '@ckeditor/ckeditor5-engine/src/conversio
 import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 
 import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
-import { downcastTable, CellSpans, upcastTable } from '../src/converters';
+import { downcastTable, upcastTable } from '../src/converters';
 
 describe( 'Table converters', () => {
 	let editor, model, viewDocument;
@@ -475,8 +475,8 @@ describe( 'Table converters', () => {
 				//                |
 				// +----+----+----+----+
 				// | 11 | 12 | 13 | 14 |
-				// |----+----+    +----+
-				// | 21 | 22 |    | 24 |
+				// |    +----+    +----+
+				// |    | 22 |    | 24 |
 				// |----+----+    +----+
 				// | 31      |    | 34 |
 				// |         +----+----+
@@ -486,9 +486,12 @@ describe( 'Table converters', () => {
 				setModelData( model,
 					'<table headingColumns="3">' +
 					'<tableRow>' +
-					'<tableCell>11</tableCell><tableCell>12</tableCell><tableCell rowspan="3">13</tableCell><tableCell>14</tableCell>' +
+					'<tableCell rowspan="2">11</tableCell>' +
+					'<tableCell>12</tableCell>' +
+					'<tableCell rowspan="3">13</tableCell>' +
+					'<tableCell>14</tableCell>' +
 					'</tableRow>' +
-					'<tableRow><tableCell>21</tableCell><tableCell>22</tableCell><tableCell>24</tableCell></tableRow>' +
+					'<tableRow><tableCell>22</tableCell><tableCell>24</tableCell></tableRow>' +
 					'<tableRow><tableCell colspan="2" rowspan="2">31</tableCell><tableCell>34</tableCell></tableRow>' +
 					'<tableRow><tableCell>43</tableCell><tableCell>44</tableCell></tableRow>' +
 					'</table>'
@@ -497,8 +500,8 @@ describe( 'Table converters', () => {
 				expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
 					'<table>' +
 					'<tbody>' +
-					'<tr><th>11</th><th>12</th><th rowspan="3">13</th><td>14</td></tr>' +
-					'<tr><th>21</th><th>22</th><td>24</td></tr>' +
+					'<tr><th rowspan="2">11</th><th>12</th><th rowspan="3">13</th><td>14</td></tr>' +
+					'<tr><th>22</th><td>24</td></tr>' +
 					'<tr><th colspan="2" rowspan="2">31</th><td>34</td></tr>' +
 					'<tr><th>43</th><td>44</td></tr>' +
 					'</tbody>' +
@@ -507,39 +510,4 @@ describe( 'Table converters', () => {
 			} );
 		} );
 	} );
-
-	describe( 'spanner', () => {
-		let spanner;
-
-		beforeEach( () => {
-			spanner = new CellSpans();
-		} );
-
-		it( 'should work', () => {
-			expect( spanner.check( 2, 1 ) ).to.be.false;
-
-			spanner.update( 1, 1, 3, 2 );
-
-			expect( spanner.check( 2, 0 ) ).to.be.false;
-			expect( spanner.check( 2, 1 ) ).to.equal( 2 );
-			expect( spanner.check( 3, 1 ) ).to.equal( 2 );
-
-			spanner.drop( 2 );
-
-			expect( spanner.check( 2, 1 ) ).to.be.false;
-			expect( spanner.check( 3, 1 ) ).to.equal( 2 );
-
-			spanner.drop( 2 );
-
-			expect( spanner.check( 2, 1 ) ).to.be.false;
-			expect( spanner.check( 3, 1 ) ).to.equal( 2 );
-
-			spanner.update( 2, 4, 2, 3 );
-			expect( spanner.check( 3, 1 ) ).to.equal( 2 );
-			expect( spanner.check( 3, 4 ) ).to.equal( 3 );
-
-			spanner.update( 1, 1, 1, 5 );
-			expect( spanner.check( 1, 1 ) ).to.be.false;
-		} );
-	} );
 } );