Pasting incorrect list is not fixed.

[scofalik]

Originally posted by @Reinmar in #2972.

I found the following case. I copied:

And got:

It's not OT related, but I'm not sure if you want to work on all possible scenarios at once or you predict that we'll be tackling them down one by one.

[scofalik]

After looking into this issue I confirm that the incorrect result is produced by post fixer.

The thing is that in described case, source of "change" is clipboard and inserted data was already incorrect. This complicates things a bit because it is unclear where the fix should be applied.

The post fixer is written in a way that it assumes that data in editor was correct, so we manipulate only correct data. This means that if there is a move of a range of 10 items, the range contents itself are correct. In the result, we only have to check boundaries (where the range was inserted and from where it was taken out).

If we don't make that assumption, we have to check all inserted contents and move contents - and I mean literally all. Whenever anything is inserted or moved, the fixer would iterate through it. OTOH, right now the fixer is fired for every change too, but it does less, that for sure.

The interesting part about this issue is that, for some reason, clipboard when pasting contents, uses separate InsertOperation for each block, so theoretically post fixer should be able to fix it, and there is indeed kind of a bug in post fixer -- it does not fix correctly if following are inserted one after another in one nq changes block:

<listItem type="bulleted" indent="1">a</listItem>
<listItem type="numbered" indent="1">b</listItem>
<listItem type="bulleted" indent="1">c</listItem>

What post fixer does is:

1. Sees list item "b", checks previous item and decides that numbered should be fixed to bulleted.
2. Sees list item "c", checks previous item and decides that bulleted should be fixed to numbered -- because the change from point 1. was enqueued but not executed yet.

So to prevent such scenario, we would have to find first previous item with same indent and use it as reference point. This is doable but takes more CPU time.

Edit: since post fixer changes only attributes there is no threat of messing up with ranges (tree structure) so maybe we could also fire fixes without putting them in enqueueChanges block so they are applied immediately.

However, if insertContents ever starts working differently (and I believe that current behaviour is weird), this will stop working.

So the question I guess is: where do we really want to fix it?

On the other hand if something generates such InsertOperation when we would have a problem but honestly, only this would be an incorrectly written feature. I wonder if that could happen during OT though and this is a serious and difficult question. Maybe this will have to be fixed in two places actually?

[scofalik]

Another point is that we have changesDone refactor looming on horizon. When it's done and we convert changes from model to view on changesDone we can also apply fixes on changesDone and at that moment we could have multiple post fixers scanning changed parts of document in whole. This would be a nice idea too.

And finally, I have another idea. In its heart, the problem is caused by incorrect DOM/HTML. We already fix incorrect DOM a bit in view to model converters: for example we remove elements not allowed by Schema. I think it would be fine to use view to model converter here to prevent generating incorrect model.

So to sum up:

• there are multiple solutions possible,
• this is the first time that we want to check and fix pasted content (other than filtering content),
• we expect to refactor parts of code that will enable different solutions to the problem,
• we are not sure how much will have to be done to enable post fixer for OT.

This leads my to a conclusion that it will be best to make a fix that fixes just this problem as we know it and to choose a fix that will be least invasive, easy to implement and test and easy to remove later. That's why I would like to introduce the fix in view to model converter - unless I stumble on a problem or discover this won't work.

[Reinmar]

However, if insertContents ever starts working differently (and I believe that current behaviour is weird), this will stop working.

The current implementation isn't weird. It's just KISS. The insertContent() function needs to check if every element can be inserted as specific position. E.g., in the content you provide to the function you can have couple of list items and then couple of elements which should not be allowed in this context. So, it's just much easier to insert them one by one.

This is possible to improve this by scanning elements forward to retrieve all of allowed ones, but this shouldn't be necessary. This would only complicate the insertContent() function. The only reason to optimise this could be performance issues with OT.

[Reinmar]

As for what to do...

We can see that our enqueueChanges() lead to a surprise. OTOH, you could be equally (or even more) surprised if someone did some changes immediately on e.g. insert, so in the middle of your code. So, I think that it doesn't negate enqueueChanges()'s sense. The alternative is even worse than what happened here.

Another topic is how "low" we'd like to implement the fixing mechanisms to be sure that the content never gets incorrect. We could put the code in insertContent, but it means that some other insertion may not be covered by this – e.g. if, in a result of OT, the rebased insertion will happen in a bit different context. Therefore, I think that even if we'll put this code to insertContent, one day we'll have to also add it as a listener of insert/remove. Question, when will that happen? How soon we'll hit the problem when collaboratively created lists will contain invalid items?

Another question is whether it needs to be done on insert/remove or should just be done before rendering (so on changesDone). For how long we want the model to be incorrect? I don't have a strong opinion on that. Since we can't enforce that the model is always correct, the situation can already lead to some troubles. Even if we'll fix everything before the conversion, there may be some mechanisms working on the model which might break after encountering an incorrect model.

Therefore, I'm now leaning towards the following mental model:

1. Rules implemented by the Schema are only hints on how the model structure should look like. They can neither describe every rule you may want to enforce, neither can they actually fix something. They are only a way to synchronise independent features and generic algorithms like insertContent().
2. Changes done between batched conversion (which is triggered by changesDone) may lead to a temporarily invalid model. This is unavoidable due to OT. The features are supposed to fix the model before it gets rendered. To do that, they should listen on the atomic changes and correct issues right before changesDone (usually, by using enqueueChanges()).
3. In order to avoid too complicated post-fixing (which may be prone to errors due to very complex situations which may be encountered), some features may do pre-fixing on other "input" methods – e.g. insertContent(), deleteContent(), etc.

Plus, there's one more thing. I had a problem even today that the content was rendered, but the model was completely incorrect. I had a <listItem> in a <paragraph>. This is prohibited by the schema, but it just happened that there was a bug in some algorithm which allowed this.

We'll be fixing those algorithms, but I'd love if the schema was used to always validate the structure. I guess we can't do that immediately on every change, but perhaps we could do that on changesDone? This could be a part of our debugging features – a plugin which enables such a listener which on every changesDone checks the entire model. WDYT?

[Reinmar]

@scofalik, I guess you saw https://github.com/ckeditor/ckeditor5-engine/issues/858#issuecomment-289694808, but I'm posting it here for the future reference anyway :)

[scofalik]

There are many possible solutions and things we could introduce to solve some problems in whole or partially. AFAIR what @pjasiun meant there is that since nodes are immediately created in some kind of tree, there would be an event sent and then some kind of fixers might start their work. It is something but I don't know if it is more helpful than other presented solutions.

[pjasiun]

Whenever someone mentions me at the bottom of the long conversation I have a feeling that you are trolling me ;) Anyway, I agree that it's not any revolutionizing change now you can use event dispatcher or tree walker, which will do the job in a few lines of code. Anyway, it's one more advantage of this change.

[scofalik]

We'll be fixing those algorithms, but I'd love if the schema was used to always validate the structure. I guess we can't do that immediately on every change, but perhaps we could do that on changesDone? This could be a part of our debugging features – a plugin which enables such a listener which on every changesDone checks the entire model. WDYT?

I already suggested that fixing might be done on changesDone and applied to all changed parts and their neighbourhood. So I guess I am also fine with validation tool that could be turned on/off or used in tests. If the Schema will already have all the rules written (some of them as callback probably), then this should be fairly easy plugin.

[Reinmar]

That's not what I meant, but it's interesting that we got back to this idea.

What I meant was that the validation tool could be a part of our dev tools. It'd help us noticing that content went bad by logging errors to the console. Currently, we don't know anything about such issues, unless they start to throw during conversion.

It'd also be great if this validator could always run, but I'm afraid that then we'd need to start considering performance and that would be a lot harder. So, first I turned towards dev tools to at least help at this stage.

As for doing fixes by Schema's callbacks – I still have mixed feeling about this. That would be a unified API to perform such fixes, but I can't really imagine it. With change events and changesDone you can target your fixes pretty well. With Schema callbacks... I don't know what would need to be checked. Makes for some kind of change buffering again :P Like in the MDC. So I rather see post-fixers as change event listeners using enqueueChanges() (which means fixing "just before changesDone").

[scofalik]

That's not what I meant.

As for doing fixes by Schema's callbacks

I never said so. All I said that some Schema rules will be probably done as callbacks (for example "list item can be only after list item with indent not lower than 1").

I rather see post-fixers as change event listeners using enqueueChanges() (which means fixing "just before changesDone").

And it's fine now (and has to be like this now). But it could be theoretically also possible on changesDone, before rendering.