Expose the input command

[Reinmar]

Currently, the input feature simply listens on the view events and transforms them into changes on the model. However, this is a pretty complex logic and it's hard to reproduce those behaviours in tests.

The idea is to expose the input command, just like delete command is exposed by the delete feature.

Naming

I've been also considering insertText command, however, I see it a bit differently – like insertHtml. Both are used in CKE4 for pasting purposes, so e.g. they create separate undo steps and may process the content in some way.

Therefore, for now, let's call the command like we called the feature, so input. If we'll see a chance to somehow squash the two things together, then we may do this in the future.

The interface

From the command's perspective alone, it should simply accept a:

• text to insert,
• target ranges (optional – if not passed, it can use the selection).

(Note: Target range concept comes from the native beforeinput event: https://github.com/ckeditor/ckeditor5-typing/issues/46)

So what the command will do is:

1. Delete contents of the target ranges.
2. Insert the text.
3. Increment the buffer.

However, I've got some doubts whether it will be possible to extract such command from the current input feature. I hope it will...

[fredck]

I think it is cool to have a "fake typing" command, so input sounds great to me as long as it behaves exactly like keyboard typing.

[Reinmar]

Yep, that's the idea. We badly need it for testing purposes and it can be useful also in other situations.

[maxbarnas]

Private class MutationHandler inside Input operates on mutations. If we want to fake typing, then executing InputCommand should transform provided string into separate mutations. Does this make sense?

[Reinmar]

Nope, input command should not fire anything on view. Input command should be the handler of "text input" behaviour, and there can be multiple user actions which should result in text input. Mutations are just one of those ways.

For instance, if we'll want to handle input using the beforeinput event (see #3056), then it would be odd if such a handler would trigger mutations. It should trigger the input command directly.

[maxbarnas]

I don't want to let InputCommand fire anything - that was unclear in my comment. I am just asking if I should create util for transforming regular text to a list of mutations acceptable by InputCommand instance concretised to handle mutations.

For now InputCommand is specified on creation to listen to keydown events or mutation events. Handling beforeinput means that I have to add another case of input inside InputCommand. This is acceptable or you had in mind separating those different cases even more?

[maxbarnas]

From the command's perspective alone, it should simply accept a:

• text to insert,

Right now in current, early version of InputCommand I accept the original event data (just like it was done in Input). If you expect InputCommand to accept just text, then I have to think more about current solution.

[Reinmar]

Please see the ticket description ;)

The interface

From the command's perspective alone, it should simply accept a:

• text to insert,
• target ranges (optional – if not passed, it can use the selection).

The command should only work with a text. All the rest (translation from a mutation to a params which the command accepts should be done inside the input feature.

[maxbarnas]

Yep, I read it, but couple of things were unclear to me and I had conflicting conclusions. Thanks for confirmation :)

(e.g. DeleteCommand has almost all logic inside, where Delete is just thin Feature wrapper - this made me think I have to do it in similar way).

[maxbarnas]

Current, rough version kind of passes the tests, but still needs cleanup and check for CC level.

[maxbarnas]

It seems, that current refactor broke manual tests. Especially the Undo feature seems to revert just one character per one undo step. This might be connected to moving variable for counting inserted characters, insertedCharacterCount, from MutationHandler private class in input.js to InputCommand class.

The InputCommand with its own insertedCharactersCount is created once, along with Input feature. In the original code, the same counter is a member of MutationHandler, which is created once every mutations event. This needs fixing and I am looking for the elegant way to solve this.

[maxbarnas]

Also, inserting text in the current form, not only expects the range and the text, but also amount of characters to insert/delete (https://github.com/ckeditor/ckeditor5-typing/blob/t/48/src/inputcommand.js#L39). It is also used to decide if we want to remove or insert text.

This is extremely ugly, but for now I can't see any better way to do this. When I fix manual tests, I'll focus on fixing this. Any suggestions are welcome.

[maxbarnas]

Quick, temporary fix confirms my assumption - when I reset insertedCharacters Count in InputCommand, it correctly batches changes in the buffer.

[Reinmar]

OK... but without the code your comment makes no sense. You mention insertedCharactersCount but we don't know what it represents.

[maxbarnas]

Here you can find my second approach to the Input refactoring: ckeditor/ckeditor5-typing@712e859?diff=unified.

Please look at the part: ckeditor/ckeditor5-typing@712e859?diff=unified#diff-8052e4286a369903aa5d7f6c2d8b7190R154.
I suspect that we can replace all those distinct operations into a list of operations that can be executed as a single batch by InsertCommand. Let me know what you think about this.

[Reinmar]

This issue is blocked by https://github.com/ckeditor/ckeditor5-typing/issues/51#issuecomment-252164451. It turned out that we need to change the behaviour of typing quite significantly anyway.

[f1ames]

After looking a little bit into input feature and how it works, here are some questions :)

1.
Apart from the InputCommand should we also introduce the InputObserver (similar like the one for delete or enter feature) which will listen on (at least) keydown event and fire more specific event (e.g. input)? If so should this event by considered as event always resulting in content insertion (which means InputObserver will filter so called safeKeycodes and fire the event only for keys resulting in content insertion)? - well, keydown does not result in text insertion to be more specific as input feature operates on mutations so I am not sure exactly about InputObserver thing.

2.
The InputCommand should accept

target ranges (optional – if not passed, it can use the selection).

does it mean the array of ranges should be passed so the command will handle multiple ranges properly?

3.
Mutations results in text insertion. While its processing will stay inside input class/feature should it at the end use InputCommand to insert the resulting text? If so what about mutations resulting in text removal? Should we assume that if no text is passed to the InputCommand only removes text based on target ranges/selection?

[Reinmar]

Apart from the InputCommand should we also introduce the InputObserver

That's a very good question. I actually introduces such an observer when I was working on a PoC of the typing feature based on the native beforeinput event. See https://github.com/ckeditor/ckeditor5-typing/issues/46. However: it'd require more work to separate such an observer because it wasn't clear for me what kind of API it should have (how the event should look like, taken the native beforeinput event). Plus, I'm actually considering removing the DeleteObserver (https://github.com/ckeditor/ckeditor5-typing/issues/56), but perhaps it should stay because it may help to abstract the native beforeinput in the future.

[Reinmar]

does it mean the array of ranges should be passed so the command will handle multiple ranges properly?

Yes. The native beforeinput event predicts getTargetRanges() method. See https://w3c.github.io/input-events. So we'll be safe going the same way.

However, it will be a bit unclear which range should be actually modified by the command, because it's not yet testable in which cases the beforeinput event will provide more than one range.

So, if we're not using multiple ranges now (I think we don't) we can accept one and implement a support for an array of ranges later on. Supporting one range will even be useful, because most of the time you want to modify just one, so there's no need to pass an array.

[Reinmar]

Mutations results in text insertion. While its processing will stay inside input class/feature should it at the end use InputCommand to insert the resulting text? If so what about mutations resulting in text removal? Should we assume that if no text is passed to the InputCommand only removes text based on target ranges/selection?

The input feature works like this (or at least I think so), currently:

1. First, it finds a range to remove and text to insert.
2. Then it removes content from that range and inserts the text in the resulting position.

So, the entire 2nd step must be replaced by the command (that's why we're passing range(s) to it, not a position).

[Reinmar]

First, it finds a range to remove and text to insert.

Just one clarificiation – don't confuse that with the safeKeycodes handling. This is a different situation. If you, e.g. use a spellchecker for this text:

"hello wurd"

Then the input feature must handle replacing "ur" letters with "orl". So that happens based on mutations. The input command should be executed with range covering the "ur" letters and insert "orl" letters.

[f1ames]

Ok, so no InputObserver for now. I was also not sure what improvements and API it may introduce.

I will stick with single range and we may extend it in the future. I was thinking about support for single range but passing it as an array (so it will be compatible with future changes), however I think it might be a little misleading (what is the point of passing an array if only first element is processed) so I think InputCommand should accept a single range for now.

There is one thing which slightly confuses me in case of integrating InputCommand into input feature. At the current state the flow which InputCommand should implement is based on two events. On keydown event the content for current selection is removed to prevent browser generating mulitple mutations (as far as I understand the description - https://github.com/ckeditor/ckeditor5-typing/blob/master/src/input.js#L62) so the mutation event is fired on collapsed range (at least for typing). And then on mutations event (after processing mutations) text is inserted.
I am not sure yet how this asynchronous flow could be "packed" into one command (unless the command would be called two times - separately for keydown and then for mutations, or unless on keydown flow is considered as a separate thing).

[Reinmar]

I am not sure yet how this asynchronous flow could be "packed" into one command (unless the command would be called two times - separately for keydown and then for mutations, or unless on keydown flow is considered as a separate thing).

Handling the keydown event is a job for the plugin itself, not the command. It's a hack which has nothing to do with the input itself if we were using the beforeinput event (or at least I hope so).

Please read https://lists.w3.org/Archives/Public/public-editing-tf/2017Feb/0029.html – I described there how this feature works currently. It may shed some light on this for you.

[f1ames]

I pushed a proposal of InputCommand to t/48 branch.

However, there are still two things I would like to discuss:

1.
In the previous implementation the mutation handler manages the buffer size which means for mutations event which contains more than one mutation the number of text additions/removals is aggregated. For example if first mutation adds 5 characters and second one adds another 5 and removes 12 the buffer.size will not change as the changes sums up to -2 (and implementations assumes buffer cannot be updated with a negative value).
After the changes, the InputCommand manages the buffer. The command is called for every mutation separately so the modifications size/length is not aggregated. Which means for the case described above:

1. If we assume that the buffer cannot be updated with a negative value it will result in buffer.size increased by 5 (+5; +0)
2. If we assume that the buffer can be updated with a negative value it will result in buffer.size decreased by 2 (+5; -7)

Both results are different than in previous implementation.
The thing is, such mutations are generated by paste and text d'n'd which probably will be handled by separate mechanism so maybe there is no need to consider such cases for normal typing. And the InputCommand may stick with what was already implemented which means the buffer.size might by only increased and not decreased on modifications, WDYT?

2.
As we talked earlier with @Reinmar, all changes which requires enqueueChanges should be executed inside command. For mutations there is some selection position fixing (https://github.com/ckeditor/ckeditor5-typing/blob/master/src/input.js#L226 and https://github.com/ckeditor/ckeditor5-typing/blob/master/src/input.js#L268). Keeping the selection up to date also should be a part of InputCommand (unless it is some case specific handling)? WDYT?

[Reinmar]

First of all, I'd notice that if we have two separate mutations, then it's not currently a problem that the input command will be called twice for them. Such mutations should never occur – they mean that some big part of content changed and I'd doubt that this is a result of typing.

For example, as you noticed correctly, the pasting and dnd operations will be handled by separate features (pasting is, in fact, so you can add the clipboard feature to the sample).

Regarding the expected buffer change... I'd say that it's most reasonable to count only the positive values. So if you type a text and "a" is replaced by "à" (composition), it's still +1. But really, it's of a low importance – for the buffer size mostly the typed text count. Spell checking and other types of changes are irrelevant as they happen rather infrequently (except autosuggestions on mobiles) and, in fact, they should be counted as separate undo steps AFAIK.

[Reinmar]

Keeping the selection up to date also should be a part of InputCommand (unless it is some case specific handling)? WDYT?

Yes, definitely. The resulting selection should always be at the end of the inserted text and it's input's commands duty to set it. Hence, it's the input feature's role to pass such data to the command to place the selection correctly.

[f1ames]

Pushed changes based on @Reinmar comments to t/48 branch.

Now the buffer size considers only added/typed text. Also the selection setting was moved to InputCommand. There are two cases here:

1. For collapsed range the position is shifted by the number of inserted characters.
2. For other cases, the new parameter was added to the command: selectionAnchor, which if passed is used to restore selection after removing/inserting given text. This are the cases where the new position cannot be calculated based on the passed text and range so the proper position must be passed as a parameter.