[drupalImage] Add ckeditor5-image's imageresize plugin to allow image resizing

Assigning to @lauriii because AFAIK he already did most of this in an earlier iteration of #3222847: <img width height>. It just got descoped from there.

See #3222847-6: <img width height>.

Furthermore, what @lauriii wrote in #3222847-5: <img width height>.1 would also pretty much solve #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5. In fact, I think that would be superior to arbitrary px sizes… 🤓 🤔

Given the major UX impact this has and the considerations in the previous paragraph … I think this qualifies as at least "major".

@vlyalko started working on this over at #3247974! I asked her in #3247974-4: [drupalImage] Image resize, Image with text wrapped shown correctly on front end and not correctly shown in backend to post her code here 😊

The only commit made by @vlyalko is basically doing this:

diff --git a/ckeditor5.ckeditor5.yml b/ckeditor5.ckeditor5.yml
index f1e0f24..f6413d6 100644
--- a/ckeditor5.ckeditor5.yml
+++ b/ckeditor5.ckeditor5.yml
@@ -381,6 +381,8 @@ ckeditor5_image:
     plugins:
       - image.Image
       - image.ImageToolbar
+      - image.ImageResize
+      - image.AutoImage
       - drupalImage.DrupalImage
     config:
       image:

… which means it was very easy to port her work into a core merge request! 😊

Next step: port everything imageresize-related that was deleted in https://git.drupalcode.org/project/ckeditor5/-/merge_requests/72/diffs?c....

I don't think we need to add AutoImage here — that feels out of scope.

@lauriii, @bnjmnm and I discussed this issue. We all greed image resizing functionality should be enabled by default, but possible to disable. This matches the state of CKEditor 4, but many sites don't actually want to allow their end users to resize images arbitrarily, and use image styles instead for example (also see #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5).

Just paired with @hooroomoo and in the process of explaining how to make resizing configurable, but then realized this is a chicken-and-egg problem. :/

This is functionality that has no toolbar item, and is just available as soon as images are supported. So none of the existing condition types work. Therefore we need some new infrastructure …

ifConfigurationMatches

I just pushed a commit that introduces a new condition type: ifConfigurationMatches. This allows you to specify a condition that requires the specified configuration subset to match (exactly). Tagging Needs change record for this.

Thanks to \Drupal\ckeditor5\Plugin\Editor\CKEditor5::shouldHaveVisiblePluginSettingsForm(), this is already guaranteed to be visible in the admin UI at the right time, so that's at least one already solved chicken-and-egg problem.

With this new infrastructure, we can define:

    conditions:
      ifConfigurationMatches:
        allow_resize: true
      plugins: [ckeditor5_image]

(i.e. the ckeditor5_imageResize plugin should be enabled when the ckeditor5_image plugin is active and the configuration for the ckeditor5_imageResize contains exactly

allow_resize: true

This happens to be the default (see top of the comment). If the user unchecks the checkbox in the CKEditor 5 admin UI, the resizing functionality is disabled.

Next steps

1. Test coverage
2. configure resizeOptions: https://ckeditor.com/docs/ckeditor5/latest/api/module_image_image-ImageC...

@Wim Leers I'm having trouble understanding the structure of the \Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::providerTestInvalidPluginDefinitions() and might need an overview of it.

#18: Happy to walk you through it later today! 😊

Basically, :: providerTestInvalidPluginDefinitions () has every possible structural permutation of a CKEditor 5 plugin definition, to ensure that each mistake an implementer could make actually provides helpful error messages.

This is an atypical kind of test coverage in Drupal, but we're trying to make CKEditor 5 have a better DX than any other core module. With new territories come new patterns 🤓

And I can see that you're already well on your way with that actually! 😄👍

I'm still unable to disable the resize plugin. (I have hit the Save button) and I also get this error in the screenshot whenever I uncheck the box.

I also added resize options and I see them in the editor data but not in the UI. Does there need to be extra configuration for that?

Reproduced. I don't know why I wasn't able to reproduce this earlier … sorry about that!

Now it should work fine again :)

This refines the code that #3248177: Language toolbar item cannot be removed from the toolbar introduced. I probably originally tested this with a state where that issue had not yet been merged.

Addressing my own feedback — this is delving into the tricker aspects of the plugin system and the configuration system 🤓

@hooroomoo added tests.

I just addressed the feedback I wrote on the MR. This now definitely needs review from someone other than @hooroomoo and I.

Wrote change record: https://www.drupal.org/node/3262942.

In writing that, I suspect that requiresConfiguration would be a better name than ifConfigurationMatches?

Spotted a few tiny things in the MR.

In writing that, I suspect that requiresConfiguration would be a better name than ifConfigurationMatches?

I support changing it. configuration is potentially more consistent with the other condition names, but I also see the benefits in expanding that to requiresConfiguration since 'configuration' is a term used in many different contexts throughout this module.

I like requiresConfiguration better too. Addressed above feedback including the name change to requiresConfiguration and updated change record.

Resize option has one option of changing it back to its original size now.

Wonderful, thank you so much for that thoroughness, @hooroomoo! 😊

All my feedback is addressed. Image resizing works great, as does the "original size" option. The bulk of the issue is the requiresConfiguration constraint and the surrounding testing, which all looks good. A big helping of constraints and tests is a little more to review, but provides me assurance that CKEditor 5 will be easier to maintain long term as it catches a wide variety of potential problems and provides useful, targeted feedback.

Tests written: https://git.drupalcode.org/project/drupal/-/merge_requests/1421/diffs?co... → this should fail.

Also, I want to credit @huzooka for his instrumental help on this issue, to get FunctionalJavascript tests running on my local machine again, which is how I was able to write the test!

Thanks again! 😊

In #16 I wrote:

Thanks to \Drupal\ckeditor5\Plugin\Editor\CKEditor5::shouldHaveVisiblePluginSettingsForm(), this is already guaranteed to be visible in the admin UI at the right time, so that's at least one already solved chicken-and-egg problem.

I was wrong 🤓

https://git.drupalcode.org/project/drupal/-/merge_requests/1421/diffs?co... solves that.

While working on 225ee3db, discovered a bug in existing tests: #3264451: ImageTest::testWidth() has wrong selector, but no assertion: increases DrupalCI by 20 seconds.

All feedback addressed.

The changes since #30 (which I'm 🤦‍♂️ to have missed) address the issue identified with excellent test coverage to back it up. I did another round of full code review while I was there and spotted a few additional nits in the added tests or things I didn't catch during initial review. This seems to be in good shape now, so RTBC 😎.

In reviewing another CKE5 issue, I realized this was not yet updating ckeditor5.api.php. Fixed that. Used the same language as in the change record.

And … converted to patches, so we can test this against all 3 branches. Due to the JS changes in 10.0.x we need a separate patch for that. Differences shown in attached TXT file.

Rerolled after #3264451: ImageTest::testWidth() has wrong selector, but no assertion: increases DrupalCI by 20 seconds introduced a conflicting change. Note that we cannot yet test this against 9.3.x because #3264451 has not yet been committed to 9.3.

Published CR: https://www.drupal.org/node/3262942

I see the resizeUnit wasn't discussed when this went in, I'm hoping to change it to px over here #3348603: CKEditor 5 resizes images with % width instead of px width (the CKEditor 4 default): breaks image captions *and* is a regression.

Has anyone had luck getting image resizing to work with embedded media library items or is this issue tracked anywhere?

k-cogs

Successfully applied this patch, along with Austin's instructions and all is well 👍

https://www.drupal.org/project/drupal/issues/3359725#comment-15089658