Opened 5 years ago

Closed 4 years ago

#11341 closed Bug (fixed)

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

Reported by: Alfonso Martínez de Lizarrondo Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.4.0
Component: UI : Widgets Version: 4.3
Keywords: Cc:

Description

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.

Change History (14)

comment:1 Changed 5 years ago by Piotrek Koszuliński

Status: newconfirmed
Summary: image2: it's not possible to link a captioned imageImage2: it's not possible to link a captioned or centered image

comment:2 Changed 5 years ago by 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.

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4

comment:6 Changed 5 years ago by 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 will require code duplication or reusing link tabs from link dialog, but the latter is very hard, because non-widget dialogs are not reusable, because they don't implement model interface (like widget's data).
  4. In the old image dialog, link tab is very basic because it duplicates links code instead of reuse 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 opens link dialog in the old way. If data object has not been passed to the dialog, the dialog looks for <a href> element 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.

What has to be checked:

  1. How data object can be passed to dialog.
Version 0, edited 5 years ago by Piotrek Koszuliński (next)

comment:7 Changed 5 years ago by Olek Nowodziński

cc

comment:8 Changed 5 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:9 Changed 5 years ago by 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?

comment:10 in reply to:  9 Changed 5 years ago by 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

comment:11 in reply to:  9 Changed 5 years ago by Olek Nowodziński

Status: assignedreview

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.

comment:12 Changed 4 years ago by Olek Nowodziński

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

comment:13 Changed 4 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

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.

comment:14 Changed 4 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

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

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy