Merge branch 'master' into t/40

Author: oleq

src/commands/mergecellcommand.js

@@ -158,16 +158,26 @@ function getVerticalCell( tableCell, direction ) {
 		return;
 	}
 
-	const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) || 1 );
-	const mergeRow = direction == 'down' ? rowIndex + rowspan : rowIndex;
+	const currentCellRowSpan = parseInt( tableCell.getAttribute( 'rowspan' ) || 1 );
+	const rowOfCellToMerge = direction == 'down' ? rowIndex + currentCellRowSpan : rowIndex;
 
-	const tableMap = [ ...new TableWalker( table, { endRow: mergeRow } ) ];
+	const tableMap = [ ...new TableWalker( table, { endRow: rowOfCellToMerge } ) ];
 
 	const currentCellData = tableMap.find( value => value.cell === tableCell );
 	const mergeColumn = currentCellData.column;
 
-	const cellToMergeData = tableMap.find( ( { row, column } ) => {
-		return column === mergeColumn && ( direction == 'down' ? mergeRow === row : mergeRow === rowspan + row );
+	const cellToMergeData = tableMap.find( ( { row, rowspan, column } ) => {
+		if ( column !== mergeColumn ) {
+			return false;
+		}
+
+		if ( direction == 'down' ) {
+			// If merging a cell below the mergeRow is already calculated.
+			return row === rowOfCellToMerge;
+		} else {
+			// If merging a cell above calculate if it spans to mergeRow.
+			return rowOfCellToMerge === row + rowspan;
+		}
 	} );
 
 	return cellToMergeData && cellToMergeData.cell;

src/table.js

@@ -13,6 +13,8 @@ import TableEditing from './tableediting';
 import TableUI from './tableui';
 import Widget from '@ckeditor/ckeditor5-widget/src/widget';
 
+import '../theme/table.css';
+
 /**
  * The table plugin.
  *

src/tableediting.js

@@ -33,7 +33,7 @@ import SetHeaderColumnCommand from './commands/setheadercolumncommand';
 import { getParentTable } from './commands/utils';
 import TableUtils from './tableutils';
 
-import './../theme/table.css';
+import '../theme/tableediting.css';
 
 /**
  * The table editing feature.

src/tabletoolbar.js

@@ -112,9 +112,9 @@ export default class TableToolbar extends Plugin {
 		if ( !editor.ui.focusTracker.isFocused ) {
 			this._hideToolbar();
 		} else {
-			const viewSeleciton = editor.editing.view.document.selection;
+			const viewSelection = editor.editing.view.document.selection;
 
-			if ( isTableWidgetSelected( viewSeleciton ) || isTableContentSelected( viewSeleciton ) ) {
+			if ( isTableContentSelected( viewSelection ) ) {
 				this._showToolbar();
 			} else {
 				this._hideToolbar();

src/tableui.js

@@ -75,6 +75,7 @@ export default class TableUI extends Plugin {
 		editor.ui.componentFactory.add( 'tableColumn', locale => {
 			const options = [
 				{ commandName: 'setColumnHeader', label: t( 'Header column' ), bindIsActive: true },
+				'|',
 				{ commandName: 'insertColumnBefore', label: t( 'Insert column before' ) },
 				{ commandName: 'insertColumnAfter', label: t( 'Insert column after' ) },
 				{ commandName: 'removeColumn', label: t( 'Delete column' ) }
@@ -86,6 +87,7 @@ export default class TableUI extends Plugin {
 		editor.ui.componentFactory.add( 'tableRow', locale => {
 			const options = [
 				{ commandName: 'setRowHeader', label: t( 'Header row' ), bindIsActive: true },
+				'|',
 				{ commandName: 'insertRowBelow', label: t( 'Insert row below' ) },
 				{ commandName: 'insertRowAbove', label: t( 'Insert row above' ) },
 				{ commandName: 'removeRow', label: t( 'Delete row' ) }
@@ -100,6 +102,7 @@ export default class TableUI extends Plugin {
 				{ commandName: 'mergeCellRight', label: t( 'Merge cell right' ) },
 				{ commandName: 'mergeCellDown', label: t( 'Merge cell down' ) },
 				{ commandName: 'mergeCellLeft', label: t( 'Merge cell left' ) },
+				'|',
 				{ commandName: 'splitCellVertically', label: t( 'Split cell vertically' ) },
 				{ commandName: 'splitCellHorizontally', label: t( 'Split cell horizontally' ) }
 			];
@@ -161,20 +164,28 @@ export default class TableUI extends Plugin {
 // @param {Array.<module:core/command~Command>} commands List of commands to update.
 // @param {module:utils/collection~Collection} dropdownItems Collection of dropdown items to update with given option.
 function addListOption( option, editor, commands, dropdownItems ) {
-	const { commandName, label, bindIsActive } = option;
-	const command = editor.commands.get( commandName );
+	const itemModel = new Model();
 
-	commands.push( command );
+	if ( option === '|' ) {
+		itemModel.set( {
+			isSeparator: true
+		} );
+	} else {
+		const { commandName, label, bindIsActive } = option;
+		const command = editor.commands.get( commandName );
+
+		commands.push( command );
 
-	const itemModel = new Model( {
-		commandName,
-		label
-	} );
+		itemModel.set( {
+			commandName,
+			label
+		} );
 
-	itemModel.bind( 'isEnabled' ).to( command );
+		itemModel.bind( 'isEnabled' ).to( command );
 
-	if ( bindIsActive ) {
-		itemModel.bind( 'isActive' ).to( command, 'value' );
+		if ( bindIsActive ) {
+			itemModel.bind( 'isActive' ).to( command, 'value' );
+		}
 	}
 
 	dropdownItems.add( itemModel );

src/ui/inserttableview.js

@@ -89,6 +89,10 @@ export default class InsertTableView extends View {
 			],
 
 			on: {
+				mousedown: bind.to( evt => {
+					evt.preventDefault();
+				} ),
+
 				click: bind.to( () => {
 					this.fire( 'execute' );
 				} )

src/ui/utils.js

@@ -9,7 +9,6 @@
 
 import BalloonPanelView from '@ckeditor/ckeditor5-ui/src/panel/balloon/balloonpanelview';
 import { getParentTable } from '../commands/utils';
-import { isTableWidgetSelected } from '../utils';
 
 /**
  * A helper utility that positions the
@@ -36,13 +35,8 @@ export function getBalloonPositionData( editor ) {
 	const editingView = editor.editing.view;
 	const defaultPositions = BalloonPanelView.defaultPositions;
 	const viewSelection = editingView.document.selection;
-	let parentTable;
 
-	if ( isTableWidgetSelected( viewSelection ) ) {
-		parentTable = viewSelection.getSelectedElement();
-	} else {
-		parentTable = getParentTable( viewSelection.getFirstPosition() );
-	}
+	const parentTable = getParentTable( viewSelection.getFirstPosition() );
 
 	return {
 		target: editingView.domConverter.viewToDom( parentTable ),

tests/commands/mergecellcommand.js

@@ -4,7 +4,7 @@
  */
 
 import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor';
-import { setData, getData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
+import { getData, setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 import { upcastElementToElement } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters';
 
 import MergeCellCommand from '../../src/commands/mergecellcommand';
@@ -483,6 +483,17 @@ describe( 'MergeCellCommand', () => {
 				expect( command.value ).to.equal( root.getNodeByPath( [ 0, 0, 0 ] ) );
 			} );
 
+			it( 'should be set to mergeable cell in rows with spanned cells', () => {
+				setData( model, modelTable( [
+					[ { rowspan: 3, contents: '00' }, '11', '12', '13' ],
+					[ { rowspan: 2, contents: '21' }, '22', '23' ],
+					[ '32', { rowspan: 2, contents: '33[]' } ],
+					[ { colspan: 2, contents: '40' }, '42' ]
+				] ) );
+
+				expect( command.value ).to.equal( root.getNodeByPath( [ 0, 1, 2 ] ) );
+			} );
+
 			it( 'should be undefined if in a cell that potential mergeable cell has different rowspan', () => {
 				setData( model, modelTable( [
 					[ { colspan: 2, contents: '00' }, '02' ],
@@ -500,10 +511,10 @@ describe( 'MergeCellCommand', () => {
 		} );
 
 		describe( 'execute()', () => {
-			it( 'should merge table cells ', () => {
+			it( 'should merge table cells', () => {
 				setData( model, modelTable( [
 					[ '00', '01' ],
-					[ '10', '11[]' ]
+					[ '10', '[]11' ]
 				] ) );
 
 				command.execute();
@@ -513,6 +524,24 @@ describe( 'MergeCellCommand', () => {
 					[ '10' ]
 				] ) );
 			} );
+
+			it( 'should properly merge cells in rows with spaned cells', () => {
+				setData( model, modelTable( [
+					[ { rowspan: 3, contents: '00' }, '11', '12', '13' ],
+					[ { rowspan: 2, contents: '21' }, '22', '23' ],
+					[ '32', { rowspan: 2, contents: '33[]' } ],
+					[ { colspan: 2, contents: '40' }, '42' ]
+				] ) );
+
+				command.execute();
+
+				expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
+					[ { rowspan: 3, contents: '00' }, '11', '12', '13' ],
+					[ { rowspan: 2, contents: '21' }, '22', { rowspan: 3, contents: '[2333]' } ],
+					[ '32' ],
+					[ { colspan: 2, contents: '40' }, '42' ]
+				] ) );
+			} );
 		} );
 	} );
 } );

tests/manual/table.html

@@ -1,28 +1,7 @@
 <style>
-
-	table {
-		border-collapse: collapse;
-		border-spacing: 0;
-		border-color: hsl(0,0%,87%);
-	}
-
-	table, th, td {
-		padding: 5px;
-		border: 1px inset hsl(0,0%,87%);
-	}
-
-	table th,
-	table td {
-		min-width: 1em;
-	}
-
-	table th {
-		background: hsl(0,0%,96%);
-		font-weight: bold;
-	}
-
-	table thead th {
-		background: hsl(0,0%,90%);
+	body {
+		font-family: Helvetica, Arial, sans-serif;
+		font-size: 14px;
 	}
 </style>
 
@@ -47,7 +26,7 @@
 		</thead>
 		<tbody>
 		<tr>
-			<th colspan="2" rowspan="4" scope="rowgroup">Terrestial planets</th>
+			<th colspan="2" rowspan="4" scope="rowgroup">Terrestrial planets</th>
 			<th scope="row">Mercury</th>
 			<td>0.330</td>
 			<td>4,879</td>

tests/tabletoolbar.js

@@ -117,10 +117,10 @@ describe( 'TableToolbar', () => {
 			editor.ui.focusTracker.isFocused = true;
 		} );
 
-		it( 'should show the toolbar on render when the table is selected', () => {
+		it( 'should not show the toolbar on render when the table is selected', () => {
 			setData( model, '<paragraph>foo</paragraph>[<table><tableRow><tableCell></tableCell></tableRow></table>]' );
 
-			expect( balloon.visibleView ).to.equal( toolbar );
+			expect( balloon.visibleView ).to.be.null;
 		} );
 
 		it( 'should show the toolbar on render when the table content is selected', () => {