Problem/Motivation
Steps to reproduce :
- 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.
Note that this affects:
- markup in CKEditor
- markup in the database (i.e. stored data)
- NOT the markup on output (at least when using Basic HTML), thanks to #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default — unless you allow whitelist
class
attribute on the<img>
tag
Note that both of the following typical actions actually already clean up the problem in HEAD:
- simply switching to source mode and back
- saving and then opening in CKEditor and then saving again
Proposed resolution
Exclude the CKEditor image widget's "captioned classes" from the classes to add; it is handled already by the widget template.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
Why RC target?
Because this pollutes
- markup in the database (i.e. stored data)
- NOT the markup on output (at least when using Basic HTML), thanks to #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default — unless you allow whitelist
class
attribute on the<img>
tag
This is a minor annoyance: it's harmless, and already in HEAD, resaving the content fixes it. This just ensures it doesn't happen at all.
Comments
Comment #1
yched CreditAttribution: yched commentedComment #2
Wim LeersGreat catch! That's definitely a bug. Reproduced. Will definitely fix. If only we had the ability to run JS tests… :(
caption
is the generic class that gets added, but by setting adata-caption
attribute on e.g. a blockquote, preformatted code block or video, you can also caption those. We want you to be able to target those captions separately via CSS, hence the extra classes. From Bartik's CSS:Comment #3
yvesvanlaer CreditAttribution: yvesvanlaer commentedJust saying that this is still an issue.
Trying to fix this but I'm not sure if I could find a solution.
Comment #4
Wim LeersThis is not a big bug, so could conceivably go into 8.0.x. Hence not tagging
, because there are much more important CKEditor issues that are RC targets. I want to maximize the probability of those fixes going in.Note that this affects:
class
attribute on the<img>
tagI've been debugging this for well over an hour. I know this is happening somewhere in CKEditor's
image2
plugin, which thedrupalimagecaption
plugin extends. But I've been unable to pinpoint the exact root cause. So, I opened a CKEditor ticket to call in their help: http://dev.ckeditor.com/ticket/13888.For now, I'm just going to fix this on the Drupal side.
Comment #5
Wim LeersNote this is a subtle/minor data corruption bug.
Comment #6
Wim Leers… and hence it should ideally be fixed before release.
Comment #7
Wim LeersAnother 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.
Comment #8
yched CreditAttribution: yched commented@wim++ ! Ouch, yeah, widgets are cool but the internals can be messy :-/ And I got lost between CK's image2 and our own override last time I looked there.
The fix seems to make sense, however I'm not at home for another couple days, so I can't in good faith say I have actually tested it. Anyone ? The IS contains steps to reproduce...
Nitpicks :
- "whatever the main element is of the widget" : order wrong looks of the sentence ;-)
- last comment sentence looks unfinished ?
Comment #9
Wim LeersComment #10
yched CreditAttribution: yched commentedI think that should be "whatever the main element of the widget is" ?
Comment #11
Wim LeersI think you're right. Dutch word order getting in the way, because that made it feel natural to me I think :)
Thanks for your patience!
Comment #12
yched CreditAttribution: yched commented;-)
Now we just need someone else to manually test this, I won't be able to before another couple days.
Comment #13
mlewand CreditAttribution: mlewand commentedI feel like your proposed solution is a little too strict, are you sure you want to cut extensibility from your plugin? With this approach any other plugin can't effectively add any classes to the widget.
e.g.
myWidget.addClass( 'foo' )
instead you might consider filtering them out:
That will bring few LoC but provide more extensibility if it's what you're caring for.
Comment #14
Wim LeersExcellent suggestion! Implemented. Updated the comment accordingly.
Comment #15
mlewand CreditAttribution: mlewand commentedAlright now it looks better, +1.
Comment #16
Wim LeersGiven @mlewand from the CKEditor team has signed off on this, RTBC'ing.
Also significantly revamped the IS.
Comment #17
Wim LeersComment #18
yched CreditAttribution: yched commentedEven better :-) Thanks @mlewand !
Comment #19
xjmDiscussed with @effulgentsia. Since this bug does not impact the actual output -- only essentially whether you see unexpected gray boxes in the editor -- the impact does not seem high enough to merit the risk of any regression during RC, especially because of the lack of frontend testing. That said, this would be acceptable for a patch release. So let's fix this after 8.0.0. Untagging and leaving RTBC.
Thanks!
Comment #20
alexpottIt'd be great to have some test evidence on issue to compensate for the lack of automated testing.
Comment #21
thpoul CreditAttribution: thpoul as a volunteer commented@alexpott here you go :)
I have reproduced the bug as described by @yched on 8.0.x. Screenshots A-01, A-02, B-01, B-02, B-03 demonstrate the problem.
After applying #14 by @wimleers the image caption can get removed as expected. Screenshots C-01, C-02, C-03 demonstrate the outcome.
Comment #22
Wim LeersThanks for your thorough review, @thpoul!
Comment #23
alexpottCommitted f8b7bd2 and pushed to 8.0.x and 8.1.x. Thanks!
Comment #26
nod_