Post-fixer: create paragraph in empty root.

[scofalik]

Original issue: #331

The "quick win" solution was already proposed by @Reinmar - on changesDone check if there are any contents in editor and if not, add <paragraph> to the model.

As I was about to implement this solution one thing came to my mind: we could have multiple roots. Since paragraph feature should be universal I think it should be ready to handle editors with multiple roots. So my idea is to check all the roots on changesDone (except of graveyard root) and fix all roots that are empty (and can have <paragraph> element according to Schema).

[scofalik]

It appears that even the easiest solution is more complicated than it looks on surface:

1. Post-fixing will create an undo step: https://github.com/ckeditor/ckeditor5-engine/issues/921
2. If we use changesDone, the <paragraph> will be added only after editor.setData() is called. And setting data does not even happen when initing core.StandardEditor. I guess that at some point changesDone will be finally called, but maybe the fixer should also be called on Editor#event:dataReady? But what if someone will create root after the editor is inited?

😱 This was supposed to be an easy issue 😱

[scofalik]

And last but not least, the editor may have unexpected behavior in tests, where we usually have empty editor and want to test whether some feature properly adds or converts something. Test already fail in heading and list feature because of it.

Do we still want to go with this solution?

[Reinmar]

TBH, I don't understand. Can't we adjust the last batch on changesDone to not create a new undo step? Initial data loading is not causing an undo step anyway and later on editor can be emptied only by some new batch so we can change exactly that batch.

Is https://github.com/ckeditor/ckeditor5-engine/issues/921 exactly about it?

Regarding failing tests, etc. I'd need to see them. We will need this behaviour coded in some way anyway. It may be done on the model layer, on the view layer, even on rendering layer – but still, it will affect some tests.

[scofalik]

TBH, I don't understand. Can't we adjust the last batch on changesDone to not create a new undo step?

Is ckeditor/ckeditor5-engine#921 exactly about it?

Yes, we don't have an access to last batch on changesDone ATM.

[scofalik]

Regarding failing tests, etc. I'd need to see them.

I've pushed t/19.

[scofalik]

I manage to did it without adding "last batch" to the document (no changes in engine were needed). Current solution may be not the best when it comes to efficiency but it required least development.

There are two listeners. First listens to change event and mark an empty root if a change made it empty. Also it saves a batch which made that root empty. Then on changesDone all marked roots are fixed and the fixing delta is added to appropriate batch. Works for multiple roots, added tests that check undo integration.

[Reinmar]

I manage to did it without adding "last batch" to the document (no changes in engine were needed). Current solution may be not the best when it comes to efficiency but it required least development.

But if we need the last batch anyway and plan to implement it anyway, then wouldn't it be better to have it first?

[scofalik]

It works without it. I don't know how many features will need this, because it appears that this is more complicated than just "having last batch". For example if two operations in different batches changed different roots you probably would run into problems if you add both fixing deltas to one batch:

1. Batch 1: remove all contents from root 1
2. Batch 2: remove all contents from root 2
3. Batch 2: add paragraph in root 1
4. Batch 2: add paragraph in root 2

Undo batch 2 -> will correctly undo root 2 but will also remove paragraph from root 1. Then root 1 will be fixed again and that change will be added to "undo batch" which lands on redo stack. Another undo will add content removed in operation 1 but extra paragraph would stay.

I don't know if that would end up exactly like this cause I didn't test (writing from top off my head) but I can imagine problematic scenarios. (If other features would need "last batch" for something and then would add fixes to wrong batches).

If we (...) plan to implement it anyway, then wouldn't it be better to have it first?

We don't plan to implement it. Yet :). We've just seen that it might be helpful sometimes but as proved above maybe we were wrong. I even thought that maybe each root could have last batch but then it is skewed because of this concrete problem that we are trying to solve in this issue. Also roots may have cross-linking operations.