Table selection should hook into model's deleteContent() and insertContent(), not the commands

[Reinmar]

📝 Provide a description of the improvement

Followup for ckeditor/ckeditor5-table#254.

The base methods are the place to hook in. They are the building blocks for more features.

This issue is a part of #6328 which is a broder topic (to ensure that the base functions work well too). Here, we should focus on making the ckeditor5-table extending the right thing.

---

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

[oleq]

I pushed 3 branches:

• table
• typing
• (docs) engine

But I came across this problem:

1. Because TableSelection now hooks into Model#deleteContent(), and the hook itself executes clearTableCellsContents() (which also executes Model#deleteContent()), I fell into an infinite loop.
   1. I solved it by restricting the hook to selections that have more that 1 range only. This way the hooks works only the first time it is called (e.g. for 4 cells → 4 ranges) but not for the internal Model#deleteContent() executions (those inside clearTableCellsContents()).
2. The bigger problem however is that dropping the command hooks completely broke the delete. Here's what's going on:
   1. Select, let's say, 4 cells.
   2. Press backspace.
   3. ✅The DeleteCommand#execute() uses Model#deleteContent().
   4. ✅Our Model#deleteContent() hook for tables kicks in.
   5. ✅It executes clearTableCellsContents(), which runs Model#deleteContent() for each individual cells.
   6. ✅The hook places the selection in the "first/last" table cell depending on Backspace or Shift+Backspace. Everything's fine until now.
   7. ❌ The DeleteCommand#execute() sets the selection on its own, overriding the workings of The Hook.
      1. This is the actual thing that is wrong, what follows is just a result.
      2. As far as I noticed, it creates the selection ON the table cell. This kind of selection is broken, and even if you want to type (not delete), the following points 👇 will happen and instead of inserting text you start deleting it.
   8. ❌ Because the selection is not collapsed after that, the injectUnsafeKeystrokesHandling() says "hello".
      1. It runs its deleteSelectionContent() that executes Model#deleteContent(), which does not trigger the logic inside The Hook because this.hasMultiCellSelection is false now (remember, the selection is ON a single cell?).
      2. An extra Model#deleteContent() at this moment shifts the content in the table cells like it would be in the case of two paragraphs next to each other.
      3. As I mentioned before, because the selection is totally broken (ON a cell), even a plain typing will trigger deleteSelectionContent() and delete stuff.
      4. Even when I somehow managed to avoid deleteSelectionContent() from being called (e.g. a condition for more than a single range in the selection around here) and there was no extra deleteContent(), the selection is still broken because of 7. So injectUnsafeKeystrokesHandling() is a victim in this story, not the bad guy.
   9. What we must do is fix the selection set in DeleteCommand#execute(). Probably it should collapse to the beginning of the first range or the end of the last one, depending on the direction. This should be discussed.
   10. P.S. I just realized that since the very beginning DeleteCommand#execute() works on the selection that anchors in tableRow(s) (and its ranges surround entire cells). So we cannot just collapse it towards the first/last range beginning/end because we're still landing in the row with a totally broken selection (just tried this).

TL;DR we may still need to hook into DeleteCommand because it's too dumb to work with multi-cell selections on its own, even after we hooked and handled Model#deleteContent(). Maybe we could return the "right" selection from the Model#deleteContent() hook and use it in the DeleteCommand#execute() when it's present?

const deleteContentSelection = model.deleteContent( selection, {
	doNotResetEntireContent,
	direction: this.direction
} );

// ...

writer.setSelection( deleteContentSelection || selection );

[Reinmar]

Because TableSelection now hooks into Model#deleteContent(), and the hook itself executes clearTableCellsContents() (which also executes Model#deleteContent()), I fell into an infinite loop.

1. I solved it by restricting the hook to selections that have more that 1 range only. This way the hooks works only the first time it is called (e.g. for 4 cells → 4 ranges) but not for the internal Model#deleteContent() executions (those inside clearTableCellsContents()).

First of all, the hook should not check things like hasMultiCellSelection. It must be stateless. The model.deleteContent() gets the selection and works on that selection. Nothing else. It may get any kind of selection from anywhere at any moment. It may not have anything to do with the table – it may be located in a completely different place.

I wanted to report earlier that the hasMultiCellSelection thing is conceptually wrong. It needs to be removed. The only thing that's public and serves as a communication between editor features is the model structure and the document selection. This must be straighten up in this code. I plan to rewrite that bit from the ground as it is seriously flawed.

The other problem with that hook, which is an off topic, but which I just notice and which we'll need to resolve at some point is that model.deleteContent() must be able to work with document selections and standard selections (check its API docs). This is tricky. Document selection needs to be set via writer. The normal selection has its own methods. We can have this as a followup.

[Reinmar]

What I wrote above will solve your first issue – if the hook will be stateless, then it will care only about the passed selection. It will iterate over ranges and handle those which contain cells. 

The corner case here is – one range contains a cell, the other is some random range. What should happen? I don't care :D This should never happen but we don't have a mechanism which prevents that. So, we can have this hook working like that – it does something with ranges which contain cells and just skip other ranges.

BTW, @mlewand is working on #6364 which will help you.

[Reinmar]

The bigger problem however is that dropping the command hooks completely broke the delete. Here's what's going on:

LOL 😂I've just read through this and it seems that I already gave the answer to that in my previous comments (by coincident).

7. ❌ The DeleteCommand#execute() sets the selection on its own, overriding the workings of The Hook.

It does not. It sets the selection to the selection that it passed to deleteContent(). It's not the document selection. It's the standalone selection (which I've been calling a standard selection above). When a standard selection is being passed to deleteContent(), it should point to the place where deleteContent() indicates that it would put the selection (if it was a document selection).

Where's the problem? The current implementation of the hook changes the document selection instead of indicating where it would put it via changing the passed standalone selection. Then, when DeleteCommand#execute() tries to use it to set the model selection weird things happen.

So, once again if the hook will correctly distinguish document and standard selection, this will probably work just fine. In that specific case, deleteContent() will be called with a standard selection, the hook will change it (to be placed in a cell) and then the DeleteCommand#execute() will set the document selection to that selection.