[upstream] Update CKEditor 5 to 40.0.0

FYI: thanks to 🌱 [meta] Test against CKEditor 5 nightly Active , we already know that we'll need to change the drupalImage plugin, because on September 1, tests still passed → , but on September 18, they failed → .

Expected failures:

1. CKEditor5Test::testEditorFileReferenceIntegration()
2. CKEditor5Test::testAttributeEncoding()
3. ImageTest::testAlignment()
4. ImageTest::testWidth()
5. ImageUrlTest::testAlignment()
6. ImageUrlTest::testWidth()

Issue summary updated to make this a 2-step process instead of a 5-step process. Will make future updates simpler 👍

Yay, both ::testAlignment() cases now pass 👍

Everything else seems centered on the presence of this new style="aspect-ratio:WIDTH/HEIGHT" in the <img> markup.

Seems doable!

⚠️ upstream <img height was implemented in an incompatible way

🤩 1. Drop a lot of overrides from drupalImage!

We actually implemented a lot of overrides to get width and height working reliably.

Looks like we can just rm -rf that: https://git.drupalcode.org/project/drupal/-/merge_requests/4989/diffs?co... — 131 lines gone 🥳

🤷‍♂️ 2. Presence of style="aspect-ratio:3456/2160"

https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-40.... says:

The aspect-ratio attribute has been added to the image’s properties to handle situations when the file is resized or scaled with a tweaked aspect ratio.

That sounds like it'd be <img aspect-ratio> … but it's actually not an attribute, but a
CSS property. 😬 IOW: a value of the style attribute: <img style="aspect-ratio:…">.

The use of the style attribute means that this will not work in Drupal, since Drupal's measures to protect content creators from XSS injection will strip any and all style attributes:

    // Apply XSS filters when editing content if necessary. Some types of text
    // editors cannot guarantee that the end user won't become a victim of XSS.
    if (!empty($element['value']['#value'])) {
      $original = $element['value']['#value'];
      $format = FilterFormat::load($element['format']['format']['#value']);

      // Ensure XSS-safety for the current text format/editor.
      $filtered = editor_filter_xss($original, $format);
      if ($filtered !== FALSE) {
        $element['value']['#value'] = $filtered;
      }

— \Drupal\editor\Element::preRenderTextFormat()

editor_filter_xss() is smart enough to only run it for text formats that do have XSS filtering, which is why it will not do anything for full_html for example.

Now, this is not even a big deal, because we simply do not need that information! Drupal uniquely never allowed resizing without respecting the aspect ratio.

Unfortunately, attachDowncastConverter's setRatioForInlineImage parameter only applies to inline images, and nothing else in the relevant commit appears us to override this behavior. Still, we can work around this.

😱 3. Reliance on style="aspect-ratio:3456/2160;width:419px;" for actually resizing

Oh noes … resizing that 3456x2160 image only seems to work in CKEditor 5 itself? It does not work in the final rendered result?

Turns out that width and height are always the original width and height, and not the resized ones! 🫣

Fine, this makes sense for certain HTML-centric approaches. But we'll at least need our downcast to behave differently, because otherwise a lot of contrib/custom modules will break.

So we'll have to restore our own downcast and adjust it.

However … it appears that the Original button then will not work at all.

This too seems surmountable: setImageNaturalSizeAttributes()'s logic should be tweaked. AFAICT just removing a single if-statement would be enough:

    this.editor.model.change( writer => {
      const img = new global.window.Image();

      this._domEmitter.listenTo( img, 'load', () => {
␡␡␡␡␡if ( !imageElement.getAttribute( 'width' ) && !imageElement.getAttribute( 'height' ) ) {␡␡␡␡␡
          // We use writer.batch to be able to undo (in a single step) width and height setting
          // along with any change that triggered this action (e.g. image resize or image style change).
          this.editor.model.enqueueChange( writer.batch, writer => {
            writer.setAttribute( 'width', img.naturalWidth, imageElement );
            writer.setAttribute( 'height', img.naturalHeight, imageElement );
          } );
␡␡␡␡␡}␡␡␡␡␡

        this._domEmitter.stopListening( img, 'load' );
      } );

      img.src = src;
    } );

👆 That would always get the loaded image width/height as the correct one. The browser will load the image anyway, so there's no extra cost.

Conclusion

It is possible in theory to do all this overriding on our end, but AFAICT a 40.0.1 release from the CKEditor 5 team with:

1. a tweaked setImageNaturalSizeAttributes()
2. always make resizedHeight available

I will discuss this in the meeting I have with them in ~30 mins. 👍

Wim Leers → credited witeksocha → .

Discussed in detail with @witeksocha. He recalls there was a reason to remove resizedHeight, but alas it was not documented. He also recalls there was a good reason to not always set the natural width & height.

He'll talk to the engineer who led this upstream change and will get back to me early next week 👍

New config option:

From the release notes:

list: Allow restricting list item content to a single text block by disabling the list.multiBlock configuration option.

👆 Now available in Drupal.

They're working on this upstream at https://github.com/ckeditor/ckeditor5/issues/15185.

Wim Leers → credited niegowski → .

No comments were posted there, but it looks like 2 days ago @niegowski linked a PR (https://github.com/ckeditor/ckeditor5/pull/15222) that provides the necessary example here.

I was able to port https://github.com/ckeditor/ckeditor5/pull/15222 by mostly copy/pasting and it works 😄🥳

However, it looks like it does not support %-based width values.

Let's find out what other tests fail…

Okay, I've got all tests passing now, except for one:

testWidth with data set "Image resize with percent unit (only allowed in HTML 4)"

… for both locally hosted (aka uploaded) images and remotely hosted images.

That makes sense: we previously already had to write custom logic (see #3249592: [drupalImage] upcast assumes HTML5: px unit, but HTML4 allowed % unit → ) to keep these working, and the code I've copy/pasted from upstream does not handle this.

Also, we absolutely need to make sure that that continues to work correctly, because until ~6 months ago, image resizing in CKEditor 5 was using %, not px: 🐛 CKEditor 5 resizes images with % width instead of px width (the CKEditor 4 default): breaks image captions *and* is a regression Fixed .

Just pushed the missing update path test coverage.

This is now ready for review 😊

Clarify the issue summary.

Since the main feature for core seems to be list

Disabled Allow the user to create paragraphs in list items (or other block elements)
Created a page placing <ul><li>a</li><li><p>paragraph 1</p><p>paragraph 2</p></li></ul>
P tags got turned to li

Enabled Allow the user to create paragraphs in list items (or other block elements)
Create a page placing <ul><li>a</li><li><p>paragraph 1</p><p>paragraph 2</p></li></ul>
P tags remained the same.

Left a comment on the MR.

The irony — @catch just talked to me about this ~1.5 week ago and I still forgot 😅🙈 Matched the pattern that was used in ✨ Ability to configure additional languages (e.g. "bash" or "SQL") for CKEditor 5 CodeBlock plugin Fixed .

Didn't think of that post_update vs presave so that's on me.

But looks to have been addressed and tests are green.

@Wim does that mean we're locked on CKEditor 39 on 10.1.x for the forseeable future or would it be possible (and if so, desirable) to backport just the update without the configuration changes - not sure how much it is API change vs. new feature.

It is possible to backport without the ckeditor5_list plugin config changes, and instead just hardcode the default behavior in HEAD (multiBlock: true.

I did not propose that because I was under the impression that the policy is to never update JS packages unless for very strong reasons. We did have such very strong reasons for CKEditor 38.1.0 ( https://www.drupal.org/project/drupal/issues/3340578#fixed-10.1.1 🌱 [meta] [upstream] Prioritized CKEditor 5 upstream blockers Active ) and 39.0.0 ( https://www.drupal.org/project/drupal/issues/3340578#fixed-10.1.3 🌱 [meta] [upstream] Prioritized CKEditor 5 upstream blockers Active ), because each of them fixed data loss issues or upgrade-blocking-missing-functionality problems.

For this one, the impact is far less severe. And the potential risk of unknown/unadvertised breaking changes does not seem to outweigh the presence of <img height> 😅

If you want to see this backported nonetheless, I'd be happy to work on a 10.1.x-specific MR 👍

I don't strongly want to backport it, I just wondered whether it's better to leave 10.1 on 39 or 40. If there's no strong reason to update let's not, given we only have about 1 normal patch release of 10.1 left anyway.

This needs an yarn lock update.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

This made it to 11.x but not 10.2.x.

Ouch thanks. Pushed now.

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