Various issues when undoing when at the bottom of snapshots stack

[Olek Nowodziński]

Surprisingly, for some reason, when multiple inline editor are in use (/samples/inlineall.html), they share (actually: abuse) one another's undo stack. It happens when CTRL+Z is pressed and there are no snapshots left on the current stack.

It doesn't look like a feature, really, and since the issue can be reproduced since 4.0 in different browsers, it's time we fixed it.

See attached video (recorded in FF) to know more.

---

Another TC. On IE open the inline all sample, focus one of the editors with more complex contents, type something and press CTRL+Z multiple times. Textfified version of the initial contents will be loaded.

---

Another TC in FF:

1. ​http://ckeditor4.t/ckeditor/samples/replacebyclass.html
2. Put the caret

   Apollo 11^
   

3. Type "moo"
4. C-z

   ^Apollo 11
   

5. C-z

   ^
   Apollo 11
   

Expected

^Apollo 11

but another, native undo snapshot reverts the selection to the position initially set by the browser.

The reason

It seems that editor does not prevent the native undo if it reached the bottom of snapshots stack. It allows browser to do stuff and weird things happen.

[Piotrek Koszuliński]

cc

[Piotrek Koszuliński]

I +1 fixing this in 4.3.2, because we found this issue again. Every release it falls to our hands.

[Olek Nowodziński]

​t/11126 introduces a patch that blocks native undo when pressing CTRL+z, CTRL+y and CTRL+SHIFT+y.

[Piotrek Koszuliński]

I don't understand the change. Why do we use getKeystroke now? What does happen in editor.setKeystroke (I mean that closure used there)? Why do you mention undo in keystroke.js? What does this change mean for other keystrokes?

[Olek Nowodziński]

The change is very simple. Keystrokes were defined explicitly in editor.setKeystroke( ... ). To avoid a nasty, redundant condition:

if ( keystroke == CKEDITOR.CTRL + 90 || keystroke == CKEDITOR.CTRL + 89 || keystroke == CKEDITOR.CTRL + CKEDITOR.SHIFT + 89 )
    return event.data.preventDefault();

I gathered them in the object. The "mysterious closure" explodes keystrokes from { key1: command1, key2: command1 } format to [ [ key1, command1 ], [ key2, command2 ] ].

Why do we use getKeystroke now?
[...]
What does this change mean for other keystrokes?

It means nothing. The ​existing condition will work both with getKey and getKeystroke. Also nothing changed in editor.setKeystroke(...).

[Piotrek Koszuliński]

It means nothing.

If it means nothing, then we should not change getKey to getKeystroke. Just to avoid confusion like the one I had :D

So, if I understand correctly, all changes are only to have keystrokes objects which can be used to easily preventDefault them. This is hack, not a solution :P.

I assume that it's possible to preventDefault from keystroke handler. E.g. when undo manager is not empty (undo/redo state is OFF) preventDefault is called, right? And it should be also called when command is disable, but for some reason it isn't. Question is - why? And another question is - should it also be always called in case of other keystrokes? I guess so, because when we have a keystroke defined we don't want the default action at all. So in my opinion the bug is here: ​https://github.com/ckeditor/ckeditor-dev/blob/master/core/keystrokehandler.js#L56

[Olek Nowodziński]

Replying to Reinmar:

So in my opinion the bug is here: ​https://github.com/ckeditor/ckeditor-dev/blob/master/core/keystrokehandler.js#L56

Human-debugger at his best ;)

Of course, the bug is in keystrokehandler. Pushed ​t/11126b with a quick fix.

[Piotrek Koszuliński]

Please don't make the code harder to understand. In proposed solution code has to be read from right to left.

Also, I don't see any tests. And tests for this case could give us an interesting answer - namely, whether we need to check the returned value at all? I think we do, but there should be tests when default action is prevented and when not.

[Olek Nowodziński]

​t/11126c: A fix using config.blockedKeystrokes, as an analogy to what we do with CTRL+B, CTRL+I, etc., without touching mysterious core.

Anyway, it is CTRL+SHIFT+Z what ​does redo (it was wrong since t/11126).

[Piotrek Koszuliński]

We were so close to miss this solution :).

[Olek Nowodziński]

​git:8f9b8b2 landed in master.