Undo / Redo Text Selection Bug

[J. Heard]

Steps to reproduce:

1. Type in a line of text
2. Press Enter
3. Type another line of text.
4. Select the second line of text.
5. Press the "backspace" key and delete all of the highlighted text, or just simply press backspace to delete all of the text.
6. Press Ctrl-Z or the Undo toolbar button

Observation: Only some of the deleted text is restored after pressing undo, and in some cases the entire second line of text is deleted. The behavior is sporadic, but reproducible. I've also noticed that if you have a lot of content in the editor, after calling the undo command the very last line of text will vanish.

Expected: The highlighted / deleted text should be restored as it was prior to deletion and the remainder of the content remains unchanged.

Notes: Redo performs in the same odd fashion. First noticed this behavior in the 2.6.3 version, but it's also reproducible in the nightly build:

​http://www.fckeditor.net/nightly/fckeditor/_samples/default.html

[Frederico Caldeira Knabben]

I confirm this problem with V2 and V3, in both IE and FF.

[Tobiasz Cudnik]

Can't reproduce it on r3495. Can someone provide content which caused this issue ?

It's also possible that it was fixed during those 4 weeks since the report with some other ticket. But this would have to be in both V3 and V2...

[Frederico Caldeira Knabben]

I'm still able to reproduce it with the current trunk. We can't preload the editor to test it, as it much probably depends on the character counting logic of the undo plugin.

It happens with long lines of text. Here you have a screencast of it, with FF3:
​http://screencast.com/t/Jw9W4b2WbIz

Note that, after the BACKSPACE deleted the selected text, the undo action doesn't restore the text until the "END" word. Further undo and redo calls will never restore that text.

[Frederico Caldeira Knabben]

A new stable TC can be found here (with voice):
​http://screencast.com/t/fWFO1E4pxiN

Reproducible in the exact same way in FF3 and IE7.

[Garry Yao]

The capturing of Control Keystroke within the undo system is definitely a good move, while it's better if we can also support other key like 'arrows' and 'del' etc. A general DFA of undo system will be like:

1. When you're not typing:
   1. You press a /\w/, go to 2.
   2. You press a modify key, take snap, record the pressed key, go to 3.
2. When you're typing:
   1. If you press a /\w/, you're still typing;
   2. If you press a modify key, same as 1.2
3. When you're modifying:
   1. If you press a /\w/, take snap, go to 2;
   2. If you press a modify key and it's same as last, go to 3;
   3. If you press a modify key and it's diff from last, same as 1.2

[Garry Yao]

Another TC

1. Type in a line of text
2. Press Enter
3. Type another line of text.
4. Select the second line of text.
5. Press "Shift-Home" to select to the beginning, and press "Del" to delete all of the highlighted text.
6. Press Ctrl-Z or the Undo toolbar button

[Tobiasz Cudnik]

TC for backspace

1. In empty editor type "foo"
2. Press backspace
3. Press Ctrl+z

TC for delete

1. In empty editor type "foo"
2. Press left arrow
3. Press delete
4. Press Ctrl+z

Capturing arrows is possible with attached patch by uncommenting L13 and change of L220 to:

if ( !( beforeTypeImage.equals( currentSnapshot ) ) )

Although this would slow down comparison and there won't be any benefit (there'is no bookmarks for carret position, only for selection).

[Tobiasz Cudnik]

Version without use of indexOf (for IE).

[Garry Yao]

The patch works, two minor issues identified:

1. Sorry for unclear explanation of the 'Arrow Keys', actually they shouldn't cause snapshot but they're responsible for reseting typing state, take the following TC as example:
   1. Delete all contents and begin to type some characters;
   2. Navigate into the middle of the existed text line with 'Arrow Left';
   3. Begin to type some characters from this new position;
   4. Perform 'undo'.
      • Expected Result: There should be two undo points saved;
      • Actual Result: There's only one snap.
2. Plz comment out the following keymap OR refactor them into constants, make sure there's no magic number:

   var modifierCodes = { 8:1, 46:1/*, 39:1, 40:1, 38:1, 37:1*/ };
   

[Tobiasz Cudnik]

Now i see you point and i agree with it. I'm attaching another patch which covers this case and explains used keycodes.

[Garry Yao]

We're closing, but yet another problem left: The Modifier Key counting is mixed with Character Key counting, which is wrong, proved by the following TC:

1. Delete all contents;
2. Type 'word' as a line;
3. Press 'Backspace' to delete to the beginning of the line;
4. Type 'another' in the same line;
   • Expected Result: When press undo, the document should be empty.
   • Actual Result：There's no snapshot before the newly typed line, do when pressed undo, the document has 'word' a a line;

[Tobiasz Cudnik]

This patch covers you TC and additionally this one:

1. In empty editor type "foo foo foo"
2. Select second word with mouse like this "foo ^foo foo"
3. Start typing
4. Press Ctrl+z
5. Expected result is state from (2).

[Tobiasz Cudnik]

Repeating step (2) due to formatting:

foo ^foo^ foo

[Garry Yao]

Much better with 3372_5, but the only problem left is that we should reset the DFA about typing after a regular 'save', e.g. after insert a textfield. I'm attaching a updated patch with this purpose.

[Garry Yao]

I've also adding necessary comments which I though match with your codes, but correct me of course.

[Tobiasz Cudnik]

I've checked you changes, added a bit of new lines to make code more readable and i'm attaching the patch.

Seems everything works right now and definitely the ticket's bug is fixed.

[Garry Yao]

Sorry for that I've just spotted another issue with the patch at L202:

isSelected = !this.editor.getSelection().getRanges()[0].collapsed // Check selection for every keystroke.

This should be avoided as Fred said - native range retrieving without properly cached is an heavy operation, and we're now treating this for every keystroke, it will definitely has an impact on the overall PF.
Based on this, I think we should find a replacing approach, e.g. reset the state on every native selection change instead of manual checking here, how do you think?

[Garry Yao]

I guess the selection change is something similar with 'Shift Key' handling logic which has impact right on the 'startedTyping' variable.

[Tobiasz Cudnik]

After talk with Garry i've moved selection case to separate ticket #3691.

I'm attaching patch without selection undo support.

[Tobiasz Cudnik]

Fixed with [3643].