Allow inline HTML comments in CKEditor 5

This is an unstable feature that is not available by default, with some known issues:

By default, all HTML comments are filtered out during the editor initialization. The HTML Comments feature allows developers to keep them in the document content and retrieve them back, e.g. while saving the editor data. The comments are transparent from the users point of view and they are not displayed in the editable content.

The HTML comment feature is experimental and not yet production-ready.

The support for HTML comments is at the basic level so far - see the known issues section below.

This feature is enabled by default in the superbuild only. See the installation section to learn how to enable it in your editor.

https://ckeditor.com/docs/ckeditor5/latest/features/general-html-support...

There is a premium plugin that supports a custom comment style:
https://ckeditor.com/docs/ckeditor5/latest/installation/plugins/features...

That makes sense. 👍

I'll work with @lauriii to prioritize this in 🌱 [meta] [upstream] Prioritized CKEditor 5 upstream blockers Active . I'll bring it up at today's meeting I have with the CKEditor 5 developers.

Discussed with @lauriii and now prioritized — see #3340578-7: [meta] [upstream] Prioritized CKEditor 5 upstream blockers → .

Clarifying the issue summary: the necessary plugin already exists upstream but has significant issues.

But … I think that in this case, it's probably better to have some support rather than none? Although perhaps that would be more infuriating?

Either way, I think that this is all that's needed — untested, please give it a try 😊

Applied the patch on Drupal 10.1 standard install
Added

to the source of the ckeditor
Clicked source again and it's gone.

You probably ran into https://github.com/ckeditor/ckeditor5/issues/10118 then 😬

I can't get the patch to work, it removes all comments no matter where they're placed, not just within certain HTML tags.

Are there suppose to be any changes necessary on the text format to make it work?

Are there suppose to be any changes necessary on the text format to make it work?

Not that I know of, but for the sake of testing, could you test it on a format that has no filters at all? 🙏

I am having the same issue as suggested in https://www.drupal.org/project/drupal/issues/3342874#comment-14943480 ✨ [upstream] Allow inline HTML comments in ckeditor5 Postponed

Also, I did checked on latest Drupal 10 installation with Basic HTML text editor, it behaved the same.

The following

<!--
comment
-->

was stripped when we go out of source ediiting mode.

I found that our core drupalHtmlEngine plugin is stripping out the comments. Tracing it back this was originally added in #3247246: Attribute value encoding not compatible with Xss::filter() → . I've added HTML comment support to the drupalHtmlEngine plugin. I'm not 100% sure about the approach, feedback please!

I opened an MR to make code review easier and I'm attaching a patch with the same changes.

Testing note: If you test by adding only an HTML comment to the editor with no other content, it will still be removed. You need to add something else: an HTML element or text for example. This seems to be the case for the upstream plugin as well so I don't think that bug is on our side: https://ckeditor.com/docs/ckeditor5/latest/features/html/html-comments.html

Issue management note: I'm removing 'upstream' from the title because I think we have what we need to implement this but I do realize there are known issues with the upstream plugin and it's experimental. Maybe we can get something into core and then open another issue to track the known upstream issues?

Aren't data loss issues supposed to be triaged as critical?

This seems more severe than a normal data loss issue because it may be months or years until someone actually notices that data has gone missing! Maybe even so long that revision pruning has made the data unrecoverable!

@star-szr in #17: Oh, hi again! 😄 After running into a colleague of yours at DrupalCon Lille and then seeing you on the GitLab CI Nightwatch issue, I'm now seeing you everywhere somehow 😊

Thanks for the research! And in fact … the MR looks pretty much ready! What I'm missing is a bit more test coverage: the Nightwatch JS unit test is great, but I think we want at least one end-to-end test. A new test method in \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5Test seems appropriate?

@Luke.Leber in #18: Yes. But HTML comments have historically never contained actual content. Exceedingly rarely sites are putting critical content in HTML comments … a statement that seems to be confirmed by the fact this issue has only 11 followers. See #5 and #6 for the considerations @lauriii and I put into this.

Hi @Wim Leers! This year I have been leading the project on our team to integrate CKEditor 5 which has involved porting and updating a contrib module → , adding table footer support to CKEditor 5 upstream, a few custom CKEditor plugins, and now this issue! So I suppose it was inevitable we would run into each other and it’s always nice to see a friendly face in the queues (and on GitHub)!

I will work on the test coverage, and agree that it’s missing.

Oh and thanks for adding the related issue. I did that at one point but had to fight with this form a bit (hence why it says there are so many patches from me in Credit & committing, I believe).

Now with more tests!

I was seeing a failure in TimestampFormatterWithTimeDiffTest and one other similar one, seems like a random fail, re-ran and it passed. I re-ran Nightwatch and the failure switched from one to another so also seems random.

Could be a coincidence for sure but I've never seen randoms back to back to back like that. Wonder if a change is causing the random errors to appear more frequent. What if we took out the change to nightwatch? Change is covered by the FunctionalJavascript test right?

If we remove the Nightwatch change it should be failing every time, in other words we need that change. I'm going to try running Nightwatch again.

Same failure as last time, in Tests/Olivero/oliveroSearchFormTest. https://git.drupalcode.org/issue/drupal-3342874/-/jobs/288500#L174

@star-szr in #20: 😊😊

@smustgrave in #23: I've been seeing it earlier today in a completely different issue too. I suspect it's a Docker image/Chrome version/infra change thing.

IOW: there are definitely random JS failures again … 😬

No more remarks, thank you so much! 🥳

Thanks @Wim Leers!

Re: Nightwatch, I can sometimes trigger the failure locally, both with these changes and on 11.x. So it does seem unreliable/random.

This test specifically: core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroSearchFormTest.js

(Hiding patches from the issue summary)

Let's get this critical feature request in to 10.2.0.

Committed and pushed 66716dc920 to 11.x and a9ee5c1044 to 10.2.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

hi! we've been watching this for client sites. We just tested 10.2.0 -- and we are not seeing the presevation of commetns, they are still being stripped.

Does 10.2.0 have this fix natively, or does it rely on some additional plugin to be installed still? was testing done? I don't see any "RTBC" comments above for the work done by star-szr

Thanks for your efforts! We're really eager to see this patched into CK5. These sorts of issues have been causing us all sorts of headaces on our enterprise sites.

@nkraft just to be sure, are you testing HTML comments along with other content? If the editor contains only an HTML comment it will still be stripped because CKEditor 5 doesn’t see any content to render.

star-szr → closed merge request !5269

Ported the patch for those who are still on Drupal 9

Works fine with 9.5.11