Opened 11 years ago

Closed 11 years ago

#2962 closed Bug (fixed)

plugin:link creation with target:popupwindow doesn't work

Reported by: Garry Yao Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (FCKeditor) - Retired
Keywords: Confirmed Review+ Cc:

Description

Procedures

  1. Open the replace by code example page in IE;
  2. Selection anchored to the existed link 'FCKeditor', click the Link button to open the link dialog.
  3. Switch to Target page, select <popup window> option, close with Ok.
  4. Click Preview button to open a preview window, click the link 'FCKeditor' within the preview document.
  • Expected Result : New window opened in popup window.
  • Actual Result : New window opened as blank parent window.

Attachments (2)

2962.patch (1.6 KB) - added by Martin Kou 11 years ago.
2962_2.patch (2.5 KB) - added by Martin Kou 11 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 11 years ago by Martin Kou

Keywords: Confirmed added
Owner: set to Martin Kou
Status: newassigned

It doesn't work in Firefox as well. Hmm, I remember I've tested that before I committed the patch.

Changed 11 years ago by Martin Kou

Attachment: 2962.patch added

comment:2 Changed 11 years ago by Martin Kou

Keywords: Review? added

comment:3 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

While the _cke_saved_src and _cke_saved_href attributes are appended to the element, leaving the original src and href there, all event attributes are instead replaced. So, the attribute name change don't have to follow the src/href code in this case.

Also, the proposal is dealing with the "click" event only, but the "_cke_pa_" prefix is added to "all" events. So, a generic solution can be found and it must go inside writeHtml (something like a = a.replace( ckePaAttrRegex, '' )).

Out of curiosity, "pa" stands for "private attribute".

Changed 11 years ago by Martin Kou

Attachment: 2962_2.patch added

comment:4 Changed 11 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:5 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Can you please also change the following (add ?:) when committing:

ckeClassRegex = /(?:^|\s+)cke_[^\s]*/g

Also, it looks like the ltrim overhead is not necessary in the ckeClassRegex replacement as the space is already matched by the regex. Feel free to remove it also, if you can confirm it.

comment:6 Changed 11 years ago by Martin Kou

The ltrim is still needed. I think I've mentioned it before... If we have something like this:

cke_some_class another_class

and we apply .replace(ckeClassRegex, '') to the above string. We'd get back

 another_class

which has a space at the beginning. The "\s+" in the regex doesn't fix that because that space isn't located before the cke_* string.

On the other hand, if we add another \s+ in towards the end of the regex, it'd still be wrong, because the regex would then merge two adjacent CSS class strings to the cke_* class.

So, in the end, the solution is still a replace and then an ltrim.

comment:7 Changed 11 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [3108].

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