Opened 6 years ago

Closed 4 weeks ago

#7154 closed New Feature (fixed)

Add support for a Display Text field to the Link dialog

Reported by: tmonahan Owned by: m.lewandowski
Priority: Normal Milestone: CKEditor 4.5.11
Component: UI : Dialogs Version:
Keywords: IBM Cc: damo, satya, jamescun

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 5 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by tmonahan

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 6 years ago by wwalc

  • Status changed from new to confirmed

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 6 years ago by tmonahan

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 5 years ago by garry.yao

comment:4 Changed 5 years ago by garry.yao

  • Component changed from General to UI : Dialogs
  • Owner set to garry.yao
  • Status changed from confirmed to review

comment:5 Changed 5 years ago by fredck

#8374 is DUP.

comment:6 Changed 5 years ago by luisete.serrano

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 5 years ago by j.swiderski

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 7 months ago by m.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 7 months ago by m.lewandowski (previous) (diff)

comment:9 Changed 7 months ago by m.lewandowski

  • Milestone set to CKEditor 4.5.8

comment:10 Changed 6 months ago by m.lewandowski

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

comment:11 Changed 6 months ago by m.lewandowski

  • Status changed from review to assigned

comment:12 Changed 6 months ago by m.lewandowski

  • Owner garry.yao deleted
  • Status changed from assigned to confirmed

comment:13 Changed 6 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.8 to CKEditor 4.5.9

comment:14 Changed 5 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.9 to CKEditor 4.5.10

comment:15 Changed 4 months ago by Tade0

  • Owner set to Tade0
  • Status changed from confirmed to assigned

comment:16 Changed 4 months ago by Tade0

  • Status changed from assigned to review

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 3 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.10 to CKEditor 4.5.11

Moving tickets to the next milestone.

comment:18 Changed 5 weeks ago by m.lewandowski

  • Owner changed from Tade0 to m.lewandowski
  • Status changed from review to assigned

Taking over this issue.

comment:19 Changed 5 weeks ago by m.lewandowski

  • Status changed from assigned to review

Pushed for a review to branch:t/7154.

comment:20 Changed 5 weeks ago by t.jakut

  • Status changed from review to review_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 4 weeks ago by m.lewandowski

  • Status changed from review_failed to review
  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 4 weeks ago by t.jakut

  • Status changed from review to review_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 4 weeks ago by m.lewandowski

  • Status changed from review_failed to review

Changes pushed back to the branch.

comment:24 Changed 4 weeks ago by t.jakut

  • Status changed from review to review_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 4 weeks ago by m.lewandowski

  • Status changed from review_failed to review
  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 4 weeks ago by m.lewandowski (previous) (diff)

comment:26 Changed 4 weeks ago by t.jakut

  • Resolution set to fixed
  • Status changed from review to closed
  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 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy