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

[Teresa Monahan]

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.

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

[Frederico Caldeira Knabben]

Branch moved to t/9230.

[Frederico Caldeira Knabben]

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.

[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 :)

[Garry Yao]

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.

[Frederico Caldeira Knabben]

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.

[Piotrek Koszuliński]

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.

[Garry Yao]

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)

[Garry Yao]

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.

[Piotrek Koszuliński]

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

[Garry Yao]

R+ with small fixes.

[Piotrek Koszuliński]

Thanks. Masterised: ​https://github.com/ckeditor/ckeditor-dev/commit/be2605c