Refactor CKEDITOR.dom.documentFragment

[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

[fredck]

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.

[garry.yao]

Remove unnecessary proposed changes.

[garry.yao]

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.

[fredck]

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.

[garry.yao]

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.

[fredck]

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 ) )

[garry.yao]

I hope this patch could address the above mentioned issues.

[garry.yao]

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