Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error which appear with columnDelete command when table contained colspans #653

Merged
merged 9 commits into from
Aug 31, 2017
Merged
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Fixed Issues:
* [#779](https://github.com/ckeditor/ckeditor-dev/issues/779): Fixed: [Remove Format](https://ckeditor.com/addon/removeformat) plugin removes elements with language definition inserted by [Language](https://ckeditor.com/addon/language) plugin.
* [#423](https://github.com/ckeditor/ckeditor-dev/issues/423): Fixed: [Paste from Word](https://ckeditor.com/addon/pastefromword) plugin pastes paragraphs into the editor even if [`CKEDITOR.config.enterMode`](https://docs.ckeditor.com/#!/api/CKEDITOR.config-cfg-enterMode) is set to `CKEDITOR.ENTER_BR`.
* [#719](https://github.com/ckeditor/ckeditor-dev/issues/719): Fixed: Image inserted using [Enhanced Image](https://ckeditor.com/addon/image2) plugin could be resized when editor is in read-only mode.
* [#577](https://github.com/ckeditor/ckeditor-dev/issues/577): Fixed: "Delete Columns" command provided by [Table Tools](https://ckeditor.com/addon/tabletools) plugin throws an error while trying to delete columns.

Other Changes:

Expand Down
80 changes: 71 additions & 9 deletions plugins/tabletools/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,56 @@
}

function deleteColumns( selection ) {
function processSelection( selection ) {
// If selection leak to next td/th cell, then preserve it in previous cell.

var ranges,
range,
endNode,
endNodeName,
previous;

ranges = selection.getRanges();
if ( ranges.length !== 1 ) {
return selection;
}

range = ranges[0];
if ( range.collapsed || range.endOffset !== 0 ) {
return selection;
}

endNode = range.endContainer;
endNodeName = endNode.getName().toLowerCase();
if ( !( endNodeName === 'td' || endNodeName === 'th' ) ) {
return selection;
}

// Get previous td/th element or the last from previous row.
previous = endNode.getPrevious();
if ( !previous ) {
previous = endNode.getParent().getPrevious().getLast();
}

// Get most inner text node or br in case of empty cell.
while ( previous.type !== CKEDITOR.NODE_TEXT && previous.getName().toLowerCase() !== 'br' ) {
previous = previous.getLast();
// Generraly previous should never be null, if statement is just for possible weird edge cases.
if ( !previous ) {
return selection;
}
}

range.setEndAt( previous, CKEDITOR.POSITION_BEFORE_END );
return range.select();
}

// Problem occures only on webkit in case of native selection (#577).
// Upstream: https://bugs.webkit.org/show_bug.cgi?id=175131, https://bugs.chromium.org/p/chromium/issues/detail?id=752091
if ( CKEDITOR.env.webkit && !selection.isFake ) {
selection = processSelection( selection );
}

var ranges = selection.getRanges(),
cells = getSelectedCells( selection ),
firstCell = cells[ 0 ],
Expand All @@ -317,10 +367,15 @@
// Figure out selected cells' column indices.
for ( var i = 0, rows = map.length; i < rows; i++ ) {
for ( var j = 0, cols = map[ i ].length; j < cols; j++ ) {
if ( map[ i ][ j ] == firstCell.$ )
// #577
// Map might contain multiple times this same element, because of existings collspan.
// We don't want to overwrite startIndex in such situation and take first one.
if ( startColIndex === undefined && map[ i ][ j ] == firstCell.$ ) {
startColIndex = j;
if ( map[ i ][ j ] == lastCell.$ )
}
if ( map[ i ][ j ] == lastCell.$ ) {
endColIndex = j;
}
}
}

Expand All @@ -332,27 +387,34 @@
cell = new CKEDITOR.dom.element( mapRow[ i ] );

if ( cell.$ ) {
if ( cell.$.colSpan == 1 )
if ( cell.$.colSpan == 1 ) {
cell.remove();
// Reduce the col spans.
else
} else {
// Reduce the col spans.
cell.$.colSpan -= 1;
}

j += cell.$.rowSpan - 1;

if ( !row.$.cells.length )
if ( !row.$.cells.length ) {
rowsToDelete.push( row );
}
}
}
}

var firstRowCells = table.$.rows[ 0 ] && table.$.rows[ 0 ].cells;

// Where to put the cursor after columns been deleted?
// 1. Into next cell of the first row if any;
// 2. Into previous cell of the first row if any;
// 3. Into table's parent element;
var cursorPosition = new CKEDITOR.dom.element( firstRowCells[ startColIndex ] || ( startColIndex ? firstRowCells[ startColIndex - 1 ] : table.$.parentNode ) );
var cursorPosition;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I wonder if it could be moved to tableselection plugin using its internal customizeTableCommand function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally tabletools doesn't depend on tableselection so we cannot assume that tableselection will be loaded.

if ( map[ 0 ].length - 1 > endColIndex ) {
cursorPosition = new CKEDITOR.dom.element( map[ 0 ][ endColIndex + 1 ] );
} else if ( startColIndex && map[ 0 ][ startColIndex - 1 ].cellIndex !== -1 ) {
cursorPosition = new CKEDITOR.dom.element( map[ 0 ][ startColIndex - 1 ] );
} else {
cursorPosition = new CKEDITOR.dom.element( table.$.parentNode );
}

// Delete table rows only if all columns are gone (do not remove empty row).
if ( rowsToDelete.length == rows ) {
Expand Down
Loading