Opened 9 years ago

Closed 9 years ago

#2970 closed Task (fixed)

Refactor CKEDITOR.dom.documentFragment

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

Description (last modified by Garry Yao)

Current implementation of documentFragment has been ported from V2 for IE 5.5 support, which have dropped for V3. We could now replace those IE hacks with standard support of native documentFragment.

Reference

Sitepoint

Attachments (3)

2970.patch (5.1 KB) - added by Garry Yao 9 years ago.
2970_2.patch (11.9 KB) - added by Garry Yao 9 years ago.
2970_3.patch (13.2 KB) - added by Garry Yao 9 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 in reply to:  description Changed 9 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

  • Make CKEDITOR.dom.element.createFromHtml return documentFragment

No! CKEDITOR.dom.element.createFromHtml is there to create an element. If we think it will be needed we can add a similar hybrid function to CKEDITOR.dom.node or a specific one to CKEDITOR.dom.documentFragment. But we are not using it for now, so no additions.

comment:2 Changed 9 years ago by Garry Yao

Description: modified (diff)

Remove unnecessary proposed changes.

Changed 9 years ago by Garry Yao

Attachment: 2970.patch added

comment:3 Changed 9 years ago by Garry Yao

Keywords: Review? added
Owner: set to Garry Yao
Status: newassigned

Two updates contained in the patch:

  • Param modification to CKEDITOR.tool.extend to allow only copy a specified list of properties.
  • CKEDITOR.dom.documentFragment now use native constructor.

comment:4 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

At documentFragment:

  • The "type" property should not be copied from element.prototype.
  • As "insertAfterNode" has been simplified, the three "var" declarations are not anymore needed.

At tools.extend:

  • Let's make it possible to have both the "properties list" and "overwrite" parameters together. It means that the function should accept the following params:
  • ( target, source[,souce(n)] )
  • ( target, source[,souce(n)], {Boolean}overwrite )
  • ( target, source[,souce(n)], {Boolean)overwrite, {Object}propertiesList )

Note that "propertiesList" has been placed "after" overwrite because we can also make a small performance improvement and use an object instead of an array (indexOf) is not that good.

  • As mentioned above, make the properties list be an object (hashtable). It means the function call must pass the paramenter like { append:1, getFirst:1, getLast:1 }, instead of [ 'append', 'getFirst', 'getLast' ].
  • Please try to follow our code standards even on comments:
// Whether it has a properties list.
NOT
//whether it has a properties list
  • Please do not add a comment in the middle of conditions, like you did at line 131.

Also, I've always bee afraid about IE's compatibility with documentFragment. So, please include also tests for all documentFragment functions.

comment:5 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

I've including test for both documentFragment and the improved extend method, yes, It's discovered that CKEDITOR.dom.documentFragment::contains method is not supported by IE and I've exclude it from the inherited list.

Changed 9 years ago by Garry Yao

Attachment: 2970_2.patch added

comment:6 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

I'm not able to apply this patch. I have an error at line 213.

Looking at the patch preview on Trac, I can find some issues though.

  • Based on the added documentation, and having you used the "in" overator over propertiesList, we correctly assume that propertiesList should be an object. But, you are still making isArray checks there, and still passing the properties list as an Array in documentFragment. Something is wrong here.
  • There is a simpler way to set overwrite and propertiesList.
  • There are still coding style issues, specially with whitespaces.
  • The square brackets are not to be removed from the documentation. They indicate that the argument is optional.
  • ( propertiesList && ( propertyName in propertiesList ) || !propertiesList ) !!! What about ( !propertiesList || ( propertyName in propertiesList ) )

Changed 9 years ago by Garry Yao

Attachment: 2970_3.patch added

comment:7 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

I hope this patch could address the above mentioned issues.

comment:8 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:9 Changed 9 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3213]. Click here for more info about our SVN system.

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