Merge branch 'master' into i/3267

src/commands/mergecellscommand.js

@@ -11,7 +11,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command';
 import TableWalker from '../tablewalker';
 import { findAncestor, updateNumericAttribute } from './utils';
 import TableUtils from '../tableutils';
-import { getRowIndexes, getSelectedTableCells } from '../utils';
+import { getColumnIndexes, getRowIndexes, getSelectedTableCells } from '../utils';
 
 /**
  * The merge cells command.
@@ -47,6 +47,11 @@ export default class MergeCellsCommand extends Command {
 			// All cells will be merge into the first one.
 			const firstTableCell = selectedTableCells.shift();
 
+			// Set the selection in cell that other cells are being merged to prevent model-selection-range-intersects error in undo.
+			// See https://github.com/ckeditor/ckeditor5/issues/6634.
+			// May be fixed by: https://github.com/ckeditor/ckeditor5/issues/6639.
+			writer.setSelection( firstTableCell, 0 );
+
 			// Update target cell dimensions.
 			const { mergeWidth, mergeHeight } = getMergeDimensions( firstTableCell, selectedTableCells, tableUtils );
 			updateNumericAttribute( 'colspan', mergeWidth, firstTableCell, writer );
@@ -199,16 +204,45 @@ function getBiggestRectangleArea( rows, columns ) {
 	return ( lastRow - firstRow + 1 ) * ( lastColumn - firstColumn + 1 );
 }
 
+// Checks if the selection does not mix header (column or row) with other cells.
+//
+// For instance, in the table below valid selections consist of cells with the same letter only.
+// So, a-a (same heading row and column) or d-d (body cells) are valid while c-d or a-b are not.
+//
+//    header columns
+//     ↓   ↓
+//   ┌───┬───┬───┬───┐
+//   │ a │ a │ b │ b │  ← header row
+//   ├───┼───┼───┼───┤
+//   │ c │ c │ d │ d │
+//   ├───┼───┼───┼───┤
+//   │ c │ c │ d │ d │
+//   └───┴───┴───┴───┘
+//
 function areCellInTheSameTableSection( tableCells ) {
 	const table = findAncestor( 'table', tableCells[ 0 ] );
 
 	const rowIndexes = getRowIndexes( tableCells );
 	const headingRows = parseInt( table.getAttribute( 'headingRows' ) || 0 );
 
-	const firstCellIsInBody = rowIndexes.first > headingRows - 1;
-	const lastCellIsInBody = rowIndexes.last > headingRows - 1;
+	// Calculating row indexes is a bit cheaper so if this check fails we can't merge.
+	if ( !areIndexesInSameSection( rowIndexes, headingRows ) ) {
+		return false;
+	}
+
+	const headingColumns = parseInt( table.getAttribute( 'headingColumns' ) || 0 );
+	const columnIndexes = getColumnIndexes( tableCells );
+
+	// Similarly cells must be in same column section.
+	return areIndexesInSameSection( columnIndexes, headingColumns );
+}
+
+// Unified check if table rows/columns indexes are in the same heading/body section.
+function areIndexesInSameSection( { first, last }, headingSectionSize ) {
+	const firstCellIsInHeading = first < headingSectionSize;
+	const lastCellIsInHeading = last < headingSectionSize;
 
-	return firstCellIsInBody === lastCellIsInBody;
+	return firstCellIsInHeading === lastCellIsInHeading;
 }
 
 function getMergeDimensions( firstTableCell, selectedTableCells, tableUtils ) {

src/tableui.js

@@ -278,19 +278,10 @@ export default class TableUI extends Plugin {
 		dropdownView.buttonView.set( {
 			label,
 			icon,
-			tooltip: true
+			tooltip: true,
+			isEnabled: true
 		} );
 
-		// The main part of the split button is bound to the "mergeTableCells" command only.
-		dropdownView.bind( 'isEnabled' ).to( editor.commands.get( mergeCommandName ) );
-
-		// The split button dropdown must be **always** enabled and ready to open no matter the state
-		// of the "mergeTableCells" command. You may not be able to merge multiple cells but you may want
-		// to split them. This is also about mobile devices where multi–cell selection will never work
-		// (that's why "Merge cell right", "Merge cell down", etc. are still there in the first place).
-		dropdownView.buttonView.arrowView.unbind( 'isEnabled' );
-		dropdownView.buttonView.arrowView.isEnabled = true;
-
 		// Merge selected table cells when the main part of the split button is clicked.
 		this.listenTo( dropdownView.buttonView, 'execute', () => {
 			editor.execute( mergeCommandName );

tests/commands/mergecellscommand.js

@@ -237,6 +237,76 @@ describe( 'MergeCellsCommand', () => {
 
 			expect( command.isEnabled ).to.be.false;
 		} );
+
+		it( 'should be true if selection has cells only from column headers - rows in body section', () => {
+			setData( model, modelTable( [
+				[ '00[]', '01', '02', '03' ],
+				[ '10', '11', '12', '13' ]
+			], { headingColumns: 2 } ) );
+
+			tableSelection._setCellSelection(
+				root.getNodeByPath( [ 0, 0, 0 ] ),
+				root.getNodeByPath( [ 0, 1, 1 ] )
+			);
+
+			expect( command.isEnabled ).to.be.true;
+		} );
+
+		it( 'should be false if selection has cells from column headers and other cells - rows in body section', () => {
+			setData( model, modelTable( [
+				[ '00[]', '01', '02', '03' ],
+				[ '10', '11', '12', '13' ]
+			], { headingColumns: 2 } ) );
+
+			tableSelection._setCellSelection(
+				root.getNodeByPath( [ 0, 0, 0 ] ),
+				root.getNodeByPath( [ 0, 1, 2 ] )
+			);
+
+			expect( command.isEnabled ).to.be.false;
+		} );
+
+		it( 'should be true if selection has cells only from column headers - rows in header section', () => {
+			setData( model, modelTable( [
+				[ '00[]', '01', '02', '03' ],
+				[ '10', '11', '12', '13' ]
+			], { headingColumns: 2, headingRows: 1 } ) );
+
+			tableSelection._setCellSelection(
+				root.getNodeByPath( [ 0, 0, 0 ] ),
+				root.getNodeByPath( [ 0, 0, 1 ] )
+			);
+
+			expect( command.isEnabled ).to.be.true;
+		} );
+
+		it( 'should be false if selection has cells only from column headers and other cells - rows in header section', () => {
+			setData( model, modelTable( [
+				[ '00[]', '01', '02', '03' ],
+				[ '10', '11', '12', '13' ]
+			], { headingColumns: 2, headingRows: 1 } ) );
+
+			tableSelection._setCellSelection(
+				root.getNodeByPath( [ 0, 0, 0 ] ),
+				root.getNodeByPath( [ 0, 0, 2 ] )
+			);
+
+			expect( command.isEnabled ).to.be.false;
+		} );
+
+		it( 'should be false if selection has cells from column headers, row headers and body sections', () => {
+			setData( model, modelTable( [
+				[ '00[]', '01', '02', '03' ],
+				[ '10', '11', '12', '13' ]
+			], { headingColumns: 2, headingRows: 1 } ) );
+
+			tableSelection._setCellSelection(
+				root.getNodeByPath( [ 0, 0, 0 ] ),
+				root.getNodeByPath( [ 0, 1, 2 ] )
+			);
+
+			expect( command.isEnabled ).to.be.false;
+		} );
 	} );
 
 	describe( 'execute()', () => {

tests/tableselection-integration.js

@@ -24,6 +24,7 @@ import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventd
 import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard';
 import Input from '@ckeditor/ckeditor5-typing/src/input';
 import ViewText from '@ckeditor/ckeditor5-engine/src/view/text';
+import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting';
 
 describe( 'TableSelection - integration', () => {
 	let editor, model, tableSelection, modelRoot, element, viewDocument;
@@ -221,6 +222,34 @@ describe( 'TableSelection - integration', () => {
 		} );
 	} );
 
+	describe( 'with undo', () => {
+		beforeEach( async () => {
+			await setupEditor( [ UndoEditing ] );
+		} );
+
+		// See https://github.com/ckeditor/ckeditor5/issues/6634.
+		it( 'works with merge cells command', () => {
+			setModelData( editor.model, modelTable( [ [ '00', '01' ] ] ) );
+
+			tableSelection._setCellSelection(
+				modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
+				modelRoot.getNodeByPath( [ 0, 0, 1 ] )
+			);
+
+			editor.execute( 'mergeTableCells' );
+
+			assertEqualMarkup( getModelData( model ), modelTable( [
+				[ { colspan: 2, contents: '<paragraph>[00</paragraph><paragraph>01]</paragraph>' } ]
+			] ) );
+
+			editor.execute( 'undo' );
+
+			assertEqualMarkup( getModelData( model ), modelTable( [
+				[ '[]00', '01' ]
+			] ) );
+		} );
+	} );
+
 	async function setupEditor( plugins ) {
 		element = document.createElement( 'div' );
 		document.body.appendChild( element );

tests/tableui.js

@@ -386,9 +386,9 @@ describe( 'TableUI', () => {
 			expect( dropdown.buttonView ).to.be.instanceOf( SplitButtonView );
 		} );
 
-		it( 'should bind #isEnabled to the "mergeTableCells" command', () => {
+		it( 'should have #isEnabled always true regardless of the "mergeTableCells" command state', () => {
 			command.isEnabled = false;
-			expect( dropdown.isEnabled ).to.be.false;
+			expect( dropdown.isEnabled ).to.be.true;
 
 			command.isEnabled = true;
 			expect( dropdown.isEnabled ).to.be.true;

theme/table.css

@@ -39,3 +39,13 @@
 		}
 	}
 }
+
+/* Text alignment of the table header should match the editor settings and override the native browser styling,
+when content is available outside the ediitor. See https://github.com/ckeditor/ckeditor5/issues/6638 */
+.ck-content[dir="rtl"] .table th {
+	text-align: right;
+}
+
+.ck-content[dir="ltr"] .table th {
+	text-align: left;
+}