Move color owner handling directly to Color class

There was already Color#_owner, now there is Color#_setter too

CHANGELOG.md

@@ -11,6 +11,7 @@
 - Fix drawing with compound-paths as clip-items (#1361).
 - Fix drawing of path selection with small handle size (#1327).
 - Correctly calculate bounds with nested empty items (#1467).
+- Fix color change propagation on groups (#1152).
 - SVG Export: Fix error when `Item#matrix` is not invertible (#1580).
 - SVG Export: Include missing viewBox attribute (#1576).
 - SVG Import: Use correct default values for gradients (#1632, #1661).

src/style/Color.js

@@ -711,8 +711,9 @@ var Color = Base.extend(new function() {
          */
         _changed: function() {
             this._canvasStyle = null;
-            if (this._owner)
-                this._owner._changed(/*#=*/Change.STYLE);
+            if (this._owner) {
+                this._owner[this._setter](this);
+            }
         },
 
         /**
@@ -1200,6 +1201,20 @@ var Color = Base.extend(new function() {
             random: function() {
                 var random = Math.random;
                 return new Color(random(), random(), random());
+            },
+
+            _setOwner: function(color, owner, setter) {
+                if (color) {
+                    // Clone color if owner changes:
+                    if (color._owner && owner && color._owner !== owner) {
+                        color = color.clone();
+                    }
+                    if (!color._owner ^ !owner) {
+                        color._owner = owner || null;
+                        color._setter = setter || null;
+                    }
+                }
+                return color;
             }
         }
     });

src/style/GradientStop.js

@@ -175,12 +175,10 @@ var GradientStop = Base.extend(/** @lends GradientStop# */{
     },
 
     setColor: function(/* color */) {
-        // Make sure newly set colors are cloned, since they can only have
-        // one owner.
-        var color = Color.read(arguments, 0, { clone: true });
-        if (color)
-            color._owner = this;
-        this._color = color;
+        // Clear old color owner before setting new one:
+        Color._setOwner(this._color, null);
+        this._color = Color._setOwner(Color.read(arguments, 0), this,
+                'setColor');
         this._changed();
     },
 

src/style/Style.js

@@ -180,17 +180,14 @@ var Style = Base.extend(new function() {
                     if (isColor) {
                         // The old value may be a native string or other color
                         // description that wasn't coerced to a color object yet
-                        if (old && old._owner !== undefined) {
-                            old._owner = undefined;
+                        if (old) {
+                            Color._setOwner(old, null);
                             old._canvasStyle = null;
                         }
                         if (value && value.constructor === Color) {
-                            // Clone color if it already has an owner.
                             // NOTE: If value is not a Color, it is only
                             // converted and cloned in the getter further down.
-                            if (value._owner)
-                                value = value.clone();
-                            value._owner = owner;
+                            value = Color._setOwner(value, owner, set);
                         }
                     }
                     // NOTE: We do not convert the values to Colors in the
@@ -216,8 +213,10 @@ var Style = Base.extend(new function() {
                 var value = this._values[key];
                 if (value === undefined) {
                     value = this._defaults[key];
-                    if (value && value.clone)
+                    // Clone defaults if available:
+                    if (value && value.clone) {
                         value = value.clone();
+                    }
                 } else {
                     var ctor = isColor ? Color : isPoint ? Point : null;
                     if (ctor && !(value && value.constructor === ctor)) {
@@ -225,8 +224,6 @@ var Style = Base.extend(new function() {
                         // conversion.
                         this._values[key] = value = ctor.read([value], 0,
                                 { readNull: true, clone: true });
-                        if (value && isColor)
-                            value._owner = owner;
                     }
                 }
             } else if (children) {
@@ -241,11 +238,10 @@ var Style = Base.extend(new function() {
                     }
                 }
             }
-            // Turn group related colors into LinkedColor instances that will
-            // allow calls like `group.fillColor.hue += 10` to be propagated to
-            // children.
-            if (owner instanceof Group && value instanceof Color) {
-                value = new LinkedColor(value, owner, set);
+            if (value && isColor) {
+                // Color._setOwner() may clone the color if it already has a
+                // different owner (e.g. resulting from `childValue` above):
+                value = Color._setOwner(value, owner, set);
             }
             return value;
         };

test/helpers.js

@@ -106,7 +106,7 @@ var equals = function(actual, expected, message, options) {
             || type === 'boolean' && 'Boolean'
             || type === 'undefined' && 'Undefined'
             || Array.isArray(expected) && 'Array'
-            || expected instanceof window.Element && 'Element' // handle DOM Elements
+            || expected instanceof window.Element && 'Element' // DOM Elements
             || (cls = expected && expected._class) // check _class 2nd last
             || type === 'object' && 'Object'; // Object as catch-all
     var comparator = type && comparators[type];
@@ -456,7 +456,7 @@ var comparators = {
             equals(actual.components, expected.components,
                     message + ' (#components)', options);
         } else {
-            QUnit.strictEqual(actual, expected, message);
+            QUnit.push(expected.equals(actual), actual, expected, message);
         }
     },
 

test/tests/Color.js

@@ -303,10 +303,10 @@ test('Gradients with applyMatrix', function() {
     comparePixels(path, shape);
 });
 
-test('LinkedColor for group colors', function() {
+test('Modifying group.strokeColor for multiple children', function() {
     var item = new Group(new Path(), new Path());
     item.strokeColor = 'red';
+    var strokeColor = item.strokeColor;
     item.strokeColor.hue = 50;
-    item.strokeColor.hue = 100;
-    expect(0);
+    equals(function() { return item.strokeColor !== undefined; }, true);
 });