CheckDirty is true after clicking a widget, but getData() returns the same

[Joel]

In Chrome33 and IE9, the checkDirty() method returns true after a widget drag handle has appeared, even though nothing has changed.

1. Open Developers console
2. Go to ​http://ckeditor.com/demo#widgets
3. Run CKEDITOR.instances.editor3.checkDirty(); in console
   

--> returns false

4. Run var d1 = CKEDITOR.instances.editor3.getData(); in console
5. Click on the apollo picture in the 3rd editor, but do not move it.
6. Run CKEDITOR.instances.editor3.checkDirty(); in console
   

--> returns true

7. Run var d2 = CKEDITOR.instances.editor3.getData(); in console
8. run d1 === d2 in console
   

--> returns true

I know this is a battle between getData() vs getSnapshot() vs _.previousValue, but as far as I can see, it is unexpected behaviour. Shouldn't checkDirty() work with getData() especially when there are widgets involved?

Switching to source mode and back does not help.

---

Bonus: In Chrome CKEDITOR.instances.editor4.checkDirty(); is always true after page has loaded.

[Piotrek Koszuliński]

There's no integration between widgets and checkDirty at all. Honestly speaking, we've got a problem with checkDirty, because whatever we do with DOM it yields. And with widgets we do a lot.

So either we'll reimplement it (only possible with undo manager...), or every single time DOM we change something irrelevant in the DOM we need to reset checkDirty to correct state.

[Jakub Ś]

or every single time DOM we change something irrelevant in the DOM we need to reset checkDirty to correct state

What if some other part of code e.g. custom plugin changes dirty state at the same time (and relies on it)? If such thing happens then, not only it breaks custom plugin but also our onchange is becoming more and more useless.

[Piotrek Koszuliński]

Other plugins should not rely on dirty flag. It is only for integration purposes. Other plugins should only take care of not changing it when it shouldn't be changed. Just like in this case - the flag should not be change to true when widget is focused or blurred.

[Artur Delura]

To reproduce this issue simply open sample with image2 widget enabled with content:

<figure class="image" style="float:left">
  <img alt="Saturn V" src="assets/image1.jpg" width="200" />
  <figcaption>Roll out of Saturn V on launch pad</figcaption>
</figure>

[Artur Delura]

Fixed on ​​branch:t/11753.

[Piotrek Koszuliński]

What about tests?

[Piotrek Koszuliński]

R- because of lack of tests.

[Artur Delura]

Test were written on ​branch:t/11753. Not sure whether this should be ticket test.

[Piotrek Koszuliński]

1. We use only lowercase letters in file names.
2. Use strict mode and a closure.
3. You should initialize editors in 'async:init', because more tests later may use it.
4. You didn't test what happens if checkDirty was true before focusing widget.
5. Don't use image2 widget in general widget tests. Use custom widget like other tests.
6. You can keep these tests in plugins/widget/, because there's more cases to test than just the one from this ticket.
7. You don't need justify plugin.
8. In 3rd line there's a missing space.

[Artur Delura]

Replying to Reinmar:

1. We use only lowercase letters in file names.
2. Use strict mode and a closure.

Didn't know it's a standard :)

3. You should initialize editors in 'async:init', because more tests later may use it.

Even until YAGNI? :)

[Piotrek Koszuliński]

Rebased branch and pushed additional commits (polished tests and added missing ones).

Even until YAGNI? :)

The thing about multiple editors is that you'll need them at some point. And if you used the single bot you'll need to alter all tests what's irritating and is a cause of bugs. Of course YAGNI in dev code is a different case.

[Artur Delura]

Changes in ​git:a2facd8

[Piotrek Koszuliński]

I wrote that I pushed additional commits to this branch. You have not included there while merging to master!

[Piotrek Koszuliński]

Added them in ​git:294b62c.