Opened 9 years ago
Last modified 9 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.
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
'sgetClasses()
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:
image2_captionedClass
settingimage2
dynamically assigns widget styles (which includes these classes) to a different element based on whether it is captioned or not (seegetStyleableElement()
)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" captionedIMO 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.