Follow-up to #2510380: Images cannot be linked in CKEditor

Problem/Motivation

If you allow more than the title attribute on links and try to add a link with these attributes on an image, you only get the href, all the other attributes are dropped.

Steps to reproduce:

  1. install Drupal standard
  2. download and install D8 Editor Advanced link
  3. edit the basic HTML filter (admin/config/content/formats/manage/basic_html)
  4. add the "title" and "target" attributes to the <a> allowed html tag
  5. create a page
  6. add a link on a text using title and target
  7. add a link on an image using title and target

Excepted result: both the text and the image get linked with a title and a target attribute
Current result: the text is properly linked but the image only have the href attribute

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

Links should get the same attributes on an image than on a text.

API changes

None.

Data model changes

None.

Why RC target ?

This issue is a follow-up of #2510380: Images cannot be linked in CKEditor.
While it works with the functions provided by the core. These ones are likely to be extended by some contrib module like D8 Editor Advanced link.
The thing is that the current way links are implemented on images does not allow to extend them.
As this patch only improves the one introduced by #2510380: Images cannot be linked in CKEditor it does not cause any disruption at all.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DuaelFr created an issue. See original summary.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue summary: View changes

I cannot reproduce this in the way that I expected it to:

  1. If I manually add a hreflang attribute (hreflang is already whitelisted.) to the link wrapping the image, it stays there no matter what I do: editing the link, switching to source mode and back, modifying the image, saving and editing again…
  2. If I first whitelist the target attribute also, then manually add it the the link wrapping the image, it stays there no matter what I do: editing the link, switching to source mode and back, modifying the image, saving and editing again…

So … it would seem this problem only happens with your module. Now testing with that module.

DuaelFr’s picture

Wim Leers’s picture

Status: Active » Needs review
FileSize
5.17 KB

Reproduced (with your module, which should totally work).

This is all because image2 (and therefore also drupalimage) reuses the same link plugin that's already roughly a decade old. There's a lot of legacy cruft in its data model, and a lot that makes no sense at all. Consequently, we have to do a very annoying, and especially very confusing translation between what is sane (what we need) and what the link plugin expects (which is not sane), and then because image2 expects a link data model, we have to do a lot of translations.

The drupallink plugin previously did not have to deal with that, but now it kinda does.

Wim Leers’s picture

FileSize
3.21 KB
5.19 KB

However, I think I might be able to get away with this simpler version, by simply not complying with link's data model, which AFAICT is not truly necessary. See https://dev.ckeditor.com/ticket/13885#comment:7. Awaiting CKEditor team confirmation.

This should work just as well as #4, but is much cleaner, much more maintainable, and just doesn't follow link's ancient data model.

Wim Leers’s picture

Wim Leers’s picture

Yay, the CKEditor team has officially confirmed that this is fine: http://dev.ckeditor.com/ticket/13885#comment:8.

DuaelFr’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +rc target triage

I did the manual testing according to the steps to reproduce.
While #4 patch wasn't working because of a problem in the scope of the saveCallback callback, #5 works perfectly.
You definitely rocks!

xjm’s picture

Issue tags: -rc target triage +rc target

@effulgentsia and I agreed on this being an rc target because of the contrib and user experience limitations, assuming there is no BC break. (There did not appear to be when we quickly reviewed the patch, but JavaScript). ;)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
@@ -70,14 +72,7 @@
-              var urlMatch = returnValues.attributes.href.match(urlRegex);

@@ -296,26 +291,14 @@
-      var urlMatch = href.match(urlRegex);

This means we can remove the var urlRegex = /^((?:http|https):\/\/)?(.*)$/; - i think. Yep eslint confirms it...

core/modules/ckeditor/js/plugins/drupallink/plugin.js
  271:7  error  urlRegex is defined but never used  no-unused-vars
Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.42 KB
605 bytes

Indeed, that was dead code. We forgot to remove that. Fixed.

Merely removing dead code doesn't change behavior, hence moving back to RTBC directly.

DuaelFr’s picture

You're too quick @Wim I planned to write that patch around 10am ;)
Thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5ba8a96 and pushed to 8.0.x. Thanks!

DuaelFr’s picture

Eased to see that in the release!
Thanks @Wim Leers and @alexpott :)

Wim Leers’s picture

Status: Fixed » Active

Actually, this caused a regression.

+++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
@@ -289,26 +282,14 @@
-      set['data-cke-saved-href'] = (url.indexOf('/') === 0) ? url : protocol + url;

Specifically this part did.

STR:

  1. Create a captioned image
  2. Wrap it in a link pointing to http://a
  3. Save the node
  4. Edit the link, change it to http://b
  5. Save the node
  6. Observe that the link still points to http://a!

I think this should therefore be reverted. We can fix this issue in 8.0.1. We had a fix for RC4, but clearly we missed something.

Wim Leers’s picture

Status: Active » Needs review
FileSize
5.66 KB
721 bytes

Fix.

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community

That was quick! You never stops amazing me ;)
Tested without and with the advanced link module to see if the same issue occured with other attributes and everything is now fixed!
Note: I reproduced the issue that caused the revert with and without the module too.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 19b0f88 on 8.1.x
    Revert "Issue #2608434 by Wim Leers, DuaelFr: Links on images can only...
  • alexpott committed 5ba8a96 on 8.1.x
    Issue #2608434 by Wim Leers, DuaelFr: Links on images can only have href...
  • catch committed d688a3e on 8.1.x
    Issue #2608434 by Wim Leers, DuaelFr: Links on images can only have href...

Status: Fixed » Closed (fixed)

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