CKEditor alignment buttons can be used even when the align filter is disabled

Thanks, @quicksketch, great find, and thank you so much for contributing it back upstream :)

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.

The code was pretty specific to Backdrop's CKEditor integration — it seems Backdrop's integration has diverged a fair bit from Drupal 8's, but it's still generally the same, and I was able to easily morph #2 into a Drupal 8 patch. It works great :)

@quicksketch++ :)

I did the manual testing for this issue and the patch works perfectly well.

The fun thing I've seen while testing is that, without that patch, when the image align filter is disabled, we can left or right align the image using the alignment buttons but we cannot center the image this way (with the align filter enabled there is no problem).
It makes me think that, even if this is quite minor according to all the other ckeditor issues, it introduces a really weird behavior to the user that could think that Drupal's CKEditor integration is totally broken.

The disruption of this fix is so low than it could totally be a RC target. I'd prioritize the issue as minor and tag it for triage but I let you judge what's the best thing to do.

Before:

After:

I'd also like a +1 from the CKEditor team.

+1 from the CKEditor team :)

Thanks!

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary.

Done.

Better title.

Discussed with @webchick and we agreed that this is not really necessary to get in to the RC for the reasons @Wim Leers outlined in the issue summary but it'll make a good patch version target to fix once 8.0.0 is out.

Agreed; thanks!

Reflecting #13.

Will re-RTBC after 8.0.0 is out :)

Per #15.

I tested this patch manually, but either I am not testing it correctly or the patch does not work. My steps to reproduce:

1. Install 8.0.x Standard.
2. On /admin/config/content/formats/manage/basic_html, drag the left and right alignment buttons into the toolbar.
3. Under the filter settings for "Limit allowed tags", add class as an allowed attribute for the P tag. (Question: I thought configuring the editor toolbar was supposed to automatically configure the allowed HTML tags as well? But if I do not do this step, the buttons do not appear in the editor.)
4. Save the form.
5. On /node/add/article, use the body's image upload button to upload an image with right alignment.
6. Confirm that the left and right align buttons also work to toggle the image aligment.
7. Save the node and confirm that the right image alignment is respected.
8. On /admin/config/content/formats/manage/basic_html, uncheck "Align images" and save the form.
9. Revisit the saved node and confirm that the image is no longer aligned right.
10. Edit the node again. In HEAD, the right and left alignment buttons still appear to work even though they are not supported. With the patch, I'd expect the left and right align buttons to be disabled when selecting the image, but they are still enabled, even though they have no effect once the node is saved.

Can someone confirm if these are the steps or if this is another aspect of the bug?

@xjm I just followed your steps and I can confirm that the patch works.
I had to empty my browser's cache after applying the patch, though.

To answer your 3: you are right, the required attributes declared by ckeditor plugins are automagically added to the allowed elements list. I think the problem comes from the fact that these two plugins are those from CKEditor by default. These plugins usually does not declare class as required attributes. We should open a follow-up to address this issue because we cannot suppose that users are going to understand that they have to add the p.class attribute in the allowed tags field.

So I've followed @xjm's steps to reproduce the patch appears to work mostly as I would expect and as described in #6 - I tested on chrome.

However one thing that I found odd was that after disabling "Align images" and resaving the node the data-align="right" was not removed the image. Shouldn't it be removed?

@alexpott I think it's the normal behavior as text filters are always applied when showing content, not when saving it.
CKEditor could eventually filter it but, given its plugins are written in JS, we have no mecanism to affect their definition according to filter settings yet.
That could be an interesting new feature but I think it's out of the scope of this issue. I let you judge.

#17:

Question: I thought configuring the editor toolbar was supposed to automatically configure the allowed HTML tags as well? But if I do not do this step, the buttons do not appear in the editor.

It is supposed to, and does. But it was only ever designed for automatically whitelisting tags. Just before release, we changed FilterHtml to also do attribute whitelisting. Hence it works only partially for attributes. See #2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11 for fixing that.

#19:

However one thing that I found odd was that after disabling "Align images" and resaving the node the data-align="right" was not removed the image. Shouldn't it be removed?

Like @DuaelFr already says in #20, we never ever change the original data. That's a key design point of the filter system. This is just another symptom of that design choice. And in fact, this is part of why Drupal 8 — after lots of careful deliberation — went with data- attributes (data-caption, data-align, data-entity-uuid …): because it means the metadata in those attributes can either be acted upon (i.e. the corresponding filters are enabled) or ignored (the case you are talking about). Which is super important for content reuse/multichannel content. When a (native or web) app renders e.g. a news article retrieved from Drupal, it could choose to ignore the alignment metadata.
If you don't want any data- attributes to be sent, then you'd have to add a strip all remaining data- attributes filter to your text format, to run as the very last filter.

Okay the arguments in #20 and #21 make sense to me. I've manually tested this patch and it fixes the bug. Committed 8eb7154 and pushed to 8.0.x and 8.1.x. Thanks!

Yay! :)

Fixed the embedded screenshots in the IS.