Opened 4 years ago

Closed 4 years ago

#12300 closed Bug (fixed)

Editor#change fired on first navigation key press after typing

Reported by: Piotrek Koszuliński Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.6
Component: General Version: 4.4.4
Keywords: Cc:

Description

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.

Change History (13)

comment:1 Changed 4 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:3 Changed 4 years ago by Artur Delura

Status: assignedreview

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

comment:4 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

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.

comment:5 Changed 4 years ago by 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 :)

comment:6 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.5CKEditor 4.4.6

comment:7 Changed 4 years ago by Artur Delura

Status: review_failedreview

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.

comment:8 Changed 4 years ago by 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.

comment:9 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

comment:10 in reply to:  8 Changed 4 years ago by 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.

comment:11 Changed 4 years ago by 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.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:12 Changed 4 years ago by Artur Delura

Status: review_failedreview

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.

comment:13 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Fixed on master with git:184dc7f.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy