[drupalImage] Some Image attributes are lost in edge cases where image upcasts into inline image

1. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalImage/src/drupalimageediting.js
   @@ -158,19 +173,12 @@ function modelImageStyleToDataAttribute() {
   -      alignBlockRight: 'right',
   -      alignBlockLeft: 'left',
   

   🤔 There is no longer any mention of alignBlock* model attribute values … how is that possible?

2. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalImage/src/drupalimageediting.js
   @@ -268,8 +276,26 @@ function viewImageToModelImage(editor) {
   +    // Create image that's allowed in the given context. If the image has
   +    // caption or alignment, the image must be created as a block image to
   +    // ensure attributes are not lost on conversion. This is based on the
   +    // assumption that preserving the image attributes is more important to the
   +    // editor than preserving the wrapping element that doesn't allow block
   +    // images.
   +    if (
   +      schema.checkChild(data.modelCursor, 'imageInline') &&
   +      !hasDataAlign &&
   +      !hasDataCaption
   +    ) {
   

   +1 — this makes sense! This avoids data loss.

3. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php
   @@ -140,6 +140,43 @@ function (ConstraintViolation $v) {
   +  /**
   +   * Ensures that attributes are retained on conversion.
   +   */
   +  public function testConversion() {
   

   🤓 Nit: I think that the name is a bit off. It's not just conversion. It's ensuring that data-align and data-caption are not lost, not even for inline images.

4. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php
   @@ -140,6 +140,43 @@ function (ConstraintViolation $v) {
   +    // Run test cases in a single test to make the test run faster.
   +    $test_attributes = [
   +      'data-caption="test caption 🦙"',
   +      'data-align="left"',
   +    ];
   +
   +    foreach ($test_attributes as $test_attribute) {
   +      $img_tag = '<img ' . $test_attribute . ' alt="drupalimage test image" data-entity-type="file" data-entity-uuid="' . $this->file->uuid() . '" src="' . $this->file->createFileUrl() . '" />';
   +      $test_cases = [
   +        // Plain image tag for a baseline.
   +        $img_tag,
   +        // Image tag wrapped with <p>.
   +        "<p>$img_tag</p>",
   +        // Image tag wrapped with a paragraph like element.
   +        "<div>$img_tag</div>",
   +      ];
   

   👍 Ran locally with and without the JS changes.

   Failed without the JS changes.

   Passed with the JS changes.

Needs work only for the third point.

1. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalImage/src/drupalimageediting.js
   @@ -276,24 +276,18 @@ function viewImageToModelImage(editor) {
   +    // preserving the image caption is more important to the editor than
   

   Übernit: "editor" here could be misinterpreted as the text editor, not the person doing the editing. "content creator" would clarify that.

2. +++ b/core/modules/ckeditor5/js/ckeditor5_plugins/drupalImage/src/drupalimageediting.js
   @@ -279,39 +296,33 @@ function viewImageToModelImage(editor) {
   -      consumable.test(viewItem, { name: true, attributes: 'data-align' })
   +      consumable.test(viewItem, {
   +        name: true,
   +        attributes: 'data-align',
   +      })
   

   Nit: No-op change, let's revert this.

3. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php
   @@ -146,33 +147,53 @@ function (ConstraintViolation $v) {
   +    foreach ($test_attributes as $test_attribute => $mode) {
   

   🤔 AFAICT $mode here really means "expected upcasting by CKEditor 5 if the starting point is an inline image".

   So I think that $expected_upcast_behavior_when_wrapped_in_block_element would be a better name?

   (This took by FAR the longest amount of time to figure out!)

4. RE #8.3: what about testDynamicUpcastBehavior() or testInlineVsBlockImageUpcasting() or testAttributeRetentionDuringUpcasting()?

Here I have tried to address the points of comment #12, please review.

#15 But this only adding a fix for upcasting, in viewImageToModelImage()? If you can think of a better name, that'd be awesome, but I do think this is a net improvement. While imperfect, it better conveys why we have this test coverage at least? 😊 So … feel free to rename again and keep the "RTBC" status, but that really was my only remaining point of confusion.

Given that you have not raised concerns about the change I proposed in #12.3 nor in @ravi.shankar's implementation of it, I think that means you're +1 to that too.

So: RTBC! :)

Adding "Needs issue summary update", the solution should be summarized at least briefly.

Basically doc & naming nits. Also note the "Needs issue summary update" from 👆. The caption part can probably be lifted directly from the comments in drupalimageediting, and the align solution could use a sentence or two about how it's effectively Block/Inline agnostic now.

1. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php
   @@ -140,6 +141,63 @@ function (ConstraintViolation $v) {
   +    $test_attributes = [
   +      '-none-' => 'inline',
   +      'data-caption="test caption 🦙"' => 'block',
   +      'data-align="left"' => 'inline',
   +    ];
   +
   +    foreach ($test_attributes as $test_attribute => $expected_upcast_behavior_when_wrapped_in_block_element) {
   

   What about s/$test_attributes/$attributes_to_retain

   and

   s/$test_attribute/$attribute_to_retain

   Just a little thing that could make the test easier to follow. It's nicely put together so I think naming may help more than additional commments that may add noise.

2. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php
   @@ -140,6 +141,63 @@ function (ConstraintViolation $v) {
   +        // Image tag wrapped with a paragraph like element.
   

   It's worth specifying (perhaps more concisely than me) that this paragraph-like element is not an allowed tag, so the

   wrapping is expected when inline is the expected upcast behavior. This would mitigate any impressions that this test is inadvertently masking a bug.

Here I tried to address the points on #18

RTBC++

Thanks all!

Committed to 9.4.x and 10.0.x, leaving at RTBC for 9.3.x in case it can be added after the @ckeditor5 version difference is reconciled.

Added to #3269651: Update Drupal 9.3.x to CKEditor 5 v34.0.0 along with other un-backported issues — do we want to mark this Fixed then?

Thanks @Wim Leers! I somehow missed that #3269651: Update Drupal 9.3.x to CKEditor 5 v34.0.0 along with other un-backported issues was keeping track of the queued issues.

Fixed!

Postponing for backport after the v33 and v34 update.

v34 is in, so this is unblocked.

Yep, the backport is a pretty safe change, it's 99% the recompiled build (which tests would 100% spot problems with) and an encoding difference of the Llama emoji in a test, which can safely be different as its encoding has no impact on the test.

So, it's backported to 9.3! Glad CKEditor 5 v34 is with us!