Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fix bug #2310 #2387

Closed
wants to merge 7 commits into from

5 participants

@kentaromiura
Collaborator

IE7 clone node
(http://msdn.microsoft.com/en-us/library/ie/ms536365(v=vs.85).aspx)
have problem when cloning element with id attribute stetted.

Cristian Carlesso fix bug #2310
IE7 clone node
(http://msdn.microsoft.com/en-us/library/ie/ms536365(v=vs.85).aspx)
have problem when cloning element with id attribute stetted.
5b0f63c
@ibolmo
Owner
Cristian Car... added some commits
Cristian Carlesso added tests
added tests, fixed some spacing, precedence issues
4a18190
Cristian Carlesso adding missing semicolon, and a little comment
adding missing semicolons in the spec, and a little comment on the fix.
78992b8
@kentaromiura
Collaborator

@ibolmo yeah, I would prefer too, but I can't use tailored feature detect on this bug, since before triggering the bug there's no clue and after is too late. It should be ok, because is IE6/7 related and on IE8 is fixed.

@arian
Owner

Is it only for input elements.. otherwise you should also check for child elements..

var elements = [this].append(Array.from(this.getElementsByName('*'));
var ids = elements.map(function(element){
    var id = element.getAttribute('id');
    element.removeAttribute('id');
    return id;
});

// later
for (var i = 0; i < elements.length; i++) elements[i].setAttribute('id', ids[i]);
@cpojer
Owner

@arian weren't there lots of crashes involved with this in IE7? Just checkin'

@arian
Owner

Yes. This is why testing is very important here.

@kentaromiura
Collaborator

I tested with nested elements, and seems like my patch will work as it is now, I just need to check on IE6.
downloading the VM right now.

@GCheung55
Collaborator

@kentaromiura 10 days is a long time to download the IE6 VM.. XD

@kentaromiura
Collaborator

It's not when you're on a mac and the vm image is in a ... exe format!

@arian
Owner

maybe this will help: https://github.com/xdissent/ievms/

@kentaromiura
Collaborator

@arian, I used that, but since it downloads an exe files it doesn't work for me.
I will trying later to uncompress the exe in another (sigh) vm.

@GCheung55
Collaborator

@kentaromiura I use Unarchiver to extract the exe from Microsoft.

@kentaromiura
Collaborator

Tested on IE6, it has the bug, and the fix works, but it break the Element.clone specs,
by looking at the specs, I found a couple of logic errors,
and I think that setAttribute('id', ...) doesn't work at all in IE6, using this.id = ... instead.
fixing these stuff.

EDIT: also removeAttribute doesn't return the attribute, but true/false in IE.
I should write a couple more tests.

Cristian Car... added some commits
Cristian Carlesso After further tests, seems like that the only way to avoid the proble…
…m is to not use clone node at all

After further tests, seems like that the only way to avoid the problem
is to not use clone node at all.

So, I wrote a clone node function using outerHTML. need further
testing, to be sure everything is ok, but since it passes regression
I'll commit it.
24fdab0
Cristian Carlesso add a little more test
more test added
b6d6523
@arian arian commented on the diff
Source/Element/Element.js
@@ -818,7 +818,25 @@ Element.implement({
clone: function(contents, keepid){
contents = contents !== false;
- var clone = this.cloneNode(contents), ce = [clone], te = [this], i;
+ var cloneNode = function(){return false};
@arian Owner
arian added a note
var self = this;
var cloneNode = function(){
    return self.cloneNode(contents);
};
@kentaromiura Collaborator

I think I need to move this on outer scope to avoid memory leaks too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arian arian commented on the diff
Source/Element/Element.js
@@ -818,7 +818,25 @@ Element.implement({
clone: function(contents, keepid){
contents = contents !== false;
- var clone = this.cloneNode(contents), ce = [clone], te = [this], i;
+ var cloneNode = function(){return false};
+ /*<ltIE8>*/
+ if (Browser.ie6 || Browser.ie7){
+ var self = this;
+ cloneNode = function(contents){
@arian Owner
arian added a note

don't need the contents argument, it can get the contents variable from the outer scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Source/Element/Element.js
@@ -818,7 +818,25 @@ Element.implement({
clone: function(contents, keepid){
contents = contents !== false;
- var clone = this.cloneNode(contents), ce = [clone], te = [this], i;
+ var cloneNode = function(){return false};
+ /*<ltIE8>*/
+ if (Browser.ie6 || Browser.ie7){
+ var self = this;
+ cloneNode = function(contents){
+ var oldID = '' + self.id;
+ self.removeAttribute('id');
+ var clonedHTML = self.outerHTML;
+ var cloned = document.createElement('div');
+ cloned.innerHTML = clonedHTML;
@arian Owner
arian added a note

should use .set('html') for all the table and other weird cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arian arian commented on the diff
Source/Element/Element.js
@@ -818,7 +818,25 @@ Element.implement({
clone: function(contents, keepid){
contents = contents !== false;
- var clone = this.cloneNode(contents), ce = [clone], te = [this], i;
+ var cloneNode = function(){return false};
+ /*<ltIE8>*/
+ if (Browser.ie6 || Browser.ie7){
@arian Owner
arian added a note

I think we could feature detect this..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arian
Owner

what about using element.erase/get/set('id') added in #2083

Specs/1.4client/Element/Element.js
((4 lines not shown))
+ it('should cloneNode properly on IE6/7', function(){
+
+ var div = new Element('div');
+ div.inject(document.documentElement);
+ div.innerHTML = "<input id='Q1' class='foo' rel='hai' />";
+
+ var q1 = document.id('Q1');
+ var clone = q1.clone();
+ clone.replaces(q1);
+
+ expect($$('input[id=Q1]').length).toEqual(0);
+ expect($$('input#Q1').length).toEqual(0);
+ clone.dispose();
+
+ var ul = new Element('ul',{id:'test'});
+ ul.set('html', '<li id="li1"></li><li id="li2"></li>'));
@arian Owner
arian added a note

a ) too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arian arian commented on the diff
Specs/1.4client/Element/Element.js
@@ -10,6 +10,32 @@ describe('Element', function(){
describe('Element.getProperty', function(){
+ it('should cloneNode properly on IE6/7', function(){
@arian Owner
arian added a note

should be split up in multiple its.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Specs/1.4client/Element/Element.js
((10 lines not shown))
+ var q1 = document.id('Q1');
+ var clone = q1.clone();
+ clone.replaces(q1);
+
+ expect($$('input[id=Q1]').length).toEqual(0);
+ expect($$('input#Q1').length).toEqual(0);
+ clone.dispose();
+
+ var ul = new Element('ul',{id:'test'});
+ ul.set('html', '<li id="li1"></li><li id="li2"></li>'));
+ ul.inject(document.documentElement);
+
+ var ulc = ul.clone();
+ ulc.replaces(ul);
+
+ expect($$('li#li1').length).toEqual(1);
@arian Owner
arian added a note

This test case should equal 0. keepid is false, so it should remove the IDs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arian
Owner

Maybe the feature test in #2083 is enough too.

Cristian Car... added some commits
Cristian Carlesso adding another failing test
add a failing test.
TODO: split tests
b5b63db
Cristian Carlesso quick fix to pass the failing test
quick fix to pass the failing test
015662f
@kentaromiura
Collaborator

@arian as you can see by running the new test, Element.erase will not pass :(
seems that the fugly outerHTML hack pass tough.

@ibolmo
Owner

Fixed?

@ibolmo ibolmo closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 30, 2012
  1. fix bug #2310

    Cristian Carlesso authored
    IE7 clone node
    (http://msdn.microsoft.com/en-us/library/ie/ms536365(v=vs.85).aspx)
    have problem when cloning element with id attribute stetted.
  2. added tests

    Cristian Carlesso authored
    added tests, fixed some spacing, precedence issues
  3. adding missing semicolon, and a little comment

    Cristian Carlesso authored
    adding missing semicolons in the spec, and a little comment on the fix.
Commits on Aug 14, 2012
  1. After further tests, seems like that the only way to avoid the proble…

    Cristian Carlesso authored
    …m is to not use clone node at all
    
    After further tests, seems like that the only way to avoid the problem
    is to not use clone node at all.
    
    So, I wrote a clone node function using outerHTML. need further
    testing, to be sure everything is ok, but since it passes regression
    I'll commit it.
Commits on Aug 19, 2012
  1. add a little more test

    Cristian Carlesso authored
    more test added
Commits on Aug 20, 2012
  1. adding another failing test

    Cristian Carlesso authored
    add a failing test.
    TODO: split tests
  2. quick fix to pass the failing test

    Cristian Carlesso authored
    quick fix to pass the failing test
This page is out of date. Refresh to see the latest.
Showing with 50 additions and 1 deletion.
  1. +22 −1 Source/Element/Element.js
  2. +28 −0 Specs/1.4client/Element/Element.js
View
23 Source/Element/Element.js
@@ -818,7 +818,26 @@ Element.implement({
clone: function(contents, keepid){
contents = contents !== false;
- var clone = this.cloneNode(contents), ce = [clone], te = [this], i;
+ var cloneNode = function(){return false};
@arian Owner
arian added a note
var self = this;
var cloneNode = function(){
    return self.cloneNode(contents);
};
@kentaromiura Collaborator

I think I need to move this on outer scope to avoid memory leaks too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ /*<ltIE8>*/
+ if (Browser.ie6 || Browser.ie7){
@arian Owner
arian added a note

I think we could feature detect this..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ var self = this;
+ cloneNode = function(contents){
@arian Owner
arian added a note

don't need the contents argument, it can get the contents variable from the outer scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ var oldID = '' + self.id;
+ self.removeAttribute('id');
+ var clonedHTML = self.outerHTML;
+ var cloned = document.createElement('div');
+ if(!keepid) clonedHTML = clonedHTML.replace(/id=[^\ \>]/gim,'');
+ cloned.innerHTML = clonedHTML;
+ self.id = oldID;
+ var clone = cloned.firstChild;
+ if (keepid) clone.id = oldID;
+ if (!contents & clone.tagName !== 'INPUT' ) clone.innerHTML = '';
+ return clone;
+ }
+ }
+ /*</ltIE8>*/
+ var clone = cloneNode(contents) || this.cloneNode(contents), ce = [clone], te = [this], i;
if (contents){
ce.append(Array.from(clone.getElementsByTagName('*')));
@@ -849,6 +868,8 @@ Element.implement({
for (i = co.length; i--;) co[i].outerHTML = to[i].outerHTML;
}
/*</ltIE9>*/
+
+
return document.id(clone);
}
View
28 Specs/1.4client/Element/Element.js
@@ -10,6 +10,34 @@ describe('Element', function(){
describe('Element.getProperty', function(){
+ it('should cloneNode properly on IE6/7', function(){
@arian Owner
arian added a note

should be split up in multiple its.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ var div = new Element('div');
+ div.inject(document.documentElement);
+ div.innerHTML = "<input id='Q1' class='foo' rel='hai' />";
+
+ var q1 = document.id('Q1');
+ var clone = q1.clone();
+ clone.replaces(q1);
+
+ expect($$('input[id=Q1]').length).toEqual(0);
+ expect($$('input#Q1').length).toEqual(0);
+ expect($$('input[id=Q1]').length).toEqual(0); //<- element.erase will fail on this :(
+ clone.dispose();
+
+ var ul = new Element('ul',{id:'test'});
+ ul.set('html', '<li id="li1"></li><li id="li2"></li>');
+ ul.inject(document.documentElement);
+
+ var ulc = ul.clone();
+ ulc.replaces(ul);
+
+ expect($$('li#li1').length).toEqual(0);
+
+ ulc.destroy();
+
+ });
+
it('should get the attrubte of a form when the form has an input with as ID the attribute name', function(){
var div = new Element('div');
div.innerHTML = '<form action="s"><input id="action"></form>';
Something went wrong with that request. Please try again.