Opened 7 years ago

Closed 16 months ago

#7154 closed New Feature (fixed)

Add support for a Display Text field to the Link dialog

Reported by: Teresa Monahan Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.5.11
Component: UI : Dialogs Version:
Keywords: IBM Cc: Damian, Satya Minnekanti, James Cunningham

Description

The current link dialog does not allow users to specify display text for the URL they are inserting. We would like to request support for a Display Text field on the link dialog so that users have this option. This would be consistent with the 'Insert Hyperlink' options in desktop editing programs such as Word, Lotus Symphony etc and has been requested by a lot of users.

Attachments (1)

7154.patch (3.6 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 7 years ago by Teresa Monahan

One further note for this requested feature is that the link dialog does not necessarily need to expose a display text field. It would be sufficient for us if an extension point was added to the existing dialog so that we can add a Display Text field ourselves.

For example, the data object in dialogs\link.js that is used to record entered values such as data.target and data.url, could also support data.text. The onOK function would need to be extended to consume this value so that the entered text value can be applied to the html for the link.

comment:2 Changed 7 years ago by Wiktor Walc

Status: newconfirmed

In Word when inserting a link there is a "Text to display" field available. If some text was initially selected, this field is filled automatically with selected text. If selected content is more complicated, "Text to display" is grayed out and <<Selection in Document>> is shown. When editing exisiting links it works in the same way.

I'd say that this is the way to go. Adding such a field will make the dialog more usable, so there is no need to keep it as a hidden feature.

comment:3 Changed 7 years ago by Teresa Monahan

It's great that you can also see value in this feature. Do you know what milestone this will be targeted for? We would love to see it in the 3.6 release if possible.

Changed 7 years ago by Garry Yao

Attachment: 7154.patch added

comment:4 Changed 7 years ago by Garry Yao

Component: GeneralUI : Dialogs
Owner: set to Garry Yao
Status: confirmedreview

comment:5 Changed 6 years ago by Frederico Caldeira Knabben

#8374 is DUP.

comment:6 Changed 6 years ago by Luis

I have applied the patch to the latest version and when you insert the link does not correctly (insert the url in the text) but when you edit and modify it if it does well.

comment:7 Changed 6 years ago by Jakub Ś

Take also this ticket #8715 into account when reviewing this feature.

Also long time ago some other feature was implemented #4612. I'm just leaving this as a note so that one could make sure that this new feature is not causing any conflicts with this old one.

comment:8 Changed 22 months ago by Marek Lewandowski

We can see if we can implement it sometime soon.

We should review if we can use code from Garry's patch or do we need to put new code.

In case of questions during my absence @j.swiderski may answer your questions, so feel free to reach out for him.

Last edited 22 months ago by Marek Lewandowski (previous) (diff)

comment:9 Changed 22 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.8

comment:10 Changed 21 months ago by Marek Lewandowski

There is a pull request sent that attempts to bring this feature.

comment:11 Changed 21 months ago by Marek Lewandowski

Status: reviewassigned

comment:12 Changed 21 months ago by Marek Lewandowski

Owner: Garry Yao deleted
Status: assignedconfirmed

comment:13 Changed 21 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:14 Changed 19 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:15 Changed 19 months ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:16 Changed 19 months ago by Tade0

Status: assignedreview

Modified the link text feature by https://github.com/ryanguill so that it conforms to the requirements described in https://github.com/ckeditor/ckeditor-dev/pull/265.

At this point I don't see a case, where the link text field would be disabled, which for me counts as a red flag.

Changed pushed to branch:t/7154.

comment:17 Changed 17 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:18 Changed 16 months ago by Marek Lewandowski

Owner: changed from Tade0 to Marek Lewandowski
Status: reviewassigned

Taking over this issue.

comment:19 Changed 16 months ago by Marek Lewandowski

Status: assignedreview

Pushed for a review to branch:t/7154.

comment:20 Changed 16 months ago by Tomasz Jakut

Status: reviewreview_failed

There are a couple of issues with the current fix.


  1. In Blink open http://tests.ckeditor.dev:1030/tests/plugins/link/manual/editlinkdisplaytext and click inside ba^r link.
  2. Click on the "Link" button. The link is whole selected.
  3. Change "Display text" to "baz".
  4. Click "OK" to close the dialog.

Expected:

Selection is not changed (the whole link is selected).

Actual:

The caret is moved just before the "baz".


  1. Click "Link" dialog.
  2. Type some URL and left "Display text" blank.
  3. Double click on newly created link and modify "Display text".
  4. Click "OK" to close the dialog.

Expected:

Link's text is changed.

Actual:

Link's text remains unchanged.


  1. Open editor with image2 plugin.
  2. Insert some image.
  3. Focus image and click "Link" button to insert link.

Expected:

"Display text" field is disabled.

Actual:

"Display text" field is enabled yet anything put there is not inserted into the editor.


http://tests.ckeditor.dev:1030/tests/plugins/link/manual/editlinkdisplaytext claims that for second test case the resulting markup should be:

<a href="http://ckeditor.com">foo </a><a href="http://example.com">baz</a>.

However the actual result is:

<p><a href="http://ckeditor.com">foo bar.</a></p>

I'm pretty sure that it's an issue with test's description, not the fix itself.

comment:21 Changed 16 months ago by Marek Lewandowski

Status: review_failedreview
  1. Fixed in git:dbcce1f87b17b3cd29fb6971b44ba1ba2b4eb440.
  2. Fixed in git:51214f0b97bf6352868121c62eae846ca16c4f09.
  3. Fixed in git:f94651ab1ab5f80635ea094f24148e6c65a272fe.

I've also marked display text field as an optional, as it gets URL if nothing is set.

I'm pretty sure that it's an issue with test's description, not the fix itself.

Absolutely right! Thanks for the catch!

Changes pushed back to branch:t/7154.

comment:22 Changed 16 months ago by Tomasz Jakut

Status: reviewreview_failed

The link is better integrated with the image widget, however there is still a small issue with it. The "Display text" field is hidden, but its container – not. The tr is still rendered and is 10px tall. Because of that, the contents in the first dialog's tab is visibly lower than in others.


I've also tested the fix with Iframe and emoticons plugins and found some very serious bug:

  1. Insert iframe/emoticon into editor.
  2. Select it.
  3. Click "Link" button.
  4. Type something into "Display Text".
  5. Add some URL.
  6. Click "OK".

Expected:

  • The "Display Text" field is hidden after opening the dialog.
  • The image/iframe is linked.

Actual:

  • The "Display Text" field is visible after opening the dialog.
  • The image/iframe is replaced by the link with the content taken from "Display Text" field.

The issue could be reproduced with nearly every non-textual content, e.g. form fields or even tables.

It would be also nice to have manual test with various widgets (image2, emoticons, iframe etc.) to test all those things.


I've also found a nice vulnerability:

  1. Open CKEditor sample.
  2. Click inside a link.
  3. Click on elements' path to select the whole link.
  4. Click "Link" button.
  5. Change "Display text" to <img src="" onerror="alert( 1 );">.

Expected:

"Display Text" is treated as a text.

Actual:

Code in "Display Text" is executed and alert is shown.


One more:

  1. Open CKEditor sample.
  2. Select "orl" in heading and apply strikethrough to it.
  3. Click on "h1" inside elements' path.
  4. Click "Link" button.
  5. Add something to the "Display Text" field (not modify, just write something at the end of current input).
  6. Type some URL.
  7. Click "OK".

Expected:

New text is just appended to the heading.

Actual:

The whole heading is replaced and the strikethrough is gone.

The same thing could be achieved by applying background to the link itself and then trying to modify link's text.

comment:23 Changed 16 months ago by Marek Lewandowski

Status: review_failedreview

Changes pushed back to the branch.

comment:24 Changed 16 months ago by Tomasz Jakut

Status: reviewreview_failed

The case with strikethrough is still not working. Probably it's the trickiest part to do (I'm not even know if it's possible with the current approach?), but I find it's pretty confusing as a user ("Hey, I styled differently every letter in my heading – why the formatting is gone?!"). It's also destroying the styles of the link itself, e.g.

  1. Add some link.
  2. Select a part of it and apply some styles to it (e.g. bold or even font color).
  3. Click "Link" button.
  4. Modify link's text.

Result:

Formatting is gone.


I still don't see even a single test with widgets.


  1. Open http://tests.ckeditor.dev:1030/tests/plugins/link/manual/editlinktextmultiline.
  2. Select the text as is described in the test.
  3. Click "Link" button.

Result:

Actually I don't know how to do it better with the current input field, but "Display text" set to eaderfo looks just like a bug…

Of course modifying that text causes the appending of the paragraph to the header.

comment:25 Changed 16 months ago by Marek Lewandowski

Status: review_failedreview
  1. I don't have a strong opinion about this one, it's possible to acomplish it branch:t/7154b but I'm not sure what's the best solution here.
  2. Isn't http://tests.ckeditor.dev:1030/tests/plugins/image2/manual/link enough?
  3. That's all we can do with it for now. It's designed to solve simplest cases.
Last edited 16 months ago by Marek Lewandowski (previous) (diff)

comment:26 Changed 16 months ago by Tomasz Jakut

Resolution: fixed
Status: reviewclosed
  1. Actually the case was different, but after F2F talk we decided to not include it now.
  2. Yes, it is. I haven't noticed that test.

Fixed with git:298ddd6.

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