Images cannot be linked in CKEditor

Is this maybe more a bug then a feature request?
In my D8beta6 i try to add the link via the source code functionality (because the icon functionality is not offered as described).
When i open the node again to edit it, the source code placed link is removed?
Is this behaviour directly related to this problem? Can someone reproduce this? If yes, should i open a new issue?

However, http://dev.ckeditor.com/ticket/11341 was fixed in 4.4.0 of CKEditor (we're now running 4.4.7). It looks like we can safely delete these lines without any effect now, but linking still does not work.

These lines were introduced in #2039163: Update CKEditor library to 4.4, which is the issue that introduced CKEditor 4.4.0, so something else must've been the reason.

Looking at that issue, I see #2039163-34: Update CKEditor library to 4.4, which says:

Linking of images goes crazy right now. However, we were polishing some stuff regarding that in 4.4.1, because it was introduced in 4.4.0, so I think that it would make sense to wait a little bit with working on that in Drupal, to avoid too frequent changes.

So it's probably only been fixed after 4.4.0, or perhaps it's not actually been fixed at all.

General +1 in any case.

Are we really going to release a product that does not allow to put links on images in 2015/2016?
Let's stop joking and consider that as a bit more important, please! :)

Cross referencing that within #2521820: Update CKEditor library to 4.5.3

Are we really going to release a product that does not allow to put links on images in 2015/2016?

In HEAD, you can already link images that are neither captioned nor aligned. But the UX for that is pretty bad, you have to have the cursor in exactly the right spot for it to work.

So technically we're only talking about making captioned/aligned images linkable.

But really, we're talking about making the UX for this great; because if the UX is not great, then we'd rather not support it now, but later. In the mean time, users that really need this can just use source mode.

I'm pretty sure it only works well for CKEditor's original image2 (which Drupal 8 core indeed extends) — and you can verify yourself that it works well by trying it at http://ckeditor.com/demo#widgets — because by default it does not output structured data, but the final output.

The only reasons for not supporting out of the box can be found in #2039163-34: Update CKEditor library to 4.4 and #2039163-35: Update CKEditor library to 4.4: there were some bugs in CKEditor that have since been fixed. However, http://dev.ckeditor.com/ticket/11341 only addressed the case of CKEditor Widgets immediately generating the final output, not with Drupal's case of generating structured content that is transformed on output by Drupal's filter system.

To figure out the best solution here, we will need help — or at least guidance — from the CKEditor team.

I stupidly forgot to mention: I did try removing all of the "disable link integration" code, but that doesn't result in working linking support and definitely not in a good UX.

Finally, note what the code hunks deleted in #10 say:

Support for that may be added by an text filter that adds a data- attribute specifically for that.

That was the thinking we had back then.

It probably would be sufficiently structured and actually even better if we'd automatically detect/downcast to <a href=""><img src="" data-caption=""></a>.

#9: I may be wrong but I tried a lot of ways to put a link on my image (not aligned and not captioned) without success. I don't understand how but even placing text before and after then selecting the text and the image then adding a link on it doesn't work well... The link only applies on the text before the image.

As you said in #11 it seems to be disabled on purpose in the code.
I don't understand why the content of the link matters from the editor point of view. Since HTML5, the only thing that cannot be in a link is an other link.

I tried to dig a bit in that plugins code to enable the drupallink on drupalimages but even forcing the button state does not work.
I have no idea about what to do now to help fixing that :/

In short: we need help from experts in this matter: the CKEditor folks. I definitely don't have time to dive deeply into this (and that is what will be necessary) before RC1. After RC1/before release, I might.

Currently (since 4.4 iirc), CKEditor's implementation of linking of the image2 is fully based on widget's data model. The link dialog sets widget.data.link property and the image2 updates the DOM accordingly. Of course it also reads link properties during upcasting, so the widget.data.link property is also available in widgets loaded from data.

The easiest solution for you is to:

1. make your link dialog discover that a drupalimage is selected,
2. if so, then set the data on it instead of creating some link in the DOM,
3. then you need to update drupalimage's downcast and upcast methods so they can produce and read the data,
4. finally, (this is optional, but highly recommended) if you use the same data property as our plugin use (I mean - data.link), the DOM inside editor will be adjusted by the image2; that's recommended because it allows to style such linked image.

@Reinmar thank you for your help.

It may be a stupid question but why can't we just have a link around our image? It seems to be the way it's done in the widgets examples linked by @Wim Leers in #9. It works on simple and enhanced images.

It may be a stupid question but why can't we just have a link around our image? It seems to be the way it's done in the widgets examples linked by @Wim Leers in #9. It works on simple and enhanced images.

That's a good question :).

Wrapping the whole image2 with a link would work only with inline (non-captioned) images. An image with a caption is wrapped with a <figure> and usually you want to link only the <img> part. Besides, while in HTML5 it is valid to wrap a block with the <a> element, CKEditor 4 does not support so called transparent elements. Its parser would try to fix that, so the link with be moved inside <figure> anyway. Plus, there's one more case if I remember correctly - centered non-captioned images which need to use additional <p> (at least inside CKEditor, because Drupal's plugin produce different output). IIRC, that <p> is a part of widget, so again, link would need to be added around that to not violate widget internals. Hence, only the image2 itself can handle creating links.

I just checked what Drupal do with centered images and it does not wrap it in a <p> so it should be easier ;)

FWIW I think we should try yo reproduce the way CKEditor handle links on images by default:

• on non-captioned images it just puts the <a> tag around the selection (which can also include some text before and after the image)
• on captioned images
  • if the image is selected without anything else it puts the link around the <img> inside the <figure>
  • if the leading and/or trailing text is also selected it puts a link on the text before, an other on the text after and an other in the caption

Do you think it's doable? Where should we start?
At the moment, we are totally unable to add a link because the drupallink widget is disabled but we cannot find where :/

I just checked what Drupal do with centered images and it does not wrap it in a

so it should be easier ;)

Also inside CKEditor?

CKEditor does not allow to center images by default but that's not really important as the way we align images in Drupal is just by adding them a CSS class.
Sometimes, I don't really know when, adding an image at the middle of a text splits it into paragraphs and the image itself is not always in one of them.

I spent almost two hours trying to figure out how to make that work as it's done in ckeditor (ie. without adding strange attributes on the image that are converted to a link at a moment).
That patch is not fully working yet, the link button is now enabled when an image is selected but adding a link will place it before the image.

I think that no one wants to be ashamed by the fact that Drupal 8 ships without the ability to put links on images so tagging as Release blocker.

This is not a release blocker; only critical issues block release and this isn't one.

However, this could be considered a usability issue because the text editor does not work like people expect.

Also tagging for project manager review, to get a sense of whether we should treat this as a bug (because I know the image embedding came up in UX testing, and the current behavior seems weird/broken), a feature request (because it doesn't work in HEAD and it'd be adding "new" functionality), or something in between.

Based on remarks from @webchick, let's go with "bug".

I pinged @reinmarpl from the CKEditor team for a review.

I'm not sure you noticed that I said in #21 that the patch was NOT working :/

I'm not a programmer: I can only say: This is the most basic thing our editors are missing in our beta installations ...
Maybe it's not a "Release blocker" but it's a "90% release blocker" ;)

@DuaelFr: I indeed missed that :(

@sachbearbeiter: I hear you! But this is something we can easily add in the future; we've all been focused hard on working the things that we cannot fix in the future. As you can read in #14 and #16, this is not trivial. Maybe we can fix this during RC, maybe we can add this in 8.0.1, but we certainly can and should fix this in 8.1. Tagging minor version target because of that. But RC1 will be tagged later today, and this definitely will not be fixed before then.

This will get fixed, but given that very few people have complained about this so far, I don't think it's a 90% feature, more a 20% feature. It sucks that we don't have it. But that's always the case for some things, some where, in every piece of software.

Let's wait people actually use D8 to see complaints about that.
Currently, we are only power users that don't really care about that but once the mass (without affront) is going to try it, thay are going to silently hate us (silently because most of them don't care about the issue queue).
I agree that's a good minor version target, though, and I hope we are going to be able to make it for 8.0.1 or 8.0.2.
*crossing fingers*

This is not a blocker. At-least for users who have little bit of HTML knowledge. Once you apply #21 , you can check the source by clicking source button of the editor and edit the code by placing the '/a' tag at the end of img tag.
And I hope it gets fixed in next release.

@DuaelFr: note that "minor version target" means 8.x.0, not 8.0.x. 8.0.x is a patch version.

@Wim Leers really? Shit we are dead.

Hi mates it's 2015 calling! I'm quite finished but what have you done? Not able to link images? Lol u kidding right?

This may be super important to you, and I understand that. But it's not a 99% use case. So I'm not worried that Drupal 8 does not yet support it. We can easily add it.

In fact, we know there is a lot of refinement work left to be done wrt the WYSIWYG. This is a great small feature to add.

P.S.: it for example took Medium >2 years to support it: https://medium.com/the-story/image-links-9e4c14bf9da3#.u8oav7w1a. I think it's much more important for us to nail the UX than to ship with a broken one from the beginning. And as @Reinmar has explained, this is not a trivial problem. (Which is surprising, I know…)

Medium never supported it before. We do, for years!
It's not a trivial problem because we decided to totally rewrite the CKEditor plugins that are working perfectly.
That's not a problem for me. I don't have any serious site where I write content. That's just about my clients that do ask me why such a regression exists (that's what it is from their point of view).
I'm not upset (even if I seem upset), I'm just really sad to be lacking time to help with that. I really think we did the wrong choice when we decided to overwrite the image2 plugin this way instead of creating our own one.
Anyway, let's move to something less annoying.

Hello,

I'm new in the forum and this is my first post but I couldn't see this issue dropped and no react. We are working on a corporate website that will be maintained by a client. Do I have to tell my client that he have to learn html to put a link on their images because he can't doing this in the CKEditor ? Sounds to be kind of incredible in 2015/2016.

Are we really going to release a product that doesn't allow to put links on images ? Like "sachbearbeiter" said, this is really the "most basic thing our editors are missing".

Please think about the editorial managers.

I find it sad too. But software is never finished nor perfect, and when there are bigger priorities to deal with first, then things like this one indeed remain unsolved for too long. That's unfortunate.

It's not a trivial problem because we decided to totally rewrite the CKEditor plugins that are working perfectly.

This is a vast exaggeration.

1. We didn't rewrite CKEditor's plugins. We extend CKEditor's image2 plugin.
2. CKEditor's plugins generate the final markup. Which means you're then locked in to that markup. That violates Drupal's filter system principles, which dictate that we should store structured content, that can be transformed on output using the filter system. So that the same content can survive redesigns without needing to be cleaned manually.

For the record: I think it'd be much nicer to not have <img data-link="URL" data-caption="llama" />, but just have <a href="URL"><img data-caption="llama"></a>, because nobody will actually want to implement this differently.

This is also what I said in #11.

One thing I haven't seen mentioned yet, is the need to update FilterCaption also then, because we want the image inside the <figure> tag to be clickable, not the entire <figure>. Making the entire <figure> clickable would break horribly, since the caption itself may also contain a link.

I think we may want to consider doing that already, in a separate issue, because that's completely independent of the JS problem here. And it also made me realize that you currently cannot even wrap a captioned image in <a href></a> manually, because it would break in the aforementioned way!

As a link can carry a lot more than just an href attribute (title, target, hreflang, class, etc.), we should not just rely on a data attribute on the img tag to generate the link.

I explained the way links on images (captioned or not) are built by the CKEditor core in #17. I think they thought about that for years so we can trust them. Isn't "Getting off the island" our new way to think? ;)

As a link can carry a lot more than just an href attribute (title, target, hreflang, class, etc.), we should not just rely on a data attribute on the img tag to generate the link.

That's another good reason for not doing data-link :)

I explained the way links on images (captioned or not) are built by the CKEditor core in #17. I think they thought about that for years so we can trust them. Isn't "Getting off the island" our new way to think? ;)

Actually, it is CKEditor lead developer @Reinmar who wrote Drupal's drupalimage and drupalimagecaption plugins. ;)

I was asked to chime in here; from a product management point of view (which granted, is totally ignorant of the background details), this is clearly a bug. A content author has no knowledge of the underlying implementation of the WYSIWYG editor, nor should she have to. She simply expects to use CKEditor in Drupal the same way she can use TinyMCE in WordPress and the WYSIWYG toolbar in the Google Docs interface. There is no such limitation around creating image links in those, therefore the limitation here will be seen as Drupal being broken. And broken for something that is at least a 60% use case. The only workaround is a content author learning HTML, which is not much of a workaround at all. (Though, sadly, not a regression from D7 core. :P)

This issue seems a bit heated (would be great if we could turn down the temp on the comments here) so not touching the status for now, but that's my 3 cents.

Sorry for my total lack of knowledge about how to say the things in the best way. Not being a native english speaker is not easy in that kind of situation. We talked with @Wim Leers on IRC and I think (and hope) we are cool :)
Thank you @webchick for your intervention.

What Piotrek tried to suggest you is to put link information object to data.link property of the widget. Doing so will trigger image2 bultin logic to handle most transformations (creating an a element, proper positioning depending on a link type, creation/managing widget.parts.link). That's actually a big deal.

Since you have CKE at 4.4+ you can actually rely on these image2 plugin features.

You can trigger all this logic just by calling:

// e.g.: var widget = CKEDITOR.instances['edit-body-0-value'].widgets.instances[ 0 ];
widget.setData('link', {
	type: 'url',
	url: {
	  protocol: 'http://',
	  url: returnValues.attributes.href
	}
});

The problem that I still see that there are few places in base code that depends on link plugin to be available. Namely these are dependencies on CKEDITOR.plugins.link.parseLinkAttributes, CKEDITOR.plugins.link.getLinkAttributes and editor.plugins.link.

In my PoC I've included it I've simply copy'n'pasted missing fragments of API into drupalimage plugin, however this is not an elegant solution and I think we can do something within image2 plugin to avoid/reduce these dependencies. I'll let you know at beginning of the next week.

You also need to provide some integration with your customized link commands, that is drupalunlink. Actually image2 plugin have this part well extracted, and it's easy to incorporate to your code.

Also another part that needs some attention are widget upcast and downcast methods. Due to your custom needs to e.g. have an img without wrapping block, generic image2 won't play well with this concept so this is why you have these methods customized. Hence now you also need to modify these methods to consider a element in structure.

I think I've covered most if not all implementation details. You can see my example implementation at my playground branch.

#44: MANY THANKS!

(For those following along, @mlewand is the new maintainer for CKEditor 4.x, taking the helm from @Reinmar, who's now the lead on CKEditor 5.x. That's why his comment is so incredibly insightful: he's a CKEditor core developer :))

This is partially working. The most notable thing that is not yet working, is when you the link button (or pressing CMD/CTRL+K) to open the link dialog: the dialog then doesn't show the current link.

But this at least gets us very much on the way, so *thank* you so much! This just became 10 times more feasible to do :)

Also, I want to stress that @mlewand stayed up until the middle of the night to help get us on the way. He indeed got the vast majority of things working. So we should all say thanks to him!

And here is @mlewand's work (from the playground branch he linked to) in patch form. Uploading on his behalf — he's used to a GitHub workflow, which is why he didn't post a patch.

Yes actually I was aware of the fact that editing link does not work, as it's more of guideline/proof of concept rather than complete solution. You also might want to check context menu integration.

As for editing links you need to adjust your dialog to implement logic from extra dialogDefinition listener. Obviously I couldn't use this code, as it's specific to CKE dialog system, you guys need to adapt it.

I'm working my way through the patch, simplifying things.

The first thing that stood out was this bit, which deals with CKEditor's "advanced" tab in the link plugin. We don't have any of that. I double-checked with @mlewand, and he confirmed we could remove that part.

A huge amount of the code that @mlewand copied is actually for CKEditor's much more advanced link plugin. @mlewand told me that they're actually already working on removing the tight coupling between the image2 plugin and the link plugin. That should allow us to remove this altogether. Slated for CKEditor 4.5.5. Seems like a good reason to postpone #2581291: Update CKEditor library to 4.5.4 until 4.5.5.

For example: it supports e-mail protection in mailto: links (for which we'd use a filter in Drupal).

This allowed me to cut the patch size in half :)

One big bug in the patches above is that if you enter the url http://example.com, you'll actually end up with <a href="http://http://example.com">.

This fixes that.

Final drupallink clean-up.

Now onto the other parts. There are two remaining bugs that I know of:

1. An image that is neither aligned nor captioned gets not just an <a href>, but <a data-entity-uuid href>. (Images that are either aligned or captioned, or both, work fine.)
2. As mentioned in #45: The most notable thing that is not yet working, is when you the link button (or pressing CMD/CTRL+K) to open the link dialog: the dialog then doesn't show the current link.

I created a ticket on CKEditor bug tracker as a followup of https://www.drupal.org/node/2510380#comment-10508182.

Thanks! Updated the patch to point to that.

CKEditor Team++++++++

Addressed #52.2: you can now edit existing image links :)

Addressed #52.1: no more <a data-entity-uuid href> for unaligned, uncaptioned images, just <a href>.

(Turns out this was caused by a bug that #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored introduced.)

This is now all working :) Please test!

And now all cleaned up, everything documented, and passes our eslint rules.

Next, and as a final step, what I said in #39:

One thing I haven't seen mentioned yet, is the need to update FilterCaption also then, because we want the image inside the <figure> tag to be clickable, not the entire <figure>. Making the entire <figure> clickable would break horribly, since the caption itself may also contain a link.

I think we may want to consider doing that already, in a separate issue, because that's completely independent of the JS problem here. And it also made me realize that you currently cannot even wrap a captioned image in <a href></a> manually, because it would break in the aforementioned way!

Just to make sure, I verified my prediction. I started with this:

<p>Plain image</p>
<p><img alt="Hello world." data-entity-type="file" data-entity-uuid="6f47ff25-97ad-4134-b000-e8ee4e88d3c8" src="/sites/default/files/inline-images/search.jpg" /></p>

<p>Aligned</p>
<img alt="Hello world." data-align="center" data-entity-type="file" data-entity-uuid="6f47ff25-97ad-4134-b000-e8ee4e88d3c8" src="/sites/default/files/inline-images/search.jpg" />

<p>Captioned</p>
<img alt="Hello world." data-caption="Hello world." data-entity-type="file" data-entity-uuid="6f47ff25-97ad-4134-b000-e8ee4e88d3c8" src="/sites/default/files/inline-images/search.jpg" />

<p>Aligned &amp; Captioned</p>
<img alt="Hello world." data-align="center" data-caption="Hello world." data-entity-type="file" data-entity-uuid="6f47ff25-97ad-4134-b000-e8ee4e88d3c8" src="/sites/default/files/inline-images/search.jpg" />

Then I made all of them link to http://test, and the resulting markup is exactly what you would expect:

<p>Plain image</p>
<p><a href="http://test"><img alt="Hello world." data-entity-type="file" data-entity-uuid="6f47ff25-97ad-4134-b000-e8ee4e88d3c8" src="/sites/default/files/inline-images/search.jpg" /></a></p>

<p>Aligned</p>
<a href="http://test"><img alt="Hello world." data-align="center" data-entity-type="file" data-entity-uuid="6f47ff25-97ad-4134-b000-e8ee4e88d3c8" src="/sites/default/files/inline-images/search.jpg" /></a>

<p>Captioned</p>
<a href="http://test"><img alt="Hello world." data-caption="Hello world." data-entity-type="file" data-entity-uuid="6f47ff25-97ad-4134-b000-e8ee4e88d3c8" src="/sites/default/files/inline-images/search.jpg" /></a>

<p>Aligned &amp; Captioned</p>
<a href="http://test"><img alt="Hello world." data-align="center" data-caption="Hello world." data-entity-type="file" data-entity-uuid="6f47ff25-97ad-4134-b000-e8ee4e88d3c8" src="/sites/default/files/inline-images/search.jpg" /></a>

So far so good. And then now the actual test: what does this look like once the filter system has processed it? It looks like this, and as you can see, the caption text is colored blue, meaning that they're just part of a link, which is precisely what I predicted.

That part still needs to be addressed. But that's trivial to address. We can easily provide solid test coverage for that. And it could in principle be done in a separate issue.

Before working on that, I'd like confirmation from both the Drupal side (I'm thinking about @DuaelFr and perhaps @nod_) and the CKEditor side (@mlewand, @Reinmar, @oleq) that this patch makes sense.

Yes @wim-leers you're totally right, link must should not wrap the caption.

(Updating issue credit.)

Marking as a bug again and as an rc target since we agreed we want to get this fix in during RC if possible. Thanks @Wim Leers and CKEditor team for your awesome work figuring this issue out!

I don't want to pollute the issue but... thanks @Wim Leers, @DuaelFr and CKEditor team (Kudos @mlewand).

Btw, I tested the #57 patch and it works. I could successfully add and remove link on an image.

I still found one little bug, since the target attribute don't seems to be set when used on an image (but works on a text)
@Duaelfr points me this issue https://www.drupal.org/node/2590403 that makes my remark not valid.

I manually tested all the cases.
The only issue I've seen is when you create an image, then you put a link on it, then you edit the image and enable the caption. In this particular case, the link surrounds the entire figure tag.
All the other cases perfectly worked.

Thank you very much @Wim Leers and @mlewand (and all the others) :)

Thanks! I'll work on fixing that last case.

And I'll also work on updating the caption filter.

Keeping at NR to get feedback from the CKEditor team.

I misread #63, #63 was just confirming the problem explained in #58, right?

This fixes that, and adds the necessary test coverage.

Given that, I think this patch should be RTBC'able :)

@Wim Leers: We can say that.
During my manual tests I was able to put a link on the image OR the caption of a captioned image properly.
My issue only happened when converting an already linked non-captioned image to a captioned image.

Before #65:

<a href="http://test.com">
  <figure role="group" class="caption caption-img">
    <img alt="" data-entity-type="file" data-entity-uuid="4ed7d878-4638-45b9-8332-50d62ce4c2a3" height="208" src="/sites/default/files/inline-images/hc_ec.jpg" width="166">
    <figcaption>My caption</figcaption>
  </figure>
</a>

After #65:

<figure role="group" class="caption caption-img">
  <a href="http://test.com">
    <img alt="" data-entity-type="file" data-entity-uuid="4ed7d878-4638-45b9-8332-50d62ce4c2a3" height="208" src="/sites/default/files/inline-images/hc_ec.jpg" width="166">
  </a>
  <figcaption>My caption</figcaption>
</figure>

Bonus: it's made by the filter so I didn't need to resave the node.

Yep, the examples in #67 are exactly what the interdiff in #65 fixed. Note that those examples are fragments from the filtered output (i.e. not what is stored in the DB).

I would like final confirmation from somebody on the CKEditor team also.

Testbot was aborted for some reason; the patch actually did come back green. So back to RTBC per #67.

But again:

I would like final confirmation from somebody on the CKEditor team also.

#71: that totally makes sense! Done.

Updated IS.

Let's reflect better that this is blocked on review from the CKEditor team.

Good news! @oleq notified me that he has a proposed solution ready for testing at http://dev.ckeditor.com/ticket/13885#comment:5. I worked on updating the above patch to use the new API that his patch adds. And it indeed allows us to simplify the code here, *and* not pretend the CKEditor link plugin is present.

Note this is a future interdiff, that we cannot use until we update to CKEditor 4.5.5 (see #2321583: Update CKEditor library to 4.5.5) and until that fix is actually committed to CKEditor 4.5.5.

So, hurray, this will become more maintainable with CKEditor 4.5.5!

Cross-posted the previous comment to the corresponding CKEditor ticket: http://dev.ckeditor.com/ticket/13885#comment:6.

Self-review in the mean time, to fix nitpicks.

1. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -176,6 +173,17 @@
   +          // Update data.link object with attributes if the link has been discovered.
   

   80 cols violation.

2. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -233,25 +241,85 @@
   +    // Nothing to integrate with if link is not loaded.
   

   s/link/the drupallink plugin/

3. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -233,25 +241,85 @@
   +    // Override default behaviour of unlink command.
   ...
   +      // Override unlink only when link truly belongs to the widget.
   ...
   +    // Override default refresh of unlink command.
   

   s/unlink/'drupalunlink'/

4. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -233,25 +241,85 @@
   +      // If wrapped inline widget in a link, let default unlink work (#11814).
   

   This is a CKEditor ticket reference. We should make it a proper link, since this is the Drupal codebase.

5. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -233,25 +241,85 @@
   +  // Exposed some API just for convenience.
   

   This language should be cleaned up.

6. +++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
   @@ -31,6 +31,7 @@
   +          var focusedWidget = CKEDITOR.plugins.drupalimage.getFocusedWidget(editor);
   
   @@ -56,9 +57,29 @@
   +          // Or, if a widget is focused, we're editing a link wrapping a widget.
   ...
   +            // If a widget is focused, we're notediting an independent link, but
   +            // we're wrapping a widget in a link. For example: a linked image.
   

   This is not just *a* focused widget, it's a focused *image* widget!

   s/notediting/not editing/

7. +++ b/core/modules/filter/src/Plugin/Filter/FilterCaption.php
   @@ -78,7 +80,7 @@ public function process($text, $langcode) {
   +        // Finally, replace the original node with the new node!
   

   We don't use exclamation points in docs.

Finally, all manual testing so far (#58, #67) only mentioned block-level images, not inline images.

So I tested that as well.

Start

<p>This is Guust Flater&nbsp;<img alt="asdfasdf" data-entity-type="file" data-entity-uuid="ccc89402-d2b3-41e9-9600-adf7b9266add" height="10" src="/sites/default/files/inline-images/search_1.jpg" width="50" />&nbsp;! Or&nbsp;"Gaston Lagaffe" in French.</p>

After applying a link

<p>This is Guust Flater&nbsp;<a href="http://sdfsd"><img alt="asdfasdf" data-entity-type="file" data-entity-uuid="ccc89402-d2b3-41e9-9600-adf7b9266add" height="10" src="/sites/default/files/inline-images/search_1.jpg" width="50" /></a>&nbsp;! Or&nbsp;"Gaston Lagaffe" in French.</p>

The proposed code looks OK, just 2 minor changes pushed to imageLinking_review branch.

Also added it for you as a patch.

#79 reviewed #72, which didn't yet include my clean-up in #77.

Only one of the changes is therefore still relevant. This interdiff is relative to #77 and reflects that one change made by #79.

RTBC per #67 (Drupal community manual testing), #71 (Drupal community CKEditor expert review) and #79 (CKEditor developer review).

+++ b/core/modules/filter/src/Plugin/Filter/FilterCaption.php
@@ -57,13 +57,15 @@ public function process($text, $langcode) {
+        $img_node = $node;
...
           // We pass the unsanitized string because this is a text format
...
-          '#tag' => $node->tagName,
+          '#tag' => $img_node->tagName,

Why not just save the tagname here - rather then duplicating the objects?

Good call!

Committed 7e458b4 and pushed to 8.0.x. Thanks!

Wouhou! \o/
Huge thanks to every one that helped here!

@mpdonadio noticed I'd included #2189345: run-tests.sh should exit with a failure code if any tests failed by accident. Thanks!

heh :P

Thanks to all people that where involved: You are great ;)