Rethink table model (or table atomic conversion)

[jodator]

📝 Provide a description of the improvement

I've been struggling with recent bugs concerning removing rows (#6502, #6437, #6544, #6502, #6391, #6406) and I've came to a conclusion that we have a problem in table model definition.

In model we do not have "redundant" elements from HTML: <tbody> and <thead>. This allows to have the model simpler. In most scenarios model operations are quite simple. The problem get more complex if we add colspan and rowspan attributes. Table cells that are merged (thus are spanned by row or column) must be treated carefully when modifying model because:

• spans can't collide (ie rowspan and colspan from different cells cannot overlap)
• spans cannot exceed from head to body (browser will cut them visually) so we must check <thead> boundary in the model (well heading section).
• table cell in <thead> must be <th> so must be renamed when moved from <tbody>.

Most of the table atomic converters for table structure operates in the scope of its knowledge:

1. The headingsRows change downcast converter will move table rows from <thead> to <tbody> and rename cells as needed. In particular it will fix cells that could break the rule of not spanning over <thead>.
2. The removeRow downcast converter will remove rows from the view and will fix rowspan attribute of cells that overlapped the removed row so the table geometry is intact.
3. Similarly other atomic converters do their job as they have knowledge only about changes that happened for them.

This works fine in most scenarios but for some operations on tables if fails. This happens because most of the table operations requires a broad knowledge of a table structure for particular change.

Consider table:

+----+
| H1 |  <-- heading row 1
+----+
| H2 |  <-- heading row 2
+----+
| B1 |  <-- body row 1
+----+
| B2 |  <-- body row 2
+----+

In (simplified) HTML the table is rendered as:

<table>
	<thead>
		<tr><th> H1
		<tr><th> H2
	<tbody>
		<tr><td> B1
		<tr><td> B2

Atomic converters works in on operation at a time basis. In order to work out some of the above constrains they calculate operations based on the model state. This fails if more than two operations happens on a table.

Consider changing header rows from 2 to 1. The downcast converter:

1. Takes second model row H2 (because it is at index which is after a heading section) finds its view element and move it to <tbody> section.
2. Taking model state calculates which view cells requires renaming to th.

Now consider removing multiple rows (H2 and B1). The RemoveRow command will:

1. Remove H2 row.
2. Change heading rows to 1.
3. Set selection in B2.

Now this will fail because heading rows converter sees the same attribute change and will assume that table is valid. But what this converter will do is:

1. Take the second model row (which is now B2 ) and move it's view to <tbody>.

After table's attributes change conversion the remove row converter will step in. Removing elements from model is done on positions (offset in parent). So it will:

1. For a removed row at given position it will get model parent (table).
2. Get view row at an expected offset (let it be 2).
3. As the view has been modified (wrongly) it will find row B2 to be removed instead of H2.
4. 💥 The model to view mapping is broken as view for B2 was removed instead of H2.

So the spotted problems are:

1. Using model in downcast conversion can cause problems. The fix for changing header rows looks quite simple (I've almost got it working in I/6437: Fix - the table breaks after removing a last header row ckeditor5-table#301).
2. Table model and its conversion is hard to operate and figure out side effects. Downcast converters are too simplistic and relays on model state / single change.
3. It may encourage developers to use enqueueChange() - I don't see other option for the problem described here. The thing with enqueueChange() is that run conversion on each step so it acts like step-by-step conversion but it looks like more work is done than necessary.
4. I don't like the headingRows conversion. One idea is to have <tableRow isHeading="true"> - this will require some other checks (no isHeading gaps have to be checked) than now but conversion "should" be simple for this case. And I don't see how we can make similar change with columns.
5. I didn't check the columns heading conversion and its problems...

---

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

[jodator]

Similar case - merging cells when other cells have rowspan: #6667.

[jodator]

Closing as dup of #6506. There's a conversation on the matter already.

[jodator]

Another problem (#7454) with @niegowski is that the "fix" with double enqueChange() couses other problems - it will run post-fixers twice. Number of fixes for the same (changing heading rows and removing/adding row) is now probably the biggest from the issues that the table squad had dealt in recent past.

At this stage I'm sure that current API lacks something - making big structural changes. At this point @niegowski will try to remove the enqueueChange() hack and mark whole table as re-inserted (`differ.refreshItem()`) to check if the @Reinmar's concept of re-rendering whole chunk of content wouldn't be better.

[niegowski]

At this stage I'm sure that current API lacks something - making big structural changes. At this point @niegowski will try to remove the enqueueChange() hack and mark whole table as re-inserted (`differ.refreshItem()`) to check if the @Reinmar's concept of re-rendering whole chunk of content wouldn't be better.

I'll try to call refreshItem only for cases when headingRows attribute is changed.