Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#11753 closed Bug (fixed)

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

Reported by: Joel Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.2
Component: General Version: 4.3
Keywords: Cc:

Description (last modified by Artur Delura)

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

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

--> returns true

  1. Run var d2 = CKEDITOR.instances.editor3.getData(); in console
  2. 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.

Change History (18)

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

Status: newconfirmed
Version: 4.3

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.

comment:2 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

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

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

Other plugins should not rely on the 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.

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

comment:5 Changed 4 years ago by Artur Delura

Description: modified (diff)

comment:6 Changed 4 years ago by Artur Delura

Description: modified (diff)

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

comment:8 Changed 4 years ago by Artur Delura

Status: assignedreview

Fixed on ​branch:t/11753.

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

What about tests?

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

Status: reviewreview_failed

R- because of lack of tests.

comment:11 Changed 4 years ago by Artur Delura

Status: review_failedreview

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

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:12 Changed 4 years ago by Artur Delura

Status: reviewassigned

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

comment:14 in reply to:  13 Changed 4 years ago by Artur Delura

Status: assignedreview

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

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

Even until YAGNI? :)

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

Milestone: CKEditor 4.4.2
Status: reviewreview_passed

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.

comment:16 Changed 4 years ago by Artur Delura

Resolution: fixed
Status: review_passedclosed

Changes in git:a2facd8

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

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

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

Added them in git:294b62c.

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