Multi-root editor implementation #13532
Conversation
…llows defining multiple editable areas handled by one editor instance. Feature (build-multi-root): Introduced new editor build featuring multi-root editor.
| <div contenteditable="true"> | ||
| <div contenteditable="false"> |
There was a problem hiding this comment.
This doesn't look good but it is necessary to have proper arrows control. But maybe you know better solutions? By adding this, we don't have to introduce logic that would be fired when arrow key is pressed at the beginning or at the end of an editable. The browser handles it by itself (and caret x-position is preserved).
However, it may add some quirkiness...? OTOH, we use similar structures this inside the editor (e.g. tables).
| @@ -605,7 +605,7 @@ Note: The number of options was reduced on purpose. We understood that configuri | |||
| <td> | |||
| <p>N/A.</p> | |||
| <p>The {@link module:editor-decoupled/decouplededitor~DecoupledEditor decoupled editor} allows configuring where should the toolbar and the editable element be inserted.</p> | |||
| <p>In addition to that, CKEditor 5 Framework architecture allows for writing a custom editor that contains multiple editable elements (document roots). See the {@link examples/framework/multi-root-editor multi-root editor example}.</p> | |||
| <p>The {@link examples/builds/multi-root-editor multi-root editor} allows for creating an editor instance that contains multiple editable elements (document roots).</p> | |||
There was a problem hiding this comment.
I am not familiar with this CKE4 option, so I don't know if this description is correct. cc @Reinmar
| initialData?: string | Record<string, string>; | ||
| language?: string | LanguageConfig; | ||
| placeholder?: string; | ||
| placeholder?: string | Record<string, string>; |
There was a problem hiding this comment.
This is a change that I needed to make to keep the official multi-root editor API the same as what was shown in the guide (and people use) and to be consistent with the data format for multi-root editors that we already have, e.g. in CS.
Other than that, DataController#set already accepts data in this format.
However, this broadens the type, which prompted other changes. I am not sure about docs -- now we need to keep two snippet examples and explain it.
Maybe a better solution would be to have new architecture for editor configs:
EditorConfig-- without things that are different for multi-root editor and single-root editor,SingleRootEditorConfig-- addsinitialData: stringandplaceholder: string,MultiRootEditorConfig-- addsinitialData: Record<string, string>and same forplaceholder.
I am not sure if this will not be even more confusing or problematic, though. For example, places that use EditorConfig now will have to know which type of config it is. OTOH, initialData and placeholder aren't used in many places in the code base. Maybe they are encapsulated inside the editor-related classes and maybe there will be no problem with recognizing what type of config interface is passed.
There was a problem hiding this comment.
BTW. { main: '<p>Foo</p>' } can be used also for single-root editors. And if somehow you created a single-editor instance with different root name, this, I believe, is the only option to set it too.
All I mean is that Record<string, string> makes sense for single-roots too, which is why I am okay with having it like this.
| @@ -19,7 +19,7 @@ import type { Constructor, Mixed } from '@ckeditor/ckeditor5-utils'; | |||
| */ | |||
| export default function DataApiMixin<Base extends Constructor<Editor>>( base: Base ): Mixed<Base, DataApi> { | |||
| abstract class Mixin extends base implements DataApi { | |||
| public setData( data: string ): void { | |||
| public setData( data: string | Record<string, string> ): void { | |||
There was a problem hiding this comment.
This goes in line with EditorConfig#initialData. Alternatively, MultiRootEditor may not extend DataApiMixin and implement these methods itself. I don't remember if that caused any problems (whether editor instance is used as DataApi).
But in this case, this is more safe, as we broaden the input parameter, and, as I said, data.set() already handles this type too.
There was a problem hiding this comment.
Also, note that getData() already supports multi-roots as you can pass the root name as an option.
So, to me it made sense to add setData() support straight here.
| return; | ||
| } | ||
|
|
||
| export default function secureSourceElement( editor: Editor, sourceElement: HTMLElement & { ckeditorInstance?: Editor } ): void { |
There was a problem hiding this comment.
I decided to change this implementation because I don't see a reason why not to pass the sourceElement as a parameter. AFAIR, all secureSourceElement uses were straight after setting editor.sourceElement.
For multi-root, though, there's no editor.sourceElement -- there can be multiple. So, the new implementation fits the multi-root editor better.
| * Read more about initializing the editor from source or as a build in | ||
| * {@link module:editor-multi-root/multirooteditor~MultiRootEditor.create `MultiRootEditor.create()`}. | ||
| */ | ||
| export default class MultiRootEditor extends DataApiMixin( Editor ) { |
There was a problem hiding this comment.
I decided not to extend ElementApiMixin.
| */ | ||
| public readonly editables: Record<string, InlineEditableUIView>; | ||
|
|
||
| public readonly editable: InlineEditableUIView; |
There was a problem hiding this comment.
This property doesn't make much sense for multi-root, where you have multiple editables, as above. This is set to the first defined editable.
I tried not extending EditorUIView but this got me into other typing problems (i.e. EditorUI requires EditorUIView, while Editor requires EditorUI, etc.).
Another idea would be to remove editable from EditorUIView and have it defined in particular EditorUIView implementations. This would require changes in places that uses editable. Alternatively, editable could be allowed to be null, then in places that requires it, we would have to tell TS that editable is set. AFAIR, editable is used in the balloon editor.
Of course, yet another idea would be to introduce hierarchy like I proposed for editor configs, so have separate ui view class for single and multi roots.
But I think that editable actually may not be needed at all. First of all, we have getEditableElement() on EditorUI that supports multiple editables (multi-roots). We could have getEditable() as well. On EditorUI or EditorUIView. We could also introduce getFocusedEditable() if there's a need to get the focused editable specifically (doesn't matter for single-editable editors, might matter for multi-root).
I do not think that we need to solve EditorConfig issues/changes; but this one is something that we might want to solve. editor.ui.editable does not make sense for multi-root, it smells. If you have proposals or strong opinion, I listen.
In general, I think there's quite a mess when it comes to which property is on what class and there are multiple ways to access some objects. E.g. we have sourceElement, we have editor.ui.view.editable.element we have getEditableElement() etc. I also don't understand why some things are on EditorUI and why other are on EditorUIView. However, this is not in a scope of this PR.
There was a problem hiding this comment.
Did you checked how SourceEditing works with the current solution? I can see the button is active (but should not) but nothing happens when clicked. This looks like related to editor.ui.view.editable.
There was a problem hiding this comment.
This PR is not supposed to make multi-root editor work with all the features, there will be follow-ups later.
I didn't check how source editing worked with earlier multi-roots but AFAICS in the code, it should work the same as earlier.
# Conflicts: # packages/ckeditor5-core/src/editor/editorconfig.ts # packages/ckeditor5-core/src/editor/utils/dataapimixin.ts # packages/ckeditor5-core/src/editor/utils/securesourceelement.ts
|
Oi, |
|
@arkflpc - done. |
| */ | ||
| public readonly editables: Record<string, InlineEditableUIView>; | ||
|
|
||
| public readonly editable: InlineEditableUIView; |
There was a problem hiding this comment.
Did you checked how SourceEditing works with the current solution? I can see the button is active (but should not) but nothing happens when clicked. This looks like related to editor.ui.view.editable.
…i-root editor types. Remove unnecessary support for `<textarea>` placeholder in editors that do not support `<textarea>`s.
|
@niegowski I fixed everything according to your suggestions, except where we had discussions. |
|
I forgot to add a comment after pushing the changes. These tests are removed because these editors do not support being created on a |
|
@niegowski Fixed your suggestions + CI looks green. |
Suggested merge commit message (convention)
Feature (multi-root-editor): Introduced multi-root editor type that allows defining multiple editable areas handled by one editor instance. Closes #11493.
Feature (build-multi-root): Introduced new editor build featuring multi-root editor.
Other (core):
EditorConfig#initialDataandEditorConfig#placeholdercan now be set toRecord<string, string>where keys are root names and values are settings for related roots.Introducing multi-root-editor editor type + a build, so two new packages.
When implementing, I run into some issues with code/class organization and types. I commented on them. Since that would require more changes that I want to do at this point, I believe that most can be left as it is. Or we may discuss it and create follow-up issues.
But one thing in particular is annoying me and it is
editablefield onMultiRootEditorUIViewwhich doesn't make much sense. This is something that we can consider solving but it will require changes in other parts of the code.