Support multiple editable roots

[pjasiun]

CKEditor 5 engine support multiple roots in the editor.

The user should be able in the constructor of the decoupled editor to use an object where keys are editable names and walues are (HTMLElements or string data). Also getData( rootName ) and setData( rootName ) should be provided there.

Then, in the documentation, we should add a sample in https://ckeditor.com/docs/ckeditor5/latest/examples/builds/document-editor.html (in the framework section?) with the editor with multiple (two?) editable areas.

Also, decoupled editor samples need to be updated.

[f1ames]

PoC

I have added a t/21 branch with a POC of multiroot editor (can be tested by running manual tests - there is manual/multirooteditor-editable.html and manual/multirooteditor.html). It required some amount of work when it comes to adjusting initialization flow of the editor so I created a separate editor type (MultiRootEditor) instead of adding too much logic to Decoupled one to make it easier to analyze what code changes are needed.

One thing to consider is if it should be separate editor type or if we could just extend decoupled one. The changes in a away how create method is used are rather small (one may pass { rootName: domElement, ...} or { rootName: initialData, ... } object instead of simple sourceElementOrData) and getData and setData will work a little bit different (so to keep backward comp we will need to handle paramless calls for this methods).

Focus indication

When any editable is focused all editables gets blue focus indicator:

this is caused by the fact that focus is listened on document and the view element when receives focus from the document doesn't check (or rather have no way to check) which editable was focused. It looks it shouldn't be too much work to fix this.

Make some editables readonly

Just to mention for future reference, ATM, there is no reliable mechanism to make only some of the editables readonly, so one can make whole editor readonly (all editables).

---

To sum, everything seems to be working quite nice and haven't noticed any issues which would make editor unusable or unstable. And the changes required to provide such editor are just changes in the flow how editor is initialized so no engine or core modifications needed. cc @pjasiun @wwalc

Anyway, if possible I would like to ask @Mgsy to also take a look and do some quick testing (no more than 30 mins) to see if something obvious is not failing and I have missed it somehow (to test, switch to t/21 branch and run manual test - manual/multirooteditor-editable.html and manual/multirooteditor.html).

[pjasiun]

That's great news! :)

so I created a separate editor type (MultiRootEditor)

I think that since multiroot editor is be able to handle all cases of the decoupled editor there is no point to keep both. It should be enough to make this editor accepts single or multiple roots in the constructor/configuration and it will be able to handle all cases.

When any editable is focused all editables gets blue focus indicator:

This is not that bad. At the end of the day, it is a single editor, so it makes some sense that all its editables show that the editor is focused.

Make some editables readonly

I think this is more like a feature request than a bug.

[Mgsy]

I've tested this editor and haven't spotted any unexpected behaviour 👍

[f1ames]

I think that since multiroot editor is be able to handle all cases of the decoupled editor there is no point to keep both. It should be enough to make this editor accepts single or multiple roots in the constructor/configuration and it will be able to handle all cases.

Makes sense to improve the decoupled editor, especially that from the perspective of API the changes are minimal. However, I have one doubt about API consistency. We have agreed that editor.create will accept the same input as before (data as string or HTMLElement) but also an object which allows creating multi root editor (e.g. { rootName1: 'data1', rootName2: 'data2' } or { rootName: HTMLElement }).

What about getData() and setData() methods? I would assume that for multiple root editor getData() call will return an object with rootName - data like { root1: '<p>Foo</p>', root2: '<p>Bar</p>' }. If editor was created with object param with one root only should getData() return { onlyRoot: '<p>Baz</p>' } or just <p>Baz</p>. What if editor was created by passing HTMLElement or data string? So there are 3 options:

Always return an object

1. editor.create( '<p>Foo</p>' ) - getData() returns { root: '<p>Foo</p>' }
2. editor.create( HTMLElement ) - getData() returns { root: '<p>Foo</p>' }
3. editor.create( '{ root: '<p>Foo</p>' }' ) - getData() returns { root: '<p>Foo</p>' }
4. editor.create( '{ root: HTMLElement }' ) - getData() returns { root: '<p>Foo</p>' }

The issue here is that this is not backward compatible (breaking change) and also getData() method from decoupled editor is not consistent with getData() from other editor types.

Return type depends on how editor was created

1. editor.create( '<p>Foo</p>' ) - getData() returns '<p>Foo</p>'
2. editor.create( HTMLElement ) - getData() returns '<p>Foo</p>'
3. editor.create( '{ root: '<p>Foo</p>' }' ) - getData() returns { root: '<p>Foo</p>' }
4. editor.create( '{ root: HTMLElement }' ) - getData() returns { root: '<p>Foo</p>' }

Backward compatible and consistency across editor types is preserved. However, getData() itself is not consistent as return type depends on how editor was created (may be confusing).

Return type depends on number of roots

1. editor.create( '<p>Foo</p>' ) - getData() returns '<p>Foo</p>'
2. editor.create( HTMLElement ) - getData() returns '<p>Foo</p>'
3. editor.create( '{ root: '<p>Foo</p>' }' ) - getData() returns '<p>Foo</p>'
4. editor.create( '{ root: HTMLElement, root2: HTMLElement }' ) - getData() returns { root: '<p>Foo</p>', root2: '<h1>Bar</h1>' }

Very similar issues to previous one - backward compatible and consistent across editor types. However, getData() itself is not consistent as return type depends on number of roots editor has.

TBH, I'm the fan of consistent results so the 1st seems reasonable to me (consistency of the return types). However, it introduces backward incompatibility and incompatibility across editor types which makes me 😢 WDYT?

Btw. The very similar issue is with setData() (Can you call it with passing data only? What if there are multiple roots? Should it do some 'smart' checks? Should it be consistent on its own, or with other editor types?).

[pjasiun]

I think that getData should always return data from a single root, main by default:

getData(); // return data of the main root, same as `getData( { rootName: 'main' } );`
getData( { rootName: 'root2' } ); // return data of the "root2" root

We may also introduce a method to get all root names, but I think it is not needed. It is the developer who set these roots, so he should know names, and he can always call editor.model.document.getRootNames().

setData, on the other hand, could support both:

setData( '<p>foo</p>' ); // set data of the "main" root, does not touch other roots
setData( '<p>foo</p>', { rootName: 'root2' } ); // set data of the "root2" root, does not touch other roots
setData( { main: '<p>foo</p>', root2: '<p>bar</p>' } ); // set data of both roots, does not touch other roots

WDYT?

[f1ames]

I think that getData should always return data from a single root, main by default.

@pjasiun I was thinking about that too, but what if there is no main root? You can initialize editor with any root names. Which root then treat as a default one? Or if there is no main root, getData() will just return empty string/null (same behaviour as passing non-existent root name)?

---

I would go with a method signature like getData( rootName = main ), so one can use getData() or getData( 'rootName' ). If we assume that it will always return data from one root, getData( { rootName: 'root2' } ) seems to be redundant.

Now if we go with all 3 setData() options:

setData( '<p>foo</p>' ); // set data of the "main" root, does not touch other roots
setData( '<p>foo</p>', { rootName: 'root2' } ); // set data of the "root2" root, does not touch other roots
setData( { main: '<p>foo</p>', root2: '<p>bar</p>' } ); // set data of both roots, does not touch other roots

it's again inconsistent with getData(). Why you should be able to get data only from one root at the time but set data on many at once? I think this methods should be as similar as possible in usage to getData() (the same as it is at current state). So we could make getData() and setData() always work on one root:

setData( '<p>foo</p>' ); // set data of the "main" root, does not touch other roots
setData( '<p>foo</p>', 'rootName' } ); // set data of the "rootName" root, does not touch other roots

or if we need to have setData() to support many roots at once I would also make getData() support getting data from many roots at once (but then we are back to https://github.com/ckeditor/ckeditor5-editor-decoupled/issues/21#issuecomment-449005715). Or we could have getData() support only one root, and setData() support many roots but it makes thinks inconsistent and it's the worst option of these 3 IMHO.

[pjasiun]

@pjasiun I was thinking about that too, but what if there is no main root? You can initialize editor with any root names. Which root then treat as a default one? Or if there is no main root, getData() will just return empty string/null (same behavior as passing non-existent root name)?

If there is no main root, and one asks for data from the default (main) root he should get an exception, the same way as using getData( rootName = 'undefined' ). I think it will be edge case when there is no main root and we do not need any magic to handle it in any special way.

I would go with a method signature like getData( rootName = main ), so one can use getData() or getData( 'rootName' )

I was thinking about it, but we may have more option for getData and setData in the future, for instance to get data without suggestions from the track changes feature or to create undo step or reset the history when setData is called, so I believe that using the object will be more future-proof.

Now if we go with all 3 setData() options it's again inconsistent with getData(). Why you should be able to get data only from one root at the time but set data on many at once?

I believe that setData need to be consistent with two APIs: create and getData. This is why I think it should create 2 interfaces.

[f1ames]

As I'm preparing the PR with the above changes, here is a quick summary of what has been changed:

1. I have decided to move creating a docs sample to a separate branch/PR (t/21-docs for now) so it can be postponed until changes in decoupled editor are reviewed and confirmed so we don't have to adjust docs few times. The change in API docs related to the code changes necessary for this issue will be included in this branch.

2. As we concluded in the discussion above about getData() and setData() methods, getData( rootName = 'main') was implemented (throwing an exception when non-existent root is passed). As for setData(), version consistent with both getData() and create() methods was added as described in:

setData( '<p>foo</p>' ); // set data of the "main" root, does not touch other roots
setData( '<p>foo</p>', { rootName: 'root2' } ); // set data of the "root2" root, does not touch other roots
setData( { main: '<p>foo</p>', root2: '<p>bar</p>' } ); // set data of both roots, does not touch other roots

3. Since DataController#init method cannot be called twice (because document version changes), I adjusted it a bit, see ckeditor/ckeditor5-engine#1627.

4. There is a change in how getData() and setData() works (mentioned in a 2. point here), which basically added support for handling multiple roots. Now it is implemented directly in decoupled editor. I'm not sure if it shouldn't be in a more generic place.

5. One breaking change was introduced. Before, one could use editor.ui.view.editable.element to access editable element. It was changed to editor.ui.view.editables[ 0 ].element (as there are multiple editables now). I was thinking if editor.ui.view.editable.element could be an "alias" for editor.ui.view.editables[ 0 ].element to keep backwards compatibility, but didn't implemented it.

[pjasiun]

More I think about it, more I realise we could skip:

setData( '<p>foo</p>', { rootName: 'root2' } ); // set data of the "root2" root, does not touch other roots

This is an edge case that one will try to overwrite a single root which is not a main root. And one can always do: { root2: '<p>bar</p>' }. So I think we should keep it simple:

setData( '<p>foo</p>' );
setData( { root2: '<p>foo</p>' } );
setData( { main: '<p>foo</p>', root2: '<p>bar</p>' } );

[Reinmar]

getData( rootName = 'main')

One problem that I see with this is compatibility with #401. We'll have getData( { dontTrim: true } ). Hence, I think that we should have getData( { rootName: 'main' } ).

[pjasiun]

It the other hand, it would be nice to make it symmetrical with getData, so nvmd, lets keep:

setData( '<p>foo</p>', { rootName: 'root2' } );

Same for data.set, it would be nice to make it symmetrical with data.get, however, for create and data.init we do not need to support it.

[f1ames]

We have decided to start with docs sample for multiple root editor - see #1447 and if there will be a high demand for multiple root editor it can be turned into separate editor build/type.

[ysct]

I think that since multiroot editor is be able to handle all cases of the decoupled editor there is no point to keep both. It should be enough to make this editor accepts single or multiple roots in the constructor/configuration and it will be able to handle all cases.

Would it be possible also to allow dynamic loading of roots, if we don't know the number of roots in advance? The MultirootEditor requires that we know the number of elements in advance as we're supplying it as an object to Editor.create() with known keys.

I was looking at the MultirootEditor source code, but I only manage to dynamically load the root (using window.editor.model.document.createRoot( '$root', rootName )) . I don't know how to proceed further. Could someone point me in the right direction? Thank you very much.

[martijnpieters]

After separating some functionality in the multi root example, I was able to create and destroy roots dynamically.

One problem over here (and when I have a lot of spare time I will create an actual bug report): the current selection / focus is lost as soon as a new root is added and setData has been executed. This setData is actively setting the selection to null, so I had to cherry-pick some lines of code from that method to be able to set the initial data for the dynamically added root.

Proposal: don't remove the selection during setData if we're not setting data for the selected root.

[bbottema]

I'm a little confused, this ticket is still open, but the documentation seems to imply that this feature is fully supported already. What's the current status?

On a side note, considering the various separated source files in the documentation, it would be great to be able to download a working demo in a zip file. Now I have to manually set up a project, create several demo files and copy paste everything.

[elsapolo]

After separating some functionality in the multi root example, I was able to create and destroy roots dynamically.

One problem over here (and when I have a lot of spare time I will create an actual bug report): the current selection / focus is lost as soon as a new root is added and setData has been executed. This setData is actively setting the selection to null, so I had to cherry-pick some lines of code from that method to be able to set the initial data for the dynamically added root.

Proposal: don't remove the selection during setData if we're not setting data for the selected root.

I am also trying to create and destroy roots dynamically, could you please share what you had to change to do this? Thanks:)

[Mgsy]

I'm a little confused, this ticket is still open, but the documentation seems to imply that this feature is fully supported already. What's the current status?

Hi! The multi-root editor is officially supported in CKEditor 5, it seems that this issue has slipped our attention, that's why it's still open. Thanks for bringing it to our attention! For everyone interested in using the multi-root editor, see the guide.

I am also trying to create and destroy roots dynamically, could you please share what you had to change to do this? Thanks:)

Adding and removing roots dynamically is not supported yet. You can find more information about it here.

[jblaisdiligent]

After separating some functionality in the multi root example, I was able to create and destroy roots dynamically.

One problem over here (and when I have a lot of spare time I will create an actual bug report): the current selection / focus is lost as soon as a new root is added and setData has been executed. This setData is actively setting the selection to null, so I had to cherry-pick some lines of code from that method to be able to set the initial data for the dynamically added root.

Proposal: don't remove the selection during setData if we're not setting data for the selected root.

Hi, I know this is an old post but, I think this post here solved your setData issues (#8915 (comment))
I'd be interested in seeing how you managed to dynamically add/destroy roots dynamically, as it is a behaviour we'd like in our project. We're just starting on that atm, but if you'd have pointers or a shareable repo, it would be awesome!
Thanks