#7454: Down-casting whole table on headingRows attribute change

[niegowski]

Suggested merge commit message (convention)

Fix (table): Table structure should not be changed while removing heading row. Closes #7454.

Fix (table): Merging multiple cells should not crash editor while removing multiple empty rows and columns that appeared after merge. 

---

Additional information

[jodator]

I caught one thing only - I think that one model.change() is not needed.

Another thing - could you check if a test for external attribute operation is able to check if the table was refreshed? There might be some tests for similar case in table content refresh post-fixer.

[jodator]

I think that this is not needed any more - it is already called in a model.change() block.

[jodator]

@scofalik & @Reinmar - I need to double check this as this changes how table is converted.

After numerous bugs with changing headings + removing rows we concluded that enough is enough. The table models sucks. Downcasting headingRows change requires moving <tr> in the view from <thead> to <tbody> or vice-versa. It can fail in multiple scenarios (#6502, #6437, #6544, #6502, #6391, #6406 + #7454) and would potentially never be OK.

Here (#7454) there was a problem with table layout post-fixer which was called after each enqueueChange() - and thus we hit limit of what API is allowing us to do. The solution is to wait with conversion and post-fixing after all table model changes are done and that table attribute converter would know that also table children are changing.

Possible considered solutions were:

1. A "hack": set heading rows to 0, remove/add rows, set heading rows to proper value - but it lacks an API for not converting stuff at once.
2. Some API to mark that some changes need to be either converted at once or should not trigger model post-fixers.
3. Mark table to be re-rendered at once.

We choose option 3 using differ.refreshItem() - which is called on every headingRows attribute operation as It requires to also take external operations into account when dealing with this bug. Also the API for many TableUtils get simplified but not passing a batch instance (:tada:).

---

In other words the solution is to re-render whole table in the view if headingRows attribute was changed by any means.

---

What is needed - some kind of 👍 / 👎 - it might impact collaboration features in some way. Do we need better API for requiring re-converting whole chunk of content?

ps.: @Reinmar this is also some sort of ADR proposal (without sections - but have intro, considered steps and proposed solution).

[scofalik]

Why this is done on applyOperation not in a post-fixer?

As a rule of thumb, we try to avoid doing stuff on applyOperation - this is the last resort solution. Getting between multiple operations is a risky business. OTOH, I see that you only mark the table to refresh, so no changes in the model tree are done. Might be okay.

The one difference that quickly comes to mind is that you might have an attribute operation for heading rows and then an operation that reverses it, so in the end, there's no change. Post-fixer would not refresh the table, AFAIU. 

So, once again, if it can be done through post-fixer and there are no counterarguments, I think I'd go with post-fixer. But I don't see real big disadvantages in doing this with applyOperation either, other than we simply avoid it because it is a more vulnerable "place", if you know what I mean.

[jodator]

@ckeditor/qa-team This change requires some testing help. The scenarios which this change touch are for all structural changes inside (or "touching") heading section, like:

• adding/removing row in table heading
• adding/removing row just after a heading section
• changing number of heading rows
• row-spanned cells (in heading or "touching" heading section) caused problems here

[jodator]

@ckeditor/qa-team This change requires some testing help. The scenarios which this change touch are for all structural changes inside (or "touching") heading section, like:

• adding/removing row in table heading
• adding/removing row just after a heading section
• changing number of heading rows
• row-spanned cells (in heading or "touching" heading section) caused problems here

[FilipTokarski]

Seems to be ok, I didn't find any new bugs 👍

[scofalik]

@FilipTokarski could you test it with real-time editing?

[FilipTokarski]

EDIT: I can also reproduce it on live docs now so it's not introduced in this PR, I'll create a separate ticket for this

I found a bug with real time editing:

1. On two clients at the same time add a row above the header row

Result:

• on one client the original header row is now a normal row with normal cells
• on the second client the same, but it keeps the grey background of header row

Screencasts:

[niegowski]

@FilipTokarski could you check that failing case once again? Maybe last change would fix it.

[FilipTokarski]

I'm still getting 2 header rows in the model, and 3 in view.

But apart from this issue, as it comes to real-time editing, things seem to work fine 👍

[scofalik]

I guess that the table is incorrect and not refreshed because headingRows attribute does not change in this case. Only new rows are added. So the table is not marked to be refreshed.

[jodator]

I guess that the table is incorrect and not refreshed because headingRows attribute does not change in this case. Only new rows are added

It looks like weird bug - the table headings must be updated if you insert row in a heading section.

before: headingRows=2

insert row in heading section

after headingRows=3

Dunno why the model doesn't reflect that but the view does o_0.