Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Umbrella] Undo/redo issues #2723

Closed
32 of 35 tasks
Reinmar opened this issue Jul 6, 2017 · 45 comments
Closed
32 of 35 tasks

[Umbrella] Undo/redo issues #2723

Reinmar opened this issue Jul 6, 2017 · 45 comments
Labels
package:undo resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Reinmar
Copy link
Member

Reinmar commented Jul 6, 2017

Since we know that there a lot of issues with undo found after the recent refactoring let's report them in one place.

@Reinmar Reinmar changed the title [Umberlla] Issues with undo [Umberlla] Undo issues Jul 6, 2017
@Reinmar
Copy link
Member Author

Reinmar commented Jul 6, 2017

Type: Redo, selection restoring

Selection is restored incorrectly:

jul-06-2017 18-11-20

@Reinmar
Copy link
Member Author

Reinmar commented Jul 6, 2017

FIXED

Rename leaks to a nearby element.

jul-06-2017 18-14-28

@Reinmar
Copy link
Member Author

Reinmar commented Jul 6, 2017

FIXED

Additional list empty block appears (wrap, split):
jul-06-2017 18-15-54

@Reinmar
Copy link
Member Author

Reinmar commented Jul 6, 2017

FIXED

Type: Undo, performance

Undo becomes noticeably slow when undoing typing :((((

It seems that typing compression will be a must-have in a short time.

jul-06-2017 18-19-31


Edit by LukaszGudel: It looks much better now, undoing and redoing typing is almost instant. IMO fixed.

@Reinmar
Copy link
Member Author

Reinmar commented Jul 6, 2017

FIXED

jul-06-2017 18-23-14

basic-transformations.js:67 Uncaught TypeError: Cannot read property 'getNode' of undefined
    at addTransformationCase (basic-transformations.js:67)
    at Object.transform (transform.js:59)
    at Object.transformDeltaSets (transform.js:268)
    at Document.transformDeltas (document.js:359)
    at RedoCommand._undo (basecommand.js:140)
    at Array.editor.document.enqueueChanges (redocommand.js:42)
    at Document.enqueueChanges (document.js:241)
    at RedoCommand.execute (redocommand.js:36)
    at RedoCommand.on (observablemixin.js:327)
    at RedoCommand.fire (emittermixin.js:281)

@Reinmar Reinmar changed the title [Umberlla] Undo issues [Umberlla] Undo/redo issues Jul 6, 2017
@scofalik
Copy link
Contributor

scofalik commented Jul 7, 2017

Undo becomes noticeably slow when undoing typing :((((

Yes... This was another gain that we got by removing deltas from History. There was much less transformations. I am sure we will spend time on this subject because it cannot work like this. I think there are places where optimisation could be applied, but this needs to be measured and researched.

Later, we can think about some tricks to maybe somehow skip some transformations or something but this has to be:

  1. Done when 99% of cases work (meaning that we solved all issues we know about).
  2. Done very carefully so we won't fall again into some kind of trap like with History.

@Reinmar
Copy link
Member Author

Reinmar commented Jul 7, 2017

FIXED

jul-07-2017 10-16-09

position.js:158 Uncaught TypeError: this.parent.offsetToIndex is not a function
    at Position.get index [as index] (position.js:158)
    at Position.get textNode [as textNode] (position.js:169)
    at Position.get nodeAfter [as nodeAfter] (position.js:181)
    at _fixItemsIndent (converters.js:629)
    at Document.<anonymous> (converters.js:576)
    at Document.fire (emittermixin.js:281)
    at Document.applyOperation (document.js:173)
    at UndoCommand._undo (basecommand.js:153)
    at Array.editor.document.enqueueChanges (undocommand.js:40)
    at Document.enqueueChanges (document.js:241)

EDIT: This TC is really fragile. You need to be very precise regarding actions.

@Reinmar
Copy link
Member Author

Reinmar commented Jul 7, 2017

As for performance issues. I did a quick check.

Here's a general overview of a single undo step:

image

20 transformDeltas() calls. Each 10ms. But the further you go back in the undo history, the longer the transformDeltas() calls.

Now, it's very interesting to see what are the widest leafs of this flame chart:

image

Yep, Position() constructors. If you'll scroll more you'll see that there also other constructors there (and they are similarly long), but the problem is that Position repeats hundreds of times.

So, it's very likely that one of the most important issues https://github.com/ckeditor/ckeditor5-engine/issues/897.

Edit: I did a small check and removed cloning from Position.createFromPosition() but it's too simplistic (because there are two position types). We'd need more time to check what we can gain by avoiding cloning all the time.

@Reinmar Reinmar changed the title [Umberlla] Undo/redo issues [Umbrella] Undo/redo issues Jul 10, 2017
@Reinmar
Copy link
Member Author

Reinmar commented Jul 10, 2017

FIXED

jul-10-2017 21-50-55

  • Enter
  • Bold
  • Forward delete (backspace or fn+delete on macOS)
  • Undo
ckeditorerror.js:38 Uncaught CKEditorError: model-history-wrong-version: Given base version points to the middle of a delta.
    at History._getIndex (http://localhost:8125/ckeditor5-presets/tests/manual/article.js:41285:10)
    at History.getDeltas (http://localhost:8125/ckeditor5-presets/tests/manual/article.js:41187:24)
    at getDeltas.next (<anonymous>)
    at Function.from (native)
    at UndoCommand._undo (http://localhost:8125/ckeditor5-presets/tests/manual/article.js:29872:32)
    at Array.editor.document.enqueueChanges (http://localhost:8125/ckeditor5-presets/tests/manual/article.js:47229:30)
    at Document.enqueueChanges (http://localhost:8125/ckeditor5-presets/tests/manual/article.js:40085:30)
    at UndoCommand.execute (http://localhost:8125/ckeditor5-presets/tests/manual/article.js:47228:24)
    at UndoCommand.on (http://localhost:8125/ckeditor5-presets/tests/manual/article.js:5973:32)
    at UndoCommand.fire (http://localhost:8125/ckeditor5-presets/tests/manual/article.js:4267:29)

The above error was reproduced on branch with a fix for https://github.com/ckeditor/ckeditor5-engine/issues/1001. On the current master first undo works, but the second gives:

basic-transformations.js:67 Uncaught TypeError: Cannot read property 'getNode' of undefined
    at addTransformationCase (basic-transformations.js:67)
    at Object.transform (transform.js:59)
    at Object.transformDeltaSets (transform.js:261)
    at Document.transformDeltas (document.js:359)
    at UndoCommand._undo (basecommand.js:140)
    at Array.editor.document.enqueueChanges (undocommand.js:40)
    at Document.enqueueChanges (document.js:241)
    at UndoCommand.execute (undocommand.js:39)
    at UndoCommand.on (observablemixin.js:327)
    at UndoCommand.fire (emittermixin.js:281)

@Reinmar
Copy link
Member Author

Reinmar commented Jul 13, 2017

FIXED

Type: Undo, incorrect data

This is not a regression but it's a bug worth checking anyway.

jul-13-2017 18-04-06


Edit by LukaszGudel: Undo block quote is now correctly creating 2 nested list items. Fixed.

@scofalik
Copy link
Contributor

The above might be one of "cannot fix with how converters work now" bug. There are actually 6 tests in integration of undo with lists that are intentionally skipped because they are failing. These are the cases that cannot be (at least easily) solved with current state of how conversion works. We skipped them to deliver Nested Lists feature faster.

If we change conversion to happen on changesDone those cases will be probably easier to solve and then we can get back to them.

@Mgsy
Copy link
Member

Mgsy commented Jul 17, 2017

FIXED

Steps to reproduce

  1. Load the clipboard plugin to the markers test.
  2. Go to http://localhost:8125/ckeditor5-engine/tests/manual/markers.html
  3. Create 2 lines of text.
  4. Select them and press Ctrl + C.
  5. Put the caret inside the marker.
  6. Press Ctrl + V.
  7. Press Ctrl + Z.

Current result

  • Some part of the marker and text is lost
  • There is the error in the console

GIF

bug_cke5

Error

position.js:221 Uncaught TypeError: Cannot read property 'root' of undefined
    at Position.compareWith (position.js:221)
    at Position.isEqual (position.js:411)
    at addTransformationCase (basic-transformations.js:396)
    at Object.transform (transform.js:59)
    at Object.transformDeltaSets (transform.js:261)
    at Document.transformDeltas (document.js:359)
    at UndoCommand._undo (basecommand.js:140)
    at Array.editor.document.enqueueChanges (undocommand.js:40)
    at Document.enqueueChanges (document.js:241)
    at UndoCommand.execute (undocommand.js:39)

Other information

OS: Windows 10, MacOS X
Browser: Chrome, Safari, Edge, Firefox, Opera
Release: 0.10.0

@Reinmar
Copy link
Member Author

Reinmar commented Aug 9, 2017

FIXED

  1. Copy all.
  2. Paste at the end of the content.
  3. Undo.

aug-09-2017 10-30-51

position.js:221 Uncaught TypeError: Cannot read property 'root' of undefined
    at Position.compareWith (position.js:221)
    at Position.isEqual (position.js:411)
    at addTransformationCase (basic-transformations.js:402)
    at Object.transform (transform.js:59)
    at Object.transformDeltaSets (transform.js:261)
    at Document.transformDeltas (document.js:359)
    at UndoCommand._undo (basecommand.js:140)
    at Array.editor.document.enqueueChanges (undocommand.js:40)
    at Document.enqueueChanges (document.js:241)
    at UndoCommand.execute (undocommand.js:39)

@Mgsy
Copy link
Member

Mgsy commented Aug 9, 2017

FIXED

Steps to reproduce

  1. Put the caret at the beginning of the paragraph.
  2. Press Enter.
  3. Press Ctrl + Z.

Current result

The caret was moved to the end of the paragraph.

GIF

bug_cke5

@scofalik
Copy link
Contributor

scofalik commented Aug 9, 2017

Selection restoring doesn't work correctly, we know about it and will change it, hopefully soon.

@scofalik
Copy link
Contributor

scofalik commented Aug 10, 2017

After I fixed this issue: https://github.com/ckeditor/ckeditor5-undo/issues/65#issuecomment-321190168 I noticed that undo+redo creates different content than it was at the beginning. It's worth to check why it happens. It fails even for fairly non-complicated copy paste. It has to be checked whether there is a bug in: copy/paste, OT or converters and whether this happens only for content with lists or also in other cases.

aug-10-2017 10-12-09

EDIT: Looks like it happens for copy paste whenever pasting is not into an empty element.

@Mgsy
Copy link
Member

Mgsy commented Aug 16, 2017

FIXED | Type: Redo, incorrect data

Block quote is removed after undo/redo operations.

Steps to reproduce

  1. Create new paragraph and new block quote.
  2. Put the caret at the end of the paragraph.
  3. Press Delete.
  4. Press Ctrl + Z.
  5. Ctrl + Y.

GIF

bug_cke5

@Mgsy
Copy link
Member

Mgsy commented Aug 17, 2017

FIXED | Type: Undo, incorrect data

Steps to reproduce

  1. Create two block elements, i.e. paragraphs.
  2. Put the caret at the end of the first paragraph.
  3. Press Enter.
  4. Press Delete.
  5. Press Ctrl + Z twice.

Current result

The second paragraph has been removed.

GIF

bug_cke5

@Mgsy
Copy link
Member

Mgsy commented Aug 17, 2017

FIXED

Steps to reproduce

  1. Create the empty paragraph.
  2. Press Ctrl + B.
  3. Type something.
  4. Put the caret at the middle of the word.
  5. Press Enter.
  6. Press Undo 4 times.

Current result

There is the error in the console.

Error

basic-transformations.js:270 Uncaught TypeError: Cannot read property 'root' of null
    at addTransformationCase (basic-transformations.js:270)
    at Object.transform (transform.js:59)
    at Object.transformDeltaSets (transform.js:261)
    at Document.transformDeltas (document.js:359)
    at UndoCommand._undo (basecommand.js:140)
    at Array.editor.document.enqueueChanges (undocommand.js:40)
    at Document.enqueueChanges (document.js:241)
    at UndoCommand.execute (undocommand.js:39)
    at UndoCommand.on (observablemixin.js:327)
    at UndoCommand.fire (emittermixin.js:281)

GIF

bug_cke5

@Mgsy
Copy link
Member

Mgsy commented Aug 21, 2017

FIXED | Type: Undo, incorrect data

Steps to reproduce

  1. Put the caret at the middle of some word, i.e. paragraph.
  2. Press Enter.
  3. Make the following selection:
parag{
raph}
  1. Press Delete.
  2. Press Ctrl + Z twice.

Current result

Undo doesn't merge the word.

GIF

bug_cke5

@Mgsy
Copy link
Member

Mgsy commented Sep 12, 2017

FIXED | Type: Redo, selection restoring

Steps to reproduce

  1. Create the paragraph below the block quote.
  2. Type something.
  3. Put the caret at the beginning of the paragraph.
  4. Press Backspace.
  5. Press Ctrl + Z.
  6. Press Ctrl + Y.

Current result

Block quote was applied to the paragraph instead of moving paragraph to the end of the block quote.

GIF

bug_cke5

Reinmar referenced this issue in ckeditor/ckeditor5-engine Sep 29, 2017
Fix: Undo did no changes instead of merging elements, in a scenario when an element was split and then the "new" element was removed. See https://github.com/ckeditor/ckeditor5-undo/issues/65#issuecomment-323682195.
@Reinmar
Copy link
Member Author

Reinmar commented Sep 29, 2017

FIXED | Type: Undo, incorrect data, crash

See https://github.com/ckeditor/ckeditor5-engine/issues/1152

@Mgsy
Copy link
Member

Mgsy commented Oct 11, 2017

Type: Redo, selection restoring

Steps to reproduce

  1. Go to http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Copy two block elements.
  3. Create new empty line.
  4. Press Ctrl + V.
  5. Press Ctrl + Z.
  6. Press Ctrl + Y.

GIF

bug_cke5

@Mgsy
Copy link
Member

Mgsy commented Oct 11, 2017

Fixed

Type: Undo, incorrect data

Steps to reproduce

  1. Create new block quote.
  2. Inside the QB create the list with one nested item.
  3. Select whole list.
  4. Press Delete.
  5. Press Ctrl + Z.

GIF

bug_cke5


Edit by LukaszGudel: Undoing list removal is now correctly restoring two items without adding third one. Fixed.

@Mgsy
Copy link
Member

Mgsy commented Oct 11, 2017

POSSIBLY FIXED

Type: Redo, crash

Steps to reproduce

  1. Create new blockquote below other block element.
  2. Type some word.
  3. Split it.
  4. Select divided word.
  5. Press Delete.
  6. Undo.
  7. Redo.
  8. Press Backspace.

Error

ckeditor.js:5 Uncaught o: move-operation-node-into-itself: Trying to move a range of nodes into one of nodes from that range. Read more: https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/framework/guides/support/error-codes.html#move-operation-node-into-itself.
    at d._execute (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:50554)
    at x.applyOperation (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:246675)
    at n (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:107507)
    at o.move (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:108252)
    at n (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:242104)
    at t.a (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:243111)
    at _.deleteContent (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:222073)
    at _.on (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:29276)
    at _.fire (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:23735)
    at _.(anonymous function) [as deleteContent] (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:29327)

GIF

bug_cke5


Edit by LukaszGudel: I can not recreate this issue using those steps. It's possible that it is fixed but it would be better if someone confirms this.

@Mgsy
Copy link
Member

Mgsy commented Oct 13, 2017

FIXED

Type: Undo, incorrect data

Steps to reproduce

  1. Put the caret in the middle of some word.
  2. Press Enter.
  3. Select divided parts.
  4. Press Delete.
  5. Add the block quote.
  6. Undo twice.

Current result

Divided word is missing bottom part.

GIF

bug_cke5


Edit by LukaszGudel: Undoing is correctly restoring both lines. Fixed.

@Mgsy
Copy link
Member

Mgsy commented Oct 13, 2017

FIXED

Type: undo, incorrect data

Steps to reproduce

  1. Type test.
  2. Put the caret after t and press Enter.
  3. Repeat step 2 for every character. You should receive:
t
e
s
t
  1. Delete every character, starting from the last one.
  2. Put the caret at the last empty line.
  3. Press Backspace 3 times.
  4. Undo until word is restored.

Current result

Word has been restored, but characters have incorrect position - the result is tets, instead of test.

GIF

bug_cke5


Edit by LukaszGudel: word is correctly restored with correct position of characters.

@Mgsy
Copy link
Member

Mgsy commented Oct 13, 2017

Fixed

Type: Undo, incorrect data

Steps to reproduce

  1. Create two paragraphs with some words.
  2. Put the caret at the second paragraph.
  3. Transform it into the block quote.
  4. Select the word inside the block quote.
  5. Press Backspace 3 times.
  6. Undo.

Current result

Deleted character from the first paragraph has been restored inside the block quote.

GIF

bug_cke5


Edit by LukaszGudel: Words are correctly restored. Fixed.

@Mgsy
Copy link
Member

Mgsy commented Oct 13, 2017

POSSIBLY FIXED

Type: Undo, crash

Steps to reproduce

  1. Create two paragraphs with some words.
  2. Put the caret at the second paragraph.
  3. Transform it into the Heading 1.
  4. Select this word.
  5. Press Backspace 3 times.
  6. Undo.

Current result

Editor crashes.

GIF

bug_cke5


Edit by LukaszGudel: I can not recreate this issue using those steps. It's possible that it is fixed but it would be better if someone confirms this.

@Mgsy
Copy link
Member

Mgsy commented Oct 13, 2017

POSSIBLY FIXED

Type: Undo, crash

Steps to reproduce

  1. Select two block elements.
  2. Click on the block quote icon.
  3. Press Delete.
  4. Click on the block quote icon.
  5. Undo twice.

Current result

Editor crashes

GIF

bug_cke5

Error

ckeditor.js:5 Uncaught o: move-operation-position-invalid: Source position or target position is invalid. Read more: https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/framework/guides/support/error-codes.html#move-operation-position-invalid.
    at i._execute (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:49878)
    at x.applyOperation (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:246824)
    at o._undo (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:202841)
    at Array.editor.document.enqueueChanges (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:336109)
    at x.enqueueChanges (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:247377)
    at o.execute (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:336076)
    at on (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:29255)
    at o.fire (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:23714)
    at o.(anonymous function) [as execute] (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:29306)
    at o.execute (http://192.168.20.68:8125/ckeditor5-build-classic/tests/manual/ckeditor.js:75:220184)

Edit by LukaszGudel: I can not recreate this issue using those steps. It's possible that it is fixed but it would be better if someone confirms this.

@Mgsy
Copy link
Member

Mgsy commented Oct 16, 2017

Fixed

Type: Undo, incorrect data

Steps to reproduce

  1. Go to http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Put the caret at the end of the Heading 1.
  3. Press Enter.
  4. Press Backspace.
  5. Delete the word character by character with Backspace.
  6. Undo.

Current result

First character of the word has been moved to the new line.

GIF

bug_cke5


Edit by LukaszGudel: Text in heading is correctly restored. Fixed.

@Mgsy
Copy link
Member

Mgsy commented Oct 16, 2017

POSSIBLY FIXED

Type: Undo, crash

Steps to reproduce

  1. Create 3 paragraphs with some text.
  2. Select first paragraph and press Delete twice.
  3. Select first remained paragraph and part of the second one and press Delete.
  4. Transform remained paragraph into the list.
  5. Undo 3 times.

Current result

Editor crashes.

GIF

bug_cke5

Error

ckeditorerror.js:46 Uncaught CKEditorError: model-position-path-incorrect: Position path must be an array with at least one item. Read more: https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/framework/guides/support/error-codes.html#model-position-path-incorrect.
 {"path":[]}
    at new Position (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:237:10)
    at addTransformationCase (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:47295:24)
    at Object.transform (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:31648:23)
    at Object.transformDeltaSets (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:31851:34)
    at Document.transformDeltas (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:42588:77)
    at UndoCommand._undo (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:36858:37)
    at Array.editor.document.enqueueChanges (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:55081:30)
    at Document.enqueueChanges (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:42470:30)
    at UndoCommand.execute (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:55080:24)
    at UndoCommand.on (http://192.168.20.68:8125/ckeditor5-core/tests/manual/article.js:5873:32)

Edit by LukaszGudel: I can not recreate this issue using those steps. It's possible that it is fixed but it would be better if someone confirms this.

@Mgsy
Copy link
Member

Mgsy commented Oct 16, 2017

FIXED

Type: Undo, incorrect data

Steps to reproduce

  1. Create 3 paragraphs with some text.
  2. Select first paragraph and press Delete twice.
  3. Select remained paragraphs.
  4. Press Delete.
  5. Undo twice.

Current result

The word from the second paragraph has been moved to the third paragraph.

GIF

bug_cke5


Edit by LukaszGudel: paragraphs are correctly restored.

@Mgsy
Copy link
Member

Mgsy commented Oct 17, 2017

FIXED

Type: Undo, selection restoring, incorrect data

Steps to reproduce

  1. Create two paragraphs with some words.
  2. Put the caret inside the word from the second paragraph.
  3. Press Backspace twice.
  4. Put the caret at the end of the first paragraph.
  5. Press Delete.
  6. Repeat step 4.
  7. Press and hold Backspace to delete whole word.
  8. Undo 3 times.

Current result

Characters deleted in step 3 appeared in the first paragraph.

GIF

bug_cke5


Edit by LukaszGudel: Paragraphs are correctly restored. Fixed

@scofalik
Copy link
Contributor

I have a feeling that many new errors connected with Undo are regressions after a change in deleteContents we merged very recently. I am sure that at least one of them is, as I tested it. This means that we will have to solve this in OT, or revert the change.

@Reinmar
Copy link
Member Author

Reinmar commented Oct 17, 2017

Could you report this in the engine and check it there?

@scofalik
Copy link
Contributor

scofalik commented Oct 27, 2017

Type: Redo, incorrect data

Steps to reproduce

  1. Create content with three <p>s.
  2. Select whole content and wrap it in blockquote.
  3. Put caret in the middle paragraph.
  4. Unwrap it from blockquote.
  5. Undo, undo.
  6. Redo, redo

Current result

The unwrapped paragraph is moved to the beginning.

GIF

oct-27-2017 15-22-35


Edit by LukaszGudel: Current behaviour is slightly different.
CKE5-undo-redo-3-blockquotes

@Mgsy
Copy link
Member

Mgsy commented Oct 31, 2017

FIXED

Type: Redo, selection restoring

Steps to reproduce

  1. Create three block quotes.
  2. Select them.
  3. Press Delete.
  4. Undo until they disappear.
  5. Redo to restore removed block quotes.

Current result

Block quotes have been restored in reversed order.

GIF

bug_cke5


Edit by LukaszGudel: Blocks are restored in correct order.

@Mgsy
Copy link
Member

Mgsy commented Oct 31, 2017

Fixed

Type: Undo, incorrect data

Steps to reproduce

  1. Go to http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Put the caret after H at Heading 1.
  3. Split the word with Enter.
  4. Put the caret at the end of the second divided part.
  5. Press Backspace.
  6. Repeat step 4 and 5 for the first divided part.
  7. Put the caret at the end of the second divided part.
  8. Press Backspace 8 times.
  9. Undo until word is restored.

Current result

Word has been restored incorrectly.

GIF

bug_cke5


Edit by LukaszGudel: Characters in heading are correctly restored.

@Mgsy
Copy link
Member

Mgsy commented Jan 8, 2018

Fixed

Type: Undo, incorrect data

Steps to reproduce

  1. Go to http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Start the selection from the end of the OL List item 2 and drag it to the first Quote.
  3. Apply the block quote.
  4. Undo.

Current result

Whole block quote has disappeared.

GIF

bug_cke5


Edit by Lukasz Gudel: Content looks the same as before applying block quote. There is no content lost. Fixed.

@Mgsy
Copy link
Member

Mgsy commented Jan 8, 2018

POSSIBLY FIXED

Type: Undo, crash

Steps to reproduce

  1. Go to http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Delete whole content.
  3. Create the block quote with two empty paragraphs.
  4. Put the caret at the first paragraph.
  5. Click on the block quote icon.
  6. Undo.

Current result

Editor crashes

Error

mapper.js:398 Uncaught TypeError: Cannot read property 'is' of undefined
    at Mapper._findPositionIn (mapper.js:398)
    at Mapper.on (mapper.js:73)
    at Mapper.fire (emittermixin.js:269)
    at Mapper.toViewPosition (mapper.js:237)
    at ModelConversionDispatcher.<anonymous> (model-selection-to-view-converters.js:86)
    at ModelConversionDispatcher.fire (emittermixin.js:269)
    at ModelConversionDispatcher.convertSelection (modelconversiondispatcher.js:253)
    at Model.EditingController.listenTo (editingcontroller.js:126)
    at Model.fire (emittermixin.js:269)
    at Model._runPendingChanges (model.js:209)

Notes

Here is the first bad commit - ckeditor/ckeditor5-image@7a1ab67

GIF

bug_cke5


Edit by LukaszGudel: I can not recreate this issue using those steps. It's possible that it is fixed but it would be better if someone confirms this.

@scofalik
Copy link
Contributor

scofalik commented Feb 19, 2018

FIXED

Type: Undo, incorrect data

Steps to reproduce

  1. Open http://localhost:8125/ckeditor5-core/tests/manual/article.html
  2. Place caret at the end of the second line.
  3. Press and keep backspace to delete text up to the beginning of the document.
  4. Press Undo.

Current result

The content is different than at the beginning

Expected result

After undo the content is same as before deleting the text


Edit by LukaszGudel: Content is the same as before deleting the text. Fixed.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-undo Oct 9, 2019
@mlewand mlewand added this to the backlog milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. labels Oct 9, 2019
@Reinmar Reinmar added the squad:core Issue to be handled by the Core team. label Nov 3, 2021
@pomek pomek removed this from the backlog milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 2, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:undo resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

6 participants