Allowed attribute 'class' not respected in CKEditor: ACF strips it

try p[class]

<p class="*"> is working for me.

OK, there is definitely an issue.
<p class> works in view, does not work on edit screen (view source.)
<p class="*"> does not work in view, does work on edit screen.

Well, you want to use <p class>

That will allow all classes. The problem is that when editing the text, if you click on view source in ckEditor, then it doesn't show the tags. But they do get displayed when you view the content.

I'm not sure that <p class> is the desired way to say that all classes are allowed. From reading #2549077, it seems like it was discussed whether allowing all classes should even be supported. But <p class> allows the tags to be outputted, it's just that ckEditor is not showing the classes. So, I think ckEditor needs to pass the attached test case?

Added a proposed resolution.

Reproduced.

The root cause is \Drupal\ckeditor\Plugin\CKEditorPlugin\Internal::generateACFSettings(). It gets its information from FilterFormat::getHtmlRestrictions(), which gets its information from FilterHtml::getHtmlRestrictions(), both of which does return allowed[p][class] = TRUE, as expected.

So the problem is in \Drupal\ckeditor\Plugin\CKEditorPlugin\Internal::generateACFSettings(), which means #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default indeed introduced this bug.

Here's a patch with test coverage to fix this.

Oops, I didn't notice that #9 contained a test case. Well, we independently came to the same conclusion; my test case just has fewer LoC changed :)

Please review!

Used the wrong tag.

IS updated.

+++ b/core/modules/ckeditor/src/Tests/CKEditorTest.php
@@ -123,11 +123,16 @@ function testGetJSSettings() {
+    // Since the class tag is configured to be white listed, all classes should
+    // be allowed.

You removed my comment!

Also, I think we need to test class="*" and you removed that from the issue summary!

#24:

1. Let me repeat #20:
   

   Oops, I didn't notice that #9 contained a test case. Well, we independently came to the same conclusion; my test case just has fewer LoC changed :)

   I don't think it makes sense to bring back that comment: it feels unnecessary.

2. Also, I think we need to test class="*" and you removed that from the issue summary!

   The IS was fairly confusing, I simplified it.

   Also, I'm afraid whitelisting <p class="*"> doesn't make much sense. It means you're whitelisting the class "*". You can't use it to match any attribute — that's what just <p class> already does!

   The description even mentions exactly that:

   Attribute names or values may be written as a prefix and wildcard like jump-*

   Emphasis mine.

So, I'm sorry for not having retained your comment, but that's because I didn't start from your patch. I overlooked it. Sorry.

You still are going to get commit credit :)

And my stupidity (for not having seen your patch) actually comes in handy here: we independently arrived at approximately the same test case, so that makes your the perfect person to review my patch, so we can get this fixed! :)

Also, I'm afraid whitelisting <p class="*"> doesn't make much sense.
Exactly! That is why we need to test that it does not allow all classes.

When the ckEditor is configured for <p class="*">, ckEditor view source still shows the classes. We could open a new issue for this since this patch solves the Major issue.

Excellent point! Thanks for the review :)

There seems to be a subtle omission in the ACF documentation. http://docs.ckeditor.com/#!/guide/dev_allowed_content_rules-section-3 says:

• String format: classes – a comma-separated list of classes or an asterisk (*) character.
• Object format: NO MENTION AT ALL OF ASTERISKS

But it seems that *both* formats accept asterisks!

In other words, in the ACF object format, an asterisk is equivalent with TRUE. I reported this to the CKEditor team, so they can update their docs.

Conclusion:

1. We need to make FilterHtml::getHtmlRestrictions() ignore the asterisk, if any is specified. And we need test coverage for that.
2. The above will fix the problem, but we want to make sure that this never regresses towards CKEditor. So we need test coverage there also.

Did both of those things.

Agreed with @alexpott that this should be an RC target.

Great!

+++ b/core/modules/filter/src/Tests/FilterAPITest.php
@@ -207,6 +207,41 @@ function testFilterFormatAPI() {
+      'FilterFormatInterface::getFilterTypes() works as expected for the very_restricted_html format.'

s/very/nonsensical/

(c/p remnant)

Can be fixed on commit, or in a next reroll.

Originally, we were going to make class="*" not allow all classes. Now, we've gone in a different direction.

So, let me write out what this latest patch does :
<b class> Allows all classes
<c class="*"> Allows all classes
<d class="foo bar-* *"> Does not allow all classes

Is that the intention?

That's also what I first thought.

But.

The problem is that * is not an allowed attribute value to whitelist. So, the patch just strips disallowed values, which means the asterisk is stripped. Hence the result is class="". In the DOM, class="" is equivalent to class, and hence…

<p class="*">
==
<p class="">
==
<p class>

Similarly:

<p class="foo bar-* *">
==
<p class="foo bar-*">

Thoughts?

The class attribute now persists on the element in ckEditor. SIte builders are likely to try <p class> or <p class="*" > and both now work.

#35 :)

This makes everything behave as documented and ensures that classes are not stripped when switching to edit source in CKEditor - this behaviour would be user-entered data destructive so this is good to fix in RC. Not upgrade path is necessary.

Committed 573fab0 and pushed to 8.0.x. Thanks!