Problem/Motivation

Steps to reproduce :

A:
  1. Add an image in a ckeditor area, leave the "caption" option unchecked
  2. Save

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

B:
  1. Add an image in a ckeditor area, check the "caption" option
  2. Re-edit the image, and uncheck the "caption" option
  3. 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:

  1. markup in CKEditor
  2. markup in the database (i.e. stored data)
  3. 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

  1. markup in the database (i.e. stored data)
  2. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Issue summary: View changes
Wim Leers’s picture

Title: Markup for non-captioned image retain the caption / img-caption classes » Removing caption from a previously captioned image fails to remove the caption-related classes
Assigned: Unassigned » Wim Leers
Issue tags: +CKEditor in core, +Spark

Great 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 a data-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:

/* Override Bartik's default blockquote and pre styles when captioned. */
.caption-pre > pre,
.caption-blockquote > blockquote {
  margin: 0;
}
.caption-blockquote > figcaption::before {
  content: "— ";
}
.caption-blockquote > figcaption {
  text-align: left;
}
[dir="rtl"] .caption-blockquote > figcaption {
  text-align: right;
}
yvesvanlaer’s picture

Issue tags: +Amsterdam2014

Just saying that this is still an issue.
Trying to fix this but I'm not sure if I could find a solution.

Wim Leers’s picture

Issue tags: -Amsterdam2014

This is not a big bug, so could conceivably go into 8.0.x. Hence not tagging rc target triage, 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:

  1. markup in CKEditor
  2. markup in the database (i.e. stored data)
  3. 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

I've been debugging this for well over an hour. I know this is happening somewhere in CKEditor's image2 plugin, which the drupalimagecaption 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.

Wim Leers’s picture

Issue summary: View changes

Note this is a subtle/minor data corruption bug.

Wim Leers’s picture

Issue tags: +rc target triage

… and hence it should ideally be fixed before release.

Wim Leers’s picture

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.

yched’s picture

@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 ?

Wim Leers’s picture

  • The order looks correct to me? :)
  • Indeed; fixed.
yched’s picture

I think that should be "whatever the main element of the widget is" ?

Wim Leers’s picture

I 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!

yched’s picture

;-)

Now we just need someone else to manually test this, I won't be able to before another couple days.

mlewand’s picture

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

var originalGetClasses = widgetDefinition.getClasses;
widgetDefinition.getClasses = function () {
  var ret = originalGetClasses.call(this);
  var excluded = (this.editor.config.image2_captionedClass || '').split(/\s+/);

  if (excluded.length && ret) {
	  // Might simplify that one depending what IE you're going to support.
	  for (var i = 0; i < excluded.length; i++) {
		  if (excluded[i] in ret) {
			  delete ret[excluded[i]];
		  }
	  }
  }

  return ret;
};

That will bring few LoC but provide more extensibility if it's what you're caring for.

Wim Leers’s picture

Excellent suggestion! Implemented. Updated the comment accordingly.

mlewand’s picture

Alright now it looks better, +1.

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Given @mlewand from the CKEditor team has signed off on this, RTBC'ing.

Also significantly revamped the IS.

Wim Leers’s picture

Issue summary: View changes
yched’s picture

Even better :-) Thanks @mlewand !

xjm’s picture

Issue tags: -rc target triage

Discussed 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!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots, +Needs manual testing

It'd be great to have some test evidence on issue to compensate for the lack of automated testing.

thpoul’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs screenshots, -Needs manual testing
FileSize
81.1 KB
40.27 KB
68 KB
42.69 KB
42.28 KB
63.41 KB
48.37 KB
45.38 KB

@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.

Wim Leers’s picture

Thanks for your thorough review, @thpoul!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f8b7bd2 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed c15433e on 8.1.x
    Issue #2268941 by Wim Leers, thpoul, yched, mlewand: Removing caption...

  • alexpott committed f8b7bd2 on
    Issue #2268941 by Wim Leers, thpoul, yched, mlewand: Removing caption...
nod_’s picture

Issue tags: +JavaScript

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.