Reordering header rows or columns with non-headers breaks view

[Dumluregn]

📝 Provide detailed reproduction steps (if any)

There are two cases with different behaviour, but in both the model and data downcast work fine and editing downcast is bugged.

Scenario 1 - moving the header row

1. Create the table with one header row.
2. Move the first row down using writer.
3. Examine the model and both downcasts.

✔️ Expected result

There should still be one header row and rows content should be swapped.

❌ Actual result

There is no more header rows in the editing downcast. Also the content of rows didn't swap in the view.

Scenario 2 - moving the non-header row

1. Create the table with one header row.
2. Move the second row up using writer.
3. Examine the model and both downcasts.

✔️ Expected result

There should still be one header row and rows content should be swapped.

❌ Actual result

There are two header rows in the view. Still only one in model and data downcast.

❓ Possible solution

The only broken pipeline is the editing downcast, so I'd look for the solution there.

📃 Other details

Same issues occur during header column reordering. On the header-row-column-test branch I've pushed some unit tests reproducing the problem.

• Browser: any
• OS: any
• Installed CKEditor plugins: table

---

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

[niegowski]

This happens because moving elements in the model is downcasted as remove + insert and current downcast converters for tables do not handle changing headings in that situation. 

The easiest way, for now, is to trigger table refresh (as we do on the headingRows attribute change) when any row is inserted to or removed from the headings (same for cell inserted/removed from heading columns). I pushed the proposed simplest solution to the test branch mentioned in the issue. Note that this currently triggers the whole table to be removed from the view and downcasted once again so we need to be careful not to do it on every change in the table.

ckeditor5/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js

Lines 38 to 54 in c252a2d

	} else if ( change.type == 'insert' || change.type == 'remove' ) {
	if ( change.name == 'tableRow' ) {
	const table = change.position.findAncestor( 'table' );
	const headingRows = table.getAttribute( 'headingRows' ) || 0;
	if ( change.position.offset < headingRows ) {
	tablesToRefresh.add( table );
	}
	} else if ( change.name == 'tableCell' ) {
	const table = change.position.findAncestor( 'table' );
	const headingColumns = table.getAttribute( 'headingColumns' ) || 0;
	if ( change.position.offset < headingColumns ) {
	tablesToRefresh.add( table );
	}
	}
	}

[Reinmar]

Remaining scope:

• Document the solution (missing code comments with a link to this issue)
• Rewrite the tests added by @Dumluregn with our table test utils (and add some tests if missing)

Note to @Dumluregn: Do you handle rowspan and colspans in your code that reorganizes the table? They are not going to be handled by the table's core implementation.

[Reinmar]

BTW: Hot tip: http://localhost:8125/ckeditor5-table/tests/manual/tablemocking.html use this to generate model data.

Idea: Add source editing to that test.

[Dumluregn]

Note to @Dumluregn: Do you handle rowspan and colspans in your code that reorganizes the table? They are not going to be handled by the table's core implementation.

Sorry for delayed response, in current implementation we handle moving rows with colspan and columns with rowspan, but not rows with rowspan or columns with colspan. To visualise it, we'll move the following column and row:

but not those:

[dawidurbanski]

@niegowski I made all necessary updates to this solution and I did put all of it in this branch: ck/10463-table-rows-columns-reordering

There is one problem with proposed postfixer fix though.

It will fail if we make an insert change via model writer that will affect number of table rows. For example:

Table with two heading rows:

+----+----+----+
|    |    |    |
+----+----+----+
|    |    |    |
+----+----+----+ <-- heading rows
|    |    |    |
+----+----+----+

Then if we add new row between first two heading rows:

const row = writer.createElement( 'tableRow' );

writer.insert( row, table, 1 );

writer.insertElement( 'tableCell', row, 'end' );
writer.insertElement( 'tableCell', row, 'end' );

So we should have now:

+----+----+----+
|    |    |    |
+----+----+----+
|    |    |    |
+----+----+----+
|    |    |    |
+----+----+----+ <-- heading rows
|    |    |    |
+----+----+----+

But we get:

+----+----+----+
|    |    |    |
+----+----+----+
|    |    |    |
+----+----+----+ <-- heading rows
|    |    |    |
+----+----+----+
|    |    |    |
+----+----+----+

It's because in the updated postfixer:

ckeditor5/packages/ckeditor5-table/src/converters/table-heading-rows-refresh-post-fixer.js

Lines 38 to 46 in c252a2d

	} else if ( change.type == 'insert' || change.type == 'remove' ) {
	if ( change.name == 'tableRow' ) {
	const table = change.position.findAncestor( 'table' );
	const headingRows = table.getAttribute( 'headingRows' ) || 0;
	if ( change.position.offset < headingRows ) {
	tablesToRefresh.add( table );
	}
	} else if ( change.name == 'tableCell' ) {

in such case we are doing change.type == 'insert' with change.name == 'tableRow' (side note: in my branch I changed all of these comparisons to strict ===).

Later then headingRows is evaluated to 2 in this exact case, and change.position.offset is 1 so it adds this table to refresh.

I'm not sure how to fix it yet though.

This exact case is covered in the following automated test which will fail at the moment:

ckeditor5/packages/ckeditor5-table/tests/converters/downcast.js

Lines 464 to 488 in c252a2d

	it( 'should insert row on proper index when table has heading rows defined - insert in heading', () => {
	setModelData( model, modelTable( [
	[ '00', '01' ],
	[ '21', '22' ],
	[ '31', '32' ]
	], { headingRows: 2 } ) );
	const table = root.getChild( 0 );
	model.change( writer => {
	const row = writer.createElement( 'tableRow' );
	writer.insert( row, table, 1 );
	writer.insertElement( 'tableCell', row, 'end' );
	writer.insertElement( 'tableCell', row, 'end' );
	} );
	assertEqualMarkup( getViewData( view, { withoutSelection: true } ), viewTable( [
	[ '00', '01' ],
	[ '', '' ],
	[ '21', '22' ],
	[ '31', '32' ]
	], { headingRows: 3, asWidget: true } ) );
	} );

[dawidurbanski]

Other than that I refactored @Dumluregn tests to use our table helpers.

By doing so I found that viewTable helper does not support headingColumns attribute similar to modelTable. So I updated it slightly.

There was one unrelated test that was incorrectly using headingColumns in viewTable util so I fixed this test as well.

EDIT: after some consideration we decided to revert edits made to viewTable helper and use other available way to specify table columns.

[niegowski]

This exact case is covered in the following automated test which will fail at the moment:

This test is invalid. It is passing now because of broken downcast conversion. There is writer.setAttribute( 'headingRows', 3, table ); missing there. Note how the command for inserting rows works. We do not expect attributes on a table to change in any "magical" way, it needs to be set by the command/code in the same change block.

[dawidurbanski]

@niegowski Thanks for that one. It works like a charm now.