Removing caption from a previously captioned image fails to remove the caption-related classes

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;
}

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

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.

Note this is a subtle/minor data corruption bug.

… and hence it should ideally be fixed before release.

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.

• The order looks correct to me? :)
• Indeed; fixed.

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!

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.

Excellent suggestion! Implemented. Updated the comment accordingly.

Alright now it looks better, +1.

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

Also significantly revamped the IS.

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!

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

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

Thanks for your thorough review, @thpoul!

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