Opened 5 years ago

Closed 4 years ago

#11801 closed Bug (fixed)

It is not possible to create a link to anchor on Image2

Reported by: Piotr Jasiun Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.4.1
Component: General Version: 4.4.0
Keywords: Cc:

Description (last modified by Piotr Jasiun)

Tested on Chrome.

  1. Open New Image sample.
  2. Select an image.
  3. Click link button.
  4. Change type of the link to "Link to anchor in the text".

Result: no anchors are displayed.

This might be related to #11800, but it's not about creating anchor from image, it's about the link. And if this function is not supported yet then "Link to anchor in the text" type should not be enabled.

Change History (8)

comment:1 Changed 5 years ago by Piotr Jasiun

Description: modified (diff)

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

cc

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

Owner: set to Olek Nowodziński
Status: newreview

It's not actually related to #11800.

Why does it work with plain links? First of all, that thing still works with plain links because the Link dialog always calls parseLinkAttributes, even if the element does not exist https://github.com/ckeditor/ckeditor-dev/blob/634bc651dd45700a95588c7591686ac2f787045a/plugins/link/dialogs/link.js#L856. As a result of this, fields are set up with data containing anchors property only — it is the only one which can be returned from parseLinkAttributes( editor, element ) when element = null. And that's fine.

Why does it fail in Image2? The root of the problem is that, while I customised dialogDef.onShow in Image2 plugin, I missed that case. My code called

this.setupContent( widget.data.link || {} );

which, in case of the image which has not been linked yet, initialises dialog fields with an empty data object, so there's no anchors property there https://github.com/ckeditor/ckeditor-dev/blob/634bc651dd45700a95588c7591686ac2f787045a/plugins/image2/plugin.js#L1374.

Long story short, I pushed a fix to branch:t/11801 (+tests) :P

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

Milestone: CKEditor 4.4.1

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

Status: reviewreview_failed

Self-criticism. anchors property should have never become a part of a link dialog data model. It's not related to a link element but to the editor contents as a whole.

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

Status: review_failedreview

Pushed branch:t/11801b (+tests). Essentially, it removes anchors from link dialog data.

From now, anchors are being discovered during setup of relevant dialog fields because that process does not concern any particular link instance. If one wishes to interfere with how anchors are discovered, CKEDITOR.plugins.link.getEditorAnchors is the right method to override.

comment:7 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_passed

R+. However, using that shared anchors variable is a little bit risky if you don't check if it has been initialized with a value. For example if fieldset's set up is not called, the following HTML field will throw error because anchors is null. Please add anchors && check before anchors.length at least in the HTML field's set up callback.

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

Resolution: fixed
Status: review_passedclosed

git:59a8141 landed in master (b4a60d7 tests).

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