Introduce breakBlock to Composer.

[scofalik]

EnterCommand introduced enterBlock method which takes Selection instance and, briefly speaking, removes contents in that selection and breaks the block in which selection was made. Additionally, it was possible to pass defaultBlockName option which changed what kind of element was created if selection was at the end of the element (i.e. <h1> was changed to <p>).

Then, EnterCommand when executed, used that function. The callback to enter event was added:

this.listenTo( editingView, 'enter', ( evt, data ) => {
    editor.execute( 'enter' );
    data.preventDefault();
}, 'low' );

You could pass defaultBlockName as execution parameter: editor.execute( 'enter', 'p' );.

It appears that there are two problems concerning this approach:

1. We might need more flexibility in how EnterCommand behaves in some features. This would lead to putting more stuff/options into the command, which would be used only in single cases.
2. It was possible to overwrite enter event callback, but it was not possible to overwrite how EnterCommand works. In other word, in different feature I could do:

this.listenTo( editingView, 'enter', ( evt, data ) => {
    if ( something ) {
        editor.execute( 'enter', 'heading1' );
        data.preventDefault();
        evt.stop();
    }
}, 'high' );

So it was possible to overwrite what happens after clicking Enter key but if other feature wanted to call EnterCommand, it would have to use the default version (or know about all other features).

That's why we decided that it is a good idea to move this whole mechanism to Composer and make it work similar to deleteContents. Then, EnterCommand can just call Composer#breakBlock while other features can add their own callbacks. This is more flexible and correct approach.

PS: breakBlock name is temporary. Maybe there is a better name. However I don't like enter in original enterBlock name because it is strictly connected with Enter key and doesn't really describe what the method does.

[Reinmar]

PS: breakBlock name is temporary. Maybe there is a better name. However I don't like enter in original enterBlock name because it is strictly connected with Enter key and doesn't really describe what the method does.

The idea behind those composer's functions (like deleteContents and modifySelection) is that they are building blocks for creating commands which then reflect the user actions. This'd mean that the new function doesn't have to mention "enter" in its name.

OTOH, we need to understand which features will extend the enter key behaviour in what ways. E.g. if the list and blockquotes features will modify composer, then how the behaviour that when you press enter in an empty list item that list item is converted into a paragraph is related to "breakBlock" (if that was the method name)? It has nothing to do with breaking anything – it's an enter support. Similarly, a table feature will need to do something with the enter key in table cells and e.g. it may want to move the selection to a next cell. Again, not related to breaking anything – it's an enter support. And remember that executing the enter command should trigger all those behaviours, so in the current state of things you can only implement those modifications in a composer.

So, in this case I'm for enter() function. Not even enterBlock(), because in general it's not about "blocks". It's about supporting enter.

[scofalik]

I am not sure if it's the same thing. Let's play devil's advocate.

You could look at it this way:

1. "Breaking" paragraphs should/could work as it is working by default.
2. Hitting "enter" in empty paragraph does not really fire EnterCommand. It fires IndentCommand.

In lists, it would be easier to overwrite enter view event and it would have sense. It's hard to argue that outdenting list is a special case of breaking list element. While for headings it is straightforward - you always break the heading, but sometimes you want paragraph instead of heading.

😈

Oh, and, BTW, what do you mean by "supporting enter"? Supporting enter key? Then it should be done using view events. Supporting enter behavior? What really is "enter behavior". Ah, the questions :). But I don't like block in the name either.

Maybe breakAtSelection? Or breakContents? (we have modifySelection and deleteContents). insertBreak? Or, really, just break.

[Reinmar]

Hm... So you mean that a list feature should override the whole view#enter event because it changes its semantics to outdent command, yes?

Similarly, in table cells, the enter should change semantics to selectNextCell.

Makes sense for me. Then, you'll be able to always use enter command and the breakBlock functions inside e.g. empty list items in order to create a new item.

[scofalik]

The question is why someone would call EnterCommand? We can be less semantic and more focused on developers intentions. I am afraid that people will call EnterCommand having in mind "I want to simulate clicking enter key".

As I think of it, I think that EnterCommand should be named BreakCommand :D. Then, "by default", clicking enter key would break elements. You could overwrite break behavior. But when you want to do something else on enter you would overwrite enter event. I smell that the problem is that we are mixing together "breaking" and "using enter".

This also is in vein with what @pjasiun said: "Why would anyone want to use EnterCommand instead of enter event?". Our response was: "To break something from the code and be sure that it works the same as clicking enter". But in fact, we want that person to use BreakCommand which is just incorrectly named.

With love,
Szymon.

[scofalik]

BUT. Do we even need BreakCommand if all it does is calling Composer#breakBlock? I don't think so.

So maybe all we need is Enter feature that will introduce EnterObserver and add listeners that will just use Composer?

[Reinmar]

LOL, I feel like discussing with myself :P. We're too open minded ;)

We've got 3 layers on which a feature like enter, backspace or typing (there's a ticket about creating a typing command and we'll have insertSth methods in the composer one day) are implemented.

1. A view event (intention).
2. A command (action).
3. A composer's methods (primitives).

The first layer lets you react if the behaviour should be changed because of the view implementation. Let's say we have a fake selection which should alter multiple keys and mouse behaviour. The view events would be the place to inject your code. It's a view's feature listening on a view.

The second layer represents a feature and its state (command can be disabled). Commands can be seen as the API which a developer could use to build an external UI. And in fact, we also use them in this way – buttons' state is bound to commands and buttons execution execute commands.

Commands should reflect editor features such as "pressing enter in the editor", "pressing backspace in the editor", "pressing a letter in the editor", "inserting an image", "linking a text". Those features have a state and action and it seems that commands could be merged into the Feature class. However, we have features like "delete" which can have two kinds of actions (forward and backward delete) which may have different states. Hence, I think that it makes sense that a feature may have multiple commands.

Finally, we have the engine logic which has to support implementation of composable algorithms which are then used by the commands. However, there's a significant difference between how a command work and such engine methods. A command is a final implementation, so it uses a real selection. The engine methods, in order to be composable, can't use the real selection. They use a static one, so other pieces of code can decide what should really be changed and when.

How's that answering our concerns? Well, it isn't really answering all questions, but I needed to draw this mental picture of this architecture in my head :D.

Let's see...

(...) @pjasiun said: "Why would anyone want to use EnterCommand instead of enter event?". Our response was: "To break something from the code and be sure that it works the same as clicking enter".

It depends. If a developer wants to simulate a real enter key (e.g. we realise that some weird key on a strange layout or an enter button on a onscreen keyboard should have the same behaviour), view event is the answer. Also, it's useful for testing purposes to reproduce user actions in the most precise way.

If, however, a developer wants to write a feature in which the logic from the "pressing enter key" action is used, then the engine's method are the answer. Using this layer, the developer has a control over undo steps and moments of rendering a selection (and which selection is actually changed) and perhaps other things as well.

Hitting "enter" in empty paragraph list item does not really fire EnterCommand. It fires InOutdentCommand.

I've got a problem with this piece. At first, it seems that since a different command should be executed, the feature like list should alter the behaviour on the view layer. This sounds right – we change the semantics and fully reuse a different, existing command. This will also apply to case like enter in a table cell and enter on a selected widget. All those actions need to have their respective commands anyway.

However, can we go that far to say that for the "enter at the end of a heading" behaviour the heading feature needs to listen on the view? This would mean that we'll need an enterParagraph command which such a heading listener could use.

If we agree that this is OK, then it's clear that we don't need the enter primitive in the engine, because all the alternatives of the default enter behaviour are actually a different commands.

This will also mean that what I wrote above regarding the view:

It's a view's feature listening on a view.

is totally untrue. I'm also fine with that. We can reword this to "controllers listen on the view to perform their action". A feature like a heading is a controller which decides to perform a different action on the enter event. @pjasiun is now super happy.

To make sure that we're on the right track, let's consider another scenario. We have a widget. Pressing enter on this widget should enter its options. However, if it's a block widget, we need a way to enter the block after the widget (with keyboard and mouse). So we tell that shift+enter moves to the block after the widget or creates a new one. We have two commands moveSelection (with options like direction and unit) which we'll use for the first behaviour and insertParagraph for the second one. The second behaviour will need to be used if there's no block after the widget, however, how can we check that in a generic way? The moveSelection command's state will represent that.

At this point it still all make sense, so I think that we should be safe.

Summing up:

1. Enter is not a primitive which needs to be exposed in the engine. Enter key has couple of variations in how it should work, but this are alternatives for the default insertBlock (which should replace the current enter) command. We'll also need insertParagraph command for the heading feature (but I'm not sure where it should be located – in the paragraph feature?).
2. The current algorithms available in the composer are different than enter. Deleting a piece of a content doesn't have alternatives – the different behaviours are inside it and are reused in many places (e.g. enter needs to delete content before doing the rest, so do inserting pasted HTML).
3. And I thought that this is the end, but please tell me now how to alter the enter behaviour so it copies the current block's attributes? You need to do that on the view level, cause that's the only layer on which you can alter things, but you don't know whether for a specific situation a new thing should be inserted or e.g. outdent command should be executed.

So what do we miss to satisfy 3rd point? Add the insertBlock primitive? What it should actually do to be usable by insertParagraph and insertBlock? I'll leave it to the reader.

[scofalik]

I hope I understood correctly at least 50% of what you wrote.

We've got 3 layers on which a feature like enter, backspace or typing (there's a ticket about creating a typing command and we'll have insertSth methods in the composer one day) are implemented.

1. A view event (intention).
2. A command (action).
3. A composer's methods (primitives).

I can agree that we can divide our code on logical layers, however I am not sure we understand them correctly, so let's sum this up.

We have view events. However, just calling this "intention" is a bit dangerous. Of course, user has some kind of intention, but it is not strictly connected with view event. What I mean is that user's intention is not to "press enter". User's intention is to break paragraph. It just happened that, in text editors, enter key always does it. If we had a different key for every intention life would be simpler as we would just map them with commands/composer methods :). So a user has an intention and to realize it, they do an action (key press? mouse click?) in given context (heading? list item? paragraph?). In view events, we are predicting what kind of intentions users could have when editing text and how they realize those intentions

Features and Commands are more ambiguous to define. I'd rather define their roles by saying what they do in relation to other, defined, concepts. Let me move to composer's methods.

Composer's methods' role is pretty obvious. These are algorithms performing common tasks done on model document. The reasons they are defined are:

• hiding complexity from developer,
• not repeating the same code,
• ensuring that we have consistency throughout the editor.
  However, we have to think of them as of "tasks performed on model document":
• deleting selected contents,
• modifying selection.
  Is "pressing enter" an action that can be done on model document? Not really. You can break an element, insert some contents, remove or move them. There are more complex things you could do on model, I am sure. But "pressing enter" is not something that has any meaning in direct relation to model. So I want to stress that it's important not to mix "user actions" with "model tasks".

So we have user actions (view events) and building blocks (composer's methods). It's obvious that we need:

• something to connect them,
• a place for additional logic / algorithms.
  Let's still forget about Commands and focus on our problems.

1. "Heading enter problem". As I understand, the question is whether "creating a paragraph after heading" is more of different user's intention or a variant of breaking element. If it's the former, it should be defined as view event callback. If it's the latter it should be defined as callback in composer. To be honest, both solutions are fine for me.
2. "List item problem". We can ask ourselves the same question. Is "outdenting" more of different user's intention or is it a variant of breaking element? For me the answer is much simpler than in previous case. If you really think that "outdenting" is breaking an element then I am afraid we will have difficulty coming to an agreement :).

I still haven't touched describing Commands role. It's because I can't feel whether these are more on "users intentions" side or "tasks performed on model" side. Still, I feel like I don't want to agree that "pressing enter" is a Command, because I see them more of a tasks that may behave differently according to their state.

"Bringing a reaction to pressing enter key and providing a default behavior for it" seems like a good candidate for a Feature. But how would you describe "enter command"? I mean you could argue, that you want to fire it at some point at get some reasonable results. But each time you would like to fire it, it comes down to the fact, that you want to break something.

For me there's no clear role for this command. If this is "emulating pressing key" then we could fire view event. If it is "breaking current block" we could call Composer#breakBlock. It doesn't even have state or visual representation. Maybe that's the problem with this command.

I think that the majority of confusion and problems comes from:

• weak Command definition,
• difficulties with extending commands.

Now, for your summary:

1. From what I understand, we do agree. There's no such thing as "enter" in model or even engine.
2. Yes, we agree on that. It's certainly easier to understand it by focusing on deleteContents. No matter of Selection (context) you want to delete something. So intention is clear at this point. The only difference is that in some widgets/tables/etc. you might do something else to realize this goal.
3. Unfortunately here I can't understand your problem. If you want to alter "pressing enter key" behavior, you do it on the view. This means that you recognize that when user presses enter in this context they have different intention than breaking element. I don't understand problem with IndentCommand here: you overwrite default callback by setting a callback with higher priority. If you still want to break element but with something "extra" you can do it in three ways:
4. you can provide view event callback if you think this can be described as different user's intention,
5. you can provide Composer callback if you think this is a common scenario for breaking such element,
6. you can provide an option for Composer.breakBlock if you think this is a behavior that can be common for more elements.

As can be seen, there are mutliple approaches to the problem and it's difficult to point to the 100% correct one. However I am happy that we did agree on some things after all.

[pjasiun]

3. And I thought that this is the end, but please tell me now how to alter the enter behaviour so it copies the current block's attributes? You need to do that on the view level, cause that's the only layer on which you can alter things, but you don't know whether for a specific situation a new thing should be inserted or e.g. outdent command should be executed.

My thought, reading your discussion was: how much simpler it would be if we would create a not pluginable code. And I have a feeling that we try too hard to create a limitless pluginable solution. I do not think there is anything wrong having cases where writing plugins will be hard as long as these are not cases we are sure we will implement soon. We will have such cases anyway.

BTW: note that the level where you listen to the view.enter event is not necessarily a view level. It is a controller level. (it's comment to: "You need to do that on the view level,").

[Reinmar]

My thought, reading your discussion was: how much simpler it would be if we would create a not pluginable code.

:D

Of course, it would be simpler. But we're not creating a "not pluginable code", so we have such discussions. And this is just one of them – it's not an exception. Do I have to remind you who was complaining in CKE4 that a command's state cannot be governed by multiple features at a time? And who proposed a solution for that in CKE5? ;)

The case that I mentioned, with copying or not copying block attributes is a real use case – it's been asked multiple times how to achieve this in CKE4. If I have to give up on it already, then it means that there's something wrong with our architecture and the sooner we realise this the better. Especially, that we haven't yet reviewed this aspect.

BTW: note that the level where you listen to the view.enter event is not necessarily a view level. It is a controller level. (it's comment to: "You need to do that on the view level,").

Yes, later on in my previous post I negated this statement:

This will also mean that what I wrote above regarding the view:

It's a view's feature listening on a view.

is totally untrue. I'm also fine with that. We can reword this to "controllers listen on the view to perform their action". A feature like a heading is a controller which decides to perform a different action on the enter event. @pjasiun is now super happy.

[Reinmar]

We have view events. However, just calling this "intention" is a bit dangerous. Of course, user has some kind of intention, but it is not strictly connected with view event. What I mean is that user's intention is not to "press enter". User's intention is to break paragraph. It just happened that, in text editors, enter key always does it. If we had a different key for every intention life would be simpler as we would just map them with commands/composer methods :). So a user has an intention and to realize it, they do an action (key press? mouse click?) in given context (heading? list item? paragraph?). In view events, we are predicting what kind of intentions users could have when editing text and how they realize those intentions

Intentions are a popular concept which is now being debated by w3c. However, I agree that view events don't make for nice intentions. At the same level we have events like keydown and enter. So indeed, we shouldn't mix intentions into the view.

I can't agree with one a thing, though – enter is an intention. It's a key but behaviour of that key is identified with an intention. Just like "delete", it exists on both planes.

So I want to stress that it's important not to mix "user actions" with "model tasks".

I think I can agree with that for now. Let's see where it gets us.

I still haven't touched describing Commands role. It's because I can't feel whether these are more on "users intentions" side or "tasks performed on model" side. Still, I feel like I don't want to agree that "pressing enter" is a Command, because I see them more of a tasks that may behave differently according to their state.

That's where the knowledge about history makes it harder. In CKE4 you'd see commands like miximize or focusToolbar which are closely related to the UI. Also, other ones like image will open a dialog, not insert the image. So they are super "button-oriented".

So, I see that we have two clear points:

1. View events represent user actions.
2. ???
3. Composer contains building blocks for algorithms – "tasks performed on the model".

The idea for 2 is missing... and we don't know what to do with commands and intentions. So maybe let's join them? ;)

[Reinmar]

We continued the discussion F2F. The summary:

1. Commands should represent intentions. They are the link between user actions (view events and other view-related input like forms) and low-level building blocks.
2. A feature like "enter key in an empty list item should outdent" is an action leading to a separate user intention and will have its own command. The list feature will glue this by listening on the view. However, "enter key at the end of a heading" doesn't sound like a new intention – it's rather a variation in the enter command. So it needs to be implemented as a modification of the enter command. For that, we'll need a way to change a command's behaviour. We can already control its state, so it's good that we'll complete this picture by adding events for command execution.
3. There's, however, an unclear topic how a command could expose the batch that it uses. Other features need it in order to alter the default behaviour (unless they override everything). So – we can either force command implementers to fire the event manually inside their callbacks for enqueueChanges or automate this by allowing to define a callback for enqueuing changes directly as a command method. So instead of:

export default class EnterCommand extends Command {
    _doExecute() {
        const doc = this.editor.document;

        doc.enqueueChanges( () => {
            enterBlock( doc.batch(), doc.selection, { defaultBlockName: 'paragraph' } );
        } );
    }
}

You'll have a simpler:

export default class EnterCommand extends Command {
    _enqueueChanges( batch ) {
        enterBlock( batch, doc.selection, { defaultBlockName: 'paragraph' } );
    }
}

[scofalik]

As far as extending commands is concerned I like the idea of firing additional event by Command. Command already mixes in EmitterMixin (through ObservableMixin) and we already use this approach by adding callbacks to refreshState event.

For start, I'll add afterExecute event after the command is executed. The event will be fired with data object, as it is done in multiple places in editor already. data object will come from whatever _doExecute() returned.

_execute( param ) {
    if ( this.isEnabled ) {
        const data = this._doExecute( param );

        this.fire( 'afterExecute', data );
    }
}

This way we won't have any magical methods and we can document what is in data in each command separately.

[scofalik]

OTOH, as I look at it now, it will be more complicated to expand command this way.

_execute( param ) {
    if ( this.isEnabled ) {
        const data = this._doExecute( param );

        this.fire( 'afterExecute', data );
    }
}

In this scenario, afterExecute event is fired only after all enqueued changes are done and all changes trigger by those changes. This means that if any feature listened to change event and modified selection, we have incorrect selection and can't really expand how the command is working because we have wrong model state at that moment.

This means that we need fire afterExecute in the same, "main" enqueueChanges block. In this case, I think it may actually be better to introduce some kind of "magic" method, or just introduce ModelCommand that will overwrite this:

_execute( param ) {
    if ( this.isEnabled ) {
        this.editor.document.enqueueChanges( () => {
            const data = this._doExecute( param );

            this.fire( 'afterExecute', data );
        } );
    }
}

Then there will be no need to open this block in _doExecute.

[scofalik]

So, final decision (for now ) is that we are adding a true/false flag to the command that will alter behavior of _execute. If the flag is set to true, _doExecute and event firing will be "wrapped" in enqueueChanges block.

I'll put this on hold for a while.

This is not-a-small change. Some commands operate on model and feel like they should have this flag set to true, but are in fact a bit specific. I.e. ToggleAttributeCommand right now shouldn't have some of it's code in enqueueChanges because it makes it's behavior incorrect. To fix this, I would have to fix something in LiveSelection (but I am not sure if changes in selection really make sense, etc., etc.). Same with UndoCommand, which already fires revert event which is similar to afterExecute, in fact.

So, for now, I'll change EnterCommand so it will fire afterExecute event on it's own. The event will have an object with a batch as a parameter. So it will look like the final solution, but only for EnterCommand. Then, I will alter EnterCommand behavior in HeadingsCommand using that event.

Then, at the end of iteration 2, we may come back to this issue and finalize it.

[Reinmar]

I'm sorry, but it seems that our plan is already outdated: https://github.com/ckeditor/ckeditor5-core/issues/20 :D.

[Reinmar]

It turned out that we don't need this in the composer. The enter command became extensible and features should either listen on the view or on the command.

[scofalik]

Yes, after this whole discussion we ended up with doing not much, really.

Here is a "follow-up" issue in ckeditor5-core: https://github.com/ckeditor/ckeditor5-core/issues/22

[Reinmar]

I've described an architecture which makes most sense to me in: #340.