Ticket #2970 (closed Task: fixed)

Opened 6 years ago

Last modified 6 years ago

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

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

2970.patch (5.1 KB) - added by garry.yao 6 years ago.
2970_2.patch (11.9 KB) - added by garry.yao 6 years ago.
2970_3.patch (13.2 KB) - added by garry.yao 6 years ago.

Change History

comment:1 in reply to: ↑ description Changed 6 years ago by 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.

comment:2 Changed 6 years ago by garry.yao

  • Description modified (diff)

Remove unnecessary proposed changes.

Changed 6 years ago by garry.yao

comment:3 Changed 6 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from new to assigned
  • Keywords Review? added

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 6 years ago by fredck

  • 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 6 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 6 years ago by garry.yao

comment:6 Changed 6 years ago by fredck

  • 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 6 years ago by garry.yao

comment:7 Changed 6 years ago by garry.yao

  • Keywords Review? added; Review- removed

I hope this patch could address the above mentioned issues.

comment:8 Changed 6 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:9 Changed 6 years ago by garry.yao

  • Status changed from assigned to closed
  • Resolution set to fixed

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

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