Images are not linkable through UI; already linked images are unlinked (data loss!)

Thanks for the write-up, @yash.rode!

I'm just making the two facets of this issue more explicit.

This is the CKEditor 5 equivalent of #2510380: Images cannot be linked in CKEditor.

The difference is that now it can result in data loss when upgrading from CKEditor 4.

I can reproduce this on https://ckeditor.com/ckeditor-5/demo/, where you can see that images are not linkable. This will therefore need an upstream feature.

In searching the CKEditor 5 GitHub repository, I stumbled upon https://github.com/ckeditor/ckeditor5/blob/9ad13ec94d77e10e4ddf678e86577..., see this live at https://ckeditor.com/docs/ckeditor5/latest/features/images/images-linkin....

It says:

The image linking feature is not enabled by default in any of the editor builds. In order to enable it, you need to load the LinkImage plugin. Read more in the installation guide.

This explains it!

Looks like simply adding the LinkImage plugin is all we need:

… but as soon as you either save this node, a JS error appears, and you'll notice that no body field value was actually saved 😔

This is also reproducible if you have Source Editing enabled: if you try to enter it, it crashes too.

Both of those crashes point to js/ckeditor5_plugins/drupalImage/src/drupalimageediting.js; I suspect there's a bug in the code there. Unfortunately I currently am unable to get any CKEditor 5 build working… some problem with node-sass, possibly due to macOS Big Sur, possibly due to random problems with node, possibly something else still.

Fixing issue summary (the "before" image was shown twice, instead of before/after)

If I change the plugin definition from

ckeditor5_image:
  ckeditor5:
    plugins:
      - image.Image
      - image.ImageToolbar
      - drupalImage.DrupalImage

to

ckeditor5_image:
  ckeditor5:
    plugins:
      - image.Image
      - image.ImageToolbar
      # - drupalImage.DrupalImage

I can see this working just fine:

Which means it's almost 100% likely this needs to be fixed in js/ckeditor5_plugins/drupalImage/src/drupalimageediting.js.

If I revert the change from #9, I get this error: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/erro...

Root cause found:

				// 2. Insert link inside the associated image.
				writer.insert( writer.createPositionAt( viewFigure, 0 ), linkElement );

in downcastImageLink() in packages/ckeditor5-link/src/linkimageediting.js in upstream CKEditor 5.

Inserting a link inside the associated image is nonsense. But … "block images" in CKEditor 5 get rendered as <figure><img></figure>. And … then it makes sense: they really mean inside the figure, around the image.

I am fairly hopeful I can override this…

Still need to port the tests, but the fix is working! 🥳

Already marking as Needs review for that.

The test failures are most likely caused by the problem @lauriii pointed out and which I confirmed locally: we should only load the LinkImage plugin when both the CKE5 link and image plugins are loaded.

This is a new kind of condition we cannot yet express.

That's why I pushed f7577ab70b4912fc1f0dd9047eeeae06d498553c just now.

Looks like #3244855: Regression in Drupal 9.3.x due to jQuery.once deprecation just got committed and unfortunately is causing a lot of irrelevant

Javascript Deprecation: jQuery.removeOnce() is deprecated in Drupal 9.3.0 and will be removed in Drupal 10.0.0. Use the core/once library instead. See https://www.drupal.org/node/3158256

deprecation errors, which in turn is causing the test failures. Which means it's now extremely hard to interpret the test results.

Before #12, d.o reports 3 failures. It's actually 2. Neither is fixed since #13. But d.o reports 6 failures. All net new failures except for one are irrelevant, and are caused by #3244855.

The analysis in #14 is wrong. See #3246605: Update filterStatus override to use @drupal/once. Tests here won't be green until that lands.

The remaining test failures are 100% identical to those in HEAD: #3233107-3: [ignore, testing issue] Fake bug report. That means we're good here.

Next up: porting \Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testLinkability().

Apparently in some scenarios unlinking does not yet quite work:

… discovered thanks to the ported test! 🥳 (Now available in 085f7c4.) So now the JS needs to be fixed so that tests actually pass 🤓

Unassigning because I will not be able to push this forward until Tuesday. So if somebody wants to continue in the meantime, go ahead!

Great, that worked!

@bnjmnm's modifications to the assertions worked, but they do reveal a change in behavior: in CKEditor 4, images could only be "block level", due to limitations in Drupal's image integration.

In CKEditor 5, the Drupal image integration inherits CKEditor 5 core's support for inline images. Which is why the selectors had to be changed by @bnjmnm, and then suddenly tests started passing. I made that more explicit in c93d0b74, by using more explicit selectors.

This also means we should now explicitly test both BLOCK and INLINE images in our test coverage. This is recognizable by .ck-widget-image vs .ck-widget.image-inline. In both cases, linkability should be available. And in both cases, the bug I demonstrated in #17 should not occur.

But we should also address @lauriii's remark that it makes no sense that we're testing image-specific stuff in the MediaTest. I tried to keep the test coverage as 1:1 to \Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testLinkability(), but at some point that obviously stops making sense.

So I'm going to tackle this in a few steps:

1. Split the image-related things out of MediaTest, into a new ImageTest
2. Also move over \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5Test::testImageCaption() into this new test class
3. First real change: add failing test coverage for #17
4. Second real change: fix this failing test
5. Third real change: add explicit test coverage for BLOCK vs INLINE

#20:

1. https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?...
2. NOT DOING because not actually in scope…
3. https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?...
4. Skipped to first do the next step: block and inline image testing …
5. https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... → in in doing so discovered that this does seems to clash with GHS:

I almost forgot: last week I noticed that the official LinkImage docs show that we can (and should!) configure the "link" button to appear in the image toolbar (balloon panel).

That makes for a much more consistent UX: you can manipulate everything about the image through the same UX.

We can now easily achieve the same, with a simple YAML addition (see https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?...):

Found the root cause for #17: GHS.

Specifically, the code in https://github.com/ckeditor/ckeditor5/commit/cc17fbe2ced81adc9c51358d903...

… which is suspiciously similar to #21, but then for block-level images.

In https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?..., I've expanded the two cases we've had so far (BLOCK and INLINE) into four: for each of the pre-existing two, testing with and without HTML restrictions.

This is why we go from 2/2 failing test cases to 3/4 test cases: INLINE image, restricted works fine. Until earlier today, that was effectively the only scenario we were testing. Now it is much clearer which scenarios are problematic.

The change I introduced in #22 makes it easier to make for consistent testing between BLOCK and INLINE, because apparently CKEditor 5's LinkImage functionality behaves slightly differently between those two when clicking the "Link" button in the main toolbar.

Simply always interacting with the image toolbar's "Link" button removes that difference.

Now only 2/4 test cases are failing; BLOCK IMAGE, restricted now also passes since https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... 👍

The last commit made things more robust and consistent, but asserting the different potential balloon panel states of CKEditor 5 quickly becomes very confusing. Let's make it more manageable.

Did that in the two commits I just pushed.

The two remaining failures are related to #3227822: [GHS] Ensure GHS works with our custom plugins, to allow adding additional attributes, linking.

To understand the remaining two failures, let's first tackle the inline one.

Copy/pasting this in the source area:

<p><a href="http://www.drupal.org/association"><img data-entity-uuid="abc878ce-4eb2-4b17-a6fc-fdca004ba525" data-entity-type="file" src="/sites/default/files/inline-images/dark_10.png"></a></p>

and then toggling off source mode, does not trigger the problem.

Not reloading the page, but just erasing the content in the source view, then typing something, followed by inserting an image (which then makes it inline), wrapping it in a link, also does not trigger the problem.

But then simply toggling source mode on and off, does not trigger the problem … because no upcasting happens. If you type anything while in source mode, THEN it is triggered.

IOW: everything points to an upcasting problem. I tracked this down to https://github.com/ckeditor/ckeditor5/issues/9916#issuecomment-900124846 as the place where it was introduced. Next step is to try to reproduce this without Drupal's additional image plugins, to see if it is reproducible then…

I've narrowed it down. I think I'll be able to finish this tomorrow morning; but now it's getting late.

I've gotten most of the way there now. I have a detailed write-up in progress. It's very late here. Expect detailed updates tomorrow morning. The commits I've pushed so far are part of proving the solution, and will help us feel far more confident! 😊

The two failures:

1. INLINE UNRESTRICTED (= GHS) is reproducible without drupalImage.DrupalImage → upstream bug. #3247634: [upstream] [drupalImage] Unlinking linked inline images while GHS is enabled: wrapping <a> is impossible to remove, CKE5 issue: https://github.com/ckeditor/ckeditor5/issues/10800
2. BLOCK UNRESTRICTED is only reproducible with drupalImage.DrupalImage. Interestingly: <a …><img …></a> gets parsed into <paragraph><imageInline></paragraph> in CKE5, and downcasts back to <p><a…><img…></a></p> … which sort of makes sense: CKEditor 5 is treating anything that has a link as inline content, even though per https://html.spec.whatwg.org/#the-a-element it is allowed, there are exceptions. CKEditor 5 just chose the simpler approach here.

   The block-level problem is a bug on our end, because

   		// To have a link element in the view, we need to attach a converter to the `linkHref` attribute.
   		// Doing this directly on `htmlLinkAttributes` will fail, as the link wrapper is not yet called at that moment.
   		function addBlockImageLinkAttributeConversion( ) {
   			dispatcher.on( 'attribute:linkHref:imageBlock', ( evt, data, conversionApi ) => {
   				if ( !conversionApi.consumable.consume( data.item, 'attribute:htmlLinkAttributes:imageBlock' ) ) {
   					return;
   				}
   
   				const containerElement = conversionApi.mapper.toViewElement( data.item );
   				const viewElement = getDescendantElement( conversionApi.writer, containerElement, 'a' );
   
   				setViewAttributes( conversionApi.writer, data.item.getAttribute( 'htmlLinkAttributes' ), viewElement );
   			}, { priority: 'low' } );
   		}
   

   in GHS' image.js crashes due to the different shape of markup that DrupalImageEditing generates: we do not downcast to block images to <figure><img …><caption>…</caption></figure>, but to <img … data-caption="…">. Consequently, we also do not generate <figure><a …><img …></a></figure> linked images, but <a …><img …></a>. This does not match with what that cited piece of code tries to do.

   So we need to expand on what I did last week, in #11 (in https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?...): we need to make it also consume htmlLinkAttributes.

   But this also revealed a bug in what I did in that commit: I am implementing it as downcast, but it should've been dataDowncast.
   downcast is the generic downcasting that is used for both the editing downcast (the HTML in the editor used for … editing) and data downcast (the HTML that ends up being returned by the editor, stored in the DB).
   The Drupal image editing plugin keeps the <figure> markup while editing, but not in the data downcast. Hence we also need to specifically tweak the htmlLinkAttributes during data downcasting.

Let's solve #30.2 step-by-step, because otherwise it'll be an impossible dump of commits to review:

1. add explicit data downcast tests, because right now we only have editing downcasts, and those are actually less important (what matters most is the data stored in the DB, and that's the data downcasts!): https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... → still the exact same test failures, meaning that the data downcast test expectations are working 👍
2. temporarily disable editing downcast tests: https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... → still the exact same test failures, meaning it's the data downcast that was the problem 👍
3. fix: https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... → the BLOCK UNRESTRICTED test is now passing, so down from 2 failures to 1 (d.o reports this incorrectly, check the results manually!) 👍
4. expand "data downcast" test coverage to also provide a class attribute on the input: https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... → in RESTRICTED cases this should be absent in the data downcast, in UNRESTRICTED cases it should be present → failing tests for all UNRESTRICTED cases meaning we've found a bug in the previous commit's fix (IOW: 2 instead of 1 failure) 👍
5. fix the logic in downcastImageLink() to incorporate the relevant piece from GHS' addBlockImageLinkAttributeConversion(): https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... → now back to 1 failure 👍
6. in manual testing I noticed that unlinking BLOCK cases (both unrestricted and restricted) causes the editing downcast to go from <a href=…><figure><img></figure></a> to <a href=…><a><figure><img></figure></a></a> — that's right, an extra link is inserted! We don't notice in tests because of the first child combinator (>) in $assert_session->elementNotExists('css', 'a[href*="//www.drupal.org/association"] > .ck-widget.' . $expected_widget_class . ' > img[src*="image-test.png"][alt="drupalimage test image"]'); — if we remove that specific bit and replace it with more robust tests assertions, tests do start failing (3/4): https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... 👍
7. The solution is simple: we're currently overriding the image link downcasting both when dataDowncasting and editingDowncasting. But Drupal is only modifying the markup generated in the dataDowncast case … so we should only run it in that case: https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... → same number of failures, but failing later 👍
8. Yay, we just failed later … but we need to not fail at all of course. Because we're now only modifying the image link dataDowncast, the editingDowncast is 100% unmodified compared to stock CKEditor 5. Rejoice! Less to maintain! 🥳 But … our editingDowncast assertions are now wrong. Because CKEditor 5 has decided to put the <a> around inline image widgets and inside block image widgets. They may have their reasons, we should not interfere, we should just assert that we're not breaking this: https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... → now down to a single failure again (INLINE UNRESTRICTED), all BLOCK cases are working perfectly now, with much stronger test coverage 👍
9. Finally, we obviously cannot make the INLINE UNRESTRICTED test case pass with correct expectations if we know it's broken upstream. So conditionally changed some assertions until #3247634: [upstream] [drupalImage] Unlinking linked inline images while GHS is enabled: wrapping <a> is impossible to remove is fixed: https://git.drupalcode.org/project/ckeditor5/-/merge_requests/142/diffs?... → GREEN!! ✅✅✅✅✅

I've reviewed the MR and all looks good. Great work, so nice to see this land this quickly!