Opened 10 years ago

Closed 10 years ago

#3372 closed Bug (fixed)

Undo / Redo Text Selection Bug

Reported by: J. Heard Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.0
Component: Core : Undo & Redo Version: FCKeditor 2.6.3
Keywords: Confirmed Review+ Cc:

Description (last modified by Frederico Caldeira Knabben)

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

Attachments (8)

3372.patch (1.4 KB) - added by Tobiasz Cudnik 10 years ago.
3372_2.patch (1.7 KB) - added by Tobiasz Cudnik 10 years ago.
3372_3.patch (1.7 KB) - added by Tobiasz Cudnik 10 years ago.
Version without use of indexOf (for IE).
3372_4.patch (2.0 KB) - added by Tobiasz Cudnik 10 years ago.
3372_5.patch (2.7 KB) - added by Tobiasz Cudnik 10 years ago.
3372_6_ref.patch (4.5 KB) - added by Garry Yao 10 years ago.
3372_6.patch (4.6 KB) - added by Tobiasz Cudnik 10 years ago.
3372_7.patch (4.5 KB) - added by Tobiasz Cudnik 10 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 10 years ago by Artur Formella

Keywords: undo redo bug removed
Priority: HighNormal

comment:2 Changed 10 years ago by Frederico Caldeira Knabben

Description: modified (diff)
Keywords: Confirmed added
Milestone: CKEditor 3.0

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

comment:3 Changed 10 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

comment:4 Changed 10 years ago by Tobiasz Cudnik

Owner: changed from Garry Yao to Tobiasz Cudnik
Status: assignednew

comment:5 Changed 10 years ago by Tobiasz Cudnik

Status: newassigned

comment:6 Changed 10 years ago by Tobiasz Cudnik

Keywords: Pending added; Confirmed removed

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

comment:7 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Confirmed added; Pending removed

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.

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

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3372.patch added

comment:9 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added

comment:10 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

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

comment:11 in reply to:  description Changed 10 years ago by 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

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3372_2.patch added

comment:12 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

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

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3372_3.patch added

Version without use of indexOf (for IE).

comment:13 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

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*/ };
    

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3372_4.patch added

comment:14 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

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

comment:15 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

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;

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3372_5.patch added

comment:16 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

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

comment:17 Changed 10 years ago by Tobiasz Cudnik

Repeating step (2) due to formatting:

foo ^foo^ foo

comment:18 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

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.

Changed 10 years ago by Garry Yao

Attachment: 3372_6_ref.patch added

comment:19 Changed 10 years ago by Garry Yao

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

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3372_6.patch added

comment:20 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

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.

comment:21 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

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?

comment:22 Changed 10 years ago by Garry Yao

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

comment:23 Changed 10 years ago by Tobiasz Cudnik

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

I'm attaching patch without selection undo support.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3372_7.patch added

comment:24 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

comment:25 Changed 10 years ago by Garry Yao

Keywords: Review+ added; Review? removed

comment:26 Changed 10 years ago by Tobiasz Cudnik

Resolution: fixed
Status: assignedclosed

Fixed with [3643].

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