Ckeditor5: Review feature split into engine and UI parts

[Reinmar]

When implementing features we decided to divide all of them into the "engine" and "UI" parts. For example, there's:

• BoldEngine – introduces schema definitions, converters and command,
• Bold (requires BoldEngine) – introduces keystroke and a button.

The original focus wasn't very clear when we've been working on the features and we had a couple of ideas why we need the division:

• to allow running the editor in Node.js (see https://github.com/ckeditor/ckeditor5-design/issues/87),
• to allow building headless editor (see https://github.com/ckeditor/ckeditor5/issues/438),
• to allow replacing the default UI easily (I guess that it's the same as headless editor).

However, we didn't really know back then what each feature will bring and how the framework API will look like. Actually, we still keep discussing many related pieces. As a result of that, many choices were done blindly and it'd be a good moment to review them.

Server side vs headless

I think that we were more focused on allowing running CKEditor in Node.js while this isn't a super clear use case yet. A far more clear use case is a headless editor which should allow building your own UI for the editor easily.

As @djanosik reported in https://github.com/ckeditor/ckeditor5/issues/438#issuecomment-311683010:

The Blockquote plugin overwrites enter key behaviour. I don't use this plugin because I don't want GUI. But I need fully functional typing. I think such overwrite should be handled somewhere else, perhaps in BlockquoteTyping plugin.

This clearly shows that the division (repeated in other packages) is unsatisfactory for this case. The typing/enter/keystroke customisations should be separated from the UI part so you're able to build a headless editor will all the editing features (except UI).

Now, should we move them to the engine part or will we break the potential server side support? It depends on what kind of APIs the engine part will need to use. If our keystroke handler, or engine's view events, then it should be fine. This code would just be dead on the server side (we wouldn't register view.Document's observers in a potential ServerSideEditor).

Questions

• Is the list of reasons to split the code complete? Or can you think about some other use cases?
• Am I right that headless editor's main use case is to replace editor UI?
• Can you think about editing-related code which can't be moved to the engine part of features in such a way to not break server side support? Or in other words – is that code entirely based on our custom APIs?

cc @pjasiun @oskarwrobel @szymonkups @oleq @scofalik @fredck

[pjasiun]

I don't think that it's that simple that there are a full-UI and a headless editor. There is a full spectrum of different levels one may want to integrate the editor:

• only the document model: schema and post-fixers, but no converters,
• only engine part: schema, converters, command (?), but no key handling and UI,
• fully custom UI: engine part, key handling, no UI,
• custom views (for instance image styles panel): engine part, key handling, UI behavior, but no UI views,
• only custom buttons (for instance separate buttons for different heading levels instead of a single drop-down).

I met this problem very often, especially last 2 cases. We should not think about the 'UI' and 'engine' parts. Instead, we should bring atomic features as separate plugins, split them as long as it makes sense, as long as they can work separately and as long as it's not an absurdly small and simple code. However, it's fine that one atomic plugin requires another. Then, each, "end-user feature", should be a plugin which only aggregates these plugins (see upload plugin https://github.com/ckeditor/ckeditor5-upload/blob/master/src/imageupload.js#L27).

At the same time, we will not be able to guess all cases from the beginning. Issues like this will appear and we will be splitting plugins. It should not be a big deal, we should be able to keep the backward compatibility thanks to requirements.

[Reinmar]

only the document model: schema and post-fixers, but no converters,

That's not a problem. Converters are event based. If you won't enable dispatchers, conversion will not happen (registered converters will be a dead code). Also, conversion happens to our virtual DOM-like structure, so it's safe to be run in any environment. So I don't think that we need extract this bit in any additional way.

only engine part: schema, converters, command (?), but no key handling and UI,

This use case is a bit unclear for me still. This is how we divided the code now thinking about server side cases. But it would all work fine too if we moved key handlers to the engine part because key handlers use abstracted listeners anyway.

fully custom UI: engine part, key handling, no UI,

This is (will be), I believe, the most popular use case.

custom views (for instance image styles panel): engine part, key handling, UI behavior, but no UI views,

Yes, we discussed this in https://github.com/ckeditor/ckeditor5/issues/425 already. It'd be nice if we figured out how to bind the views as late as possible. To have one, final plugin which glues the code, reusing other plugins and injecting the right views. This would make them easy to "reglue" using different views. I'd like to investigate this one day.

only custom buttons (for instance separate buttons for different heading levels instead of a single drop-down).

This is a too small piece of code to treat it using plugin split. If you want a different button, define one, bind it to the exposed commands and register under myCustomBoldAndItalic name.

Instead, we should bring atomic features as separate plugins, split them as long as it makes sense

That "as long as it makes sense" really depends on your POV. One could expect us to divide the bold feature into 5 parts in order to satisfy his/her use case. Also, we need some standard for how the features are split – a common types of pieces. Otherwise, the developers will have a reallyyyyy hard time reading the code. You don't like code being split into too many pieces yourself, don't you ;). So, we'd come with a standard... and then face exceptions on every corner.

Besides, there are factors like code size and already mentioned ease of read.

So, I understand your idea, but we must be smart. We can't go crazy splitting the code, especially not based on new requests appearing every now and then.

Right now I'm thinking about:

• editing: schema, converters, commands, keystrokes,
• UI: logic,
• glue: editing + UI + injecting views – absolutely minimal code to make recomposing the feature cheap.

Perhaps we could consider splitting the editing part into model and view parts, but I don't see a technical reason for that yet.

[Reinmar]

The custom UI example created by @oleq (it will appear in the nightly docs soon) perfectly shows how important is resolving this ticket ASAP.

You can quite easily create this:

And the editor works. But there are exceptions like keystrokes because they are defined in the UI part of features, which was dropped here.

[fredck]

Related on Gitter: https://gitter.im/ckeditor/ckeditor5?at=59dd5c8ee44c43700a19d1d6

[davidpolberger]

We're currently building a custom user interface for CKEditor 5. As we don't want to make Webpack part of our build process, we're creating a custom, headless build of CKEditor 5. Completing this work proved straight-forward, thanks to the documentation. (Specifically the guide on how to create custom builds and the documentation on how to create a custom editor using Bootstrap.) Where "engines" are available, we're taking care to only use these engines, meaning that CKEditor makes no attempt to register buttons with our non-existent user interface.

Keystroke handling is, as @Reinmar pointed out, the main problem. Conceptually, keystrokes are part of the user interface and it could thus be argued that they have no place in the "engines." However, it is surprising to lose the special behavior associated with keys such as Enter, backspace and tab when editing complex structures such as lists and block quotes. When these keys don't work the way you're used to, the user experience is significantly degraded.

However, not all applications can be expected to want to use the same keystrokes for high-level commands. While an application in English will likely want to use Ctrl+B for bold and Ctrl+I for italic, for instance, that is not true for a French, Polish or Swedish application.

I would thus argue that handling "fundamental keys" such as Enter, tab and backspace in the engines themselves may make sense, while high-level keys should be handled the way they are today. (We have elaborate keystroke handling in our application, including integration with a help panel showing all available keystrokes. We'd need to disable CKEditor's keystrokes in favor of our own if the high-level keys were handled by CKEditor directly.)

As our application is only available in English today, the easiest course of action may be for us to temporarily make use of the full suite of plug-ins (thus gaining keystroke handling), and mock out the user interface parts to ensure that no exceptions are thrown when CKEditor tries to register buttons. We'll also investigate the feasibility of implementing the "fundamental" keystrokes ourselves, but we'll then need to maintain that code in the future, which isn't an attractive prospect given that CKEditor's APIs will likely change significantly in the coming months.

[davidpolberger]

Looking at the code for the block quote and list plug-ins, I realize that Shift+Tab also needs special handling (in addition to Enter, backspace and tab). If the code for these "fundamental" keys is not moved to the engines, perhaps it can be moved to utility classes, for easy reuse by third parties?

[davidpolberger]

As I noted in a previous comment, using many plug-ins directly from a headless editor does not work, as a headless editor typically does not have a "ui" property. We just tried adding a trivial mock implementation of a "ui" object to our headless editor, and everything works fine, including keystroke handling:

this.ui = {
  componentFactory: {
    add: function() {}
  },
  focusTracker: {
    add: function() {}
  },
  view: {
    body: {
      add: function() {}
    }
  }

This is, obviously, a terrible hack, and it will definitely break in the future. However, it's an easy way for us to get keystrokes working while we wait for the CKEditor team to decide on a better solution.

[Reinmar]

Thanks @davidpolberger for the feedback. Lots of interesting information!

It's a good time now to think about cleaning up the features that we have before we'll start implementing new ones. Therefore, we need to define new rules for splitting plugins and refactor a couple of existing ones (or all, cause it's a simple and quick change) to see if all this works.

I think that based on the feedback we can see that the "headless editor" concept is the most popular use case. Running part of the code base in Node.js is second and worth keeping in mind.

Additionally, by redefining the feature split we'll also want to improve the ability to inject dependencies. We didn't develop any particular tooling for DI. Instead, we want to rely on a proper modularity and, in some hopeless cases, replacing modules by Webpack (with aliases). Such approach should be KISS while still being flexible enough. This topic was previously discussed in https://github.com/ckeditor/ckeditor5/issues/425.

I'm going to propose some new scheme tomorrow and let's see where it gets us :)

[szymonkups]

I had f2f talk with @Reinmar about this, I made small proof of concept for splitting fetures it in a different way, and apply dependency injections to UI part: https://github.com/ckeditor/ckeditor5-basic-styles/compare/t/ckeditor5/488 (Bold feature).

[Reinmar]

@szymonkups created a beautiful ASCII art for this, so I'm gonna use it here:

 ==============================================================================|===============
|                               editing                                        |      UI       |
|==============================================================================|===============|
|                    engine                                |  keystrokes, ...  |
|==========================================================|===================|
|              model              |    view(controller)    |
|=================================|========================|
| schema | commands | post-fixers | converters | listeners |
 ==========================================================

This diagram shows common parts of functionality which features need to implement.

The current scheme divides this picture into FeatureEngine and Feature parts (where Feature has FeatureEngine as its dependencies). This is wrong for two reasons:

Keystrokes

The keystrokes (and other beyond-engine logic) is implemented in the Feature part which creates the problem that we discussed in the previous comments. It's simple – this bit of functionality needs to be reused by headless editors, so the most common reason for feature split. Therefore, a bit different feature split needs to be proposed:

• FeatureEditing
• FeatureUI

We could go deeper and divide FeatureEditing into FeatureModel, FeatureViewController and FeatureKeystrokes and what else, but:

• We don't gain much. Perhaps the only more common case would be to replace the FeatureViewController layer (so to render the feature differently in the view) but:
  
  
  
  • this is a pretty big change and indicates a deep integration (and, hence, ability to maintain a customized code),
  
  
  • is possible (up to some limits) without replacing any code thanks to our event system and pluggable conversion.
  
  
• The other use case would to run the editor in Node.js but this can be done with FeatureEditing anyway if a "deaf-mute" EditingController (one which doesn't render anything to DOM and does not plug any observers).
• We actually do that because commands are always separated and in more complex features converters are separated too. So, reimplementing a feature means gluing together a couple lines of code.

Problematic keystroke configuration

I'd also like to take this occasion to mention that we have more than one type of key-handlers:

• Some keystrokes are handled on the FeatureViewController level – there, we define low-level, strictly editing oriented keystrokes such as Enter or Backspace. To implement them we listen directly on the view events (to be more precise, we use EnterObserver and DeleteObserver) which makes those listeners inconfigurable. We've been considering trying to reimplement these listeners using KeystrokeHandler (which is meant to be configurable) but we didn't find a way to do that in a reasonable way. The reason was that keys like Enter are handled by multiple features at the same time so there's no single key -> action mapping. We'd need to introduce an intermediate, predefined layer which would allow doing key -> STH and STH -> action mappings. It's seems like an overkill.
• Other keystrokes are registered through KeystrokeHandler and should be defined as key -> action mappings. Here we define things like assigning Ctrl+B to bold command. Those are meant to be reconfigurable (https://github.com/ckeditor/ckeditor5-core/issues/8).

This topic was discussed in (I think) https://github.com/ckeditor/ckeditor5/issues/340. Back then it was a monster and I hoped that the time will clarify this for us, but it didn't - it's still convoluted. There's some code which doesn't match what I described above (listeners which aren't configurable but which use KeystrokeHandler – e.g. Tab handling).

However, we can deliberately overlook this for a little longer as it should be relatively simple to clean up in the future.

Plugin decoupling

Right now the Bold plugin requires the BoldEngine plugin. This means that you wouldn't be able to implement your own engine part and use it with our UI (which is the use case if you want to heavily modify feature behaviour). It also creates odd inheritance-like chains of events where Bold is just an extension of BoldEngine.

For a long time we've been discussing to decouple this (see https://github.com/ckeditor/ckeditor5/issues/425). The new rule of thumbs are:

• Create independent plugins. Plugins communicate through the editor (using commands, schema, model, events, etc.) so they don't need to additionally require one another. As long as a plugin implements a similar signature (e.g. uses the same element names and exposes the same commands) it can replace the one it mimics.
• Create end-user plugins which glue the sub-plugins, making them easier to enable. The glue should contain minimal (ideally – zero) logic.
• All plugins should contain minimal code.

So, as @szymonkups showed, the bold feature can be refactored to be build out of completely independent BoldEditing and BoldUI parts which are glued by the Bold plugin.

Some plugins will also need to refactored in a similar way to that proposed for the link feature (https://github.com/ckeditor/ckeditor5/issues/425#issuecomment-338201179) because the third rule means that a plugin like LinkUI shouldn't implement itself the whole UI because it will make it non-configurable and non-reusable (plugins have no configuration of their own).

[davidpolberger]

Sounds good, @Reinmar. Our main concern is that we need to have control over keystrokes, preferably without having to register a capturing event handler on the document that prevents keystrokes from reaching CKEditor. Today, we actually import the bold and italic plug-ins as-is (as we're fine with their default keystrokes), but import only the link engine, as we need to handle the Ctrl+K keystroke ourselves. KeystrokeHandler doesn't appear to allow existing keystrokes to be unset at present, but I'm assuming that https://github.com/ckeditor/ckeditor5-core/issues/8 will fix that.

[pjasiun]

 ==============================================================================|===============
|                               editing                                        |      UI       |
|==============================================================================|===============|
|                    engine                                |  keystrokes, ...  |
|==========================================================|===================|
|              model              |    view(controller)    |
|=================================|========================|
| schema | commands | post-fixers | converters | listeners |
 ==========================================================

To be precise:

• commands are not part of the engine,
• converters are not part of the view,
• we have observer not listeners in the view.

After the updated:

|================================================================================|==================|
|                                  editing                                       |        UI        |
|================================================================================|==================|
|                    engine                      |  commands  | keystrokes, ...  |
|================================================|===============================|
|         model        | controller |    view    |
|======================|============|============|
| schema | post-fixers | converters | obeservers |
|================================================|

[Reinmar]

That's a different POV and it described editor's architecture. But it misses the point of this discussion.

1. All commands operate on the model – they are methods of changing the model. We agreed to not implement model-unrelated commands in CKEditor 5 and AFAICS we kept it this way. So, while coming from the core editor architecture, they implement model actions.
2. There's a concept of view-controllers and, AFAICT, it applies to our features very well. We could pedantically talk about them in separation but that misses the point because it makes the picture more complex than it needs to be. Besides, how are converters different from the UI templates? They are not – they are pretty much the same thing. Converters tell the editor how to present the data (and handle the input) and they cooperate with listeners to add behaviours.
3. And yes, features implement listeners. We have a ClickObserver but what it does is firing an event. Then, features plug listeners to implement actions.

So, once again, you presented editor's architecture. And we're discussing here architecture of features. Different POV.

[ZaHgO]

To enable keystrokes I just added this code at the start of setupButtons on the Bootstrap demo:

editor.keystrokes.set( 'CTRL+B', 'bold' );
editor.keystrokes.set( 'CTRL+I', 'italic' );
editor.keystrokes.set( 'CTRL+U', 'underline' );
editor.keystrokes.set( 'CTRL+Z', 'undo' );
editor.keystrokes.set( 'CTRL+Y', 'redo' );
editor.keystrokes.set( 'CTRL+SHIFT+Z', 'redo' );

https://plnkr.co/edit/n1pqyFXk1h3yQ6z9A8Zc?p=info

[pjasiun]

So, once again, you presented editor's architecture. And we're discussing here architecture of features. Different POV.

I understand your separation, but I don't think that keeping multiple points of views make anything simpler. For developers who want to jump-in CKE 5, create plugins and move on, it will be misleading. They will look for the Commands class in the engine repository, and be surprised it's not there. They will not get why there are separate folders for view and controller in API docs if they have it together here. I think we should keep our architecture consistent. For sure tutorial for plugins developers should be consistent with the architecture overview. At the end of the day, plugins developers are the main group of the framework architecture users.

[jodator]

To sum up

Packages that requires refactoring:

• [x] ckeditor5-autoformat - rename *Engine -> *Editing
• [x] ckeditor5-basic-styles - rename *Engine -> *Editing, extract *UI
• [x] ckeditor5-block-quote - rename *Engine -> *Editing, extract *UI
• [x] ckeditor5-heading - rename *Engine -> *Editing, extract *UI
• [x] ckeditor5-image - rename *Engine -> *Editing, extract *UI
• [x] ckeditor5-link - rename *Engine -> *Editing, extract *UI
• [x] ckeditor5-list - rename *Engine -> *Editing, extract *UI
• [x] ckeditor5-undo - rename *Engine -> *Editing, extract *UI

The below packages seems OK as they are right now but to be consistent should they extract *Editing from main plugin? It looks like such extraction would be superfluous so I would leave them as they are:

• [ ] ckeditor5-enter
• [ ] ckeditor5-paragraph
• [ ] ckeditor5-widget

Packages that are either already refactored or are not applicable for this change:

• ckeditor5-adapter-ckfinder
• ckeditor5-alignment
• ckeditor5-clipboard
• ckeditor5-font
• ckeditor5-highlight
• ckeditor5-upload
• ckeditor5-build-balloon
• ckeditor5-build-classic
• ckeditor5-build-inline
• ckeditor5-cloudservices
• ckeditor5-core
• ckeditor5-easy-image
• ckeditor5-editor-balloon
• ckeditor5-editor-classic
• ckeditor5-editor-inline
• ckeditor5-engine
• ckeditor5-essentials
• ckeditor5-markdown-gfm
• ckeditor5-theme-lark
• ckeditor5-typing
• ckeditor5-ui
• ckeditor5-utils

ps.: @Reinmar mentioned something about checking Keystrokes but I'm not sure what to check with them. I've oly found that enter has TODO that suggest that keystroke handler might be used there (https://github.com/ckeditor/ckeditor5-enter/blob/94d7a01e913c7e50e916fd7c503914ad027c6fe9/src/enter.js#L35-L40).

[jodator]

@Reinmar I'm moving keystrokes from UI part to Editing part and I can see that CTRL+B (bold as an example) is used in both of them:

• to define what command to execute in EditingKeystrokeHandler.
• to show keystroke in Button's tooltip.

Defining the same value in two places is at least prone to refactoring errors in future.

It should be defined by Editing part of a feature (as in this ticket) but the UI part should be able to get this keystroke from editor.

I can see two options:

1. Allow to get keystroke(s) for a commandName (add method EditingKeystrokeHandler.get( commandName ).
2. Attach keystroke to command (as UI already obtains instance of a command).

I think that the first is better in terms of current API as it does not require to have those commands to be executed in any particular order:

// In BoldEditing:
editor.commands.add( BOLD, new AttributeCommand( editor, 'bold' ) );
editor.keystrokes.set( 'Ctrl+B', 'bold' );

// In BoldUI:
const keystroke = editor.keystrokes.get( 'bold' );
view.set( {
    label: t( 'Bold' ),
    icon: boldIcon,
    keystroke,
    tooltip: true
} );

[Reinmar]

editor.keystrokes.set( 'CTRL+B', 'bold' );

The right "keystroke style" is Ctrl+B :D There's a ticket on GH that we need to unify it across the codebase.

[jodator]

The right "keystroke style" is...

and what about the issues of getting the keystroke rather then duplicating it's string in two plugins?

[Reinmar]

BTW2, tickets about the keystroke handler that might interest you:

• https://github.com/ckeditor/ckeditor5-core/issues/8
• https://github.com/ckeditor/ckeditor5-core/issues/20

[Reinmar]

So, a short F2F talk left us with this – keystrokes.get() is a good idea, especially when keystrokes will become configurable (https://github.com/ckeditor/ckeditor5-core/issues/8).

However, not all keystrokes are bound to command – some are defined through callbacks. To solve this, we'd need to allow naming keystrokes.

• If a keystroke is bound to command, then command's name becomes keystroke name.
• If a keystroke is bound to a callback then you can specify the name when registering it.

That will allow us to make those callbacks configurable and available in keystrokes.get().

An API could look like this:

keystrokes.set( 'Ctrl+B', 'bold' );
keystrokes.set( 'Ctrl+B', 'bold', callback );
keystrokes.set( 'Ctrl+B', callback ); // anonymous keystroke

[jodator]

@Reinmar: Initially I've wanted to report this as bug but it is rather a feature split problem:

While working on feature split I've found that Editing and UI parts must be loaded in specific order. I'm not sure if we can do something about it though.

As an example after split you have to:

ClassicTestEditor.create( editorElement, { plugins: [ ListEditing, ListUI ] } );

doing it in reverse order:

ClassicTestEditor.create( editorElement, { plugins: [ ListUI, ListEditing  ] } );

will yield some errors - mostly due to missing commands and UI trying to bind to them.

This is due command caching "optimization":

const editor = this.editor;
const command = editor.commands.get( commandName );

editor.ui.componentFactory.add( commandName, locale => {
    const buttonView = new ButtonView( locale );
    buttonView.bind( 'isOn', 'isEnabled' ).to( command, 'value', 'isEnabled' );

    return buttonView;
} );

above will fail if order is "reversed" - simple fix is:

const editor = this.editor;

editor.ui.componentFactory.add( commandName, locale => {
    const command = editor.commands.get( commandName ); // There, I fixed it.
    const buttonView = new ButtonView( locale );
    buttonView.bind( 'isOn', 'isEnabled' ).to( command, 'value', 'isEnabled' );

    return buttonView;
} );

Probably it should go as some kind of good practice of writing plugins. WDYT?

[Reinmar]

Yes, this is unfortunate, but if we want to keep these plugins decoupled then they need to be loaded in a correct order.

[jodator]

@Reinmar but what about requiring command in factory method rather then at plugin init?

[Reinmar]

👍 It's a good idea.

However, there may be some shared things which may be needed earlier. So, as a general rule of thumb we should load the engine part before the UI part.

[scofalik]

I don't want to ruin your party, but now as I am thinking, Editing might not be the best name, as it defines stuff not only for "editing pipeline" (editor.editing) but also "data pipeline" (editor.data).

[pjasiun]

It defines even more than engine since it touches things like keystrokes. This is why we do not call it Engine AFAIR. But I am not sure if it is wrong.

[oleq]

I don't want to ruin your party, but now as I am thinking, Editing might not be the best name, as it defines stuff not only for "editing pipeline" (editor.editing) but also "data pipeline" (editor.data).

I wouldn't worry that much about that. IMO the name does the trick even if it's not 100% semantically correct.

[Reinmar]

Closed by https://github.com/ckeditor/ckeditor5/pull/844.