Opened 7 years ago

Closed 7 years ago

#6641 closed Bug (fixed)

Copy and pasting a newly created link breaks

Reported by: joolee Owned by: Sa'ar Zac Elias
Priority: Normal Milestone: CKEditor 3.5.1
Component: General Version: 3.0
Keywords: Oracle Cc:

Description

To recreate the problem:

  1. enter new text.
  2. select the text and create a link.
  3. copy the link and paste it on another part of the editor.

Result: href gets stripped out.

Browser: FF 3.6.12 OS: Mac OS X 10.6.4

Attachments (3)

6641.patch (389 bytes) - added by Garry Yao 7 years ago.
6641_2.patch (2.5 KB) - added by Garry Yao 7 years ago.
6641_3.patch (3.3 KB) - added by Sa'ar Zac Elias 7 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by Garry Yao

Milestone: CKEditor 3.5.1
Status: newconfirmed
Version: 3.4.23.0

Link href was initially done as "javascript:void(0)/*323*/" for unknown reason, which cause it been converted into a text node after then.

Changed 7 years ago by Garry Yao

Attachment: 6641.patch added

comment:2 Changed 7 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

comment:3 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Status: reviewreview_failed

I think that the idea about using such special href was to avoid problems with the browsers and this change might cause some unexpected problems.

I would review if it's still needed the

			var attributes = { href : 'javascript:void(0)/*' + CKEDITOR.tools.getNextNumber() + '*/' },
				removeAttributes = [],
				data = { href : attributes.href },

Because these patch seems to override that part, and in

					for ( i = 0 ; i < links.length ; i++ )
					{
						if ( links[i].href == attributes.href )
						{
							links[i].id = data.adv.advId;
							break;
						}
					}

we could change the "links[i].href == attributes.href" part to compare the _cke_saved_href instead.

comment:4 Changed 7 years ago by Garry Yao

I've traced back to the root ticket of that though still find no context of it (sad there's no comment of it), do you mean the original hack was for the 'id' application?

The problem is that browser has no knowledge of "_cke_saved_href " so with a real "href" the clipboard link still not functioning.

comment:5 Changed 7 years ago by Alfonso Martínez de Lizarrondo

I'm not sure about why it's done currently that way, I remember that it FCKeditor something similar was done in order to call the browsers "createLink" command and then get back all the created links.

And back to the patch:

As I said, if we're gonna set the real href, I don't see a reason to initiate the href property in those two objects with fake data, it can be simplified.

And just in case the browser modifies the href under certain conditions (IE6, some firefoxes, relative/absolute paths, ...) if we change the second check so it doesn't use href but instead a property that they won't touch, then we don't have a risk of a regression.

comment:6 Changed 7 years ago by George Vilches

Just an FYI, the patch as currently described applied to 3.5.x trunk does seem to help Mac OSX 10.6.5 Firefox 3.6, but breaks Mac OS X 10.6.5 Safari 5.0.3. The resulting source after the paste is:

This is where <a href="http://www.google.com">a link</a> goes.<a href="undefined">a link</a>

The second link is just a paste of the first right after creating it, then going to View Source.

For a point of interest, the current release version has a similar problem in Safari. Using http://ckeditor.com/demo:

<p>
	This is where <a href="http://www.google.com">a link</a> goes.<a href="">a link</a></p>

The second link was just the immediate paste of the first one. This definitely seems like this is related to the existing bug, even though the resulting output is a touch different.

comment:7 Changed 7 years ago by Wiktor Walc

Keywords: Oracle added

Changed 7 years ago by Garry Yao

Attachment: 6641_2.patch added

comment:8 Changed 7 years ago by Garry Yao

Status: review_failedreview

I now understand that the original purpose of having a unique (faked) href is to make sure the adv:id application successful (to not considering any existed link of the same href), but this should be done instead inside the styles plugin.

comment:9 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • Copying and pasting a link results in ID duplication.
  • The fix must be ported to the image dialog (link tab) as well.

comment:10 Changed 7 years ago by Sa'ar Zac Elias

Another TC:

  • Create a link with url 'test'.
  • Right click on the link -> link properties.
  • Change the URL to 'example' and click OK.
  • Copy the link and paste it somewhere in the editor.
  • Right click on the new link -> link properties.

Expected: URL field should show "example". Actual: URL field shows "test".

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6641_3.patch added

comment:11 Changed 7 years ago by Sa'ar Zac Elias

Owner: changed from Garry Yao to Sa'ar Zac Elias
Status: review_failedreview

Id duplication will be handled with #6915.

comment:12 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:13 Changed 7 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: review_passedclosed

Fixed with [6290].

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