[Embed] Support for pasting links to resources

[Piotrek Koszuliński]

When a link is pasted, we can try to embed it. Of course not all links can be embedded and we cannot be sure if user wishes to embed that link or just leave the link as it is. Therefore, there must be a mechanism to undo our automatic change (e.g. a double undo step).

What makes this ticket pretty hard is that embedding is asynchronous. We will need to find a good way to handle this.

[Piotrek Koszuliński]

I think it makes most sense to implement this feature in form of a separate plugin.

1. Listen on editor#paste and wrap pasted links (which are not anchors yet) with <a> elements. Then add data-cke-autoembed="<id>" attribute to anchor if the anchor is the only content that's in the dataValue and its text equals its href value. (Note: on IE and in some other cases links are automatically wrapped with <a> elements, so that's why wrapping and marking with attribute are two separate steps).
2. On editor#afterPaste look for an anchor with data-cke-autoembed with id of the previous editor#paste and create an embed widget. Call widget.loadContent and wait for success. On success replace the link with the widget. Note: do not store reference to the link - use the attribute every time, because DOM may be rebuilt in the meantime. Note2: User may copy&paste a link in the meantime and we would like to avoid having two links with the same id. This may be assured in editor#paste listener.

Things I'm not sure:

• There are two embed widgets already and they may be more in the future. What's more - there may be more than one embed plugin enabled at one time. How to decide which widget to create for a pasted link? I would try to go with a config option autoEmbed which can accept two type of values - string (widget names separate by comma) or a callback (which will be executed with URL as an argument and should return a widget name or null). In the first case the first existing widget is used (see editor.widgets.registered), so if we set it by default to embed,semanticembed both will work. Second gives more power to the developer.

[Piotr Jasiun]

Replying to Reinmar:

User may copy&paste a link in the meantime and we would like to avoid having two links with the same id. This may be assured in editor#paste listener.

If user copy&paste the link, both links should be replaced with widgets imho. And I do not see the point why we should do this in 2 requests. We could find all elements with this ID, when data are loaded, and replace them, but probably as 2 separate undo steps (or not?).

In general undo and user modifications might be tricky: what if user edit something before link was replaced with widget? It should be saved as a separate undo step, I believe.

config.autoEmbed sounds cool, but I think that it is an edge case (the case when user has multiple embed plugins and wants to use one of them as a default replacer). I think that by default the first one should be chosen so the user do not need to set additional configuration options.

[Piotrek Koszuliński]

If user copy&paste the link, both links should be replaced with widgets imho. And I do not see the point why we should do this in 2 requests. We could find all elements with this ID, when data are loaded, and replace them, but probably as 2 separate undo steps (or not?).

If user copy&paste the link it means that the link was pasted again, so it can be handled as a separate paste. Both solutions are possible, but I think that handling it separately is much easier, because a widget cannot be that easily cloned.

In general undo and user modifications might be tricky: what if user edit something before link was replaced with widget? It should be saved as a separate undo step, I believe.

IMO there need to be: snapshot before paste, snapshot after paste, snapshot before replace and snapshot after replace. That will give you the ability to undo the replace, to undo your changes and to undo pasting.

config.autoEmbed sounds cool, but I think that it is an edge case (the case when user has multiple embed plugins and wants to use one of them as a default replacer). I think that by default the first one should be chosen so the user do not need to set additional configuration options.

Of course. By default the option should be set to embed,embedsemantic. This means that you don't need to configure anything as long as you use only one of our plugins, because it will be the first that is found.

[Piotr Jasiun]

Replying to Reinmar:

If user copy&paste the link, both links should be replaced with widgets imho. And I do not see the point why we should do this in 2 requests. We could find all elements with this ID, when data are loaded, and replace them, but probably as 2 separate undo steps (or not?).

If user copy&paste the link it means that the link was pasted again, so it can be handled as a separate paste. Both solutions are possible, but I think that handling it separately is much easier, because a widget cannot be that easily cloned.

And what should happen if user undo the change (widget back to link), so will have a link and copy it? I do not think it should be replaced with the widget again. It could be pretty annoying. Internal copy&paste should not be handled, but we can not implement this without improving dataTransfer.getTransferType (#12872, #12873).

[Piotrek Koszuliński]

And what should happen if user undo the change (widget back to link), so will have a link and copy it? I do not think it should be replaced with the widget again. It could be pretty annoying. Internal copy&paste should not be handled, but we can not implement this without improving dataTransfer.getTransferType (#12872, #12873).

Good point. I think that we can handle only external and cross-editor paste despite the problems with dataTransfer.getTransferType. It will be a rare situation that someone copies a link (only – no other content) after previously reverting the auto embed. With time we'll hopefully improve getTransferType and this issue will disappear. We can also think about some other conditions, but I would start with the base.

BTW. Think about such case: paste a link, copy it, link gets replaced, undo this, paste it. On the second paste you won't know whether you should embed this link or not in both cases (I mean – regardless of how you'll mark them). Cases like this would need an additional mechanism and checking transfer type is a solution here. At least we'll have a reason to improve it :D

[Piotrek Koszuliński]

I rebased and pushed one commit to ​branch:t/13214.

The code works well, but there are few details that we can polish:

1. evt.data.dataValue.search( /^<a.*<\/a>$/ ) === 0
   • Use match() - it's shorter and more semantically adequate.
   • In RegExp dot does not match new lines. So [\s\S]+ will be a better choice because if link was not created by the autolink plugin, but natively, then it may contain new lines (e.g. around tags). Remember about a test - you can exec paste command and listen with low prior for the paste event to assert it and cancel it.
   • On IE8 the tag name may be uppercase. Add test.
2. Test pasting multiple links and links formatted in many different ways. Check also pasting anchors (<a> with ids and names). This is the most fragile aspect of this feature and must be tested properly.
3. '<a data-cke-autoembed' - I would be worried that attr without a value will surprise us on some old IE.
4. autoembed.js:L23 - what if there's no href (anchors case)?
5. Test for double undo/redo is missing.
6. In autolink plugin's tests use the mechanism I described above - exec the paste command but listen to editor#paste with low prior to assert and cancel it. This way you will not need to set data (slow) and assert the whole content (imprecise).
7. I also think that more tests for autolink would need to implemented. For example - link followed by a text or space (these are tested actually), by comma, dot, linkst that are already linked, etc. All the realistic cases in which autolink should not be applied and there's quite a lot of them.
8. Add the transfer type check which we discussed in comment:5 and comment:6.

[Tade0]

Ad 1: Can you describe a case where there is a newline character in the pasted content? I managed to produce such an input only when it was surrounded by a <pre> tag, which does not qualify for autolinking(though perhaps it should?).

[Piotrek Koszuliński]

Tade0: I don't know such case and I don't say that it exists. But it's theoretically possible that IE will turn a pasted link "​http://x" into:

<a href="http://x">
http://x
</a>

Because simply... why not? :D It's IE.

I agree that it's unlikely to happen, but if with a simple change from .+ to [\s\S]+ we can have it covered, then why not.

[Tade0]

The reason why I was asking is that in the clipboard plugin there's a paste listener with priority 1, which invokes (under special conditions and indireclty) transformPlainTextToHtml from core/tools that in turn replaces all newlines with <br/> tags.

If that doesn't get rid of all the newlines there's another listener, this time with priority 6, that invokes htmlifiedTextHtmlification, which in turn replaces all newlines with spaces.

I've solved this by giving the paste listener in the autolink plugin a priority of 5, but I'm wondering if this was wise.

[Piotrek Koszuliński]

The reason why I was asking is that in the clipboard plugin there's a paste listener with priority 1, which invokes (under special conditions and indireclty) transformPlainTextToHtml from core/tools that in turn replaces all newlines with <br/> tags.

If that doesn't get rid of all the newlines there's another listener, this time with priority 6, that invokes htmlifiedTextHtmlification, which in turn replaces all newlines with spaces.

None of these function should be executed if paste's dataValue equals something like <a>foo</a>. Please check that your test is correct - i.e. the proper source is passed to editor#paste's dataValue.

[Tade0]

Replying to Reinmar:

I rebased and pushed one commit to ​branch:t/13214.

2. Test pasting multiple links and links formatted in many different ways. Check also pasting anchors (<a> with ids and names). This is the most fragile aspect of this feature and must be tested properly.

By "pasting multiple links", do you mean something like this: ​http://dev.ckeditor.com/ticket/13214http://dev.ckeditor.com/ticket/13214 and this: ​http://dev.ckeditor.com/ticket/13214 ​http://dev.ckeditor.com/ticket/13214 ?

[Piotrek Koszuliński]

By "pasting multiple links", do you mean something like this:

I meant generally a case when more than one link is pasted. Thing about real cases like pasting a whole paragraph with multiple links inside or a text which starts and end with links. Try to think as a tester - what other cases could break your code?

[Tade0]

I've done improvements according to the suggestions in comment:9.

[Piotrek Koszuliński]

This ticket should not be on review while tests are failing.

[Piotrek Koszuliński]

I rebased ​branch:t/13214 and pushed there few additional commits.

You'll find that I added and modified many tests. I noticed today that some assumptions were wrong and I adjusted tests accordingly, but I haven't made them pass. The most important changes:

• Links with href != text should not be auto embedded. The assumption is that if user pastes a URL which can be a plain text link (which is autolinked) or a link that matches precisely this pattern: /^<a href="([^"]+)">\1</a>$/i. I say "precisely" because if the link text is different than its href it means that user pasted a linked text, not a link. So you can use precisely this regexp in autoembed.
• Like I wrote in the comment:2 - data-cke-autoembed must be an id. We must be able to recognise that this link in the DOM.
• Internally pasted links must not be auto embedded.

Please work first on making this tests green.

[Tade0]

I ensured the remaining failing tests pass and modified the first one so that it tests undo & redo. Also changed the id assignment method to CKEDITOR.tools.getNextId().

Changes pushed to ​branch:t/13214.

[Piotrek Koszuliński]

Usage of getNextId() is wrong because you don't need an id. You need a number. So either revert all that or use getNextNumber(). I would choose reverting.

Another thing - no tests for undo redo. As we talked on Friday, we need at least one test for that.

[Tade0]

Replying to Reinmar:

Usage of getNextId() is wrong because you don't need an id. You need a number. So either revert all that or use getNextNumber(). I would choose reverting.

I did not revert, because that would make it impossible to mock id assignment in the tests.

Changes pushed to ​​branch:t/13214.

[Piotrek Koszuliński]

I didn't thought about it before, but mocking getNextId()/Number() is unsafe because other features may use it. And when a non-unique number will be returned strange things may start happening.

[Tade0]

Replying to Reinmar:

I didn't thought about it before, but mocking getNextId()/Number() is unsafe because other features may use it. And when a non-unique number will be returned strange things may start happening.

I'm reverting to your solution for now then.

Changes pushed to ​​​branch:t/13214.

[Piotrek Koszuliński]

Unfortunately, there are still things that need to be fixed:

1. Unused variable nextIdMock.
2. Many changes in tests done after b4529478f are wrong.
3. The undo test replaced the first test while it would be much better if it was a separate TC.

I propose to drop all commits done after my last commit (regarding dependencies) and start with a clean sheet.

[Piotrek Koszuliński]

Pushed ​branch:t/13214b.

[Olek Nowodziński]

Related ticket #13410.

[Olek Nowodziński]

Another related issue #13413.

[Olek Nowodziński]

Merged into major in ​git:171ef31.