Opened 9 years ago

Closed 9 years ago

#2852 closed Task (fixed)

Move link dialog to v3 trunk.

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

Description

The link dialog did not make it to the v3 beta. It should be moved to trunk with all discovered issues (#2831, #2832, #2833, #2839) fixed.

Attachments (10)

2852.patch (79.6 KB) - added by Martin Kou 9 years ago.
2852_fakeobjects.patch (25.1 KB) - added by Martin Kou 9 years ago.
Temporary fakeobjects plugin for making the patch work.
2852_2.patch (85.4 KB) - added by Martin Kou 9 years ago.
2852_3.patch (85.6 KB) - added by Martin Kou 9 years ago.
2852_4.patch (91.7 KB) - added by Martin Kou 9 years ago.
2852_5.patch (49.3 KB) - added by Martin Kou 9 years ago.
2852_6.patch (183.4 KB) - added by Martin Kou 9 years ago.
2852_7.patch (48.7 KB) - added by Martin Kou 9 years ago.
2852_8.patch (48.3 KB) - added by Martin Kou 9 years ago.
2852_9.patch (48.3 KB) - added by Martin Kou 9 years ago.

Download all attachments as: .zip

Change History (26)

Changed 9 years ago by Martin Kou

Attachment: 2852.patch added

Changed 9 years ago by Martin Kou

Attachment: 2852_fakeobjects.patch added

Temporary fakeobjects plugin for making the patch work.

comment:1 Changed 9 years ago by Martin Kou

Keywords: Review? added
Status: newassigned

#2832 needs to be fixed in the fakeobjects plugin, which Fred is working on.

2852.patch needs a fakeobjects plugin to work, however. So I'm providing a temporary fakeobjects patch which enables the patch to work correctly but does not fix #2832.

comment:2 Changed 9 years ago by Martin Kou

#2831, #2833, #2839 are marked as dup to this ticket.

Changed 9 years ago by Martin Kou

Attachment: 2852_2.patch added

comment:3 Changed 9 years ago by Martin Kou

Updated the patch to use the new fakeobjects system, with some related fixes and additions to the fakeobjects system:

  1. Added CKEDITOR.editor::restoreRealElement() in fakeobjects plugin to convert a fake object back to the real element so that dialogs can edit the real element.
  2. Added a new argument "realElementType" in CKEDITOR.editor::createFakeElement() in fakeobjects plugin for specifying the element type to display instead of "img" in the element path bar.
  3. Modified the pagebreak plugin so that page breaks are displayed as "div" elements in the element path bar when selected, instead of "img".
  4. Fixed erroneous detection of anchors in the document in the Link dialog.

Changed 9 years ago by Martin Kou

Attachment: 2852_3.patch added

comment:4 Changed 9 years ago by Martin Kou

Ok, one more update with two more things fixed.

  1. The issue in #2832 is fixed. So #2832 is marked as a dup to this ticket.
  2. Creating a link from a collapsed selection does not work.

comment:5 Changed 9 years ago by Martin Kou

Keywords: Review? removed

Retracted review request... Still some IE bugs to be fixed (e.g. setting name attribute to an <a> does not work in IE the usual way).

Changed 9 years ago by Martin Kou

Attachment: 2852_4.patch added

comment:6 Changed 9 years ago by Martin Kou

  1. Fixed <a> name change bugs in IE.
  2. Added CSS styles for non-empty anchors.

comment:7 Changed 9 years ago by Martin Kou

Keywords: Review? added

Changed 9 years ago by Martin Kou

Attachment: 2852_5.patch added

comment:8 Changed 9 years ago by Martin Kou

Fixed duplicated patches generated by TortoiseSVN.

comment:9 Changed 9 years ago by Frederico Caldeira Knabben

Component: UI : DialogsGeneral
Keywords: Review- added; Review? removed
  • The changes at htmlparser/element.js:
    • There is no need to lower case the attribute name. It will always be lowercased.
    • There is no need to trim the replace. It's enough to change the reges to: ckeClassRegex = /(|\s+)cke_[\s]*/g. It also makes the regex more correct.
    • The regex declaration is reusing the multiline "var" statement for sortAttribs, which is to be avoided. Having several "var"s one after the other is automatically optimized by the packager.
  • The "defaultValues" are to be removed from the configurations. See #2792. The uploadTab, showAdvancedTab and showTargetTab should also be removed in favor of the API features.
  • The plugin configurations must not go under the CKEDITOR.config.link object, but directly under CKEDITOR.config. Maybe prefixed with "link_". See #2822.
  • Reusing a single "var" statement for all local functions in the link dialog make it a big mess. Multiline variable declarations should have its own "var" statement.
  • It's a pity to see all the logic being done inside the onOk function. That's is not the way to go, but that's the way it will be as we don't have time to change it now. This dialog is be rewritten later, and hopefully we'll not have that much people basing their custom plugins on it.

comment:10 Changed 9 years ago by Martin Kou

/(^|\s+)cke_[^\s]*/g

alone wouldn't work. Let's say I have "cke_sth sth_else" as the class attribute, if I run replace with that and an empty string I'd end up with " sth_else", which is wrong because there's an extra space in front of the value. If trim() is not called then a second regex would still be needed to fix that, which is just doing the same thing as a trim anyway.

Changed 9 years ago by Martin Kou

Attachment: 2852_6.patch added

comment:11 Changed 9 years ago by Martin Kou

Keywords: Review? added; Review- removed

Ok. The following are fixed:

  1. defaultValues is removed.
  2. Config sub-namespaces are removed. Now link related configs are named as config.linkCamelCase, following the naming conventions in config.js.
  3. Multi-line var declarations are changed to conform to the coding style guidelines.
  4. ckeClassRegex is changed to
    /(^|\s+)cke_[^\s]*/g
    }}}, but after replacing with it an ltrim() is still done.
     5. Removed unnecessary toLowerCase() in htmlparse/element.js.
    

Changed 9 years ago by Martin Kou

Attachment: 2852_7.patch added

comment:12 Changed 9 years ago by Martin Kou

Ok, another patch posted. The previous patch missed the newly added files and had a lot of unnecessary newline changes.

Changed 9 years ago by Martin Kou

Attachment: 2852_8.patch added

comment:13 Changed 9 years ago by Martin Kou

New patch with pushDefault() and popDefault() removed due to #2803.

Changed 9 years ago by Martin Kou

Attachment: 2852_9.patch added

comment:14 Changed 9 years ago by Martin Kou

Fixed a remaining reference to config.link sub-namespace in the link plugin.

comment:15 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Please don't forget the images when committing (they were not included into the patch, obviously).

comment:16 Changed 9 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [3048].

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