Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #271 from ckeditor/i/6368
Browse files Browse the repository at this point in the history 
Fix: Merge left and right commands should be always enabled if the execution does not cross the heading column boundary. Closes ckeditor/ckeditor5#6368.
  • Loading branch information
@mlewand
mlewand authored Mar 11, 2020
2 parents b231e4d + d6d542f commit c088814
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 74 deletions.
16 changes: 9 additions & 7 deletions src/commands/mergecellcommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}

Expand Down
27 changes: 6 additions & 21 deletions src/commands/setheadercolumncommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand All @@ -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;
Expand All @@ -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 );
}

/**
Expand Down Expand Up @@ -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 changes: 15 additions & 0 deletions src/commands/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
100 changes: 60 additions & 40 deletions tests/commands/mergecellcommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
} );
} );
} );

Expand Down Expand Up @@ -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;
} );
} );
} );

Expand Down
58 changes: 52 additions & 6 deletions tests/commands/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand All @@ -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>' );

Expand All @@ -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;
} );
} );
} );

0 comments on commit c088814

Please sign in to comment.