Opened 7 years ago

Closed 7 years ago

#9230 closed Bug (fixed)

Undo state incorrect when an editor instance is created on an empty element

Reported by: Teresa Monahan Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.0
Component: Core : Undo & Redo Version: 4.0
Keywords: IBM Cc: Damian, Satya Minnekanti

Description

To reproduce: Save the attached file into the samples directory and open in a browser. This sample contains 2 editor instances:

  • the 1st was created on a textarea that contains no text
  • the 2nd editor instance was created on a textarea that had some initial text.

Problem: The undo icon is enabled in the 1st editor instance and available to be used, even though there is no action to undo. It should be disabled.

The state of the undo icon is correct in the 2nd editor instance.

This issue also occurs when creating the editor on a div element and it happens for both CKEDITOR.replace and CKEDITOR.appendTo.

Attachments (1)

replacebycode_sample.html (1010 bytes) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (17)

Changed 7 years ago by Garry Yao

Attachment: replacebycode_sample.html added

comment:1 Changed 7 years ago by Garry Yao

Status: newreview

comment:2 Changed 7 years ago by Garry Yao

Other than the fact that the domChanged flag is not properly set on empty content, another critical issue (we've visited before) is recursive call (twice) of fixDom function on selectionChange event, is causing chaos on the undo manager states.

As I stated before, it's just plain wrong to force save a snapshot on each selection change (we were doing that, just for making the "updateSnapshot" feature work), so the way how it work to update snapshot must be reinvented, at this moment.

Opened t/9230 for the following proposals, which should resolve the issue:

  1. Published editor.undoManager (required by 2.)
  2. Reworked undoManager::update to perform a interceptor alike update on undo snapshots, with the DOM modification function passed in (this's not achievable by using event), besides, this method now takes care of detecting the DOM changes, to only amend the stack on demand, thus the domChanged flag is removed.
  3. Moved original undoManager::update to private method (amendSnapshots)
  4. The original "updateSnapshot" event is kept.
Last edited 7 years ago by Piotrek Koszuliński (previous) (diff)

comment:3 Changed 7 years ago by Frederico Caldeira Knabben

Branch moved to t/9230.

comment:4 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewassigned

Wouldn't it be a much simpler solution exposing the lock/unlock feature of the undo manager. In this way one can do this:

editor.undoManager.lock();
// Do stuff
editor.undoManager.unlock();  // Could even accept an optional "true" to update straight.
editor.undoManager.update();

Looks like a simpler and more powerful feature, without even touching the current update() implementation.

comment:5 Changed 7 years ago by Piotrek Koszuliński

I pushed some docs corrections to t/9230.

Regarding the uM.lock() - it has to do update too. Because just before locking someone could changed something and then for this change another snapshot won't be created, but previous will be updated.

I don't remember previous issue very well, but I remember that we had some serious problems with straightening this impl. I hope you'll do better :)

comment:6 Changed 7 years ago by Garry Yao

Status: assignedreview

Exposing the lock/unlock feature of the undo manager may resolves the recursive call issue, but it's not to bring a fix to the current situation where snapshots are incorrectly saved on each selection change, as stated in comment:2.

Besides the "lock" is actually an undo manager internal required only by the update method, which will be useful outside of this scope.

comment:7 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Still the current implementation is hacky... we don't need to change the update method with an ugly signature just to make it work.

I insist on the lock/unlock solution, which is much more efficient. For example, it will also contemplate its usage on asynchronous operations.

Of course, we will them use lock/unlock on editable to fix the current solution, just like it is done in the branch currently.

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

Owner: changed from Garry Yao to Piotrek Koszuliński
Status: review_failedassigned

comment:9 Changed 7 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed t/9320b on both dev&tests.

I implemented lock/unlock methods. I kept them as simple as possible, so save/update are done by them by default. When other use case will be discovered special argument may be added.

comment:10 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

I've pushed further fixes to 9320b concerning the following points:

  1. Remove the "transaction" variable which is redundant
  2. Avoid saving wrong snapshot on each lock (pushed one small tc to support it)

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

Status: review_failedassigned

comment:12 Changed 7 years ago by Piotrek Koszuliński

Status: assignedreview

comment:13 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

Considering the we now proposed it in such a way, let's still have it event based as before (editor#lockSnapshot/editor#unlockSnapshot) as the undo is still a plugin for now.

comment:14 Changed 7 years ago by Piotrek Koszuliński

Status: review_failedreview

Done. I've also remove editor#undoManager, so I had to rewrite assertions in few tests, so they don't use it (in favor for events and commands states).

Last edited 7 years ago by Piotrek Koszuliński (previous) (diff)

comment:15 Changed 7 years ago by Garry Yao

Component: GeneralCore : Undo/Redo
Status: reviewreview_passed

R+ with small fixes.

comment:16 Changed 7 years ago by Piotrek Koszuliński

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