Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Steps to reproduce:
- Enable the 'Limit allowed HTML tags and correct faulty HTML' filter in the ckeditor format
- Add
<p class>
to the Allowed HTML tags. - Create content and add a class to an element.
Expected behaviour:
The class attribute should persist on the element in ckeditor.
Current behaviour:
The class attribute is stripped from the element.
Proposed resolution
Fix CKEditor's ACF setting.
Remaining tasks
Review & commit.
User interface changes
None. Except that classes won't be stripped anymore in CKEditor :)
API changes
None.
Data model changes
None.
Why RC target?
Because without this, it's impossible to use any class
in CKEditor.
Comment | File | Size | Author |
---|---|---|---|
#29 | cke_acf_class_attr_allowed-2596083-29.patch | 8.13 KB | Wim Leers |
#29 | cke_acf_class_attr_allowed-2596083-29-test-only-FAIL.patch | 3.58 KB | Wim Leers |
Comments
Comment #2
jamin_melville CreditAttribution: jamin_melville commentedComment #3
lokapujyatry p[class]
Comment #4
jamin_melville CreditAttribution: jamin_melville commentedUsing
<p[class]>
also results in class being stripped out in ckeditor.Comment #5
lokapujya<p class="*">
is working for me.Comment #6
lokapujyaOK, 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.Comment #7
jamin_melville CreditAttribution: jamin_melville commentedThat's the same result I get. In
core/modules/filter/src/Plugin/Filter/FilterHtml.php
it states// A trailing * indicates wildcard, but it must have some prefix.
. So using<p class="*">
is an invalid rule.Comment #8
lokapujyaWell, 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.
Comment #9
lokapujyaI'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?Comment #11
lokapujyaComment #12
jamin_melville CreditAttribution: jamin_melville commentedI've tried with rc2 on standard profile and still having this issue.
Comment #13
lokapujyaComment #14
lokapujyaAdded a proposed resolution.
Comment #15
lokapujyaComment #16
lokapujyaComment #17
lokapujyaComment #18
lokapujyaComment #19
Wim LeersReproduced.
The root cause is
\Drupal\ckeditor\Plugin\CKEditorPlugin\Internal::generateACFSettings()
. It gets its information fromFilterFormat::getHtmlRestrictions()
, which gets its information fromFilterHtml::getHtmlRestrictions()
, both of which does returnallowed[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.
Comment #20
Wim LeersOops, 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!
Comment #21
Wim LeersUsed the wrong tag.
Comment #23
Wim LeersIS updated.
Comment #24
lokapujyaYou removed my comment!
Also, I think we need to test class="*" and you removed that from the issue summary!
Comment #25
Wim Leers#24:
I don't think it makes sense to bring back that comment: it feels unnecessary.
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:
Emphasis mine.
Comment #26
Wim LeersSo, 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! :)
Comment #27
lokapujyaAlso, 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.
Comment #28
lokapujyaWhen 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.Comment #29
Wim LeersExcellent 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:
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:
FilterHtml::getHtmlRestrictions()
ignore the asterisk, if any is specified. And we need test coverage for that.Did both of those things.
Comment #31
xjmAgreed with @alexpott that this should be an RC target.
Comment #32
Wim LeersGreat!
s/very/nonsensical/
(c/p remnant)
Can be fixed on commit, or in a next reroll.
Comment #33
lokapujyaOriginally, 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 classesIs that the intention?
Comment #34
Wim LeersThat'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 isclass=""
. In the DOM,class=""
is equivalent toclass
, and hence…Similarly:
Thoughts?
Comment #35
lokapujyaThe class attribute now persists on the element in ckEditor. SIte builders are likely to try
<p class>
or<p class="*" >
and both now work.Comment #36
Wim Leers#35 :)
Comment #38
alexpottThis 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!