Skip to content
This repository
Browse code

Fixed (or at least returned to 3.4.1 state), the sublest regression e…

…ver,

related to Y.cloning Attribute driven objects with Node values (e.g. Anim),
on IE.

Fundamentally, Y.cloning nodes is broken in IE and has been since 3.3.0.

If an object holds multiple references to a Node instance, the first
reference encountered while cloning, will be a damaged clone. Subsequent
references encountered will not clone (and hence be OK).

After the Attribute refactor, the value stored in State was the first
reference encountered, resulting in permanent damage.

Before the refactor, a transient event based reference was the first reference
encountered, and hence was less likely to cause real-world problems.

* Added unit test cases to cover cloning Attribute based objects, with node values
* Tested on IE6, 7, 8, 9.
* Ran some core unit tests (App, Plugin, Widget, Attribute, Base, Node) through Yeti on IE8, Chrome, Safari, FF.
* Spot tested a couple of examples (Widget/App/Attribute).

Wanted to get this in before testfest tomorrow.

Fixes #2531929

Longer term fix for Y.clone(node) on IE is still required.
  • Loading branch information...
commit 0b4bad3bc665dbd4cfc6d3c2b65a85b7c6781767 1 parent 31689fe
authored March 26, 2012
10  src/attribute/js/Attribute.js
@@ -47,8 +47,16 @@
47 47
      * @uses AttributeCore
48 48
      * @uses AttributeEvents
49 49
      * @uses AttributeExtras
50  
-     */    
  50
+     */
51 51
     var Attribute = function() {
  52
+
  53
+        // Fix #2531929 
  54
+        // Complete hack, to make sure the first clone of a node value in IE doesn't doesn't hurt state - maintains 3.4.1 behavior.
  55
+        // Too late in the release cycle to do anything about the core problem.
  56
+        // The root issue is that cloning a Y.Node instance results in an object which barfs in IE, when you access it's properties (since 3.3.0).
  57
+        this._ATTR_E_FACADE = null;
  58
+        this._yuievt = null;
  59
+
52 60
         Y.AttributeCore.apply(this, arguments);
53 61
         Y.AttributeEvents.apply(this, arguments);
54 62
         Y.AttributeExtras.apply(this, arguments);
114  src/attribute/tests/attribute.html
@@ -700,6 +700,61 @@
700 700
                     Y.Assert.isTrue(valueChangeCalled, "after(valueChange) not called");
701 701
 
702 702
                     node.remove(true);
  703
+                },
  704
+
  705
+                testAugmentedCloneWithNodeValue : function() {
  706
+                    function FooBar(userVals) {
  707
+                        Y.Attribute.call(this, null, userVals);
  708
+                    };
  709
+
  710
+                    FooBar.ATTRS = {
  711
+                        node : {
  712
+                            value:null
  713
+                        }
  714
+                    };
  715
+
  716
+                    // Straightup augment, no wrapper functions
  717
+                    Y.mix(FooBar, Y.Attribute, false, null, 1);
  718
+
  719
+                    var n = Y.Node.create('<div id="foobar"></div>');
  720
+
  721
+                    var o1 = new FooBar();
  722
+                    o1.set("node", n);
  723
+
  724
+                    var o2 = Y.clone(o1);
  725
+
  726
+                    Y.Assert.areEqual("foobar", o2.get("node").get("id"), "Expected foobar as the id of the cloned node");
  727
+                    Y.Assert.isObject(o2.get("node")._node.style, "Unable to access the cloned HTMLElement");
  728
+
  729
+                    n.destroy();
  730
+                },
  731
+
  732
+                testAugmentedCloneWithNodeValueConstructor : function() {
  733
+                    function FooBar(userVals) {
  734
+                        Y.Attribute.call(this, null, userVals);
  735
+                    };
  736
+
  737
+                    FooBar.ATTRS = {
  738
+                        node : {
  739
+                            value:null
  740
+                        }
  741
+                    };
  742
+
  743
+                    // Straightup augment, no wrapper functions
  744
+                    Y.mix(FooBar, Y.Attribute, false, null, 1);
  745
+
  746
+                    var n = Y.Node.create('<div id="foobar2"></div>');
  747
+
  748
+                    var o1 = new FooBar({
  749
+                        node: n
  750
+                    });
  751
+
  752
+                    var o2 = Y.clone(o1);
  753
+
  754
+                    Y.Assert.areEqual("foobar2", o2.get("node").get("id"), "Expected foobar2 as the id of the cloned node");
  755
+                    Y.Assert.isObject(o2.get("node")._node.style, "Unable to access the cloned HTMLElement");
  756
+
  757
+                    n.destroy();
703 758
                 }
704 759
             };
705 760
 
@@ -1348,7 +1403,66 @@
1348 1403
                     q.A.value = "modified value";
1349 1404
 
1350 1405
                     Y.Assert.areNotEqual(Y.dump(AttrHost.ATTRS), Y.dump(q));
  1406
+                },
  1407
+
  1408
+                testCloneWithNodeValue : function() {
  1409
+
  1410
+                    function FooBar(userVals) {
  1411
+                        FooBar.superclass.constructor.apply(this, arguments);
  1412
+                    };
  1413
+
  1414
+                    FooBar.NAME = "foobar"; 
  1415
+
  1416
+                    FooBar.ATTRS = {
  1417
+                        node : {
  1418
+                            value:null
  1419
+                        }
  1420
+                    };
  1421
+
  1422
+                    Y.extend(FooBar, Y.Base);
  1423
+
  1424
+                    var n = Y.Node.create('<div id="foobar3"></div>');
  1425
+
  1426
+                    var o1 = new FooBar();
  1427
+                    o1.set("node", n);
  1428
+
  1429
+                    var o2 = Y.clone(o1);
  1430
+
  1431
+                    Y.Assert.areEqual("foobar3", o2.get("node").get("id"), "Expected foobar3 as the id of the cloned node");
  1432
+                    Y.Assert.isObject(o2.get("node")._node.style, "Unable to access properties of the cloned HTMLElement");
  1433
+
  1434
+                    n.destroy();
  1435
+                },
  1436
+
  1437
+                testCloneWithNodeValueConstructor : function() {
  1438
+
  1439
+                    function FooBar(userVals) {
  1440
+                        FooBar.superclass.constructor.apply(this, arguments);
  1441
+                    };
  1442
+
  1443
+                    FooBar.NAME = "foobar"; 
  1444
+
  1445
+                    FooBar.ATTRS = {
  1446
+                        node : {
  1447
+                            value:null
  1448
+                        }
  1449
+                    };
  1450
+
  1451
+                    Y.extend(FooBar, Y.Base);
  1452
+
  1453
+                    var n = Y.Node.create('<div id="foobar4"></div>');
  1454
+
  1455
+                    var o1 = new FooBar({
  1456
+                        node: n
  1457
+                    });
  1458
+                    var o2 = Y.clone(o1);
  1459
+
  1460
+                    Y.Assert.areEqual("foobar4", o2.get("node").get("id"), "Expected foobar4 as the id of the cloned node");
  1461
+                    Y.Assert.isObject(o2.get("node")._node.style, "Unable to access properties of the cloned HTMLElement");
  1462
+
  1463
+                    n.destroy();
1351 1464
                 }
  1465
+
1352 1466
             };
1353 1467
 
1354 1468
             basicTemplate = Y.merge(basicTemplate, sharedEventTests);

4 notes on commit 0b4bad3

Eric Ferraiuolo
Owner

@sdesai I was talking to @rgrove the other week about this. It seems like we need to protect against cloning certain classes of objects, like HTMLElements, to avoid having objects which are broken.

Satyen Desai
Collaborator

Right. We do that for certain types already (for example we don't attempt to clone RegExp objects or Function objects, or recurse into HTMLElement properties, for obvious reasons).

In fact I'd say we may want to make clone "YUI" aware (at least in a certain mode), and not clone heavier objects like Anim, DD, Base-based objects etc. It's rarely (20%) the user's intention to do this. It usually happens inadvertently when one of these heavier objects hangs off something more basic [ like a multi-level object hash ] which they're cloning.

Also, bigger picture - we just want to do, and promise, less deep cloning.

Satyen Desai
Collaborator

Couple more clarifications which I forgot to add:

  1. In IE's case the root issue is Y.Object(HTMLElement) results in a "bad" object.

  2. The other clone cleanup required (based on my debugging) seems to be that if we have 2 references to the same object o1, in an object o, say o.foo = o1; o.bar = o1, when o is cloned, we end up replacing the first reference (foo) with a clone of o1, but bar remains the uncloned o1. I'm not sure if this is intentional. It seems like we'd want to use the cached clone, but that's just off the top of my head.

Please sign in to comment.
Something went wrong with that request. Please try again.