diff --git a/CHANGELOG.md b/CHANGELOG.md index 257583d6..9f0ddee8 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/src/style/Color.js b/src/style/Color.js index 0a2ee035..d751d824 100644 --- a/src/style/Color.js +++ b/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; } } }); diff --git a/src/style/GradientStop.js b/src/style/GradientStop.js index 72367d11..b1d2d427 100644 --- a/src/style/GradientStop.js +++ b/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(); }, diff --git a/src/style/Style.js b/src/style/Style.js index 5e6a4bf7..ea27669b 100644 --- a/src/style/Style.js +++ b/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; }; diff --git a/test/helpers.js b/test/helpers.js index cc8cabf5..28e8be91 100644 --- a/test/helpers.js +++ b/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); } }, diff --git a/test/tests/Color.js b/test/tests/Color.js index 802bcb72..363cdcc8 100644 --- a/test/tests/Color.js +++ b/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); });