Confusing behavior after pressing "Apply changes to allowed tags" with invalid value

Hm … in hindsight, I think this should not be a stable blocker.

You cannot run into this when updating existing text formats using CKEditor 4.

You can only run into this when creating new text formats. There are plenty of confusing things about creating new text formats: the importance of the filter order and how they allow you to shoot yourself in the foot for starters. If anything, CKEditor 5 at least provides you with feedback and strong validation.

Therefore I think this should definitely not be a stable blocker and perhaps it should even just be normal priority.

This is definitely a major bug, but if it affects CKE4 configuration as well, then by definition it would not be a stable blocker because it is existing technical debt.

I have this case on an existing format, where the 'br' missing from the allowed HTML list:

• The "Apply changes to allowed tags" fails to add the 'br' to the list (no error in the console). The result is the situation as described above.
• Manually adding the 'br' to the list, allows to switch to CKEditor 5.

If the 'br' is present and 'p' is missing, at least in the corresponding config yml, it is auto-added when the config form is build (so it seems), and the problem does not arise.

• The 'p' tag is added automatically to the "Allowed HTML tags" if the "Formats" button is in the toolbar, because "paragraph" is one of the formats in there, but 'br' isn't.
• Neither the 'p' nor the 'br' tag have to be in the list of allowed tags, if Convert line breaks into HTML (i.e. <br> and <p>) is enabled.

The default "Restricted HTML" has the conversion of line breakes enabled by default, and if CKEditor is added to that, then that stays enabled and 'p' and 'br' are not added to the list of "Allowed HTML tags".
Also on older sites that where configuration has been moved along from previous versions, the CKEditor 4 configuration can therefore end up without explicitely allowing 'p' and 'br' tags.
In both cases, the line breaks are displayed correctly, but it will break if "Convert line breaks" is disabled.

#5: Thanks for testing this at Drupal Dev Days!

The "Apply changes to allowed tags" fails to add the 'br' to the list (no error in the console). The result is the situation as described above.

That's the thing: the Apply changes to allowed tags button does not add any tags! It just applies whatever changes you made by typing in there … 😅

I 100% agree this is very confusing — I had the exact same assumption about what this button would do 🤓

#6: Thanks for testing this at Drupal Dev Days!

I think that when switching from CKEditor 4, we should not throw a validation error if the text format currently does not allow <br> and instead just add it automatically. That's clearly the impliedly intended behavior, since they were using CKEditor 4 before. Could you maybe create a new issue for that, with a screenshot showing the current UX if your text format allowed <p> but not <br> and was using CKEditor 4? 🙏

Opened MR, and here is a video of this change in action https://youtu.be/50EpKjlPgo8

Thanks for the screencast in #10 — that makes it much easier to compare with HEAD (by looking at the GIF in the issue summary).

Observations:

1. 😅 This MR now conflicts with #3265626: Changes to "Manually editable HTML tags" lost if form is submitted without triggering AJAX which just landed, needs rebase.
2. 👍 This addresses the styling not looking like this is a validation error
3. 🤔 AFAICT this does not yet address the fact that the Apply changes to allowed tags. button behaves differently than what most people expect. AFAICT most people expect this button to add the <br> and <p> tags. But all it does is allowing changed settings to be re-submitted to allow re-attempting switching to CKEditor 5. IOW: the label is ambiguous.
   @andreasderijcke mentions this too in his comment in #5. I know that this was originally my expectation too. Then we could also make the button label clearer: Add tags needed for CKEditor 5 to work and switch to CKEditor 5 — that leaves no ambiguity.
4. 🤔 … perhaps better still is to distinguish between two cases
   1. if the current text editor is ckeditor (i.e. CKE4) and you switch to ckeditor5 (i.e. CKE5), then automatically add tags needed for CKEditor to function — this is already implied to be allowed by the text format having been using CKEditor 4 up till now. This is similar to #3273312: Upgrading from CKEditor 4 for a text format that has FilterInterface::TYPE_MARKUP_LANGUAGE filters enabled: we had zero validation for verifying whether text format configuration was actually compatible with CKEditor 4, the burden of validity was shifted towards the site builder 🙃
   2. keep the current behavior (but with the previous point also addressed) if you're attempting to enable CKEditor 5 on a text format that had a text editor other than ckeditor (i.e. CKE4) or no editor at all

Point 4.1 also has an important other consequence: it would mean this issue hardens the upgrade path. Because right now, the automatic upgrade path for sites upgrading from CKEditor 4 to 5 will fail if they have filter_html enabled but do not allow <br> (or <p>). We should ensure the CKEditor 4 to 5 upgrade path is guaranteed to succeed. And auto-allowing those two tags does not pose a security risk and is extremely unlikely to break sites. Not having any text editor at all anymore after updating to Drupal 10 would likely be seen as a bigger disruption.

Re #11 I am very on board with automatically allowing <p> and <br>, provided there is messaging that informs the user which tag(s) were enabled.

If memory serves, we made the <p> and <br> enabling a manual step out of concerns it may be too opinionated, especially if we discovered use cases where more tags needed adding. This was early in the CKEditor 5 process, and it's now clear that the "need to add" are truly limited to the very safe <p> and <br> tags.

If updating a format to CKEditor5 auto-adds these tags, it eliminates the most complex-to-the-user step from the process. Although doing so is technically opinionated, the benefits of leaning into this opinion will be immediate and appreciated, while the drawbacks would only apply to unlikely+theoretical use cases that could be mitigated anyway.

In other words, auto-adding makes updating significantly easier for 99.9+% of users and would slightly inconvenience the theoretical user. The theoretical user could potentially switch to CKEditor 5 and ignore the messages and field values indicating the (safe!) <p> and/or <br> were added to allowed tags then be mad. If those tags being allowed pose a major concern for a site, I think it's reasonable to expect its admin to read the messages informing them of what the process is doing. The addition of the tags will still be quite visible, it just won't trigger validation errors. There should be sufficient information to let this theoretical user know CKEditor 5 is not a great choice for their theoretical site.

As the person who added the whole UI step of adding these tags, I'd love to re-scope this issue to get rid of that and just auto-add the tags.

Another +1! 😄👏

The MR is currently removing a bit too much 🤓😅

But definitely well on its way to rip out a lot of complexity! 👍

Looks RTBC, just one typo nit 🤓 😅

Once that typo is fixed and this passes the entire core test suite, let's get this committed!

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
@@ -153,16 +162,9 @@ public function testSwitchToVersion5() {
+    $assert_session->pageTextContains('The following tag(s) were added to Limit allowed HTML tags and correct faulty HTML, because they are needed to provide fundamental CKEditor5 functionality : <br> <p>');

+++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
@@ -817,11 +817,10 @@ public function provider() {
+          'The following tag(s) were added to <em>Limit allowed HTML tags and correct faulty HTML</em>, because they are needed to provide fundamental CKEditor5 functionality : &lt;br&gt; &lt;p&gt;.',

@@ -934,6 +933,7 @@ public function provider() {
+          'The following tag(s) were added to <em>Limit allowed HTML tags and correct faulty HTML</em>, because they are needed to provide fundamental CKEditor5 functionality : &lt;br&gt; &lt;p&gt;.',

These 3 expectation strings also still need to be updated 😅

🚢 Really glad we can leave this one behind us! Especially because it was proven at DDD that this had a significant impact on testing CKEditor 5’s upgrade path! 🎉

😄

The 9.4 patch passed 9.3 tests!

Let's get this committed to 9.3.x too — otherwise subsequent issues/MRs/patches are very hard to land — see #3261599-20: Use CKEditor 5's native <ol start> support (and also support <ol reversed>).