Add support for a Display Text field to the Link dialog

[Teresa Monahan]

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.

[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.

[Wiktor Walc]

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.

[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.

[Frederico Caldeira Knabben]

#8374 is DUP.

[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.

[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.

[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.

[Marek Lewandowski]

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

[Tade0]

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.

[Marek Lewandowski]

Moving tickets to the next milestone.

[Marek Lewandowski]

Taking over this issue.

[Marek Lewandowski]

Pushed for a review to ​branch:t/7154.

[Tomasz Jakut]

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.

[Marek Lewandowski]

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.

[Tomasz Jakut]

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.

[Marek Lewandowski]

Changes pushed back to the branch.

[Tomasz Jakut]

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.

[Marek Lewandowski]

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.

[Tomasz Jakut]

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.