Editor#change fired on first navigation key press after typing

[Piotrek Koszuliński]

TC:

• Add editor#change listener to see when it's fired.
• Start typing. Change is fired for every letter.
• Press arrow key. Change is fired. It should not be.

Reproduced on all browsers.

Follow up of #11739 and #11611.

[Artur Delura]

Changes and tests in ​branch:t/12300. I'm not sure whether we should apply same checking in diffrent places. Search for this: .equalsContent(

[Piotrek Koszuliński]

I don't like this solution :). I don't know the code well enough to give details, but my gut says that this is a wrong place making it a hack instead of real solution.

As I can see undo manager knows pretty much nothing about editing handler - so far it used it in only one place - ​https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/undo/plugin.js#L293 - which is already too much and I didn't like this either. But let's say that undoManager#type should be moved to editingHandler class because it does its job, so it's ok, because after the change undoManager would know nothing about it again.

What you did is made the undoManager#save method dependent on editingHandler and that's worse, because the save() method is totally generic - it's not and should not be related to typing, hence it should not access editingHandler.

What I would suggest is that this case should be handled before save() is called - most likely it should not be called at all.

[Artur Delura]

Let's close #12354, masterize and then rebase branch for this ticket to new master, before we make some further changes in this ticket branch. Now we both have wider view about undo manager, so I can find out better solution I guess :)

[Artur Delura]

I used the editing handler to get the lastKeydownImage which is by the way passed as a second argument in the save function. I moved out call to the editing handler to the onKeydown method in the nativeEditingHandler. But I don't see this as a good solution (I preffer the previous one). Anyway, changes in ​branch:t/12300.

[Piotrek Koszuliński]

1. I don't like and see a need for the change in equalsContent. It's not tested, not documented and finally I don't see that we are starting using this method without passing an argument there. What's more, you should check whether some image exists before calling equalsContent.
2. I don't think that one test is enough for this change. For example I changed the line to:

   undoManager.save( false, this.lastKeydownImage, false );
   

And all the tests passed. So either my change is correct, or the tests are not sufficient.

[Artur Delura]

Replying to Reinmar:

1. I don't like and see a need for the change in equalsContent. It's not tested, not documented and finally I don't see that we are starting using this method without passing an argument there.

But what is wrong with it? This additional check won't break nothing.

What's more, you should check whether some image exists before calling equalsContent.

Why overcomplicate things and why it's important? When we call equalsContent we don't need to check anymore whether passed argument exists.

[Piotrek Koszuliński]

The change must have a reason. In this case the doc says that the otherImage is a required argument and I don't see any place where we stop passing the otherImage. So starting checking if it's passed, without changing the docs and without a reason is totally wrong.

Moreover, it's also wrong because the existence of other image should be checked outside the method, in the code that uses it. The reason for that are silent errors that may be caused by such checks inside the method and confusion which such checks create when they are placed inside (if that's not backed up by the docs).

Furthermore, if the argument isn't passed the code should throw an error, not return false...

tl;dr Changing this without a reason is the overcomplication. Do not add unnecessary code.

[Artur Delura]

We decided that checking whether current editor snapshot is same as lastKeydownImage is not necessary because it's always true. Changes in ​branch:t/12300.

[Piotrek Koszuliński]

Fixed on master with ​git:184dc7f.