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

msamsel · 2017-07-17T12:44:48Z

What is the purpose of this pull request?

bug fix

Does your PR contain necessary tests?

yes

This PR contains

Unit tests
Manual tests

What changes did you make?

fix bug which forbid to delete columns when those were under colspans.
fix bug where only last column was removed when colspan cell was selected.
fix bug where cursor position might won't be returned, what generat the console error.
add some brackets {} to if statements.

close #577

Comandeer · 2017-07-24T11:20:31Z

plugins/tabletools/plugin.js

 		// 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;
+		if ( map[ 0 ].length - 1 > endColIndex && map[ 0 ][ endColIndex + 1 ].cellIndex !== -1 ) {


Hmm… I don't get the logic behind this condition. It seems to work, but I don't understand why :D Could elaborate?

First part of condition map[ 0 ].length - 1 > endColIndex check if there was some columns on the right after deleted part.

map[ 0 ].length - 1 is the last index of possible column value

endColIndex is last index of deleted column.

Second part of condition map[ 0 ][ endColIndex + 1 ].cellIndex !== -1 checks, if cell (endColIndex + 1) located after deleted column, is still in table. Theoretically it should be always true, if first part is true ;)

I removed second part it seems to be confusing.

Comandeer · 2017-07-24T11:22:52Z

tests/plugins/tabletools/columndeletion.js

@@ -0,0 +1,74 @@
+/* bender-tags: editor,unit */


Tests connected with tableselection plugin should be inserted into tableselection/integrations directory. It allows to easily find all issues related to integrations between tableselection and other plugins.

I'll prepare two sets of test with and without tableselection plugin to cover more cases.

Comandeer · 2017-07-24T11:23:01Z

tests/plugins/tabletools/manual/columndeletionerror.md

@@ -0,0 +1,14 @@
+@bender-tags: 4.7.2, bug, 577


Tests connected with tableselection plugin should be inserted into tableselection/integrations directory. It allows to easily find all issues related to integrations between tableselection and other plugins.

it's done. ;)

Comandeer · 2017-07-24T11:23:31Z

tests/plugins/tabletools/manual/columndeletionerror.md

+1. Open browser console.
+1. Select some cells somwhere under merged header.
+1. Selection should use `tableselection` plugin.
+1. Right click to open cntentxt menu.


Please fix typos in test description.

Comandeer · 2017-07-24T11:23:58Z

tests/plugins/tabletools/columndeletion.js

+			this.doTest( 'table-1', 'columnDelete' );
+		},
+
+		'test remove 2 last columns by single row selection': function() {


Please add ticket references before every test connected with it.

Comandeer · 2017-07-24T11:30:40Z

plugins/tabletools/plugin.js

 		// 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;


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

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

Comandeer

Fix is not working properly

Case 1

Open http://tests.ckeditor.dev:1030/tests/plugins/tableselection/manual/integrations/tabletools/columndeletionerror
Select cells from two last columns.
Open context menu and select Column → Delete Columns.
Press Undo.
Click into cell in the last column (selection should become collapsed).
Open context menu and select Column → Delete Columns.

Expected:

Last column is deleted.

Actual:

Nothing happens.

Case 2

Open http://tests.ckeditor.dev:1030/tests/plugins/tabletools/manual/columndeletionerror
Select cells from two last columns (actually it's equivalent to select some rows).
Click inside cell in last column to collapse selection.
Open context menu and select Column → Delete columns.

Expected:

Last column is deleted.

Actual:

Nothing happens.

msamsel · 2017-08-03T13:31:02Z

@Comandeer
It's actually browser problem. I've report it to them:

Browser natively, after opening context menu, highlight clicked word. In this case it cause enlarging selection to next cell if we're at the end of cell.

Do you think that I should prepare some custom solution to fix in CKEditor?

Comandeer · 2017-08-09T08:45:28Z

There are already some workarounds for context menu in our tableselection plugin, so it won't be any extraordinary situation. Such temporary workaround could be deleted when upstream issues will be fixed.

Comandeer · 2017-08-18T09:25:47Z

plugins/tabletools/plugin.js

+		}
+
+		// Problem occures only on webkit in case of native selection (#577
+		if ( CKEDITOR.env.webkit && !selection.isFake && selection.getNative().type === 'Range' ) {


Is the last part of the statement really needed? Probably it could be replaced with checking if range is non-collapsed.

Comandeer · 2017-08-18T09:26:23Z

plugins/tabletools/plugin.js

+			return range.select();
+		}
+
+		// Problem occures only on webkit in case of native selection (#577


It'll be nice to link also upstream issue here.

Comandeer · 2017-08-18T09:27:17Z

tests/plugins/tabletools/manual/columndeletionerror.md

+1. Select some cells somewhere under merged header.
+1. Selection should use native selection.
+1. Right click to open context menu.
+1. Select Column -> Delete Column.


It's "Delete Columns", not "~~Column~~".

Comandeer

I've found one more issue with fix for Chrome.

Put collapsed selection on the end of cell in column 4.
Press right mouse button and select "Column"→"Delete Columns"
Press "Undo" button

Expected result: selection is contained inside one cell.

Actual result: two cells are selected: the desired one and the first one from the next row.

msamsel · 2017-08-30T15:03:48Z

@Comandeer mentioned behaviour seems to be not related to this bug.

You can reproduce your steps but instead of right-click use shortcut to open context menu Shift + F10. You will see that applying undo after that will restore proper collapsed selection.
You can modify your steps and after opening context menu close it with ESC button. You will see that selection is extend to both cells even without using column delete.

Entire situation has happens because of using right-click not because of using column delete. And that's why such case should be fixed separately not in a scope of this task.

And task for it, if we decide to make fix for that ;) #846

…est and description to manual

…est case to manual tests.

msamsel force-pushed the t/577 branch from 55eb724 to 77bffe3 Compare July 17, 2017 12:54

Comandeer self-requested a review July 24, 2017 09:13

Comandeer requested changes Jul 24, 2017

View reviewed changes

msamsel force-pushed the t/577 branch 2 times, most recently from d29de25 to 7e9cd3a Compare July 24, 2017 13:14

Comandeer self-requested a review August 1, 2017 12:26

Comandeer requested changes Aug 1, 2017

View reviewed changes

msamsel force-pushed the t/577 branch from 7e9cd3a to 9eeb22a Compare August 10, 2017 10:20

Comandeer requested changes Aug 18, 2017

View reviewed changes

Comandeer requested changes Aug 29, 2017

View reviewed changes

Comandeer approved these changes Aug 31, 2017

View reviewed changes

Mateusz Samsel and others added 9 commits August 31, 2017 12:37

Fix for deleting columns.

9dd47f7

Improve condition in solution. Remove debugger referrence. Add unit t…

252446d

…est and description to manual

Update tests after review.

77190cc

Remove problematic condition check.

ba085c4

Little hack for Webkit to remove nicely deleted columns. Additional t…

38064d2

…est case to manual tests.

Review fixes.

893dcf6

Remove unnecessary ignore in manual test.

d1e8f9f

Add missing ticket numbers and fix typos.

d1ab289

Update changelog [ci skip].

5c685b9

Comandeer force-pushed the t/577 branch from 5335dc5 to 5c685b9 Compare August 31, 2017 10:45

Comandeer merged commit 8340048 into master Aug 31, 2017

Comandeer deleted the t/577 branch August 31, 2017 10:47

This was referenced Sep 12, 2017

T/13729 Table: Insert Column before and After fails if a cell vertical split exist #592

Merged

New column inserted into wrong position #900

Open

Opening context menu in the last table cell after text deselects selected rows #903

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

msamsel commented Jul 17, 2017 •

edited

Loading

Comandeer Jul 24, 2017

msamsel Jul 24, 2017 •

edited

Loading

msamsel Jul 24, 2017

Comandeer Jul 24, 2017

msamsel Jul 24, 2017

Comandeer Jul 24, 2017

msamsel Jul 24, 2017

Comandeer Jul 24, 2017

Comandeer Jul 24, 2017

Comandeer Jul 24, 2017

msamsel Jul 24, 2017

Comandeer left a comment

msamsel commented Aug 3, 2017

Comandeer commented Aug 9, 2017

Comandeer Aug 18, 2017

Comandeer Aug 18, 2017

Comandeer Aug 18, 2017

Comandeer left a comment

msamsel commented Aug 30, 2017 •

edited

Loading

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

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

Conversation

msamsel commented Jul 17, 2017 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Choose a reason for hiding this comment

msamsel Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

Case 1

Case 2

msamsel commented Aug 3, 2017

Comandeer commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

msamsel commented Aug 30, 2017 • edited Loading

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

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

msamsel commented Jul 17, 2017 •

edited

Loading

msamsel Jul 24, 2017 •

edited

Loading

msamsel commented Aug 30, 2017 •

edited

Loading