Improve the HTML filter configuration UX

Since users are not able to configure allowed HTML tags, remove the form field and hard code the value instead based on the requirements of the current CKEditor 5 configuration.

Huh, this would be a very drastic departure.

Also, the Text Editor module infrastructure does not allow for this I think, because the Text Editor-specific configuration UI is loaded via AJAX, which means the CKEditor 5 module cannot just dynamically make other parts of the form disappear — except through JS.

Aren't we removing too much functionality here?

At least in 2012–2013 people felt very strongly that they should continue to be able to configure the filter_html format. It seems like now we are feeling differently?

I'd like to understand the rationale more. This will be tremendously scrutinized if we bring this to core!

I started working on #3201637: Figure out how to prevent data loss during upgrade/migration path, and part of the code I'll be writing for that should also help here. 🤞

I agree the UX on this needs to be improved, and I also think the concerns raised in #2 should be taken into account for the solution. Two approaches come to mind that could improve the UX without imposing limitations beyond those required by CKEditor.

Add a "disabled" field showing the mandatory allowed tags, accompanied by a field to provide additional tags in addition to those.

Add clientside hints (and possibly submit-preventing validation?) to the field. This would be more difficult to implement, but might be more effective at conveying the tags that will be allowed in their CK'd markup.

Postponed on #3207660: Allow using a subset of the tags supported by the enabled CKEditor 5 plugins as that has the potential to change the experience considerably.

Updated issue summary with the approach that was agreed upon based on a discussion with @lauriii and @zrpnr. This can move forward.

I'm not sure I agree with

Make HTML filter always enabled

Why require this? It could in principle be any other \Drupal\filter\Plugin\FilterInterface::TYPE_HTML_RESTRICTOR-type filter besides \Drupal\filter\Plugin\Filter\FilterHtml. We would then not be able to configure it automatically (because we would not know what kinds of settings it has), but we would be able to tell the user "Hey, yo, the restrictions don't match, go tweak it" until they do match.

I already have exactly that working in #3201637: Figure out how to prevent data loss during upgrade/migration path since https://git.drupalcode.org/project/ckeditor5/-/merge_requests/17/diffs?c...

Furthermore, there is nothing preventing you from configuring multiple \Drupal\filter\Plugin\FilterInterface::TYPE_HTML_RESTRICTOR-type filters on a single text format. In that case, the intersection of restrictions (i.e. "most strict") applies. See \Drupal\filter\Entity\FilterFormat::getHtmlRestrictions().

The current MR takes care of the "Make HTML filter configuration read only, generated based on enabled plugins" part mentioned in the issue summary. It does not address "Make HTML filter always enabled " as that's still up for discussion based on #7.

If we go with the approach in the MR, some tests will need updating, and the validator can probably be removed. Those tests are marked as skipped and the validator is still there since that can be done at a later stage if there's agreement the approach is a good one.

Discussed with @lauriii and @zrpnr. They are happy with the current approach, but there's a concern that in its current state, the user must save editor config before newly added plugins can impact the allowed tags config.
Is there a way to either dynamically update this config, or have FilterHtml pull the allowed tags from somehwhere other than editor config? Looking into it.

Impression

🤩🤩🤩 this merge request!

SO MUCH AWESOMENESS!

THIS IS HOW I ALWAYS WANTED THE CKEDITOR 4 TOOLBAR CONFIGURATION UI TO WORK BUT IT WAS NOT TECHNICALLY POSSIBLE.

I'll stop shouting of excitement now 🙈😅😊

UI impact

So it looks like this:

And if you apply #3215506-4: Plugins should be enableable based on toolbar configuration on top of it (which is the issue to happen after this one), it beautifully only allows the HTML of the actually enabled buttons:

Perfect!

Concern 1: UI vs API

While this results in a magnificent UX, there still is still a problem here I'm afraid. It only works while using the UI. While using the Drupal admin UI, this works perfectly. But as soon as you drop down a level, to the level of configuration, it fails: it doesn't run at that point.

The obvious solution is to apply the same logic when saving a text format (using FilterFormat::save()) or a text editor (using Editor::save()). Easy: add ckeditor5_filter_format_presave() and ckeditor5_editor_presave() run the same logic.

But then what about a configuration sync? We're not supposed to get in the way in that scenario. Well, I do think it is reasonable to only run validation logic in that case and in case of a mismatch: A) log the problem, B) make it show up in the status report. Much like the Entity/field definitions: Up to date entry, we could have a CKEditor 5/text formats: In sync entry.

EDIT: oh, this is very closely related to #10! So let's dig into that:

Concern 1 seems related to #10?

But it sounds slightly different: #10 seems to be referring to "if I interact with the CKEditor 5 admin UI, I expect changes that cause more CKEditor 5 plugins to be loaded to immediately update the allowed HTML tags setting" — is that right? If so, we could trigger more AJAX updates by listening to more events — that should be pretty easy to fix. Perhaps the change or formUpdated events can be used for this?

Or … is it referring to the fact that new modules get installed that would add more CKEditor 5 plugins and hence would require the filter_html settings to be updated for every text format that has a CKEditor 5 text editor associated? If it's this, then I would say we should not be dynamically updating all CKEditor 5 instances and text formats without the site builder knowing about it. #3215506: Plugins should be enableable based on toolbar configuration would solve that AFAICT? 😊

Concern 2: tightly coupling to filter_html

This is basically what I wrote in #7. My concern centers on not providing any feedback anymore in case of a different \Drupal\filter\Plugin\FilterInterface::TYPE_HTML_RESTRICTOR-type filter. I'm fine with moving forward this merge request as-is for now. We can bring it back in that issue.

Concern 3: requiring filter_html

One thing I failed to mention in #7: this implicitly prevents ever adding Full HTML support to CKEditor 5. They are working on "general HTML support", @zrpnr and I got a demo last week. It looks promising.

If we start forcefully adding the filter_html filter, it would automatically break at least the Full HTML text format, and also any format that uses a different HTML-restricting filter. If we want to KISS for now, then I think it's more prudent to always throw a validation error if filter_html is disabled. Because in that case at least

Conclusion

I am fine with the general approach in the current merge request (although I found at least one regression in it). It's simple and elegant. It's a big step forward!

It's awesome to see this working, it's something that was discussed really early on but shelved for later in favor of the temporary band-aid validation message.

It currently doesn't catch plugin isEnabled conditions, for example if "embed drupal media" is checked the <drupal-media> element should get added.

There's also a problem with the ajax call, it seems to break the "attach" for ckeditor when switching between editors.

I'd be glad to see a better solution for replacing validateFilterForm and the CKEditor5PluginContextualValidationInterface however I think it's out of scope here, that interface is meant to account for Plugins that require some editor form condition, not specifically related to the allowed elements.

I definitely see where @bnjmnm was trying to take this. But I think just reverting the Media-related things in this merge request and focusing only on FilterHtml would keep the scope tight and would allow us to land this quickly.

For doing what @bnjmnm envisaged, but taking it a step further, I present to you: #3216244: Remove CKEditor5PluginContextualInterface and CKEditor5PluginContextualValidationInterface in favor of annotation — inspired by #3215506-11: Plugins should be enableable based on toolbar configuration 🤓

#3216244: Remove CKEditor5PluginContextualInterface and CKEditor5PluginContextualValidationInterface in favor of annotation is ready to go now 🤓

It occurred to me there are two tests marked as skipped. If a reviewer gets to this issue before I have time to update those tests, everything else should still be review-able and reviewing the tests can be tacked on before commit.

I excitedly came in with the full intent to merge this magnificent improvement! 🤩

Unfortunately I could not because two tests were still marked skipped.

And in working to get them to pass, I found a critical bug in the current code. Don't worry, it's not the most complicated, but it's an important oversight to fix :) See https://www.drupal.org/project/ckeditor5/issues/3201641#mr36-note27982.

Basically, once that bug is fixed and tests are passing — and tests are updated to take this into account, I think this is ready to go. 👍

It occurred to me there are two tests marked as skipped. If a reviewer gets to this issue before I have time to update those tests, everything else should still be review-able and reviewing the tests can be tacked on before commit.

D'oh — sorry, I saw this after I started reviewing the merge request. Still throws me off that I now have to read stuff in two different places… 😬

#3216013: Move validation logic out of form into validation constraints ⇒ use config validation for form, audit command, upgrade path, et cetera and #3215506: Plugins should be enableable based on toolbar configuration made this more complex so this required some additional work on top of the most recent feedback round. Config can now update on save based on enabled plugins, and tests have been updated to cover this. I'm glad the additional complexity happened before this landed, I think it's easier to have this all scoped in one MR.

I think it's easier to have this all scoped in one MR.

My thoughts exactly :)

I left a few comments, but the critical one is https://www.drupal.org/project/ckeditor5/issues/3201641#mr36-note28139. I think that without that, the UX will remain confusing. Might be good to pair on this 🤓 Then again, you've shown yourself to be an AJAX-jedi so I think I'd mostly be a rubber duck sidekick 😆

There's a possibility it will be necessary to change the admin UI JS a bit in order to provide functionality that listens to active button config changes in the filter form. That will be much easier if this lands: #3216255: Move admin UI build process into module

order to provide functionality that listens to active button config changes in the filter form

I doubt you need that, because just dragging in a new button instantaneously updates the hidden textarea:

document.querySelectorAll('textarea')[0].value
"["heading","textPartLanguage","strikethrough","bold","italic"]"

// Wim drags the "subscript" button into the toolbar

document.querySelectorAll('textarea')[0].value
"["heading","textPartLanguage","strikethrough","subscript","bold","italic"]"

🤓

At the moment, the MR now has the ability to successfully respond to button list changes and update the allowed tags list dynamically.

While manual testing, I discovered an additional bug: when switching to CKEditor 5 from a different (or no) editor, every filter config is visible even if the filter isn't available. There's also a JavaScript error if a filter is enabled.

This isn't a problem if an editor form is loaded and already uses CKEditor 5. I believe this is due to the filter and editor configs being updated via two separate AJAX commands, and there's JS behaviors that need to run after both have been updated (Drupal.behaviors.filterStatus, Drupal.behaviors.verticalTabs). Looking into this.

This won't work without #3217732: filterStatus behavior can't find settings markup after AJAX update. (I can continue working on this issue, but tests won't pass until that is in core)

#24: But … this is contrib :) :D So let's just go ahead and apply it using a core patch?

Example at https://git.drupalcode.org/project/range/-/blob/8.x-1.x/drupalci.yml#L9-11 — per #2974870: Allow additional core patches for contrib tests..

Very promising! 🤩

Found two bugs during manual testing. Details on the merge request.

I just pushed a change that adds the enabling of a filter to the events that update the allowed tags field, for situations such as the Media Embed filter requiring of <drupal-media>. Now, enabling Media Embed will result in the dynamic addition of <drupal-media> to the allowed tags. Note that disabling Media Embed will not dynamically remove <drupal-media>. I haven't figured out how to make that happen yet -- but it could potentially happen in a followup as the <drupal-media> is still properly removed (without validation errors) on form submit if Media Embed is disabled.

Looking forward to reviewing this tomorrow! 🤓

I thoroughly tested this. It works magnificently 🤩

I tried:
- text format already on CKE 5
- text format switching from CKE 4 to CKE 5
- new text format

It's a joy to use!

🐛 I did find one remaining bug: changing settings of CKEditor 5 plugins should also trigger an AJAX update. Test case: \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Image::isEnabled(). If you enable the uploadImage button, then toggling the Enable image uploads button should toggle the presence of the <img> tag.

🐛 And I also found one regression (which IMHO can be addressed in a follow-up to not keep this already long issue alive unnecessarily long). As soon as ckeditor5_form_filter_format_form_alter() is running, the filter settings form always shows all filter settings, even for filters that are not enabled. I suspect this is a Drupal behaviors bug deep in Drupal core, and not really a bug here though?

I did find one remaining bug: changing settings of CKEditor 5 plugins should also trigger an AJAX update. Test case: \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Image::isEnabled(). If you enable the uploadImage button, then toggling the Enable image uploads button should toggle the presence of the tag.

This is now supported, and I added testImgAddedViaUploadPlugin() to confirm.

And I also found one regression (which IMHO can be addressed in a follow-up to not keep this already long issue alive unnecessarily long). As soon as ckeditor5_form_filter_format_form_alter() is running, the filter settings form always shows all filter settings, even for filters that are not enabled. I suspect this is a Drupal behaviors bug deep in Drupal core, and not really a bug here though?

If I'm understanding this correctly, you may have manually tested using a core branch that didn't have #3217732: filterStatus behavior can't find settings markup after AJAX update yet. Without that change, there will be filter settings forms for all possible filters, not just the enabled ones. testMediaElementAllowedTags() includes assertions that confirm filter settings forms aren't present for non-enabled filters. Those were passing last week since this module's drupalci.yml applied the necessary patch. Now that the core issue has landed, this should work without patching.

If I'm understanding this correctly, you may have manually tested using a core branch that didn't have #3217732: filterStatus behavior can't find settings markup after AJAX update yet.

Whoops I definitely did! 😅

testImgAddedViaUploadPlugin()

🥳

Very nice! But … found one more edge case that is not yet working (see comment on MR).

Edge case mentioned in #33/MR is addressed.

Awesome, that (RE: #id) works for me 👏

I've looked through the MR multiple times. the JS changes look appropriate, but I don't know enough to sign off on that. but the PHP and tests portions are fine, and it looks like all of Wim's feedback has been addressed. Let's get this in!

Added a commit that expands the docs to better explain some of today's changes, which I don't believe conflicts with that RTBC. This beast is committed.

This beast is committed.

😆

See you in:

• #3218985: CKEditor 5 admin UI's AJAX logic and form alterations are too complex and brittle
• #3218900: Ensure only CKE5 gets the AJAXy live syncing of settings
• #3215597: CKEditor 5 plugin settings form available always, even when the plugin is not enabled
• #3218906: The active "CKEditor 5 plugin settings" vertical tab must not lose focus

And #3218901: Ensure no CKEditor5Plugin plugins need to be aware of the triggering element to determine whether to validate was actually fixed in e876e819 🥳

And found a bug we missed here: #3219076: When switching from CKEditor 4 to 5, the allowed HTML tags are not yet immediately auto-computed 🤓