浏览代码

Merge pull request #271 from ckeditor/i/6368

Fix: Merge left and right commands should be always enabled if the execution does not cross the heading column boundary. Closes ckeditor/ckeditor5#6368.
Marek Lewandowski 5 年之前
父节点
当前提交
69d48fdfc0

+ 9 - 7
packages/ckeditor5-table/src/commands/mergecellcommand.js

@@ -9,7 +9,10 @@
 
 import Command from '@ckeditor/ckeditor5-core/src/command';
 import TableWalker from '../tablewalker';
-import { updateNumericAttribute } from './utils';
+import {
+	updateNumericAttribute,
+	isHeadingColumnCell
+} from './utils';
 import { getTableCellsContainingSelection } from '../utils';
 
 /**
@@ -157,7 +160,7 @@ function getHorizontalCell( tableCell, direction, tableUtils ) {
 	const tableRow = tableCell.parent;
 	const table = tableRow.parent;
 	const horizontalCell = direction == 'right' ? tableCell.nextSibling : tableCell.previousSibling;
-	const headingColumns = table.getAttribute( 'headingColumns' ) || 0;
+	const hasHeadingColumns = ( table.getAttribute( 'headingColumns' ) || 0 ) > 0;
 
 	if ( !horizontalCell ) {
 		return;
@@ -172,13 +175,12 @@ function getHorizontalCell( tableCell, direction, tableUtils ) {
 	const { column: rightCellColumn } = tableUtils.getCellLocation( cellOnRight );
 
 	const leftCellSpan = parseInt( cellOnLeft.getAttribute( 'colspan' ) || 1 );
-	const rightCellSpan = parseInt( cellOnRight.getAttribute( 'colspan' ) || 1 );
 
-	// We cannot merge cells if the result will extend over heading section.
-	const isMergeWithBodyCell = direction == 'right' && ( rightCellColumn + rightCellSpan > headingColumns );
-	const isMergeWithHeadCell = direction == 'left' && ( leftCellColumn + leftCellSpan > headingColumns - 1 );
+	const isCellOnLeftInHeadingColumn = isHeadingColumnCell( tableUtils, cellOnLeft, table );
+	const isCellOnRightInHeadingColumn = isHeadingColumnCell( tableUtils, cellOnRight, table );
 
-	if ( headingColumns && ( isMergeWithBodyCell || isMergeWithHeadCell ) ) {
+	// We cannot merge heading columns cells with regular cells.
+	if ( hasHeadingColumns && isCellOnLeftInHeadingColumn != isCellOnRightInHeadingColumn ) {
 		return;
 	}
 

+ 6 - 21
packages/ckeditor5-table/src/commands/setheadercolumncommand.js

@@ -9,7 +9,10 @@
 
 import Command from '@ckeditor/ckeditor5-core/src/command';
 
-import { updateNumericAttribute } from './utils';
+import {
+	updateNumericAttribute,
+	isHeadingColumnCell
+} from './utils';
 import { getTableCellsContainingSelection } from '../utils';
 
 /**
@@ -35,7 +38,7 @@ export default class SetHeaderColumnCommand extends Command {
 		const model = this.editor.model;
 		const doc = model.document;
 		const tableCell = getTableCellsContainingSelection( doc.selection )[ 0 ];
-
+		const tableUtils = this.editor.plugins.get( 'TableUtils' );
 		const isInTable = !!tableCell;
 
 		this.isEnabled = isInTable;
@@ -48,7 +51,7 @@ export default class SetHeaderColumnCommand extends Command {
 		 * @readonly
 		 * @member {Boolean} #value
 		 */
-		this.value = isInTable && this._isInHeading( tableCell, tableCell.parent.parent );
+		this.value = isInTable && isHeadingColumnCell( tableUtils, tableCell );
 	}
 
 	/**
@@ -85,22 +88,4 @@ export default class SetHeaderColumnCommand extends Command {
 			updateNumericAttribute( 'headingColumns', headingColumnsToSet, table, writer, 0 );
 		} );
 	}
-
-	/**
-	 * Checks if a table cell is in the heading section.
-	 *
-	 * @param {module:engine/model/element~Element} tableCell
-	 * @param {module:engine/model/element~Element} table
-	 * @returns {Boolean}
-	 * @private
-	 */
-	_isInHeading( tableCell, table ) {
-		const headingColumns = parseInt( table.getAttribute( 'headingColumns' ) || 0 );
-
-		const tableUtils = this.editor.plugins.get( 'TableUtils' );
-
-		const { column } = tableUtils.getCellLocation( tableCell );
-
-		return !!headingColumns && column < headingColumns;
-	}
 }

+ 15 - 0
packages/ckeditor5-table/src/commands/utils.js

@@ -114,3 +114,18 @@ export function addDefaultUnitToNumericValue( value, defaultUnit ) {
 
 	return `${ numericValue }${ defaultUnit }`;
 }
+
+/**
+ * Checks if a table cell belongs to the heading column section.
+ *
+ * @param {module:table/tableutils~TableUtils} tableUtils
+ * @param {module:engine/model/element~Element} tableCell
+ * @returns {Boolean}
+ */
+export function isHeadingColumnCell( tableUtils, tableCell ) {
+	const table = tableCell.parent.parent;
+	const headingColumns = parseInt( table.getAttribute( 'headingColumns' ) || 0 );
+	const { column } = tableUtils.getCellLocation( tableCell );
+
+	return !!headingColumns && column < headingColumns;
+}

+ 60 - 40
packages/ckeditor5-table/tests/commands/mergecellcommand.js

@@ -95,44 +95,46 @@ describe( 'MergeCellCommand', () => {
 				expect( command.isEnabled ).to.be.false;
 			} );
 
-			it( 'should be false if mergeable cell is in other table section then current cell', () => {
-				setData( model, modelTable( [
-					[ '00[]', '01' ]
-				], { headingColumns: 1 } ) );
+			describe( 'when the heading section is in the table', () => {
+				it( 'should be false if mergeable cell is in other table section then current cell', () => {
+					setData( model, modelTable( [
+						[ '00[]', '01' ]
+					], { headingColumns: 1 } ) );
 
-				expect( command.isEnabled ).to.be.false;
-			} );
+					expect( command.isEnabled ).to.be.false;
+				} );
 
-			it( 'should be false if merged cell would cross heading section (mergeable cell with colspan)', () => {
-				setData( model, modelTable( [
-					[ '00[]', { colspan: 2, contents: '01' }, '02', '03' ]
-				], { headingColumns: 2 } ) );
+				it( 'should be true if merged cell would not cross heading section (mergeable cell with colspan)', () => {
+					setData( model, modelTable( [
+						[ '00[]', { colspan: 2, contents: '01' }, '02', '03' ]
+					], { headingColumns: 3 } ) );
 
-				expect( command.isEnabled ).to.be.false;
-			} );
+					expect( command.isEnabled ).to.be.true;
+				} );
 
-			it( 'should be true if merged cell would not cross heading section (mergeable cell with colspan)', () => {
-				setData( model, modelTable( [
-					[ '00[]', { colspan: 2, contents: '01' }, '02', '03' ]
-				], { headingColumns: 3 } ) );
+				it( 'should be false if merged cell would cross heading section (current cell with colspan)', () => {
+					setData( model, modelTable( [
+						[ { colspan: 2, contents: '00[]' }, '01', '02', '03' ]
+					], { headingColumns: 2 } ) );
 
-				expect( command.isEnabled ).to.be.true;
-			} );
+					expect( command.isEnabled ).to.be.false;
+				} );
 
-			it( 'should be false if merged cell would cross heading section (current cell with colspan)', () => {
-				setData( model, modelTable( [
-					[ { colspan: 2, contents: '00[]' }, '01', '02', '03' ]
-				], { headingColumns: 2 } ) );
+				it( 'should be true if merged cell would not cross heading section (current cell with colspan)', () => {
+					setData( model, modelTable( [
+						[ { colspan: 2, contents: '00[]' }, '01', '02', '03' ]
+					], { headingColumns: 3 } ) );
 
-				expect( command.isEnabled ).to.be.false;
-			} );
+					expect( command.isEnabled ).to.be.true;
+				} );
 
-			it( 'should be true if merged cell would not cross heading section (current cell with colspan)', () => {
-				setData( model, modelTable( [
-					[ { colspan: 2, contents: '00[]' }, '01', '02', '03' ]
-				], { headingColumns: 3 } ) );
+				it( 'should be true if merged cell would not cross the section boundary (regular section)', () => {
+					setData( model, modelTable( [
+						[ '00', '01', '02[]', '03' ]
+					], { headingColumns: 1 } ) );
 
-				expect( command.isEnabled ).to.be.true;
+					expect( command.isEnabled ).to.be.true;
+				} );
 			} );
 		} );
 
@@ -315,20 +317,38 @@ describe( 'MergeCellCommand', () => {
 				expect( command.isEnabled ).to.be.false;
 			} );
 
-			it( 'should be false if mergeable cell is in other table section then current cell', () => {
-				setData( model, modelTable( [
-					[ '00', '01[]' ]
-				], { headingColumns: 1 } ) );
+			describe( 'when the heading section is in the table', () => {
+				it( 'should be false if mergeable cell is in other table section then current cell', () => {
+					setData( model, modelTable( [
+						[ '00', '01[]' ]
+					], { headingColumns: 1 } ) );
 
-				expect( command.isEnabled ).to.be.false;
-			} );
+					expect( command.isEnabled ).to.be.false;
+				} );
 
-			it( 'should be false if merged cell would cross heading section (mergeable cell with colspan)', () => {
-				setData( model, modelTable( [
-					[ { colspan: 2, contents: '00' }, '02[]', '03' ]
-				], { headingColumns: 2 } ) );
+				it( 'should be false if merged cell would cross heading section (mergeable cell with colspan)', () => {
+					setData( model, modelTable( [
+						[ { colspan: 2, contents: '00' }, '02[]', '03' ]
+					], { headingColumns: 2 } ) );
 
-				expect( command.isEnabled ).to.be.false;
+					expect( command.isEnabled ).to.be.false;
+				} );
+
+				it( 'should be true if merged cell would not cross the section boundary (in regular section)', () => {
+					setData( model, modelTable( [
+						[ '00', '01', '02[]', '03' ]
+					], { headingColumns: 1 } ) );
+
+					expect( command.isEnabled ).to.be.true;
+				} );
+
+				it( 'should be true if merged cell would not cross the section boundary (in heading section)', () => {
+					setData( model, modelTable( [
+						[ '00', '01[]', '02', '03' ]
+					], { headingColumns: 2 } ) );
+
+					expect( command.isEnabled ).to.be.true;
+				} );
 			} );
 		} );
 

+ 52 - 6
packages/ckeditor5-table/tests/commands/utils.js

@@ -4,20 +4,24 @@
  */
 
 import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor';
+import TableUtils from '../../src/tableutils';
 import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
-
 import { defaultConversion, defaultSchema, modelTable } from '../_utils/utils';
-
-import { findAncestor } from '../../src/commands/utils';
+import { findAncestor, isHeadingColumnCell } from '../../src/commands/utils';
 
 describe( 'commands utils', () => {
-	let editor, model;
+	let editor, model, modelRoot, tableUtils;
 
 	beforeEach( () => {
-		return ModelTestEditor.create()
+		return ModelTestEditor
+			.create( {
+				plugins: [ TableUtils ]
+			} )
 			.then( newEditor => {
 				editor = newEditor;
 				model = editor.model;
+				modelRoot = model.document.getRoot();
+				tableUtils = editor.plugins.get( TableUtils );
 
 				defaultSchema( model.schema );
 				defaultConversion( editor.conversion );
@@ -28,7 +32,7 @@ describe( 'commands utils', () => {
 		return editor.destroy();
 	} );
 
-	describe( 'getParentTable()', () => {
+	describe( 'findAncestor()', () => {
 		it( 'should return undefined if not in table', () => {
 			setData( model, '<paragraph>foo[]</paragraph>' );
 
@@ -44,4 +48,46 @@ describe( 'commands utils', () => {
 			expect( parentTable.is( 'table' ) ).to.be.true;
 		} );
 	} );
+
+	describe( 'isHeadingColumnCell()', () => {
+		it( 'should return "true" for a heading column cell', () => {
+			setData( model, modelTable( [
+				[ '00', '01', '02', '03' ]
+			], { headingColumns: 2 } ) );
+
+			const tableCell = modelRoot.getNodeByPath( [ 0, 0, 1 ] );
+
+			expect( isHeadingColumnCell( tableUtils, tableCell ) ).to.be.true;
+		} );
+
+		it( 'should return "true" for a heading column cell with colspan', () => {
+			setData( model, modelTable( [
+				[ { colspan: 2, contents: '00' }, '01', '02', '03' ]
+			], { headingColumns: 2 } ) );
+
+			const tableCell = modelRoot.getNodeByPath( [ 0, 0, 0 ] );
+
+			expect( isHeadingColumnCell( tableUtils, tableCell ) ).to.be.true;
+		} );
+
+		it( 'should return "false" for a regular column cell', () => {
+			setData( model, modelTable( [
+				[ '00', '01', '02', '03' ]
+			], { headingColumns: 2 } ) );
+
+			const tableCell = modelRoot.getNodeByPath( [ 0, 0, 2 ] );
+
+			expect( isHeadingColumnCell( tableUtils, tableCell ) ).to.be.false;
+		} );
+
+		it( 'should return "false" for a regular column cell with colspan', () => {
+			setData( model, modelTable( [
+				[ '00', { colspan: 2, contents: '01' }, '02', '03' ]
+			], { headingColumns: 1 } ) );
+
+			const tableCell = modelRoot.getNodeByPath( [ 0, 0, 1 ] );
+
+			expect( isHeadingColumnCell( tableUtils, tableCell ) ).to.be.false;
+		} );
+	} );
 } );