T/209: Table cell post-fixer will refresh a cell only when it is needed.

[jodator]

Suggested merge commit message (convention)

Fix: Table cell post-fixer will refresh a cell only when it is needed. Closes ckeditor/ckeditor5#3283.

---

Additional information

• Needed by: T/ckeditor5/1985: Change how the temporary upload image is handled. ckeditor5-image#316 but also can be merged anyway. as a stand-alone PR.

ps.: I need to add some tests for this.

[scofalik]

To be honest, I don't like how this post-fixer is built. It only checks the current state and honestly, I simply can't trust it. It might fix the things currently at hand but it still will not be done properly.

The post-fixer should either:

• check the change, or best, all the changes that happened to the cell, and evaluate them together to see the bigger picture and see what kind of a change was performed, or
• save the previous state, so it will be easier to evaluate what should happen (so we will know it was "a simple cell" and now it is "a complex cell" -> refresh, or the other way).

The first proposal is much more complicated, so I propose going with the second one. We can store last known cell state ("simple" vs "complex") in a WeakMap and use it to make a more informed decision. Then the post-fixer will have "two small parts":

1. The first part will check what is the current state (will be easier than what we have now).
2. The second part will decide if a refresh is needed.

By using WeakMap we don't store it in the model.

This solution seems cleaner, more trustable and easier to implement and review. Do you see any problems with this solution? I might have overlooked something.

Apart from that, I'd like to see more tests. For example, for the cases that I've mentioned in the review comments, maybe some other cases. And please, remember that we are not limited only to changes that can be introduced by commands. We have undo and real-time collaboration, so you can't assume anything about the change. A whole lot may change in one batch in one cell. This is why I don't trust this solution.

[scofalik]

Okay, we talked F2F about this and the solution with WeakMap has a small "initialization" problem. Additionally, I realized that there are fewer cases that we really have to worry about, so I trust the proposed solution more.

We concluded that we can go with the proposed solution but I'd like to see more tests to feel safer and maybe some additional comments in the code explaining why the ifs that we have are enough and what assumptions we took so that it can work (for example, we were talking that in the end we only need to refresh if there is a paragraph because only existing paragraph might need that -- there is an if in the code but it is not explained clear enough).

[jodator]

I've rewrite this one case and added more comments on why those checks.

Also I've updated the proper test file - I don't know why I've changed tests for other post-fixer 🤦‍♂️.

[jodator]

One more finding... this text wasn't suppose to be here.

So I've also found out that the table cell post-fixer for paragraphs wasn't checking inserting text nodes into table cell directly (after rewriting the refresh post-fixer the tests for table cell paragraph were failing).

Also, I was changing wrong tests before 🤦‍♂️ (I was changing tests for paragraph post-fixer, not the refresh post-fixer tests...)

[coveralls]

Coverage remained the same at 100.0% when pulling 37c524b on t/209 into 9f64452 on master.