This project is archived and is in readonly mode.

#1196 ✓resolved
Garrick Cheung

Cloning element twice crashes IE7

Reported by Garrick Cheung | March 15th, 2011 @ 10:10 PM | in 1.4.0 (closed)

I've come across a bug where I'm cloning a template (html) twice and it crashes IE7.

http://jsfiddle.net/garrickcheung/m7cxW/1/

Using cloneNode(true) works fine though:
http://jsfiddle.net/garrickcheung/m7cxW/2/

Comments and changes to this ticket

  • Garrick Cheung

    Garrick Cheung March 16th, 2011 @ 01:03 AM

    Also, Daniel Buchner asked if removing the ID would work. It works if I removed it in the HTML but not if I use Element.erase method:
    http://jsfiddle.net/garrickcheung/m7cxW/4/

  • Christoph Pojer

    Christoph Pojer March 24th, 2011 @ 11:53 AM

    • State changed from “new” to “open”
    • Milestone set to 1.4.0
    • Milestone order changed from “894” to “0”

    Ugh!

  • ronin-133141 (at lighthouseapp)

    ronin-133141 (at lighthouseapp) April 20th, 2011 @ 08:30 PM

    I can reproduce this on IE 7.0.5730.13, but not on IE 6.0.2900.5512.xpsp_sp3_gdr.101209-1647.

    This appears to be a regression from #1058 – if I revert the patch (https://github.com/mootools/mootools-core/commit/302afdb6c72a8864fc..., IE7 no longer crashes (but #1058 is back to square one, of course).

  • super-mario

    super-mario May 19th, 2011 @ 02:24 PM

    please update this bugfix in "Sortables.getClone" on mootools more:
    Here the Code that work's in IE7

    chang line from:

    var clone = element.clone(true)
    

    to

    var clone = element.clone(true, true)
    

    complete fix

    Sortables = new Class({
        Extends: Sortables,
        getClone: function(event, element){
            if (!this.options.clone) return new Element(element.tagName).inject(document.body);
            if (typeOf(this.options.clone) == 'function') return this.options.clone.call(this, event, element, this.list);
            var clone = element.clone(true, true).setStyles({ //the fix 
                margin: 0,
                position: 'absolute',
                visibility: 'hidden',
                width: element.getStyle('width')
            }).addEvent('mousedown', function(event){
                element.fireEvent('mousedown', event);
            });
            
            //prevent the duplicated radio inputs from unchecking the real one
            if (clone.get('html').test('radio')){
                clone.getElements('input[type=radio]').each(function(input, i){
                    input.set('name', 'clone_' + i);
                    if (input.get('checked')) element.getElements('input[type=radio]')[i].set('checked', true);
                });
            }
        
            return clone.inject(this.list).setPosition(element.getPosition(element.getOffsetParent()));
        }
    });
    
  • Arian

    Arian June 6th, 2011 @ 12:05 PM

    Garrick, could you update the jsfiddles? It seems they don't work anymore...

  • Arian

    Arian June 24th, 2011 @ 04:33 PM

    • Assigned user set to “Christoph Pojer”

    Maybe we should revert that fix until we find a definite solid solution for both cases. IE7 crash is worse than that other one imo.

  • Sanford Whiteman

    Sanford Whiteman July 3rd, 2011 @ 12:30 AM

    OK Folks, I have worked this thing to the bone and I think this should be the IE codepath for the new clone(), fixing both #1058 and this one. The main thrust is that for a clean slate, we have to clone with no (null) id, then restore the id after cloning, but that same DOM Attr assignment cannot be used to assign an id to the clone; that needs to be done using dot-notation (clone.id = ) or else instant crash.

    Element.implement('clone', function(contents, keepid, newid){
        contents = contents !== false;
    
        contents = contents !== false;
        
        if (!keepid) {
            var holdid = this.id; // keep id around for later restore
            var anode = document.createAttribute('id'); // empty anode
            this.setAttributeNode(anode);
        }
    
        var clone = this.cloneNode(contents), i;
    
        
        if (contents){
            var ce = clone.getElementsByTagName('*'), te = this.getElementsByTagName('*');
            for (i = ce.length; i--;) cleanClone(ce[i], te[i], keepid);
        }
    
        cleanClone(clone, this, keepid);
        
        
        if (!keepid) { 
            anode.nodeValue = holdid; // restore value
        if (newid !== null) { // pass null in order to have no id, otherwise...
            clone.id = newid || holdid + Math.random().toString().substr(2); // user-provided or random id
        }
        }
    
    
    
        
        if (Browser.ie){
            var co = clone.getElementsByTagName('object'), to = this.getElementsByTagName('object');
            for (i = co.length; i--;) co[i].outerHTML = to[i].outerHTML;
        }
        
        
        return document.id(clone);
    });
    

    The reason I say "IE codepath" is I haven't tested this function in non-IE browsers (IE 6+ is enough work as it is), so you may have to wrap the new stuff in browser detection. The goal of my work was to not have the IE runtime crash while still clone()ing and dispose()ing elements, using either duplicate, user-specified, random, or empty ID.

    ALSO: this may have brought out a new bug in document.id (???) -- can't get a handle to the clones, but I certainly can with document.getElementById. Someone needs to verify this part.

  • Sanford Whiteman

    Sanford Whiteman July 3rd, 2011 @ 12:33 AM

    • Tag changed from element.clone to element.clone, ie7
  • Arian

    Arian July 4th, 2011 @ 11:23 PM

    • Assigned user changed from “Christoph Pojer” to “Arian”

    Thanks man, i'll try to implement it and run it against our tests as soon as possible.

  • Arian
  • Garrick Cheung

    Garrick Cheung August 4th, 2011 @ 10:21 PM

    I've created a new test:

    http://jsfiddle.net/garrickcheung/VzGCJ/

    I've tried adding your fix here:
    http://jsfiddle.net/garrickcheung/VzGCJ/7/

    Issue is still there.

  • Sanford Whiteman

    Sanford Whiteman August 5th, 2011 @ 05:52 PM

    Garrick, have you tried my code as-is for IE instead of Arian's integration into a patch?

  • Thomas Aylott

    Thomas Aylott August 9th, 2011 @ 04:52 PM

    • State changed from “open” to “resolved”

Create your profile

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

Shared Ticket Bins

Pages