Opened 3 years ago

Last modified 3 years ago

#13888 new Bug

image2: editor.config.image2_captionedClass is not removed when going from captioned to uncaptioned with Drupal's extended image2

Reported by: Wim Leers Owned by:
Priority: Normal Milestone:
Component: General Version: 4.5.3
Keywords: Drupal Cc: wim.leers@…

Description

Steps to reproduce

From https://www.drupal.org/node/2268941:

A:

  • Add an image in a ckeditor area, leave the "caption" option unchecked
  • Save

--> OK, the img tag that gets produced only has the 'align-*' class (if an alignement was chosen)

B:

  • Add an image in a ckeditor area, check the "caption" option
  • Re-edit the image, and uncheck the "caption" option
  • Save

--> NOK, the img tag has the 'caption' and 'img-caption' classes. I'd expect A and B to produce the same final markup.

Other details (browser, OS, CKEditor version, installed plugins)

I already spent a lot of time debugging image2, and was unable to pinpoint the cause. Somehow, the widget's data suddenly contains data.classes == editor.config.image2_captionedClass, but I'm not sure how this happens. It seems to be a race condition almost.

Debugging changes made to a widget's data is a massive PITA, or at least, I haven't figured out yet where to put breakpoints to make debugging this more doable.

Change History (1)

comment:1 Changed 3 years ago by Wim Leers

A lot of painful research later… this is the result. AFAICT there are *very* significant problems with how "widget styles" work, and particularly with how the image2 plugin integrates with it. In fact, image2's getClasses() override seems plain wrong: it indiscriminately returns both alignment and captioned classes, even when the image is not aligned or not captioned!

That being said, I could be entirely wrong. I'm deeply concerned by how many layers there are, and how it all fits together. It (the widget system) seems very fragile. I totally understand that though; the widget system "is" very difficult to build/get working at all. I'm hopeful the widget system in CKE5 will be much simpler, because you've already learned a lot of hard lessons during the CKE4 cycle :)


My analysis from my d.o comment (https://www.drupal.org/node/2268941#comment-10513174) copied verbatim:

Another hour of so of extremely painful debugging (the widget system is "very" complex) later and I have the solution. The problem is the combination of:

  • "widget styles" (since CKEditor 4.4: http://ckeditor.com/blog/CKEditor-4.4-Released)
  • how widget styles interact with the image2_captionedClass setting
  • plus how image2 dynamically assigns widget styles (which includes these classes) to a different element based on whether it is captioned or not (see getStyleableElement())
  • … and ironically, how that is doing wrong/double work anyway, because the widget template for the captioned image already includes the classes specified in image2_captionedClass anyway, so it's only re-adding some classes that already exist, plus it's actually wrong for those classes to be applied always, because they should only be applied when the image actually "is" captioned

IMO that part of CKEditor is in need of drastic simplification.

For now, the solution on the Drupal side is simple: opt out from all that complexity.

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