Merge pull request #90 from ckeditor/t/34

Fix: Toggling headers should always include the column or row the selection is anchored to. Closes #34.

Author: oleq

src/commands/setheadercolumncommand.js

@@ -74,15 +74,12 @@ export default class SetHeaderColumnCommand extends Command {
 		const table = tableRow.parent;
 
 		const currentHeadingColumns = parseInt( table.getAttribute( 'headingColumns' ) || 0 );
+		const { column: selectionColumn } = tableUtils.getCellLocation( tableCell );
 
-		let { column } = tableUtils.getCellLocation( tableCell );
-
-		if ( column + 1 !== currentHeadingColumns ) {
-			column++;
-		}
+		const headingColumnsToSet = currentHeadingColumns > selectionColumn ? selectionColumn : selectionColumn + 1;
 
 		model.change( writer => {
-			updateNumericAttribute( 'headingColumns', column, table, writer, 0 );
+			updateNumericAttribute( 'headingColumns', headingColumnsToSet, table, writer, 0 );
 		} );
 	}
 

src/commands/setheaderrowcommand.js

@@ -74,24 +74,22 @@ export default class SetHeaderRowCommand extends Command {
 		const table = tableRow.parent;
 
 		const currentHeadingRows = table.getAttribute( 'headingRows' ) || 0;
-		let rowIndex = tableRow.index;
+		const selectionRow = tableRow.index;
 
-		if ( rowIndex + 1 !== currentHeadingRows ) {
-			rowIndex++;
-		}
+		const headingRowsToSet = currentHeadingRows > selectionRow ? selectionRow : selectionRow + 1;
 
 		model.change( writer => {
-			if ( rowIndex ) {
+			if ( headingRowsToSet ) {
 				// Changing heading rows requires to check if any of a heading cell is overlapping vertically the table head.
 				// Any table cell that has a rowspan attribute > 1 will not exceed the table head so we need to fix it in rows below.
-				const cellsToSplit = getOverlappingCells( table, rowIndex, currentHeadingRows );
+				const cellsToSplit = getOverlappingCells( table, headingRowsToSet, currentHeadingRows );
 
 				for ( const cell of cellsToSplit ) {
-					splitHorizontally( cell, rowIndex, writer );
+					splitHorizontally( cell, headingRowsToSet, writer );
 				}
 			}
 
-			updateNumericAttribute( 'headingRows', rowIndex, table, writer, 0 );
+			updateNumericAttribute( 'headingRows', headingRowsToSet, table, writer, 0 );
 		} );
 	}
 

tests/commands/setheadercolumncommand.js

@@ -133,60 +133,43 @@ describe( 'HeaderColumnCommand', () => {
 	describe( 'execute()', () => {
 		it( 'should set heading columns attribute that cover column in which is selection', () => {
 			setData( model, modelTable( [
-				[ '00', '01' ],
-				[ '[]10', '11' ],
-				[ '20', '21' ]
+				[ '00', '01[]', '02', '03' ]
 			] ) );
 
 			command.execute();
 
 			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
-				[ '00', '01' ],
-				[ '[]10', '11' ],
-				[ '20', '21' ]
-			], { headingColumns: 1 } ) );
+				[ '00', '01[]', '02', '03' ]
+			], { headingColumns: 2 } ) );
 		} );
 
-		it(
-			'should set heading columns attribute if currently selected column is a heading so the heading section is before this column',
-			() => {
-				setData( model, modelTable( [
-					[ '00', '01' ],
-					[ '[]10', '11' ],
-					[ '20', '21' ]
-				], { headingColumns: 2 } ) );
-
-				command.execute();
-
-				expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
-					[ '00', '01' ],
-					[ '[]10', '11' ],
-					[ '20', '21' ]
-				], { headingColumns: 1 } ) );
-			}
-		);
+		it( 'should set heading columns attribute below current selection column', () => {
+			setData( model, modelTable( [
+				[ '00', '01[]', '02', '03' ]
+			], { headingColumns: 3 } ) );
+
+			command.execute();
+
+			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
+				[ '00', '01[]', '02', '03' ]
+			], { headingColumns: 1 } ) );
+		} );
 
 		it( 'should toggle of selected column', () => {
 			setData( model, modelTable( [
-				[ '00', '01' ],
-				[ '10', '11[]' ],
-				[ '20', '21' ]
+				[ '00', '01[]', '02', '03' ]
 			], { headingColumns: 2 } ) );
 
 			command.execute();
 
 			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
-				[ '00', '01' ],
-				[ '10', '11[]' ],
-				[ '20', '21' ]
+				[ '00', '01[]', '02', '03' ]
 			], { headingColumns: 1 } ) );
 
 			command.execute();
 
 			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
-				[ '00', '01' ],
-				[ '10', '11[]' ],
-				[ '20', '21' ]
+				[ '00', '01[]', '02', '03' ]
 			], { headingColumns: 2 } ) );
 		} );
 	} );

tests/commands/setheaderrowcommand.js

@@ -135,60 +135,85 @@ describe( 'HeaderRowCommand', () => {
 	describe( 'execute()', () => {
 		it( 'should set heading rows attribute that cover row in which is selection', () => {
 			setData( model, modelTable( [
-				[ '00', '01' ],
-				[ '[]10', '11' ],
-				[ '20', '21' ]
+				[ '00' ],
+				[ '[]10' ],
+				[ '20' ],
+				[ '30' ]
 			] ) );
 
 			command.execute();
 
 			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
-				[ '00', '01' ],
-				[ '[]10', '11' ],
-				[ '20', '21' ]
+				[ '00' ],
+				[ '[]10' ],
+				[ '20' ],
+				[ '30' ]
 			], { headingRows: 2 } ) );
 		} );
 
 		it( 'should toggle heading rows attribute', () => {
 			setData( model, modelTable( [
-				[ '[]00', '01' ],
-				[ '10', '11' ],
-				[ '20', '21' ]
-			] ) );
+				[ '00' ],
+				[ '[]10' ],
+				[ '20' ],
+				[ '30' ]
+			], { headingRows: 2 } ) );
 
 			command.execute();
 
 			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
-				[ '[]00', '01' ],
-				[ '10', '11' ],
-				[ '20', '21' ]
+				[ '00' ],
+				[ '[]10' ],
+				[ '20' ],
+				[ '30' ]
 			], { headingRows: 1 } ) );
 
 			command.execute();
 
 			setData( model, modelTable( [
-				[ '[]00', '01' ],
-				[ '10', '11' ],
-				[ '20', '21' ]
-			] ) );
+				[ '00' ],
+				[ '[]10' ],
+				[ '20' ],
+				[ '30' ]
+			], { headingRows: 2 } ) );
 		} );
 
 		it( 'should set heading rows attribute if currently selected row is a heading so the heading section is below this row', () => {
 			setData( model, modelTable( [
-				[ '00', '01' ],
-				[ '[]10', '11' ],
-				[ '20', '21' ]
-			], { headingRows: 2 } ) );
+				[ '00' ],
+				[ '[]10' ],
+				[ '20' ],
+				[ '30' ]
+			], { headingRows: 3 } ) );
 
 			command.execute();
 
 			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
-				[ '00', '01' ],
-				[ '[]10', '11' ],
-				[ '20', '21' ]
+				[ '00' ],
+				[ '[]10' ],
+				[ '20' ],
+				[ '30' ]
 			], { headingRows: 1 } ) );
 		} );
 
+		it( 'should unsetset heading rows attribute', () => {
+			setData( model, modelTable( [
+				[ '[]00' ],
+				[ '10' ],
+				[ '20' ],
+				[ '30' ]
+			], { headingRows: 3 } ) );
+
+			command.execute();
+
+			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
+				[ '[]00' ],
+				[ '10' ],
+				[ '20' ],
+				[ '30' ]
+			] ) );
+		} );
+
 		it( 'should fix rowspaned cells on the edge of an table head section', () => {
 			setData( model, modelTable( [
 				[ '00', '01', '02' ],
@@ -229,29 +254,29 @@ describe( 'HeaderRowCommand', () => {
 
 		it( 'should fix rowspaned cells on the edge of an table head section when creating section', () => {
 			setData( model, modelTable( [
-				[ { rowspan: 2, contents: '[]00' }, '01' ],
-				[ '11' ]
+				[ { rowspan: 2, contents: '00' }, '01' ],
+				[ '[]11' ]
 			], { headingRows: 2 } ) );
 
 			command.execute();
 
 			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
-				[ '[]00', '01' ],
-				[ '', '11' ]
+				[ '00', '01' ],
+				[ '', '[]11' ]
 			], { headingRows: 1 } ) );
 		} );
 
 		it( 'should fix rowspaned cells inside a row', () => {
 			setData( model, modelTable( [
-				[ '00', { rowspan: 2, contents: '[]01' } ],
-				[ '10' ]
+				[ '00', { rowspan: 2, contents: '01' } ],
+				[ '[]10' ]
 			], { headingRows: 2 } ) );
 
 			command.execute();
 
 			expect( formatTable( getData( model ) ) ).to.equal( formattedModelTable( [
-				[ '00', '[]01' ],
-				[ '10', '' ]
+				[ '00', '01' ],
+				[ '[]10', '' ]
 			], { headingRows: 1 } ) );
 		} );