Image2: it's not possible to link a captioned or centered image

[Alfonso Martínez de Lizarrondo]

Use ​http://ckeditor.com/demo#widgets select the image and try to link it to a full version, the typical structure of showing the scaled down version in the page and a link to the original image. When you use now the link button, the caption text is linked instead of the image.

If you start with a non-captioned image you can link the image, but if you then try to convert it to a captioned image the link is lost.

It seems that it would be better if the widget took care of handling only the figure and figcaption elements, and it included two editables: one for the image and other for the figcaption content. That way everything would be much simpler.

[Piotrek Koszuliński]

This has pretty high priority for us, but on the other hand it's not trivial to fix. Image2 would have to handle images in it - link plugin will not be able to do that and making image inside figure is not a good solution too, because it won't be possible to control this.

[Alfonso Martínez de Lizarrondo]

For me the real problem is that no one else has reported this, although it was a very obvious problem for me since I started testing image2. I even mentioned this to Fred in a recent mail exchange, but he only created the ticket about the integration with the Styles system.

Before trying to make this plugin the default image option for the next version I would suggest you to really test it thoroughly with real situations. If you want instead something to show how to use the Widgets system, you can opt to alter the existing flash and iframe plugins to use it, they seem a perfect fit for them. And while you're at it a Video and Audio plugin would be nice.

[Piotrek Koszuliński]

A ticket is just a ticket. What really matters is if someone cares about the problem. We do, we talked about possible solutions and we try to find a place in upcoming releases for this ticket.

Regarding making image2 a default plugin we didn't yet make any final decision. We know about missing features like linking and styling and we want to complete the plugin first. This is a normal situation when a new, complex feature is introduced. We could spend even more time on it before releasing it and perhaps we would add some of these missing pieces, but the sooner we have feedback, the best decisions we can make based on it. And the only good way to have feedback quickly is release a feature, perhaps incomplete, but still useful in some cases.

[Piotrek Koszuliński]

This is my proposal for image2 and link integration.

Problems:

1. Link plugin cannot handle link inside <figure>, because it's image2's territory.
2. Linked captioned image has to stay linked when caption is removed and vice versa. This has to be handled by image2.
3. We should not duplicate link feature inside image2 dialog. This would require code duplication or reusing link tabs from link dialog, but the latter is very hard, because non-widget dialogs are not reusable. Additionally, link button would have to work in any case, so it's just simpler to open link dialog.
4. In the old image dialog, link tab is very basic because it duplicates links code instead of reusing it.

Therefore:

1. Link dialog should be reused to link captioned image2.
2. Image2 does not have to create link - it has to update image2's data with link's properties. Link should be created only when downcasting image2 or removing caption.
3. By default link command will open link dialog in the old way. If data object has not been passed to the dialog, the dialog looks for <a href> element in current selection as it does now, scans it and transforms it to data object which is passed to every field on setup and commit. (FYI: Link plugin does this already! The only difference will be that we will be able to pass custom data object to link dialog.)
4. Now, image2 has to pass link data object to link dialog when opening it on captioned image2 (empty object has to be passed if image is not yet linked). This can be achieved e.g. by listening on link's command exec and altering the default action.
5. On commit, widget's data has to be updated.

What cannot be solved:

1. Applying link to selection containing more than a widget. In this case link will be applied the old way, only to nested editables. For me it's acceptable and even expected.

What has to be checked:

1. How data object can be passed to dialog. Especially that passing data object to dialog fields may become a standard, so a clean way of doing that would be reused in the future in other dialogs.
2. What worries me though, is that widgets pass widget instance to the dialog fields, so there will be already two ways to setup a dialog. Therefore, we may use the same idea as implemented in widgets for links. Instead of passing data object, link object (instance of link feature class) would be passed. A model interface would be mixed in link class (the same way as CKEDITOR.event is) adding #setData method and #data property and event. Additionally I would implement transactions (start, end methods), because currently in widgets to avoid too much #data events, we cancel them during committing dialog and fire one at the end. If this idea is implemented we'll have one, standardised way of setting up and committing dialogs.

[Olek Nowodziński]

cc

[Piotrek Koszuliński]

My findings so far:

• Use getCommand() instead of editor.commands.
• You don't need to access command again in 'exec' listener, because it equals this.
• "Don't open link dialog if double-clicked focused widget." Are you sure that this is needed? I commented it out and doubleclick was still opening image editing dialog.
• There's no link option in image2's context menu.
• We have to rethink what styles can be applied to a > img. I don't think that there may be a straight answer, because most sites don't style linked images at all on all the rest do it differently. Therefore our styles may be confusing. I think that some link icon displayed on the image could be most intuitive and we could create it by the ::after style. On the other hand, then people may start clicking it and we can't handle that. Unless... clicking a::after fires proper mouse event with target set to the link. May be worth checkign. Especially that we can have this feature only on browsers that support it properly.
• Link is not displayed in elements path when widget is focused. Clicking it should do the same as clicking a widget's element, so focus widget.
• `<p><a href="​http://foo">x<img alt="" src="assets/image1.jpg" />x</a></p> - in such case, when link is not pulled into the widget, link command (so button and keystroke) should not trigger linking the image if image is focused. Instead, normal linking mode should be chosen, which mean editing the "​http://foo" link. Exactly how it works now on major.

I'll check the 'd' branches later. Am I right that the decision regarding including them don't block the 'c' ones?

[Olek Nowodziński]

Replying to Reinmar:

I'll check the 'd' branches later. Am I right that the decision regarding including them don't block the 'c' ones?

Yes. But since they touch the same code, which, in a matter of fact had already been touched in "c"s, I recommend them as a part of this ticket. Test coverage of the link plugin is so poor that I didn't realise that many times I seriously damaged some key features. "d"s unify data format of the link plugin and bring a comprehensive test coverage:

<a>...</a> ⇒ parsing ⇒ assert parsed data ⇒ generate attributes ⇒ assert attributes ⇒ synthesise <a>...</a> from attributes and compare with the initial one

[Olek Nowodziński]

Replying to Reinmar:

My findings so far:

• Use getCommand() instead of editor.commands.

Fixed.

• You don't need to access command again in 'exec' listener, because it equals this.

Fixed.

• "Don't open link dialog if double-clicked focused widget." Are you sure that this is needed? I commented it out and doubleclick was still opening image editing dialog.

Yes. It turned out that it's necessary. Link plugin doesn't open the dialog if link is read-only — it's OK in all standard cases when <a> belongs to the widget (contenteditable=false). However in such case as

<a>foo<inline widget/>bar</a>

link is truly editable and two dialogs are open if double-clicked the widget (see: the last point).

• There's no link option in image2's context menu.

Fixed.

• We have to rethink what styles can be applied to a > img. I don't think that there may be a straight answer, because most sites don't style linked images at all on all the rest do it differently. Therefore our styles may be confusing. I think that some link icon displayed on the image could be most intuitive and we could create it by the ::after style. On the other hand, then people may start clicking it and we can't handle that. Unless... clicking a::after fires proper mouse event with target set to the link. May be worth checkign. Especially that we can have this feature only on browsers that support it properly.

Referring to our recent talks, we agreed that a simple border defined in contents.css is enough. We don't want to implement additional indicators such as link icon in the widget because it would need to be handled as a button to preserve UX. Most of the websites don't implement any special CSS to tell linked images from non-linked ones and this approach, bearing in mind that CKEditor is WYSIWYG editor, gives developers the freedom to choose. Because contents.css is just a suggestion, it's fine to put some styles there instead of i.e. hard-coded definition in Image2 plugin.

• Link is not displayed in elements path when widget is focused. Clicking it should do the same as clicking a widget's element, so focus widget.

Such implementation is quite complex both in terms of UX and code so it will be postponed:

1. While for inline images

   body > a > image
   

   makes sense, neither

   body > a > image
   

   nor

   body > image > a
   

   feel like a good path for captioned widgets:
   1. Both paths would be wrong if editing the caption, i.e.

      body > a > image > caption > a
      

      or

      body > image > a > caption > a
      

      (link inside of link?).
   2. The second one suggests that link is what is selected — is not entirely true.
2. While clicking "a" in

   body > a > image
   

   (inline image) is quite OK (selects the widget), for captioned widget it's not a perfect solution because link is a part of the widget.
3. The conclusion is that widget structure is a tree, which cannot be easily described with a linear path.
4. Perhaps

   body > image (linked)
   

   would be better?
5. Current implementation of elements path has no interface to extend the path with additional element. I would need to be implemented and tested.

• `<p><a href="​http://foo">x<img alt="" src="assets/image1.jpg" />x</a></p> - in such case, when link is not pulled into the widget, link command (so button and keystroke) should not trigger linking the image if image is focused. Instead, normal linking mode should be chosen, which mean editing the "​http://foo" link. Exactly how it works now on major.

Fixed with additional tests.

PS: Don't forget about ​branch:t/11341d (+tests). See comment:10.

[Olek Nowodziński]

I rebased ​branch:t/11341c on latest major and ​branch:t/11341d on ​branch:t/11341c (all with tests).

[Frederico Caldeira Knabben]

There are some reds on tests with IE10. Nothing to do with the code, just bad tests (or bad IE). Please fix them before majorizing.

[Olek Nowodziński]

Finally ​git:f9f0525 landed in major (9d69339 tests)! Whew.