Update CKEditor 5 to 39.0.1

Based on the test results → at #3379104-7: [meta] Test against CKEditor 5 nightly → , I expect that

html-support: A link attached to an image should not be lost when loading content with the LinkImage plugin and full General HTML Support enabled. Closes #12831. (commit)

will cause some tests to fail — but likely this means a simplification on our end, since we won't need to work around it anymore 🤞

Hm, looks like this is causing a genuine regression:

    // When unrestricted, additional attributes on links should be retained.
    $xpath = new \DOMXPath($this->getEditorDataAsDom());
    $this->assertCount($unrestricted ? 1 : 0, $xpath->query('//a[@href="http://www.drupal.org/association" and @class="trusted"]'));

👆 when $unrestricted === TRUE, we expect that that class="trusted" is retained. That is no longer true once we update to 39.0.1. 🐛

When $unrestricted === TRUE, Drupal will have:

ckeditor5_arbitraryHtmlSupport:
  ckeditor5:
    plugins: [htmlSupport.GeneralHtmlSupport]
    config:
      htmlSupport:
        allow:
          -
            name:
              regexp:
                pattern: /.*/
            attributes: true
            classes: true
            styles: true
  drupal:
    label: Arbitrary HTML support
    elements: false
    library: core/ckeditor5.htmlSupport
    # @see \Drupal\ckeditor5\Plugin\CKEditor5PluginManagerInterface::getEnabledDefinitions()
    conditions: []

i.e. it will enable the GHS that allows EVERYTHING.

It looks like https://github.com/ckeditor/ckeditor5/commit/3d155169b2cad4b40330059382b... introduced a genuine regression against this 🤔 Or maybe we need to change our logic in core/modules/ckeditor5/js/ckeditor5_plugins/drupalImage/src/drupalimageediting.js

Either way, this is causing a net regression. 🙈

On our side, only @lauriii can figure this out efficiently. He’ll be back from vacation next week. Assigning to him. Welcome back, Lauri 😜

Looks like this is ready for review but I don't know enough about the change to make it RTBC.

@lauriii, could you add some comments explaining why this change is needed? Why is https://github.com/ckeditor/ckeditor5/commit/3d155169b2cad4b40330059382b... not handling this for us? I hoped/expected that we'd be able to delete code rather than add more.

Especially because the conditionality in that upstream commit:

			if ( editor.plugins.has( 'LinkImage' ) ) {
				conversion.for( 'upcast' ).add( viewToModelLinkImageAttributeConverter( dataFilter, editor ) );
			}

… indicates that the necessary upcasting should be added only if LinkImage is loaded, and \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest::setUp() does place the link toolbar item, which means that the CKEditor 5 LinkImage plugin should be loaded according to:

ckeditor5_linkImage:
  ckeditor5:
    plugins:
      - link.LinkImage
    config:
      # Append the "Link" button to the image balloon toolbar.
      image:
        toolbar:
          - '|'
          - linkImage
  drupal:
    label: Linked Image
    elements: false
    conditions:
      plugins:
        - ckeditor5_link
        - ckeditor5_image

And hence I thought we'd be able to delete the "image link GHS" integration code on our end:

    // Modified alternative implementation of GHS' addBlockImageLinkAttributeConversion().
    // This is happening here as well to avoid a race condition with the link
    // element not yet existing.
    …

which we had introduced in #3246168: Images are not linkable through UI; already linked images are unlinked (data loss!) → (before it was added to core!).

The DrupalImage plugin heavily customizes the way Image plugin works. The key difference being, that we never wrap images with a <figure> in the data view.

It turns out that the commit mentioned in #8 adds more assumption to the CKEditor 5 linked image upcast process. In particular, it assumes that the link is inside the imageBlock element. This is not true in the case of Drupal, because in our case <img> is directly converted into imageBlock, meaning that the link is wrapping the imageBlock element.

We could try to document this inline in the JavaScript file, but I want to point out that majority of the code in that same file exist for this same reason.

I think just adding #9 as a comment to the modified file would indeed be sufficient 😊👍

Tried to write down #9 in the code docs 👍

No more concerns — thanks! 😊

🚢

Committed and pushed 5cc339ea60 to 11.x and 4befb2b456 to 10.1.x. Thanks!

Lovely, thank you! 😊 🚀

Is it possible to also apply it on Drupal 9.5.x? we have some problems with inline styles and the remove format button 🐛 [upstream] Remove Format button does not remove `style` attributes Postponed

@Eduardo Morales Alberti No, not possible. Other contrib modules will break. If you are using no CKEditor 5 contrib modules, then you could.

But it'll have to be you doing it for your site, an official release Drupal 9.5 release with this would break countless sites.

Let's see then how we fix the inline styles format remove scenario because is something important on our site and we have contrib modules not compatible with Drupal 10.x (yet).
Thank you.

Wish there was more I could do — but our hands are tied! Good luck 😊 Personally, I'd look into getting those contrib modules on D10!

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