[GHS] Custom/unofficial HTML tags not retained: <drupal-media>, <drupal-entity>, <foobar>

Wow, this is bad.

Added to #3238333: Roadmap to CKEditor 5 stable in Drupal 9.

So it would seem that

…
    config:
      htmlSupport:
        allow:
          -
            name:
              regexp:
                pattern: /.*/
            attributes: true
            classes: true
            styles: true

configures GHS to support every HTML element … as long as CKEditor 5 knows about it.

IOW: custom elements are stripped?! 🙈😱

Sadly, yes, reproduced with simply this:

<p>p</p>
<yo>yo</yo>

gets converted to

<p>p</p>
<p>yo</p>

by CKE5. In CKE4, this was kept unchanged.

This feels/sounds similar to #3245950: [upstream] <script> tag support in GHS and #3256566: [upstream] <style> tag support in GHS, which were trivial fixes upstream for them, both using the same approach. Let's hope the same approach works for arbitrary unknown elements 🤞

This is impossible to solve on the Drupal side because Drupal does not and cannot know which HTML elements are being used in a particular piece of rich text. And even if we analyzed a stored piece of rich text, then we still wouldn't be able to predict which additional custom elements would be entered (or pasted) by the user in the future.

Will raise this in Thursday's call with the CKEditor 5 team. Sending them this right away, because this is of the highest possible priority.

Upstream bug report filed: https://github.com/ckeditor/ckeditor5/issues/11432

Confirmed with @lauriii.

⚠️ Was informed by the CKEditor 5 team that this will not be fixed in the April 6 release (https://github.com/ckeditor/ckeditor5/issues/11387), but in the release after that (May).

The issue title makes it sound like this is a bug specific to <drupal-media>, but that's not the case. Improved the title + issue summary to clarify the impact scope.

Thanks @dom for confirming this problem with <drupal-entity>, for which a filter exists (https://www.drupal.org/project/entity_embed), but no CKEditor 5 plugin exists yet, and so no work-around is possible … and simply starting to use CKEditor 5 on a "Full HTML-like" text format that uses the Entity Embed filter will today simply lose all the embedded entities upon editing!

In the case of my website, this issue completely prevent any adoption of any Drupal version where CKE5 is the norm.
For that reason, I added Entity Embed in the list of "needs Drupal 10 & CKE5 readiness" tracked at : https://www.drupal.org/node/3273985

Note that because the tag is completely stripped, if a user edits a node with any custom element, the data are lost as the node is saved. If you don't have revisions activated, those data are lost forever.

• Entity Embed blocked : 71,663 sites report using this module
• Media Directories blocked : 3,034 sites report using this module

I would be interested to build an MVP workaround for those modules that users or even the module maintainers could use in the meantime of a proper implementation. That MVP should just register the tag and all it's possible attributes to CKEditor 5 so they are not stripped. Eventually, it could replace it in CKE view (not in save) by a red square marked "no preview available" for instance. If you have instructions, documentation or direction to point for helping on that, it would be great.

In the case of my website, this issue completely prevent any adoption of any Drupal version where CKE5 is the norm.

We know, that's why this is a critical data loss bug — the CKEditor team knows this is hard blocker for the CKEditor 5 module from becoming stable.

Thanks for confirming this though, having confirmation from concrete sites also helps convey this to them :) 👍

Per @lauriii: https://github.com/ckeditor/ckeditor5/pull/11744 was merged which should address this stable blocker! 🥳

unfortunately the latest ckeditor release does not include the change yet

This was released in ckeditor 34.2.0. Not sure what specifically needs to happen in this issue.

No longer blocked on upstream.

… because #3301495: Update CKEditor 5 to 35.0.1 landed 🤓

Manual testing

I wrote this in #3:

Sadly, yes, reproduced with simply this:

<p>p</p>
<yo>yo</yo>

gets converted to

<p>p</p>
<p>yo</p>

by CKE5. In CKE4, this was kept unchanged.

But today (thanks to #3301495: Update CKEditor 5 to 35.0.1), it gets converted to:

<p>
    p
</p>
<yo>yo</yo>

So … the bug has indeed been fixed upstream! 🥳

Tests

All that remains is test coverage.

@lauriii wrote that in #19.

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
@@ -495,7 +495,7 @@ public function testFullHtml() {
+    $assert_session->responseContains('<foo></foo><p><a foo="bar" hreflang="en" href="https://example.com"><abbr title="National Aeronautics and Space Administration">NASA</abbr> is an acronym.</a></p>');

I'd like this to be expanded slightly: I'd like both

• <foo></foo>, which this already
  has
• <bar>bar</bar>, to test the expected behavior for text contents of custom/unofficial tags when switching to a text format that does not allow this custom tag(I expect it to be converted to <p>bar</p>)

Once that's addressed, this is RTBC!

Better still! 👏

1. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
   @@ -529,7 +529,7 @@ public function testFullHtml() {
        $assert_session->responseContains('<p><a href="https://example.com" hreflang="en">NASA is an acronym.</a></p>');
   

   It' very possible I'm missing something, but shouldn't this assertion also check that contents of the foo tag is still there? i.e.:

   ⬅️✌️➡️

OK glad I asked! Worth having even if it just makes the test easier to read.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

We should still get this committed to 9.4.x.

#24 still applies cleanly to 9.4.x — queued a test to prove it passes tests.

Committed/pushed to 9.4.x, thanks!

Restoring previous status.