Opened 6 years ago

Closed 6 years ago

#11126 closed Bug (fixed)

Various issues when undoing when at the bottom of snapshots stack

Reported by: Olek Nowodziński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.3.2
Component: Core : Undo & Redo Version: 4.0
Keywords: Cc:

Description (last modified by 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.

Attachments (1)

FF_sharedUndo_selectionBug.mp4 (1.3 MB) - added by Olek Nowodziński 6 years ago.

Download all attachments as: .zip

Change History (19)

Changed 6 years ago by Olek Nowodziński

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

cc

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

Status: newconfirmed

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

Description: modified (diff)
Summary: Shared undo stack between editorsVarious issues when undoing when at the bottom of snapshots stack

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

Description: modified (diff)

comment:5 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.3.1

comment:6 Changed 6 years ago by Olek Nowodziński

Description: modified (diff)
Milestone: CKEditor 4.3.2

comment:7 Changed 6 years ago by Olek Nowodziński

Description: modified (diff)

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

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

Version 0, edited 6 years ago by Piotrek Koszuliński (next)

comment:9 Changed 6 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:10 Changed 6 years ago by Olek Nowodziński

Status: assignedreview

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

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

Status: reviewreview_failed

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?

comment:12 Changed 6 years ago by 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(...).

comment:13 Changed 6 years ago by 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

comment:14 in reply to:  13 Changed 6 years ago by Olek Nowodziński

Status: review_failedreview

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.

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

Status: reviewreview_failed

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.

comment:16 Changed 6 years ago by Olek Nowodziński

Status: review_failedreview

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).

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

Status: reviewreview_passed

We were so close to miss this solution :).

comment:18 Changed 6 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

git:8f9b8b2 landed in master.

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