I/6502: Refactor TableUtils.removeRows() logic.

[jodator]

Suggested merge commit message (convention)

Fix: Remove row in complex tables now properly move cells from removed rows. Closes ckeditor/ckeditor5#6502.

---

Additional information

This PR also includes fixes for some issues when not operating on TableEditing model (no post-fixer). ckeditor/ckeditor5#6574.

The actual fix is commented below but to fix that I had to understand the logic once again. Thus refactoring to removing all rows at once (this bring also a benefit of fewer operations when removing more than one row.

As a side effect, I've fixed some of the removeRows() test as it operated on wrongly created table. See: ckeditor/ckeditor5#6574.

[jodator]

This is the line that caused a problem in previous implementation.

[coveralls]

Coverage remained the same at 100.0% when pulling b04996d on i/6502 into 7213f93 on master.

[niegowski]

Comments below, also some missing scenario:

Given:

+----+----+----+
| 00 | 01      |
+----+----+----+
| 10 | 11 | 12 |  <-- remove this row
+----+----+----+

Then:

+----+----+----+
| 00 | 01      |
+----+----+----+

Expected:

+----+----+
| 00 | 01 |
+----+----+

[niegowski]

first - row is invalid, consider this test scenario:

it( 'should decrease rowspan of table cells from previous rows', () => {
	// +----+----+----+----+
	// | 00 | 01 | 02 | 03 |
	// +----+    +    +    +
	// | 10 |    |    |    |
	// +----+----+    +    +
	// | 20 | 21 |    |    |
	// +----+----+----+    +
	// | 30 | 31 | 32 |    |
	// +----+----+----+----+
	// | 40 | 41 | 42 | 43 |
	// +----+----+----+----+
	setData( model, modelTable( [
		[ '00', { contents: '01', rowspan: 2 }, { contents: '02', rowspan: 3 }, { contents: '03', rowspan: 4 } ],
		[ '10' ],
		[ '20', '21' ],
		[ '30', '31', '32' ],
		[ '40', '41', '42', '43' ]
	] ) );

	tableUtils.removeRows( root.getChild( 0 ), { at: 2, rows: 2 } );

	// +----+----+----+----+
	// | 00 | 01 | 02 | 03 |
	// +----+    +    +    +
	// | 10 |    |    |    |
	// +----+----+----+----+
	// | 40 | 41 | 42 | 43 |
	// +----+----+----+----+
	assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
		[ '00', { contents: '01', rowspan: 2 }, { contents: '02', rowspan: 2 }, { contents: '03', rowspan: 2 } ],
		[ '10' ],
		[ '40', '41', '42', '43' ]
	] ) );
} );

[jodator]

You were right. The code wasn't clear enough - even for me.

I've refactored it once again and moved it outside main method execution. Hopefully this is can be parsed a bit better.

As for other cases we will probably see 🤞.

[jodator]

Change (fix for this issue) is here: c4bfddb.

[jodator]

Comments below, also some missing scenario:

New/old bug - unrelated to the change. Also a corner case for me and requires much more tests. I'll create a follow up for this issue.

[jodator]

@oleq So the main review is done by @niegowski.

[oleq]

Can you reproduce this @jodator?

[oleq]

The only concern as commented: #303 (comment)

[jodator]

The only concern as commented: #303 (comment)

@oleq It looks like it is downcast error - I'll handle this in ckeditor/ckeditor5#6437.