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:
- install Drupal standard
- download and install D8 Editor Advanced link
- edit the basic HTML filter (admin/config/content/formats/manage/basic_html)
- add the "title" and "target" attributes to the
<a>
allowed html tag - create a page
- add a link on a text using title and target
- 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.
Comment | File | Size | Author |
---|---|---|---|
#16 | 2608434-16.patch | 5.66 KB | Wim Leers |
Comments
Comment #2
Wim LeersI cannot reproduce this in the way that I expected it to:
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…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.
Comment #3
DuaelFrIf you want to test without my module you can also revert #2590403: Remove "Open in new window" checkbox from EditorLinkDialog — Was: "Consider whitelisting <a>'s target attribute in the Standard install profile"
That's how we discovered the issue.
Comment #4
Wim LeersReproduced (with your module, which should totally work).
This is all because
image2
(and therefore alsodrupalimage
) reuses the samelink
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 thelink
plugin expects (which is not sane), and then becauseimage2
expects alink
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.Comment #5
Wim LeersHowever, 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.Comment #6
Wim LeersComment #7
Wim LeersYay, the CKEditor team has officially confirmed that this is fine: http://dev.ckeditor.com/ticket/13885#comment:8.
Comment #8
DuaelFrI 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!
Comment #9
xjm@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). ;)
Comment #10
alexpottThis means we can remove the
var urlRegex = /^((?:http|https):\/\/)?(.*)$/;
- i think. Yep eslint confirms it...Comment #11
Wim LeersIndeed, that was dead code. We forgot to remove that. Fixed.
Merely removing dead code doesn't change behavior, hence moving back to RTBC directly.
Comment #12
DuaelFrYou're too quick @Wim I planned to write that patch around 10am ;)
Thank you!
Comment #13
alexpottCommitted 5ba8a96 and pushed to 8.0.x. Thanks!
Comment #14
DuaelFrEased to see that in the release!
Thanks @Wim Leers and @alexpott :)
Comment #15
Wim LeersActually, this caused a regression.
Specifically this part did.
STR:
http://a
http://b
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.
Comment #16
Wim LeersFix.
Comment #17
DuaelFrThat 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.
Comment #18
catchCommitted/pushed to 8.0.x, thanks!