Allow using a subset of the tags supported by the enabled CKEditor 5 plugins

Tagging for Contribution event

In addition to the larger issues noted above I would like to add that having a single text field to configure allowed tags becomes difficult to use. It gets hard to manage a large whitelist classes on elements like divs and multiple heading tags in a single textbox. Right now I copy the value to a text editor with regex support then convert spaces to newlines with find and replace, make edits, keeping things in alphabetical order to keep myself sane, then regex replace back to spaces and paste back in.

A better ux that allows each tag to be added or removed then have rules for classes and attributes configured on that tag would be awesome.

But this is a smaller issue compared to other outstanding items.

Note: Edited for clarity.

Fix tags.

A better ux that allows each tag to be added or removed then have rules for classes and attributes configured on that tag would be awesome.

Right … but that's a bug/weakness in Drupal core's \Drupal\filter\Plugin\Filter\FilterHtml::settingsForm() and is out of scope to fix here.

adding tags to Limit allowed HTML tags that aren't tags registered with an active plugin will not work.

By "will not work", you mean "will not work if you load the text in CKEditor 5 and then save again, because you'll have lost that data" — right?

For example, a text format for comments may want to disallow links. While it's possible to not have the link button in the CK Toolbar, an <a> tag can still be added.

How does CKEditor 5 behave in this case? AFAIK it does not allow you to have HTML that does not get rendered but won't get lost?

IOW: to be able to solve this issue, we need to answer the question: does a CKEditor 5 instance configured to not have the link or bold buttons still render <a> and <strong> tags? (Same goes for any other plugin/button + corresponding HTML.)

Turs out it is impressively not possible to do this

😆

if the instance does not have the corresponding buttons. I can't even sneak them into the editiable area via Chrome inspector or JavaScript. CKEditor has it locked down.

This appears to confirm what I suspected:

How does CKEditor 5 behave in this case? AFAIK it does not allow you to have HTML that does not get rendered but won't get lost?

So I started investigating \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::getCKEditorPluginSettings() and \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::getEnabledDefinitions() to check if this was something in our use of CKEditor 5, or something inherent.

1. I set up CKEditor 5 on Basic HTML on a fresh install.
2. I stuck with the default toolbar configuration aka
   

     public function getDefaultSettings() {
       return [
         'toolbar' => [
           'items' => ['heading', 'bold', 'italic'],
         ],
       ];
     }
   

3. I created a piece a new node with the body set to test, which gets stored as <p>test</p>
4. I then modified the node_body field to <p>test <a href="https://drupal.org">link</a></p>, followed by a drush cr
5. if #6 is universally true, then this link should not get rendered. But … it did:
   
   … and it even allowed me to edit the link:
   
6. Conclusion 1: you must've been using one of the buttons whose plugin only gets enabled if and only if the button has been added to the toolbar. I think that means we can conclude this isn't quite as bad as it was starting to sound. I think it means that #6 is only true for buttons that add additional HTML tags.
7. Same steps as above, but now changing it to
   

   <p><p>test <a href="https://drupal.org">link</a></p>
   <table><tr><td>yay</td></tr><tr><td>nay</td></tr></table>
   <h6>woohoo a heading!</h6>
   

   (again followed by drush cr)

8. Result: everything seems to have been converted to <p> tags:
   .
9. Upon saving the entity without editing the body field (but still having clicked into CKEditor 5), the data in node_body is unchanged!
10. Conclusion 2: data is not modified by CKEditor 5 as long as we don't make changes in it 🤞
11. Upon saving the entity after changing "test" to "test2", we see our fears confirmed:
    

    <p>test2 <a href="https://drupal.org">link</a></p><p>yay</p><p>nay</p><p>woohoo a heading!</p>
    

12. Conclusion 3: our worst fear has come to pass! Data loss (or to be more precise: semantics loss). The <table> markup was lost. But … is this a special case, because our CKEditor 5 build does not even include the Table functionality? 🤔Requires investigation. Worse, even though the heading button is present, our <h6> was lost. Is this because the <h6> came after the table? Or is it because even though the CKEditor 5 module currently forces H1–H6 to be marked as supported, the CKEditor 5 "Heading" dropdown in the toolbar actually only lists H1–H3? 🤔 Yet more investigation needed.

@bnjmnm: can you confirm my findings, and continue this investigation?

I'm concerned this issue is getting clouded by the other questions around data loss and whether unknown elements are parseable by CKEditor 5.

Plugins in CKEditor 5 may provide support for an element, so for example the test in #7 for a <table> tag will result in conclusion 3- data loss- because there is no "table" plugin currently loaded at all.
Just created #3209613: Add "Table" plugin

Likewise, if you edit the ckeditor5.yml file to remove the link plugin, then you switch to CKEditor 5 on existing content with a link, that<a> element will be stripped from saved data.

The more plugins we have enabled, the less likely there is any data loss when enabling CKEditor 5.

However, this issue is about intentionally disallowing elements/attributes.

Elements are tied to plugins, so we could choose to not to load a plugin in order to disallow support for that element.
Don't load Table, no support for tables.

For Headings this is more complex, we have some control over which headings to allow in the editor options but I haven't tested in detail what happens to existing content based on those options.

The List plugin provides both <ol> and <ul> so we can't easily let a Drupal user disable only one and not both. We brought this up on the call with CKSource last time and they were going to think about it more, perhaps splitting this plugin into 2.

Ignoring these exceptions, we could list all the "available" elements and attributes visible, and give the user the option to disable them. Disabling an element in this UI could be set to prevent the CKEditor 5 plugin from being loaded.

from #3

having a single text field to configure allowed tags becomes difficult to use

I agree with this, and it's also more complex because the administration of allowed tags and elements is spread across other filters too.

Core filters like "align images" and "caption images" which add data-align and data-caption to image (and secretly to media as well!)
ignore the user's setting in the allowed html, you can delete data-caption from here and it won't have any effect if "caption images" is enabled.

On the other hand, the filter_html field also can enable features.
If you add/remove alt from the filter_html textarea for <drupal-media> the UI changes in the dialog when you edit an image media element. I did not realize this until working on #3206461: Support alt text for image Media
This is a bit "magic" IMO - having UI depend on an html filter is not at all intuitive or clear.

I'd propose we should have a more explicit CKEditor 5 menu on the format page that lists all the elements and attributes currently supported by CKEditor 5 based on user settings. If a user wants to explicitly disable an element, they could do so from this new UI. If multiple elements are provided by one plugin, they could be grouped here instead of listed separately.

I think we'd want to display some messaging as well- to make the consequences more clear. For example, if I had 2 plugins that each provided markup using <div> and the user disabled that element- we'd want to have a clear notice that both of those 2 plugins are now going to be disabled.

I think the filter_html should be disabled/hidden when CK5 is active, and it should automatically match the CK5 allowed html, which is the proposed solution in #3201641: Improve the HTML filter configuration UX

#8++
#9++

Great discussion here! 😊

This is a bit "magic" IMO - having UI depend on an html filter is not at all intuitive or clear.

Agreed that this is a bit "magic" — but this is what it took to be able to provide this functionality. Drupal has historically made the text format (and the settings for each filter) the source of truth, i.e. the deciding factor. Therefore removing alt from the allowed attributes MUST result in the UI not allowing you to create markup in violation of the text format's restrictions. Doing so would allow the markup to be visible only to the author: the text format would filter it away before the end user would get to see it. This was probably the #1 UX concern in the Drupal 8/CKEditor 4 integration work: that should never happen.

The scope of this issue is specific to plugins that support multiple tags, and the current inability to restrict certain tags within that set of supported tags.

Based on this, I think Allow using a subset of the tags supported by the enabled CKEditor 5 plugins would be a more accurate title?

With #3220293: Make all supported heading types visible in the UI moving forward again, this becomes more important. Once that lands, we should start working on this.

#3220293: Make all supported heading types visible in the UI just landed. In this issue, we need to make sure that only the elements corresponding to the configuration are added to the "allowed HTML" settings.

#16++

#3215466: Attribute values not accounted for in CKEditor5PluginManager::getProvidedElements landed :)

The test coverage #3223477: Enabled configurable CKEditor 5 plugins without configuration should trigger validation errors is adding to ValidatorsTest is already paving the path for this issue by specifying the correct heading plugin configuration!

Thoughts:

1. Devil's advocate: why not add a new method to \Drupal\ckeditor5\Plugin\CKEditor5PluginConfigurableInterface?
2. If we believe a separate interface is warranted, it should definitely extend \Drupal\ckeditor5\Plugin\CKEditor5PluginConfigurableInterface — because if it's not configurable, there can't be a configured elements subset.
3. Even more devilish: this is the exact same pattern as we already have for plugin_config in YAML + \Drupal\ckeditor5\Plugin\CKEditor5PluginInterface::getDynamicPluginConfig(): the former is the static default, the latter allows adding to it or generating a subset. So perhaps it should be added to the default interface even? 🤔
   Okay so it's not the exact same pattern because there a superset would be allowed, here only a subset would be allowed. Subset only because otherwise the automatic upgrade path becomes impossible: we wouldn't be able to know in #3216015: Generate CKEditor 5 configuration based on pre-existing text format configuration which CKEditor 5 plugins to enable.
   So definitely not add it to the base interface. Only the first two points are still relevant. 👍🤓

Aha … now we are getting somewhere! The remaining failures here are entirely due to the fact that there is no CKE5 plugin configuration yet when creating a new text format. That is where getElementsSubsetDefault() came into play! Tricky, but it must be possible to solve this in a different way.

i.e. we should be able to re-compute the filter configuration after showing the enabled plugins's configuration UI (vertical tab) for the first time, i.e. after the "submitted editor" computed from form values will actually result in the corresponding configuration appearing in the Editor config entity.

Stay tuned 😬

Created #3226011: Introduce CKEditor5PluginConfigurableInterface::getDefaultSettings() to solve this more elegantly in the future … or we could even do that first!

Woah, @bnjmnm pushed #3226011: Introduce CKEditor5PluginConfigurableInterface::getDefaultSettings() forward on Friday and it's now on the verge of being merged … let's wait for that to land, and then I'll rebase this 🤓

I think this is ready now.

@bnjmnm is still the lead on this, so I'd like his blessing before merging.

Also, because of several upstream changes having been merged since I started working on this, a diff is not straightforward. To help @bnjmnm review my changes quickly, I created a tailored diff: I did git diff 4fc6091327d4086866ba27c953fc8c5cfa11afab HEAD > changes-since-2021-07-27.txt (that's the last commit he pushed) and then manually modified the file.

I removed all diff hunks that are 100% irrelevant (i.e. that are due to merge requests to merge in upstream commits). I kept all others. That does mean that the remaining ones show all my changes, but also some conflict resolution things from upstream changes (most notably those from #3215689: Rename CKEditor 5 plugin IDs to guarantee all plugins can be made configurable). Still, this should make reviewing what's happened since you last touched this a lot simpler, @bnjmnm! 🤓🤞