#53 ✓resolved
John-David Dalton

add insertAfter(), insertBefore(), insertBottom(), insertTop() methods to fine grain control over insert()

Reported by John-David Dalton | August 8th, 2009 @ 02:28 AM

Having fine grain control over methods allows for users who know what they want to execute the method of their choice with a greater performance boost.

I have done something similar for Fuse.List.from(), which accepts objects, nodeLists, arrays.
That general method is great but if I know I am passing it an array why should I bother with a general method.
I have Fuse.List.fromArray(), and Fuse.List.fromNodeList() to give those cases a performance boost.

Comments and changes to this ticket

  • Kit Goncharov

    Kit Goncharov August 10th, 2009 @ 12:09 PM

    • State changed from “new” to “open”

    Definitely a great idea. The only problem with that, though, is that Element#insertBefore is already a DOM method, and totally different than what Element#insert({bottom:...}) does (plus, it's used by the insertion translations, so overwriting it wouldn't work and would definitely break a lot of scripts). Maybe renaming them to something like Element#insertAtBottom, insertAtTop, etc. would make more sense, since that's what Element#insert really does.

    FWIW, here's how my Element#insert looks in Miniatures (adapted for Fuse). It's definitely faster than Prototype's, but performance could considerably be improved by splitting it into individual methods:

    var toString = Object.prototype.toString, hasKey = Fuse.Object.hasKey;
    function insert(insertions){
            var content, index, length, position;
            for(position in insertions){
                if(hasKey(insertions, position)){
                    content = ((toString.call(insertions[position]) === '[object Array]') ? insertions[position] : [insertions[position]]);
                    switch(position.toLowerCase()){
                        case 'after':
                            for(index = content.length - 1; index >= 0; index--){
                                //Loop in reverse.
                                this.parentNode.insertBefore(content[index], this.nextSibling);
                            }
                            break;
                        case 'top':
                            for(index = content.length - 1; index >= 0; index--){
                                //Loop in reverse.
                                this.insertBefore(content[index], this.firstChild);
                            }
                            break;
                        case 'before':
                            for(index = 0, length = content.length; index < length; index++){
                                this.parentNode.insertBefore(content[index], this);
                            }
                            break;
                        default:
                            //Insert at the bottom of the element if no insertion point is specified (or if it's not `top`, `after`, or `before`).
                            for(index = 0, length = content.length; index < length; index++){
                                this.appendChild(content[index]);
                            }
                            break;
                    }
                }
            }
            return this;
        }
    
  • John-David Dalton

    John-David Dalton August 10th, 2009 @ 12:32 PM

    Quick notes:

    Kit keep in mind within a week or so Fuse will be completely off of the native Element objects and instead use Fuse.Dom.Node. so naming conflicts will not be a concern.

    Also the use of Fuse.Object.hasKey is expensive esp when the keys we are looking for aren't know to cause problems so a simple for loop without a check should be fine.

    Fuse has a Fuse.Object.isArray().. so we don't need to duplicate that usage here.

  • John-David Dalton
  • Juriy Zaytsev

    Juriy Zaytsev August 10th, 2009 @ 12:40 PM

    Kit,

    you see, most of complexity in Prototype's insert lies in its overload-ish nature:

    1) It can take an object of insertions or a single insertion.
    2) It can take up to 4 different kind of insertions.
    3) For any of 4 insertions, it can take something that has `toElement` and delegate to that method.
    4) For any of 4 insertions, it can take something that has `toHTML` and delegate to that method.
    5) For any of 4 insertions, it can take string value and turn it into a set of elements.
    6) For any of 4 insertions, if value is a string it can normalize troublesome insertions (such as table-related ones)
    7) For any of 4 insertions, if value is a string it will strip scripts and evaluate them.
    8) For any of 4 insertions, if value is not a string it will turn it into a string before proceeding.
    9) And finally, it considers something to be an element only when that something has `nodeType == 1`
    

    It's pretty clear how prototype's insert gets burried under all these convenience "layers" and is noticeable slower than something as simple as a wrapper around insertBefore, appendChild and other native methods.

    I was just experimenting with breaking insert up into smaller chunks, but it looks like it all leads to some serious repetition :/ I'll play with it a bit more.

  • Kit Goncharov

    Kit Goncharov August 10th, 2009 @ 04:34 PM

    Wow, fast responses, guys! :)

    @JDD: I'm using Fuse.Object.hasKey as a replacement for Object#hasOwnProperty. I agree it's not really needed, but, in case someone decides to extend Object.prototype, this would be a lifesaver. On an unrelated side note, I have used Fuse.Object.hasKey/Object.definesOwnProperty to unintentionally make Prototype's Object.toQueryString compatible with arrays and other objects (see https://prototype.lighthouseapp.com/projects/8886/tickets/126-toque... ), so it's a handy tool to have to filter unwanted properties from the prototype.

    I dig the fact that Fuse will have its own DOM namespace, but I'm still a bit hesitant about using insertBefore, insertAfter, etc. as the method names -- it's nonintuitive to those who are familiar with the W3C method (also, the heavily-simulated W3C-style insertAfter follows the insertBefore scheme, so that creates an opportunity for confusion). Element#insert inserts code at the bottom, top, before, or after the element, so maybe something like insertBefore/AfterElement, and insertAtTop/BottomOf would be more descriptive.

    @Juriy: So that explains why I had such a hard time understanding Prototype's code! :D I think we should go ahead and just allow unlimited arguments for our individual methods -- if there's only one argument passed, we should just perform the insertion without looping for performance. For the sake of consistency, I also think we should go ahead and make the insertion element-only -- essentially, we defer to the W3C DOM methods to see if the insertion is valid or not (for example, if a string is passed, the DOM methods will automatically throw a NOT_FOUND error).

  • John-David Dalton

    John-David Dalton August 10th, 2009 @ 05:18 PM

    • State changed from “open” to “new”

    @kit: Bonus! You helped uncover a bug in Fuse :D. Fuse uses Fuse.Object._each() inside Fuse.Object.toQueryString() which is like a corrected form of for-in so it helps find shadowed props and fixes quirks with for-in but we don't check Fuse.Object.hasKey there... a fix is coming soon.

    Update: Resolved the Fuse.Object.toQueryString() bug here https://github.com/jdalton/fusejs/commit/2e52e54cec26fd3150522efa82e6c9db3ace4231

  • John-David Dalton

    John-David Dalton August 11th, 2009 @ 10:44 AM

    Fuse currently uses document fragments when converting strings to html elements.
    It does so by:

     1) creating a dummy node for the context (current doc or iframe)


    2) creating a document fragment for the context (current doc or iframe)

    fragments don't play nice with elements from other frames thats why we must have a
    fragment for each context. All dummy nodes and fragments are cached and recycled for
    future calls
    
    
    
    
    3) Using the translation table to work around issues with tables, select elements and others
    we write the html to the dummy div
    
    
    
    
    4) We pass the dummy node to the getContentAsFragment(cache, node) internal method that then
    grabs the child nodes by either
    a) IE inserting the dummy node into the fragment and calling removeNode() (which leaves the child nodes)
    b) Or if document range is supported create a range and selectNodeContents of the dummy node and append the extracted contents to the fragment
    c) Or manually iterate over the dummy node inserting the children into the fragment
    
    
    
    
    5) return the fragment and insert into target dom
    Note: when fragments are inserted into elements they auto-clear their contents and are ready to be used again. There is no need to clone document fragments we can simply re-use the same fragment over and over.
  • Kit Goncharov

    Kit Goncharov August 11th, 2009 @ 10:59 AM

    If document.implementation.createHTMLDocument is available, we could
    use that instead of a document fragment. AFAIK, WebKit is the only one
    that supports it -- Firefox definitely doesn't; I don't know about IE.

  • John-David Dalton

    John-David Dalton August 11th, 2009 @ 11:11 AM

    what are the benefits of using createHTMLDocument ?

  • Kit Goncharov

    Kit Goncharov August 11th, 2009 @ 11:52 AM

    @Diego: Nope, I meant createHTMLDocument. :) Try it in Safari:

    var htmlDoc = document.implementation.createHTMLDocument('');
    //Safari 2.x doesn't create a body for this document, so we have to create our own content element
    var htmlEl = htmlDoc.createElement('content');
    //Now create our fragment
    htmlEl.innerHTML = '<p>I am a <span>series</span> of HTML nodes!</p>';
    //And import the content element into the current document
    var importedElement = document.importNode(htmlEl, true);
    

    The problem with using createDocument is that it doesn't create an HTML document, so it doesn't support innerHTML (which really isn't part of the spec to begin with). Your code snippet will create an innerHTML property on the documentElement, but it'll do nothing (i.e., there's no setter defined for it). If you inspect base.documentElement after setting the innerHTML, you'll see that it's still just a single node. Also, before you set the innerHTML of the element, 'innerHTML' in base.documentElement === false.

    @JDD: I guess there really isn't a significant advantage. I just think it'd be more efficient for browsers that support it, since you don't have to worry about cross-frame issues, etc (you can just use ownerDocument and essentially convert an HTML string to a series of nodes in 4 lines). I definitely like that we're using document fragments instead of Prototype's approach, though (creating a DIV in the context of the current document, then setting its innerHTML, which is quirky on XHTML, etc.)

    On the other hand, if only WebKit supports this, then maybe it's not such a good idea, after all.

  • John-David Dalton

    John-David Dalton August 11th, 2009 @ 11:57 AM

    @kitsg: Have you tried appending an element from another iframe to the htmlDoc object of the primary document ?

  • Kit Goncharov

    Kit Goncharov August 11th, 2009 @ 11:58 AM

    No, you have to use htmlDoc.importNode for that.

    I have an errand to run, but I'll whip up some testcases when I get back (including documentFragments, too, á la Fuse).

  • John-David Dalton

    John-David Dalton August 21st, 2009 @ 11:08 PM

    • Assigned user changed from “John-David Dalton” to “Joe Gornick”
  • Joe Gornick

    Joe Gornick August 26th, 2009 @ 01:39 AM

    So, after reading all the comments, I have a few of my own ...

    1. I agree with Kit that we should not name methods after natives. It could get really confusing especially when working with the raw property on the Fuse.Dom.Node wrapper.

    2. Before I go in an make these changes, I'm assuming we are going to split up into separate methods, but also trim down what arguments are accepted by the insert* methods?

    @Juriy: did you have any luck with splitting up the insert method?

  • Joe Gornick

    Joe Gornick October 7th, 2009 @ 01:08 AM

    • State changed from “new” to “patched”
    • Assigned user changed from “Joe Gornick” to “John-David Dalton”
  • John-David Dalton

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

JavaScript frameworks share similar features and functionality such as DOM manipulation, event registration, and CSS selector engines. FuseJS attempts to incorporate the strengths of these frameworks into one stable, efficient, and optimized core JavaScript framework.

Pages