CKEDITOR.editor#setData should generate an undo snapshot

[Frederico Caldeira Knabben]

Calling setData should generate an undo snapshot.

Note that setData participates on the editor data initialization, which should not be impacted by it.

[Frederico Caldeira Knabben]

Milestone CKEditor 3.x deleted

[Pieter Fibbe]

This is still not working. When calling setData whithout having focussed the editor first there is no undo snapshot generated. You now have to make a snapshot manually before calling the setData, it would be nice to include it in the setData.

[Paul Veldema]

Is there a workaround for this? Or is there a simple way to enable the undo button after making a snapshot. That would help a lot. Because looking at the time this ticket was created this ticket is not a priority.

[Jakub Ś]

You could try using isDirty() method. Although undo manager is not updated in this case, dirty flag informs about right state editor is in.

I will forward this to my colleagues asking if this can be considered in one of future releases.

[Paul Veldema]

No joy on the dirty functions.

I needed a workaround for ckeditor 4.2.1. Therefore I played around with the code some more after taking a good look at the undo plugin source code. Here are is what I arrived at:

var someTestString = ...;
var editorInstance = ...;
editorInstance.fire( 'saveSnapshot' );
editorInstance.setData(someTestString , function() {
	var command = this.getCommand( 'undo' );
	if (command) {
		command.canUndo = true;
		command.setState(CKEDITOR.TRISTATE_OFF);
	}
});

It would be nice for this not to be necessary though. I am sure sometimes the undo button state will be wrong in this solution. Like when a setData() results in no change in the Editor.

[Piotr Jasiun]

I've added 'saveSnapshot' at the begining and end of setData method what solve the problem. Moreover now there is change event if data changed using setData.

I also added noSnapshots parameter to allow users call setData without making snapshots (as it works before changes). This parameters is also needed internally.

One more change is to prevent saving snapshots if editor is not ready.

I pushed my solution to ​t/5217 and tests to corresponding branch.

[Piotr Jasiun]

Tested on Chrome, FF, IE10 and IE8, with framed, div-based, inline and shared-spaces editor.

[Piotrek Koszuliński]

1. Code style issues in tests.
2. !internal && !noSnapshots && this.fire( 'saveSnapshot' ); if statement should be used.
3. The way two callbacks are added in setData should be unified.
4. Last 3 parameters of setData are optional - that should be indicated by docs.
5. There's no comments why setData in divarea and wysiwygarea cannot save snapshots.
6. There are no tests for:
   • all editables (normal, divarea, wysiwygarea)
   • ×
   • TCs:
     • initial undo manager state (after initializing editor),
     • after switching between modes (with - and it is a failing TC according to my manual tests - and without data change)),
     • and perhaps more.
   • undo manager ignoring snapshots when editor is not ready.
7. And finally - I'm not sure I like this solution. We should not make it more complex.

[Piotrek Koszuliński]

More straightforward solution would be to save snapshot only when editable is ready. All editables would have the status property and would change it in appropriate moments. Undo manager would only check it, similarly to how it checks editor.status in solution proposed in t/5217.

[Joel]

How about adding a second optional argument to setData() that can be used to control this (and possibly future) option(s).

var options = {
    callback: function() { alert("done!"); },
    createSnapshot: false,
    resetUndoStack: true,
    resetDirty: true
};
editor.setData(htmlString, options)

I would extend this suggestion to all functions everywhere. I know it's a breaking API change so it probably won't be implemented and if it is, that would mean CKEditor v5.0.0.

[Piotr Jasiun]

I changed the solution. Now editable has a status witch is ready after first setData and undo manager decide if snapshot should be saved.

I also added tools for testing the same on different editors.

Changes in ​t/5217 and corresponding test branch.

[Piotrek Koszuliński]

1. Commit fixing code style should be merged to the one that introduced the issue.
2. The branch should be based on master, not on previous solution. So it should be t/5217b, because it's completely new proposal.
3. if ( this.editor.mode && this.editor.mode != 'wysiwyg' ) first part is not needed, because it duplicates the 2nd part.
4. There's no 'detached' status for detached editable.

[Piotr Jasiun]

Changes in ​t/5217b. I also added tests for status (in t/5217).

[Piotrek Koszuliński]

I like the new proposal, but I want to make one thing more accurate - the moment when status is set to ready.

[Piotrek Koszuliński]

I pushed many commits to t/5217b simplifying, improving and fixing various issues. However, there's still one more for which I don't have a good idea.

1. Open any framed editor in FF.
2. Focus editor.
3. Switch to source and back.
4. Undo becomes enabled.

Why? When selection is moved inside editable Firefox or editor itself adds bogus <br>s to selected paragraphs. Somehow, perhaps in listener calling fixDom, this is hidden from undo manager (by lock&unlockSnapshot). So the state before switching to source mode is:

<p>foo<br></p><p>bar<br></p>

And this HTML is saved in snapshot before switching to source mode. In source mode user gets the data which looks like:

<p>foo</p><p>bar</p>

And after switching to wysiwyg mode, snapshot is saved. Unfortunately, <br>s are gone, because editable's HTML is set to the data, so second snasphot is saved. First contains bogus <br>s and the second does not.

The only solution I can think of is overwriting the last snapshot with normalised, internal HTML after switching to source mode.

[Piotrek Koszuliński]

I pushed a proposal of a solution to the problem explained in comment:20. The only possible way was to compare data saved when entering source mode with data retrieved when leaving it and locking snapshot if data has not been changed. There were couple of troubles with this solution though, so two last commits in the branch contains necessary changes with long comments. Also, data strings are compared without normalization (which could be achieved by parsing and writing HTML), so the comparison can be broken by simple new line added between paragraphs or any other insignificant change. We may add normalization in the future, let's keep it simple for now.

There are no tests yet and undo manger changes lack some comments, but I don't want to waste time on them if the solution won't be accepted.

Also, there's one bug - after undoing changes done in source mode selection is set directly in body. That's because snapshot taken right after switching to wysiwyg contains incorrect selection or none at all. It is a separate issue.

[Frederico Caldeira Knabben]

The solution is simple and generally good... buuuut... I'm not so positive about the additional editor.getData() call on themeui.js. That's because getData is already executed when changing modes and it's a very demanding method. We should try optimizing it.

Other than this, the ticket can be coded fully, including tests at this point.

[Piotrek Koszuliński]

Pushed rebased t/5217b branches.

[Piotrek Koszuliński]

It looks that it is possible to save that one getData call by detaching editable before checking if data was changed. But I have to create more tests to be sure.

[Piotrek Koszuliński]

We're closer to closing this ticket, but it still requires a lot of work on tests, so it will not fit in 4.3.2.

[Piotrek Koszuliński]

Back to this lovely issue ;>.

[Piotrek Koszuliński]

Time for final review. Pushed ​branch:t/5217b on dev and tests. I added many tests to ensure that most tricky parts of introduced changes have coverage.

[Olek Nowodziński]

I spent a lot of time trying to understand the nuances of implementation but I gave up and focused mostly on real-life cases and/or test coverage.

Generally speaking I'm OK with ​​branch:t/5217b though there are some cases which fail, i.e.

1. Open plugins/image2/samples/image2.html
2. CKEDITOR.instances.editor1.setData( CKEDITOR.instances.editor1.getData() )
3. Undo snapshot is available.

This is because widgets are being given new ids while created and fixing it at this stage doesn't make much sense for me.

Let's move on and, if something fails, we'll figure it out during testing phase.

[Piotrek Koszuliński]

Generally speaking I'm OK with ​​​branch:t/5217b though there are some cases which fail, i.e.

There are, and there are not fixable currently. It's all due to limited undo manager capabilities. It wasn't designed to be able to handle changes in attributes order, changes of irrelevant attributes, changes of whitespaces. We need an entirely new solution for that.

Fixed on master with ​git:a81e759 on dev and 600d2a1 on tests.

[Piotrek Koszuliński]

During testing phase it turned out that this patch revealed a problem with inline editor (#11585) and setting data when editor is blurred. We decided that it'll be safer to revert it because we cannot predict how it will affect various configurations.

[Piotrek Koszuliński]

The problem mentioned in comment:30 is that on inline editor selection isn't unlocked before setting data, so selection retrieved after altering editable's innerHTML returns cached ranges pointing to nonexisting elements (old contents). This doesn't happen on framed editor, because after #9521 we're unlocking selection on contentDomUnload which is fired before setting data.

[Piotrek Koszuliński]

I pushed ​branch:t/5217 branches with recreated commits of previous patch.

[Piotrek Koszuliński]

There's a TC which has to be tested after fixing issue mentioned in comment:31.

1. Open inlinebycode sample on IE8
2. Focus editable.
3. Call CKEDITOR.instances.editable.setData( 'foo' );
4. Error is thrown.

This error is thrown from 3 plugins - bidi, indentblock and justify. All of them tries to read computed styles. If this bug will still exist after fixing comment:31, then it should be extracted to separate ticket.

[Piotrek Koszuliński]

I pushed few additional commits to recovered ​branch:t/5217 branch.

1. First of all I found out that ​this listener is not needed any more. I added a missing TC for #7174.
2. Then I added an unlockSelection call on setData. This fixed comment:34, comment:32 (#11585, although I'm not able to reproduce it now even on branch:t/5217~2 :|) and #11500.

[Frederico Caldeira Knabben]

Four tests are failing here:
​http://ckeditor4.t/dt/core/selection/fake.html

[Piotrek Koszuliński]

The test branch is here ​https://github.com/cksource/ckeditor-tests/tree/t/5217 and fake selection tests pass on it. So I guess that you didn't find it.

[Frederico Caldeira Knabben]

As the solution touches important parts of the editor workflow, I would propose move this to the next major.

One small remark, in the undo plugin please rename “alwaysUpdate” to “forceUpdate”.

[Piotrek Koszuliński]

Fixed on major with ​git:ef06fd5 on dev and fbfb74c on tests.