Introduced support for removing multiple rows / columns using multi-cell selection

[mlewand]

Suggested merge commit message (convention)

Type: Introduced support for removing multiple rows or cells when multiple columns are selected selection. Closes ckeditor/ckeditor5#6126.

---

Additional information

Currently there's added row removal support.

• There's one disturbing fact with removing a selection containing mixed header and regular rows. Unit tests are passing, while real editor usage throws an exception. It shows that unit tests approach could be more holistic (we're testing only model ATM AFAICT).
• There's one known wrong handling of rowspanned cells.

There's a followup: ckeditor/ckeditor5#6370 related to:

ckeditor5-table/tests/commands/removerowcommand.js

Lines 130 to 151 in 23d3a40

	// This test is blocked by (#6370).
	//
	// it( 'should properly remove when tailing rows are selected', () => {
	// setData( model, modelTable( [
	// [ '00', '01' ],
	// [ '10', '11' ],
	// [ '20', '21' ],
	// [ '30', '31' ]
	// ] ) );
	// const tableSelection = editor.plugins.get( TableSelection );
	// const modelRoot = model.document.getRoot();
	// tableSelection.startSelectingFrom( modelRoot.getNodeByPath( [ 0, 2, 0 ] ) );
	// tableSelection.setSelectingTo( modelRoot.getNodeByPath( [ 0, 36, 0 ] ) );
	// command.execute();
	// assertEqualMarkup( getData( model ), modelTable( [
	// [ '00', '01' ],
	// [ '10', '11[]' ]
	// ] ) );
	// } );

[coveralls]

Coverage increased (+0.2%) to 97.176% when pulling 9bb7439 on i/6126 into df088f7 on master.

[mlewand]

@ckeditor/qa-team guys can you give it some testing?

[FilipTokarski]

Steps:

1. In your table, you need to have some multi-merged vertical cell
2. Select some adjacent cells together with this multi-merged cell
3. Delete row

NOTES:

• if you start selection from the white cell above the yellow one, you will not get an error. To reproduce this crash, the cell on which you start selection, must not be at the same level as top of the multi-merged vertical cell, it needs to be lower.
• if you start selection from the multi-merged cell you will not get a crash
• I cannot reproduce it on master, if I try to remove rows one by one

Uncaught TypeError: Cannot read property 'column' of undefined
    at RemoveRowCommand.execute (removerowcommand.js:70)
    at RemoveRowCommand.<anonymous> (observablemixin.js:255)
    at RemoveRowCommand.fire (emittermixin.js:209)
    at RemoveRowCommand.<computed> [as execute] (observablemixin.js:259)
    at CommandCollection.execute (commandcollection.js:68)
    at ClassicEditor.execute (editor.js:288)
    at DropdownView.<anonymous> (tableui.js:225)
    at DropdownView.fire (emittermixin.js:209)
    at fireDelegatedEvents (emittermixin.js:625)
    at ListItemView.fire (emittermixin.js:232)

browser: any

EDIT

I guess this scenario can be simplified by simply selecting cells upwards like this:

[FilipTokarski]

When you want to delete columns and select cells from right to left - nothing happens:

Similarly, when you select from left to right and up ( if you go down, then it works fine ):

Note: there is no problem with removing columns one by one in this directions on master

[mlewand]

Forgot to mention that there's an external issue ckeditor/ckeditor5#6370 when you include the last row to be removed. It's not caused by this PR, it's on master too, and should be fixed with a separate PR.

[FilipTokarski]

You cannot fully remove multiple columns if selection contains horizontally multi-merged cell ( it also forces content to jump from cell to cell ):

This behaviour is similar as on master - to fully remove multi-merged cell, you have to remove columns one by one. But, in this case there is no jumping content on master:

[mlewand]

1. In your table, you need to have some multi-merged vertical cell
2. Select some adjacent cells together with this multi-merged cell
3. Delete row

Pushed a fix - this has been addressed in a way that it no longer crashes.

However handling rowspan/colspan in case of multi-cell selection is still not ideal, which is something I'd like to extract to a separate task.

Reason is that it will take substantial amount of time, for a relatively minor effect.

This also affects: 

You cannot fully remove multiple columns if selection contains horizontally multi-merged cell ( it also forces content to jump from cell to cell )

[mlewand]

Ok pushed some polishing. Better handling for colspan/rowspan edge cases extracted to ckeditor/ckeditor5#6406. The code wasn't changed that much that it would require QA team retest (all the cases are covered by unit tests too).

[mlewand]

Merged master branch, resolved conflicts.

[Reinmar]

I tried to remove the very first row (bulleted, numbered, todo) from http://localhost:8125/ckeditor5/tests/manual/all-features.html. I select this entire row and when I execute the command it crashes with:

position.js:914 Uncaught TypeError: Cannot read property 'is' of undefined
    at Function._createAt (position.js:914)
    at Model.createPositionAt (model.js:606)
    at Writer.createPositionAt (writer.js:597)
    at Object.callback (removerowcommand.js:82)
    at Model._runPendingChanges (model.js:801)
    at Model.enqueueChange (model.js:227)
    at RemoveRowCommand.execute (removerowcommand.js:81)
    at RemoveRowCommand.<anonymous> (observablemixin.js:255)
    at RemoveRowCommand.fire (emittermixin.js:209)
    at RemoveRowCommand.<computed> [as execute] (observablemixin.js:259)

[Reinmar]

I also get this in:

removerowcommand.js:153 Uncaught TypeError: Cannot read property 'getChild' of null
    at getCellToFocus (removerowcommand.js:153)
    at Object.callback (removerowcommand.js:77)
    at Model._runPendingChanges (model.js:801)
    at Model.enqueueChange (model.js:227)
    at RemoveRowCommand.execute (removerowcommand.js:73)
    at RemoveRowCommand.<anonymous> (observablemixin.js:255)
    at RemoveRowCommand.fire (emittermixin.js:209)
    at RemoveRowCommand.<computed> [as execute] (observablemixin.js:259)
    at CommandCollection.execute (commandcollection.js:68)
    at ClassicEditor.execute (editor.js:288)

[mlewand]

 I tried to remove the very first row (bulleted, numbered, todo) from

That's unfortunately issue inherited from master branch. You get the same error after placing a cursor in the last column and executing remove row command.

I have fixed this.

I also get this in:

That's the last row issue mentioned here #261 (comment). Luckily it has been already merged on master branch, so I merged it here. Should no longer be reproducible.

Merged master branch and resolved bunch of conflicts.