Uncaught exception when data-caption contains markup upcasting to a model element

Due to #2508421: FilterCaption hard-codes allowed tags, I say this is Minor. Chances of this happening are tiny.

That would be prudent, and a nice nice-to-have indeed 🤓

A specific concrete case of this was encountered: <br> is not allowed right now in the caption, even though FilterCaption explicitly allows it. See #3309204: CKEditor 5 crashes with <br> inside of data-caption for details + failing test. Crediting @mherchel for that.

Bumping to Major because this constitutes a regression compared to CKEditor 4.

Per @longwave in Slack, this is potentially related to #2441811-69: Upgrade filter system to HTML5.

Failing test, not quite sure how to actually fix the problem.

Thought it was upstream issue, doesn't appear so :/

Well … that's a good thing 🤓 That means we can fix it! 😜

I bet it's a bug in

    // Parse HTML from data-caption attribute and upcast it to model fragment.
    const viewFragment = editor.data.processor.toView(
      viewItem.getAttribute('data-caption'),
    );
[…]
    conversionApi.convertChildren(viewFragment, modelFragment);

    // Insert caption model nodes into the caption.
    // eslint-disable-next-line no-restricted-syntax
    for (const child of Array.from(modelFragment.getChildren())) {
      writer.append(child, caption);
    }

I wonder if it's simply the fact that some elements are not allowed to exist in those places, because that's what #3309204-5: CKEditor 5 crashes with <br> inside of data-caption's stack trace suggests:

ckeditor5.js?ri55dw:430 TypeError: Cannot read properties of undefined (reading 'name')
    at d._checkContextMatch (ckeditor5-dll.js?v=35.1.0:5:271947)
    at d.checkChild (ckeditor5-dll.js?v=35.1.0:5:268547)
    at d.<anonymous> (ckeditor5-dll.js?v=35.1.0:5:408865)
    at d.fire (ckeditor5-dll.js?v=35.1.0:5:394092)
    at <computed> [as checkChild] (ckeditor5-dll.js?v=35.1.0:5:408917)
    at d.findAllowedParent (ckeditor5-dll.js?v=35.1.0:5:270959)
    at k._splitToAllowedParent (ckeditor5-dll.js?v=35.1.0:5:84709)
    at k._safeInsert (ckeditor5-dll.js?v=35.1.0:5:84253)
    at Object.safeInsert (ckeditor5-dll.js?v=35.1.0:5:82254)
    at k.<anonymous> (ckeditor5-dll.js?v=35.1.0:5:136002)

Thanks to some help from the CKE folks we have a fix. https://github.com/ckeditor/ckeditor5/pull/12798

That was fast! :O

only updated the image plugin, not the drupal media one.

Added test for the image plugin + update code for the drupal media plugin

Queued 10.0, 9.5 and 9.4 tests of #24.

1. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalImage/src/drupalimageediting.js
   @@ -478,20 +478,13 @@ function viewImageToModelImage(editor) {
   -      const modelFragment = writer.createDocumentFragment();
   ...
   -      conversionApi.convertChildren(viewFragment, modelFragment);
   ...
   +      conversionApi.convertChildren(viewFragment, caption);
   

   So … was the problem that there were two distinct document fragments, and nodes cannot be moved from one document fragment to another?

2. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -300,4 +301,43 @@ public function testAlt() {
   +  public function testCaptionMarkup() {
   

   🤔 What is the purpose of this new test?

   Why isn't \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testEditableCaption() adequate?

   The Media Library plugin is just for inserting, the Media plugin is the one that provides this functionality. So this new test is IMHO at the very least in the wrong test class 😅

1. So I don't have 100% grasp on the cke5 code, but I think transforming text into nodes behaves differently based on the context in which it happens. Context was an empty document fragment before, now it's the caption element. Somehow somewhere it's the right thing to do.

2. no idea 🤷 :D

Updated the test location.

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
@@ -627,6 +627,31 @@ public function testEditableCaption() {
+    $caption_markup = 'Test<br>caption';

I'd rather just add a <br> to the existing \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testEditableCaption() test … 😅 These tests are relatively slow, no need for repeating 99% of the test, if we can test both aspects at the same time!

It wasn't straightforward so went this way to keep it green will try again later today

Easier after some sleep

Great! Could you also upload a failing test-only patch, now that the test coverage is just updating an existing test? 🙏

Failing test

Perfect! 👏

1) Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest::testImageCaption
Failed asserting that a NULL is not empty.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTestBase.php:580
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

which is the first assertion after setting a data-caption containing a <br>, hence #31's failure proves that the green test result for #29 can be trusted! 👍

rerolled

crediting wim for review and lauriii for actually finding the fix

the 9.5 version

Committed and pushed 8b1819bf3b to 10.1.x and 10544d8d07 to 10.0.x. Thanks!
Committed and pushed afc43ea00f to 9.5.x and c62e7d1c80 to 9.4.x. Thanks!