Explorar el Código

Merge branch 'master' into i/6123

Maciej Gołaszewski hace 5 años
padre
commit
4f70f29f94

+ 29 - 17
packages/ckeditor5-table/src/commands/removecolumncommand.js

@@ -76,28 +76,40 @@ export default class RemoveColumnCommand extends Command {
 				removedColumnIndex >= removedColumnIndexes.first;
 				removedColumnIndex--
 			) {
-				for ( const { cell, column, colspan } of tableMap ) {
-					// If colspaned cell overlaps removed column decrease its span.
-					if ( column <= removedColumnIndex && colspan > 1 && column + colspan > removedColumnIndex ) {
-						updateNumericAttribute( 'colspan', colspan - 1, cell, writer );
-					} else if ( column === removedColumnIndex ) {
-						const cellRow = cell.parent;
-
-						// The cell in removed column has colspan of 1.
-						writer.remove( cell );
-
-						// If the cell was the last one in the row, get rid of the entire row.
-						// https://github.com/ckeditor/ckeditor5/issues/6429
-						if ( !cellRow.childCount ) {
-							writer.remove( cellRow );
-						}
-					}
-				}
+				this._removeColumn( removedColumnIndex, table, writer );
 			}
 
 			writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );
 		} );
 	}
+
+	/**
+	 * Removes a column from the given `table`.
+	 *
+	 * @private
+	 * @param {Number} removedColumnIndex Index of the column that should be removed.
+	 * @param {module:engine/model/element~Element} table
+	 * @param {module:engine/model/writer~Writer} writer
+	 */
+	_removeColumn( removedColumnIndex, table, writer ) {
+		for ( const { cell, column, colspan } of new TableWalker( table ) ) {
+			// If colspaned cell overlaps removed column decrease its span.
+			if ( column <= removedColumnIndex && colspan > 1 && column + colspan > removedColumnIndex ) {
+				updateNumericAttribute( 'colspan', colspan - 1, cell, writer );
+			} else if ( column === removedColumnIndex ) {
+				const cellRow = cell.parent;
+
+				// The cell in removed column has colspan of 1.
+				writer.remove( cell );
+
+				// If the cell was the last one in the row, get rid of the entire row.
+				// https://github.com/ckeditor/ckeditor5/issues/6429
+				if ( !cellRow.childCount ) {
+					writer.remove( cellRow );
+				}
+			}
+		}
+	}
 }
 
 // Updates heading columns attribute if removing a row from head section.

+ 41 - 35
packages/ckeditor5-table/src/commands/removerowcommand.js

@@ -10,7 +10,7 @@
 import Command from '@ckeditor/ckeditor5-core/src/command';
 
 import TableWalker from '../tablewalker';
-import { updateNumericAttribute } from './utils';
+import { findAncestor, updateNumericAttribute } from './utils';
 import { getSelectionAffectedTableCells } from '../utils';
 
 /**
@@ -33,15 +33,16 @@ export default class RemoveRowCommand extends Command {
 		const firstCell = selectedCells[ 0 ];
 
 		if ( firstCell ) {
-			const table = firstCell.parent.parent;
+			const table = findAncestor( 'table', firstCell );
 			const tableRowCount = this.editor.plugins.get( 'TableUtils' ).getRows( table );
+			const lastRowIndex = tableRowCount - 1;
 
-			const tableMap = [ ...new TableWalker( table ) ];
-			const rowIndexes = tableMap.filter( entry => selectedCells.includes( entry.cell ) ).map( el => el.row );
-			const minRowIndex = rowIndexes[ 0 ];
-			const maxRowIndex = rowIndexes[ rowIndexes.length - 1 ];
+			const selectedRowIndexes = getRowIndexes( selectedCells );
 
-			this.isEnabled = maxRowIndex - minRowIndex < ( tableRowCount - 1 );
+			const areAllRowsSelected = selectedRowIndexes.first === 0 && selectedRowIndexes.last === lastRowIndex;
+
+			// Disallow selecting whole table -> delete whole table should be used instead.
+			this.isEnabled = !areAllRowsSelected;
 		} else {
 			this.isEnabled = false;
 		}
@@ -51,34 +52,41 @@ export default class RemoveRowCommand extends Command {
 	 * @inheritDoc
 	 */
 	execute() {
-		const referenceCells = getSelectionAffectedTableCells( this.editor.model.document.selection );
+		const model = this.editor.model;
+		const referenceCells = getSelectionAffectedTableCells( model.document.selection );
 		const removedRowIndexes = getRowIndexes( referenceCells );
 
 		const firstCell = referenceCells[ 0 ];
-		const table = firstCell.parent.parent;
-		const tableMap = [ ...new TableWalker( table, { endRow: removedRowIndexes.last } ) ];
-		const batch = this.editor.model.createBatch();
-		const columnIndexToFocus = getColumnIndexToFocus( tableMap, firstCell );
-
-		// Doing multiple model.enqueueChange() calls, to get around ckeditor/ckeditor5#6391.
-		// Ideally we want to do this in a single model.change() block.
-		this.editor.model.enqueueChange( batch, writer => {
+		const table = findAncestor( 'table', firstCell );
+
+		const columnIndexToFocus = this.editor.plugins.get( 'TableUtils' ).getCellLocation( firstCell ).column;
+
+		model.change( writer => {
 			// This prevents the "model-selection-range-intersects" error, caused by removing row selected cells.
 			writer.setSelection( writer.createSelection( table, 'on' ) );
-		} );
 
-		let cellToFocus;
+			let cellToFocus;
 
-		for ( let i = removedRowIndexes.last; i >= removedRowIndexes.first; i-- ) {
-			this.editor.model.enqueueChange( batch, writer => {
+			for ( let i = removedRowIndexes.last; i >= removedRowIndexes.first; i-- ) {
 				const removedRowIndex = i;
-				this._removeRow( removedRowIndex, table, writer, tableMap );
+				this._removeRow( removedRowIndex, table, writer );
 
 				cellToFocus = getCellToFocus( table, removedRowIndex, columnIndexToFocus );
-			} );
-		}
+			}
+
+			const model = this.editor.model;
+			const headingRows = table.getAttribute( 'headingRows' ) || 0;
+
+			if ( headingRows && removedRowIndexes.first < headingRows ) {
+				const newRows = getNewHeadingRowsValue( removedRowIndexes, headingRows );
+
+				// Must be done after the changes in table structure (removing rows).
+				// Otherwise the downcast converter for headingRows attribute will fail. ckeditor/ckeditor5#6391.
+				model.enqueueChange( writer.batch, writer => {
+					updateNumericAttribute( 'headingRows', newRows, table, writer, 0 );
+				} );
+			}
 
-		this.editor.model.enqueueChange( batch, writer => {
 			writer.setSelection( writer.createPositionAt( cellToFocus, 0 ) );
 		} );
 	}
@@ -90,16 +98,11 @@ export default class RemoveRowCommand extends Command {
 	 * @param {Number} removedRowIndex Index of the row that should be removed.
 	 * @param {module:engine/model/element~Element} table
 	 * @param {module:engine/model/writer~Writer} writer
-	 * @param {module:engine/model/element~Element[]} tableMap Table map retrieved from {@link module:table/tablewalker~TableWalker}.
 	 */
-	_removeRow( removedRowIndex, table, writer, tableMap ) {
+	_removeRow( removedRowIndex, table, writer ) {
 		const cellsToMove = new Map();
 		const tableRow = table.getChild( removedRowIndex );
-		const headingRows = table.getAttribute( 'headingRows' ) || 0;
-
-		if ( headingRows && removedRowIndex < headingRows ) {
-			updateNumericAttribute( 'headingRows', headingRows - 1, table, writer, 0 );
-		}
+		const tableMap = [ ...new TableWalker( table, { endRow: removedRowIndex } ) ];
 
 		// Get cells from removed row that are spanned over multiple rows.
 		tableMap
@@ -167,8 +170,11 @@ function getCellToFocus( table, removedRowIndex, columnToFocus ) {
 	return cellToFocus;
 }
 
-// Returns the index of column that should be focused after rows are removed.
-function getColumnIndexToFocus( tableMap, firstCell ) {
-	const firstCellData = tableMap.find( value => value.cell === firstCell );
-	return firstCellData.column;
+// Calculates a new heading rows value for removing rows from heading section.
+function getNewHeadingRowsValue( removedRowIndexes, headingRows ) {
+	if ( removedRowIndexes.last < headingRows ) {
+		return headingRows - ( ( removedRowIndexes.last - removedRowIndexes.first ) + 1 );
+	}
+
+	return removedRowIndexes.first;
 }

+ 12 - 19
packages/ckeditor5-table/src/tablecellproperties/tablecellpropertiesediting.js

@@ -69,6 +69,7 @@ export default class TableCellPropertiesEditing extends Plugin {
 		const editor = this.editor;
 		const schema = editor.model.schema;
 		const conversion = editor.conversion;
+		const locale = editor.locale;
 
 		editor.data.addStyleProcessorRules( addBorderRules );
 		enableBorderProperties( schema, conversion );
@@ -76,7 +77,7 @@ export default class TableCellPropertiesEditing extends Plugin {
 		editor.commands.add( 'tableCellBorderColor', new TableCellBorderColorCommand( editor ) );
 		editor.commands.add( 'tableCellBorderWidth', new TableCellBorderWidthCommand( editor ) );
 
-		enableHorizontalAlignmentProperty( schema, conversion );
+		enableHorizontalAlignmentProperty( schema, conversion, locale );
 		editor.commands.add( 'tableCellHorizontalAlignment', new TableCellHorizontalAlignmentCommand( editor ) );
 
 		enableProperty( schema, conversion, 'width', 'width' );
@@ -117,37 +118,29 @@ function enableBorderProperties( schema, conversion ) {
 //
 // @param {module:engine/model/schema~Schema} schema
 // @param {module:engine/conversion/conversion~Conversion} conversion
-function enableHorizontalAlignmentProperty( schema, conversion ) {
+// @param {module:utils/locale~Locale} locale The {@link module:core/editor/editor~Editor#locale} instance.
+function enableHorizontalAlignmentProperty( schema, conversion, locale ) {
 	schema.extend( 'tableCell', {
 		allowAttributes: [ 'horizontalAlignment' ]
 	} );
 
+	const options = [ locale.contentLanguageDirection == 'rtl' ? 'left' : 'right', 'center', 'justify' ];
+
 	conversion.attributeToAttribute( {
 		model: {
 			name: 'tableCell',
 			key: 'horizontalAlignment',
-			values: [ 'right', 'center', 'justify' ]
+			values: options
 		},
-		view: {
-			right: {
-				key: 'style',
-				value: {
-					'text-align': 'right'
-				}
-			},
-			center: {
+		view: options.reduce( ( result, option ) => ( {
+			...result,
+			[ option ]: {
 				key: 'style',
 				value: {
-					'text-align': 'center'
-				}
-			},
-			justify: {
-				key: 'style',
-				value: {
-					'text-align': 'justify'
+					'text-align': option
 				}
 			}
-		}
+		} ), {} )
 	} );
 }
 

+ 23 - 0
packages/ckeditor5-table/tests/commands/removecolumncommand.js

@@ -324,6 +324,29 @@ describe( 'RemoveColumnCommand', () => {
 					[ '10', '[]14' ]
 				], { headingColumns: 1 } ) );
 			} );
+
+			it( 'should properly calculate truncated colspans', () => {
+				setData( model, modelTable( [
+					[ { contents: '00', colspan: 3 } ],
+					[ '10', '11', '12' ],
+					[ '20', '21', '22' ]
+				] ) );
+
+				const tableSelection = editor.plugins.get( TableSelection );
+				const modelRoot = model.document.getRoot();
+				tableSelection._setCellSelection(
+					modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
+					modelRoot.getNodeByPath( [ 0, 1, 1 ] )
+				);
+
+				command.execute();
+
+				assertEqualMarkup( getData( model ), modelTable( [
+					[ '00' ],
+					[ '[]12' ],
+					[ '22' ]
+				] ) );
+			} );
 		} );
 
 		it( 'should change heading columns if removing a heading column', () => {

+ 85 - 4
packages/ckeditor5-table/tests/commands/removerowcommand.js

@@ -3,19 +3,20 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
-import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor';
+import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
 import { getData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
+import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 
 import RemoveRowCommand from '../../src/commands/removerowcommand';
 import TableSelection from '../../src/tableselection';
-import { defaultConversion, defaultSchema, modelTable } from '../_utils/utils';
+import { defaultConversion, defaultSchema, modelTable, viewTable } from '../_utils/utils';
 import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils';
 
 describe( 'RemoveRowCommand', () => {
 	let editor, model, command;
 
 	beforeEach( () => {
-		return ModelTestEditor.create( { plugins: [ TableSelection ] } )
+		return VirtualTestEditor.create( { plugins: [ TableSelection ] } )
 			.then( newEditor => {
 				editor = newEditor;
 				model = editor.model;
@@ -215,7 +216,7 @@ describe( 'RemoveRowCommand', () => {
 				] ) );
 			} );
 
-			it( 'should support removing multiple headings', () => {
+			it( 'should support removing multiple headings (removed rows in heading section)', () => {
 				setData( model, modelTable( [
 					[ '00', '01' ],
 					[ '10', '11' ],
@@ -238,6 +239,37 @@ describe( 'RemoveRowCommand', () => {
 				], { headingRows: 1 } ) );
 			} );
 
+			it( 'should support removing multiple headings (removed rows in heading and body section)', () => {
+				setData( model, modelTable( [
+					[ '00', '01' ],
+					[ '10', '11' ],
+					[ '20', '21' ],
+					[ '30', '31' ],
+					[ '40', '41' ]
+				], { headingRows: 3 } ) );
+
+				const tableSelection = editor.plugins.get( TableSelection );
+				const modelRoot = model.document.getRoot();
+
+				tableSelection._setCellSelection(
+					modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
+					modelRoot.getNodeByPath( [ 0, 3, 0 ] )
+				);
+
+				command.execute();
+
+				assertEqualMarkup( getData( model ), modelTable( [
+					[ '00', '01' ],
+					[ '[]40', '41' ]
+				], { headingRows: 1 } ) );
+
+				// The view should also be properly downcasted.
+				assertEqualMarkup( getViewData( editor.editing.view, { withoutSelection: true } ), viewTable( [
+					[ '00', '01' ],
+					[ '40', '41' ]
+				], { headingRows: 1 } ) );
+			} );
+
 			it( 'should support removing mixed heading and cell rows', () => {
 				setData( model, modelTable( [
 					[ '00', '01' ],
@@ -258,6 +290,55 @@ describe( 'RemoveRowCommand', () => {
 					[ '[]20', '21' ]
 				] ) );
 			} );
+
+			it( 'should properly calculate truncated rowspans', () => {
+				setData( model, modelTable( [
+					[ '00', { contents: '01', rowspan: 3 } ],
+					[ '10' ],
+					[ '20' ]
+				] ) );
+
+				const tableSelection = editor.plugins.get( TableSelection );
+				const modelRoot = model.document.getRoot();
+				tableSelection._setCellSelection(
+					modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
+					modelRoot.getNodeByPath( [ 0, 1, 0 ] )
+				);
+
+				command.execute();
+
+				assertEqualMarkup( getData( model ), modelTable( [
+					[ '[]20', '01' ]
+				] ) );
+			} );
+
+			it( 'should create one undo step (1 batch)', () => {
+				setData( model, modelTable( [
+					[ '00', '01' ],
+					[ '10', '11' ],
+					[ '20', '21' ],
+					[ '30', '31' ]
+				], { headingRows: 3 } ) );
+
+				const createdBatches = new Set();
+
+				model.on( 'applyOperation', ( evt, args ) => {
+					const operation = args[ 0 ];
+
+					createdBatches.add( operation.batch );
+				} );
+
+				const tableSelection = editor.plugins.get( TableSelection );
+				const modelRoot = model.document.getRoot();
+				tableSelection._setCellSelection(
+					modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
+					modelRoot.getNodeByPath( [ 0, 1, 0 ] )
+				);
+
+				command.execute();
+
+				expect( createdBatches.size ).to.equal( 1 );
+			} );
 		} );
 
 		describe( 'with entire row selected', () => {

+ 120 - 1
packages/ckeditor5-table/tests/tablecellproperties/tablecellpropertiesediting.js

@@ -59,7 +59,7 @@ describe( 'table cell properties', () => {
 			expect( editor.commands.get( 'tableCellHorizontalAlignment' ) ).to.be.instanceOf( TableCellHorizontalAlignmentCommand );
 		} );
 
-		it( 'adds tableCellAlignment command', () => {
+		it( 'adds tableCellVerticalAlignment command', () => {
 			expect( editor.commands.get( 'tableCellVerticalAlignment' ) ).to.be.instanceOf( TableCellVerticalAlignmentCommand );
 		} );
 
@@ -712,6 +712,51 @@ describe( 'table cell properties', () => {
 
 					expect( tableCell.getAttribute( 'horizontalAlignment' ) ).to.equal( 'justify' );
 				} );
+
+				describe( 'for RTL content language', () => {
+					let editor, model;
+
+					beforeEach( async () => {
+						editor = await VirtualTestEditor.create( {
+							plugins: [ TableCellPropertiesEditing, Paragraph, TableEditing ],
+							language: 'ar'
+						} );
+
+						model = editor.model;
+					} );
+
+					afterEach( async () => {
+						await editor.destroy();
+					} );
+
+					it( 'should not upcast text-align:right style', () => {
+						editor.setData( '<table><tr><td style="text-align:right">foo</td></tr></table>' );
+						const tableCell = model.document.getRoot().getNodeByPath( [ 0, 0, 0 ] );
+
+						expect( tableCell.getAttribute( 'horizontalAlignment' ) ).to.be.undefined;
+					} );
+
+					it( 'should upcast text-align:left style', () => {
+						editor.setData( '<table><tr><td style="text-align:left">foo</td></tr></table>' );
+						const tableCell = model.document.getRoot().getNodeByPath( [ 0, 0, 0 ] );
+
+						expect( tableCell.getAttribute( 'horizontalAlignment' ) ).to.equal( 'left' );
+					} );
+
+					it( 'should upcast text-align:center style', () => {
+						editor.setData( '<table><tr><td style="text-align:center">foo</td></tr></table>' );
+						const tableCell = model.document.getRoot().getNodeByPath( [ 0, 0, 0 ] );
+
+						expect( tableCell.getAttribute( 'horizontalAlignment' ) ).to.equal( 'center' );
+					} );
+
+					it( 'should upcast text-align:justify style', () => {
+						editor.setData( '<table><tr><td style="text-align:justify">foo</td></tr></table>' );
+						const tableCell = model.document.getRoot().getNodeByPath( [ 0, 0, 0 ] );
+
+						expect( tableCell.getAttribute( 'horizontalAlignment' ) ).to.equal( 'justify' );
+					} );
+				} );
 			} );
 
 			describe( 'downcast conversion', () => {
@@ -774,6 +819,80 @@ describe( 'table cell properties', () => {
 
 					assertTableCellStyle( editor, 'text-align:justify;' );
 				} );
+
+				describe( 'for RTL content language', () => {
+					let editor, model;
+
+					beforeEach( async () => {
+						editor = await VirtualTestEditor.create( {
+							plugins: [ TableCellPropertiesEditing, Paragraph, TableEditing ],
+							language: 'ar'
+						} );
+
+						model = editor.model;
+
+						setModelData(
+							model,
+							'<table headingRows="0" headingColumns="0">' +
+								'<tableRow>' +
+									'<tableCell>' +
+										'<paragraph>foo</paragraph>' +
+									'</tableCell>' +
+								'</tableRow>' +
+							'</table>'
+						);
+
+						tableCell = model.document.getRoot().getNodeByPath( [ 0, 0, 0 ] );
+					} );
+
+					afterEach( async () => {
+						await editor.destroy();
+					} );
+
+					it( 'should consume converted item\'s horizontalAlignment attribute', () => {
+						editor.conversion.for( 'downcast' )
+							.add( dispatcher => dispatcher.on( 'attribute:horizontalAlignment:tableCell', ( evt, data, conversionApi ) => {
+								expect( conversionApi.consumable.consume( data.item, evt.name ) ).to.be.false;
+							} ) );
+
+						model.change( writer => writer.setAttribute( 'horizontalAlignment', 'center', tableCell ) );
+					} );
+
+					it( 'should be overridable', () => {
+						editor.conversion.for( 'downcast' )
+							.add( dispatcher => dispatcher.on( 'attribute:horizontalAlignment:tableCell', ( evt, data, conversionApi ) => {
+								conversionApi.consumable.consume( data.item, evt.name );
+							}, { priority: 'high' } ) );
+
+						model.change( writer => writer.setAttribute( 'horizontalAlignment', 'center', tableCell ) );
+
+						assertTableCellStyle( editor, '' );
+					} );
+
+					it( 'should not downcast horizontalAlignment=right', () => {
+						model.change( writer => writer.setAttribute( 'horizontalAlignment', 'right', tableCell ) );
+
+						assertTableCellStyle( editor );
+					} );
+
+					it( 'should downcast horizontalAlignment=left', () => {
+						model.change( writer => writer.setAttribute( 'horizontalAlignment', 'left', tableCell ) );
+
+						assertTableCellStyle( editor, 'text-align:left;' );
+					} );
+
+					it( 'should downcast horizontalAlignment=center', () => {
+						model.change( writer => writer.setAttribute( 'horizontalAlignment', 'center', tableCell ) );
+
+						assertTableCellStyle( editor, 'text-align:center;' );
+					} );
+
+					it( 'should downcast horizontalAlignment=justify', () => {
+						model.change( writer => writer.setAttribute( 'horizontalAlignment', 'justify', tableCell ) );
+
+						assertTableCellStyle( editor, 'text-align:justify;' );
+					} );
+				} );
 			} );
 		} );