Holding Backspace or Delete no longer generates undo snapshots every 25 characters

[Rick Schnorenberg]

Steps to Reproduce

1. Open the ​Nightly Demo Page
2. Place your cursor in the document and hold the backspace or delete key until over 25 characters are deleted
3. Undo

Expected Result: The last 25 deleted characters are restored to the document

Actual Result: All characters that were deleted are restored

[Marek Lewandowski]

This one will be tricky, since reliable keystrokes count is available in ​type method. It's called from keyup event, so that's why it's not invoked during key holding ( only keydown and keypress are firing ).

This one might be taught to deal with, I took a quick look at the problem, and I don't see an obvious solution. Still we need to be careful with major changes.

[Piotrek Koszuliński]

Could we implement something like safety valve? A mechanism standing besides the current implementation that will handle just the hold key?

[Artur Delura]

By the way, I have tricky use case:

1. Open editor with 26 characters and caret position: <p>aaaaaaaaaaaaaaaaaaaaaaaaaa^</p>
2. Press Backspace 25 times.

In theory snapshot should be created here, but is that good behaviour? State of editor is like that: <p>a^</p> but snapshot is <p>aa</p>. As result we can press Crtl + Z and snapshot last will be restored. So I think that we shouldn't create snapshot yet but it should be created after 25 + 1 keyDown`s. And calling method type In keyDown callback make sense here because in this callback is called before any DOM modifications, so state will be saved from 25 keyDown. And everything will work fine.

[Marek Lewandowski]

Replying to a.delura:

By the way, I have tricky use case:

1. Open editor with 26 characters and caret position: <p>aaaaaaaaaaaaaaaaaaaaaaaaaa^</p>
2. Press Backspace 25 times.

Btw. type method was called after DOM modifications, but it saved snapshot before modifications. See ​this, so this is another argument why it should be called in keyDown callback.

That was the special case needed for group change.

In theory snapshot should be created here, but is that good behaviour? State of editor is like that: <p>a^</p> but snapshot is <p>aa</p>. As result we can press Crtl + Z and snapshot last will be restored. So I think that we shouldn't create snapshot yet but it should be created after 25 + 1 keyDown`s. And calling method type In keyDown callback make sense here because in this callback is called before any DOM modifications, so state will be saved from 25 keyDown. And everything will work fine.

UndoManager.type method was ment only to be called in case when:

• pritnable key
• functional key ( backspace / delete )

Input event (with exception of functional keys in IE) provides us reliable information in that regard.

You're not able to determine if input event will occur in keydown event. Skipping that will cause undoManager.type method to be called on Print Screen, Play / Pause, F22 key etc. And all of them does not change the content.

You need to remember that this is also related to ​change event, we can fire it only in case of printable keys.

[Artur Delura]

I must say, I'm a bit confused now. And my approach is a bit diffrent than presented in undomanager.

1. I would not create initial snapshot when CKEditor instance is created. It brings mess: There is a snapshot but UI suggest that snapshot array is empty (undo/redo buttons are disabled). We can not see this because UI is refresh on keyUp event.
2. I would move saving snapshot logic and setting flags to keyDown event callback. And there I would save last keyDown snapshot - Yes, that one before current changes in DOM (We have access to current state all the time).
3. This is less important but I would create object {{{SnapshotsCollection}} which will be responsible for adding, removing, manipulating current index, getting previous/next snapshot and so on.
4. I wonder why in undomanager there is no selectionChange event listener. I think that it would be helpfull. Of course if this event will be fired when user call key combination like Shift + Left

[Marek Lewandowski]

Replying to a.delura:

I must say, I'm a bit confused now. And my approach is a bit diffrent than presented in undomanager.

1. I would not create initial snapshot when CKEditor instance is created. It brings mess: There is a snapshot but UI suggest that snapshot array is empty (undo/redo buttons are disabled). We can not see this because UI is refresh on keyUp event.
2. I would move saving snapshot logic and setting flags to keyDown event callback. And there I would save last keyDown snapshot - Yes, that one before current changes in DOM (We have access to current state all the time).

That would only make firing reliable change events so much harder.

As a workaround to possibly make things works the way you described you'd need to perform a snapshot compare on each keyup event. The input event handles it all for us.

3. This is less important but I would create object {{{SnapshotsCollection}} which will be responsible for adding, removing, manipulating current index, getting previous/next snapshot and so on.

This part of old undo was working pretty, so well there was no need to change it.

4. I wonder why in undomanager there is no selectionChange event listener. I think that it would be helpfull. Of course if this event will be fired when user call key combination like Shift + Left

@see ​selectionChange docs, it's optimized, threre's no use for it in that case.

[Artur Delura]

Method getNextImage don't work as expected in situation like this:

snapshots.length == 1 // true
// and first snapshot has contents like this:
'<p>aaaaaaaaaa</p>'

// and current editor state is like this
'<p>aaaaa</p>'

and we call getNextImage with first argument equals to true which identify that we want next undo snapshot - it will return null` but should return first item.

I think we should fix this, because it will resolve my current issues. I hope it will not break nothing, because in each cases method will work same, but only in described above will be diffrences. Are you guys with me?

[Piotrek Koszuliński]

If you want getNextImage() to return the first snapshot you must first save the current one. getNextImage() has only one role - return the next image regardless of editor contents about which undo manager knows nothing.

[Artur Delura]

Not so many changes, mainly tests added. But it was a long trip :D. Changes in ​branch:t/12354c.

[Piotrek Koszuliński]

I pushed few commits to ​branch:t/12354c. Still, this needs to be clarified:

• isNavigationKey, navigationKeyCodes, keyGroupsEnum, getKeyGroup - all must be moved to UndoManager (class - so become static properties/methods).
• ignoreInputEvent, inputFired and keyEventsStack properties and ignoreInputEventListener method are not documented.
• I don't like the way how keyEventsStack is used in onInput - you bypass the public API it has. You should use the increment() method (which, by the way, isn't used anywhere).
• In the onInput method you arbitrary use number 25 and compare it with inputs - this is incorrect. First of all, the default of config.undoStackSize is 20, second of all, you should of course use it. The same in tests.
• There are no tests for keyEventsStack.
• There's no code documentation for places where keyEventsStack is used. This is not an easy logic, so make sure that it's clear what's happening.
• Couldn't we stop using inputFired when we have keyEventsStack?
• There's no documentation why keydown listener is added with priority 5. And this is a good question - why?
• I again don't like the onInput's code :D. Why do you check only last key's inputs count? Aren't you interested in a global count?

[Piotrek Koszuliński]

Couple of additional questions - why do we need a stack? Precisely - what are the reasons that it must be a stack and not a single counter? I know that we talked about this, but I want to have it once more explained, this time "on paper". This implementation is 100LOC, so we must have strong reasons.

[Piotrek Koszuliński]

There's no documentation why keydown listener is added with priority 5. And this is a good question - why?

BTW. This conflicts with #12332.

[Artur Delura]

Replying to Reinmar:

Couple of additional questions - why do we need a stack?

First of all it describes well actual keys situation, i.e.:

• Which keys are currently pressed.
• How many input events was called for each key.
• Which key was pressed after another.

As we can see it’s some kind of model. The idea behind providing KeyEventsStack was to step by step separate model from the rest of code, but also to avoid regressions. I tried to touch current code as little as possible. So for example current code still rely on inputFired property. My plan was to:

1. Create parallel model (and keep current one).
2. Write tests for it.
3. Check differences between new model and old data.
4. If there are no differences - remove old data.
5. If there are, check why, fix them and then remove old data.

It seems that KeyEventsStack is not needed, but for now I intentionally tried to use it only to fix issue reported in this ticket, just to avoid regressions.

On the other hand we can say that we don’t need KeyEventsStack we can move everything into NativeEditingHandler because it’s also some kind of model for events. The problem is that we make it even more complicated. There is also problem with NativeEditingHandler implementation, because it mix model and functionality. For example has reference to UndoManager and call methods on it, but it could just trigger some event. Another question is why UndoManager have field strokesRecorded? Shouldn’t it be defined in NativeEditingHandled? To simplify things we need to have one reliable model - single source of truth. It can be KeyEventsStack either NativeEditingHandler.

[Artur Delura]

Replying to Reinmar:

• Couldn't we stop using inputFired when we have keyEventsStack?

Yes, we could. And also we could move strokesRecorded from UndoManager. Btw. I found out just now, that strokesRecorded is defined in prototype, so it’s shared between all instances. As well as previousKeyGroup.

[Piotrek Koszuliński]

Replying to a.delura:

Replying to Reinmar:

• Couldn't we stop using inputFired when we have keyEventsStack?

Yes, we could. And also we could move strokesRecorded from UndoManager. Btw. I found out just now, that strokesRecorded is defined in prototype, so it’s shared between all instances. As well as previousKeyGroup.

That's not true. Only the default value is set on prototype but when it's modified it gets set on the object itself, so it's no longer shared.

[Piotrek Koszuliński]

Replying to comment:15 - the model must exist for some reason and your comment does not explain the reasons. It justifies the separation, but not the need to exist. Which must be justified in the first place. So the question is - why isn't it possible to handle this bug using inputFired or other existing property of NativeEditingHandler?

[Artur Delura]

Replying to Reinmar:

Replying to a.delura:

Replying to Reinmar:

• Couldn't we stop using inputFired when we have keyEventsStack?

Yes, we could. And also we could move strokesRecorded from UndoManager. Btw. I found out just now, that strokesRecorded is defined in prototype, so it’s shared between all instances. As well as previousKeyGroup.

That's not true. Only the default value is set on prototype but when it's modified it gets set on the object itself, so it's no longer shared.

I see... It's very unreadbale and misleading, because it's natural that prototype values are shared between instances. It should be definitely moved to constructor. It works because before any modifications it's overwritten in resetType method. It's very easy to make mistake while refactoring, and make it shared.

[Artur Delura]

Replying to Reinmar:

Replying to comment:15 - the model must exist for some reason and your comment does not explain the reasons. It justifies the separation, but not the need to exist. Which must be justified in the first place. So the question is - why isn't it possible to handle this bug using inputFired or other existing property of NativeEditingHandler?

Only property which we could rely on is inputFired but this property is not reliable. Just add console.log(this.inputFired); around ​this line, and play with editor. This property is not reset when we want to. I don't see way to rely on it. We decided before to touch existing code as little as possible. That's why we introduced stack: "[...] safety valve? A mechanism standing besides the current implementation that will handle just the hold key?"

[Piotrek Koszuliński]

But if it's unreliable, why not fixing it? It cannot be fixed? Or a fix will be too vast? You still didn't prove that we need the stack - you only showed that something is wrong with the current implementation, but not even what.

[Artur Delura]

Current implementation of UndoManager does not take into consideration that there is possibility to keep key pressed which cause fire input event few times in a row. inputFired value is incremented on each input event. On keyup event it simply decrement inputFired value blindly assumed that there was only one input event in a row. For example: User press and hold "a" key for a period of time what ends up with added a character 13 times in editor and inputFired have now value 13. After relesing "a" key inputFired is decremented by one, which result inputFired 12. That is unexpected behaviour, we should end up with value set to 0. To fix this issue we need mechanism which register how many input events was registered in a row for same keyCode. As a rescue comes KeyEventsStack which register how many input events was called for each keyCode.

Goal: Get rid of KeyEventsStack.

Instead of making KeyEventsStack we could also introduce fields called like lastKeyCode and lastKeyCodeInputs in NativeEditingHandler. Which will be set: lastKeyCode - set key code in onKeyDown method. lastKeyCodeInputs - reset to 0 when current key code is different than last one, increment by one. This will be happened in onInput method.

Problem: In onInput method we don’t know which keycode was last one.

Solution: KeyEventsStack keep track of that. But that’s conflict of interest.

Another solution: We can reset lastKeyCodeInputs to 0 in onKeyDown method when current keyCode is different than last one.

When I get deeper into this I see that KeyEventsStack is not ideal yet. First of all it does not handle input or drop events. Secondly it lost registered input for keyCode when key is up (problem can appear when holding multiple keys). Last but not least it does not sums up input events for all pressed keys (but that’s just one method to write).

So yes, we can get rid of KeyEventsStack but, it will make code more complicated, because we introduce more fields, and logic in NativeEditingHandler. It does not separate logic from model. KeyEventsStack model keyboard situation in more sophisticated way. We need less fields in model and more methods instead, to make things clear.

[Piotrek Koszuliński]

Kudos! Now it's clear that we need the stack. But it's also clear that it conflicts with the inputFired flag and I would try to replace it with the keys stack.

[Artur Delura]

Changes in the same branch.

[Artur Delura]

Replying to Reinmar:

There's no documentation why keydown listener is added with priority 5. And this is a good question - why?

BTW. This conflicts with #12332.

It's no longer needed. There was problem with keyStrokes, I remember that some test failed. We talked about it on skype, but even I don't understand myself, what I was talking about.

[Piotrek Koszuliński]

I don't like the fact that you put the code for last fix outside the onKeyDown listener and code style issues in both - tests and dev code.

[Artur Delura]

Replying to Reinmar:

I don't like the fact that you put the code for last fix outside the onKeyDown listener

I made it intentionally. That is the responsibility for NativeEditingHandler this class is mainly responsible for handling events, so one of subresponsibility is to blur diffrences between browsers in context of events.

[Piotrek Koszuliński]

But the onKeyDown is NativeEditingHandler's method!

[Artur Delura]

Replying to Reinmar:

But the onKeyDown is NativeEditingHandler's method!

Right. But onKeyDown method is reaction for key down event it shouldn't call any extra event callbacks. Why complicate things? Place where event callbacks are called is attachListeners method. attachListeners - responsible for call event callbacks in proper order or as react for event - fixing incompatibilities.

on* - callbacks which are called on this events.

[Piotrek Koszuliński]

OK, let's say I buy this :). Then I'll review the rest as soon as you push it on review.

[Piotrek Koszuliński]

I pushed 8 commits to the ​branch:t/12354c and in fact I rewrote a big part of the type() method. I did this after I found a very ugly issue - type few characters and then press and hold backspace. In such case lastKeydownImage was totally forgotten because type() was called too late. I realised that when key groups is being changed we need to save snapshot on keydown, not later. This proved that big part of type() method wasn't needed at all. Then I had to fix couple of broken tests which, as I noticed earlier, passed because of minor bugs in the code (comparing total inputs to strokes limit using ">" instead of ">="). Then I noticed that change is fired twice on every held key. Fixed as well.

Known issues:

• #12300,
• similar to #12300, but changing key group

Both totally ok to be ignored now.

[Artur Delura]

Issue:

1. Type some letter.
2. Press backspace.

Actual result: Change event is called twice. I will write test for it. Don't want to touch before write test. This is #12300.

Known issues:

• #12425

I pushed one minor commit. Couldn't find nothing new.

[Piotrek Koszuliński]

Merged to master with ​git:65b6845.

[Piotrek Koszuliński]

This was a long journey indeed. Good job, Artur!