Opened 9 years ago

Closed 4 years ago

#5217 closed Bug (fixed)

CKEDITOR.editor#setData should generate an undo snapshot

Reported by: Frederico Caldeira Knabben Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.4.0
Component: General Version:
Keywords: Cc:

Description

Calling setData should generate an undo snapshot.

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

Change History (40)

comment:1 Changed 8 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.x

Milestone CKEditor 3.x deleted

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

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by 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.

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

comment:6 Changed 5 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.3.1

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

Summary: CKEDITOR.setData should generate an undo snapshotCKEDITOR.editor#setData should generate an undo snapshot

comment:8 Changed 5 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:9 Changed 5 years ago by 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.

comment:10 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

comment:11 Changed 5 years ago by Piotr Jasiun

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

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

Milestone: CKEditor 4.3.1CKEditor 4.3.2
Status: reviewreview_failed
  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.

comment:13 Changed 5 years ago by 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.

comment:14 Changed 5 years ago by Piotr Jasiun

Status: review_failedassigned

comment:15 Changed 5 years ago by 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.

comment:16 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

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.

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

Status: reviewreview_failed
  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.

comment:18 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

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

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

Owner: changed from Piotr Jasiun to Piotrek Koszuliński
Status: reviewassigned

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

comment:20 Changed 5 years ago by 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.

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

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

Status: assignedreview

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.

comment:22 Changed 5 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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.

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

Pushed rebased t/5217b branches.

comment:24 Changed 5 years ago by 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.

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

Milestone: CKEditor 4.3.2CKEditor 4.3.3

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.

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

Status: review_failedassigned

Back to this lovely issue ;>.

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

Status: assignedreview

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.

comment:28 Changed 5 years ago by Olek Nowodziński

Status: reviewreview_passed

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.

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

Resolution: fixed
Status: review_passedclosed

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.

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

Milestone: CKEditor 4.3.3CKEditor 4.3.4
Resolution: fixed
Status: closedreopened

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.

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

Status: reopenedconfirmed

comment:32 Changed 5 years ago by 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.

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

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

comment:34 Changed 4 years ago by 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.

comment:35 Changed 4 years ago by Piotrek Koszuliński

Status: confirmedreview

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.
Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:36 Changed 4 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

comment:37 Changed 4 years ago by 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.

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

comment:38 Changed 4 years ago by Piotrek Koszuliński

Status: review_failedreview

comment:39 Changed 4 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.3.4CKEditor 4.4
Status: reviewreview_passed

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

comment:40 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

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

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