Opened 10 years ago
Closed 9 years ago
#13214 closed New Feature (fixed)
[Embed] Support for pasting links to resources
Reported by: | Piotrek Koszuliński | Owned by: | Piotrek Koszuliński |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.5.0 |
Component: | General | Version: | 4.5.0 Beta |
Keywords: | Cc: |
Description
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.
Change History (30)
comment:1 Changed 10 years ago by
Status: | new → confirmed |
---|
comment:2 follow-up: 3 Changed 10 years ago by
comment:3 Changed 10 years ago by
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.
comment:4 follow-up: 5 Changed 10 years ago by
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.
comment:5 Changed 10 years ago by
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).
comment:6 Changed 10 years ago by
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
comment:7 Changed 10 years ago by
Owner: | set to Tade0 |
---|---|
Status: | confirmed → assigned |
comment:8 Changed 10 years ago by
Status: | assigned → review |
---|
comment:9 follow-up: 14 Changed 10 years ago by
Status: | review → review_failed |
---|
I rebased and pushed one commit to branch:t/13214.
The code works well, but there are few details that we can polish:
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.
- Use
- 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.
'<a data-cke-autoembed'
- I would be worried that attr without a value will surprise us on some old IE.- autoembed.js:L23 - what if there's no href (anchors case)?
- Test for double undo/redo is missing.
- 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).
- 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.
- Add the transfer type check which we discussed in comment:5 and comment:6.
comment:10 Changed 10 years ago by
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?).
comment:11 Changed 10 years ago by
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.
comment:12 Changed 10 years ago by
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.
comment:13 Changed 10 years ago by
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.
comment:14 Changed 10 years ago by
Replying to Reinmar:
I rebased and pushed one commit to branch:t/13214.
- 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 ?
comment:15 Changed 10 years ago by
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?
comment:16 Changed 10 years ago by
Status: | review_failed → review |
---|
I've done improvements according to the suggestions in comment:9.
comment:17 Changed 10 years ago by
Status: | review → assigned |
---|
This ticket should not be on review while tests are failing.
comment:18 Changed 10 years ago by
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.
comment:19 Changed 10 years ago by
Status: | assigned → review |
---|
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.
comment:20 follow-up: 21 Changed 10 years ago by
Status: | review → review_failed |
---|
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.
comment:21 Changed 10 years ago by
Status: | review_failed → review |
---|
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.
comment:22 follow-up: 23 Changed 10 years ago by
Status: | review → review_failed |
---|
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.
comment:23 Changed 10 years ago by
Status: | review_failed → review |
---|
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.
comment:24 Changed 10 years ago by
Status: | review → review_failed |
---|
Unfortunately, there are still things that need to be fixed:
- Unused variable
nextIdMock
. - Many changes in tests done after
b4529478f
are wrong. - 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.
comment:25 Changed 10 years ago by
Owner: | changed from Tade0 to Piotrek Koszuliński |
---|---|
Status: | review_failed → assigned |
comment:28 Changed 9 years ago by
Status: | review → review_passed |
---|
comment:30 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Merged into major in git:171ef31.
I think it makes most sense to implement this feature in form of a separate plugin.
editor#paste
and wrap pasted links (which are not anchors yet) with <a> elements. Then adddata-cke-autoembed="<id>"
attribute to anchor if the anchor is the only content that's in thedataValue
and its text equals itshref
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).editor#afterPaste
look for an anchor withdata-cke-autoembed
with id of the previouseditor#paste
and create an embed widget. Callwidget.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 ineditor#paste
listener.Things I'm not sure:
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 toembed,semanticembed
both will work. Second gives more power to the developer.