fix(fabric.Group) make addWithUpdate compatible with nested groups (#…

…6774)

src/canvas.class.js

@@ -1262,7 +1262,7 @@
         layoutProps.forEach(function(prop) {
           originalValues[prop] = instance[prop];
         });
-        this._activeObject.realizeTransform(instance);
+        fabric.util.addTransformToObject(instance, this._activeObject.calcOwnMatrix());
         return originalValues;
       }
       else {

src/shapes/group.class.js

@@ -156,17 +156,27 @@
      * @chainable
      */
     addWithUpdate: function(object) {
+      var nested = !!this.group;
       this._restoreObjectsState();
       fabric.util.resetObjectTransform(this);
       if (object) {
+        if (nested) {
+          // if this group is inside another group, we need to pre transform the object
+          fabric.util.removeTransformFromObject(object, this.group.calcTransformMatrix());
+        }
         this._objects.push(object);
         object.group = this;
         object._set('canvas', this.canvas);
       }
       this._calcBounds();
       this._updateObjectsCoords();
-      this.setCoords();
       this.dirty = true;
+      if (nested) {
+        this.group.addWithUpdate();
+      }
+      else {
+        this.setCoords();
+      }
       return this;
     },
 
@@ -357,12 +367,21 @@
 
     /**
      * Restores original state of each of group objects (original state is that which was before group was created).
+     * if the nested boolean is true, the original state will be restored just for the
+     * first group and not for all the group chain
      * @private
+     * @param {Boolean} nested tell the function to restore object state up to the parent group and not more
      * @return {fabric.Group} thisArg
      * @chainable
      */
     _restoreObjectsState: function() {
-      this._objects.forEach(this._restoreObjectState, this);
+      var groupMatrix = this.calcOwnMatrix();
+      this._objects.forEach(function(object) {
+        // instead of using _this = this;
+        fabric.util.addTransformToObject(object, groupMatrix);
+        delete object.group;
+        object.setCoords();
+      });
       return this;
     },
 
@@ -371,37 +390,20 @@
      * i.e. it tells you what would happen if the supplied object was in
      * the group, and then the group was destroyed. It mutates the supplied
      * object.
+     * Warning: this method is not useful anymore, it has been kept to no break the api.
+     * is not used in the fabricJS codebase
+     * this method will be reduced to using the utility.
+     * @private
+     * @deprecated
      * @param {fabric.Object} object
+     * @param {Array} parentMatrix parent transformation
      * @return {fabric.Object} transformedObject
      */
-    realizeTransform: function(object) {
-      var matrix = object.calcTransformMatrix(),
-          options = fabric.util.qrDecompose(matrix),
-          center = new fabric.Point(options.translateX, options.translateY);
-      object.flipX = false;
-      object.flipY = false;
-      object.set('scaleX', options.scaleX);
-      object.set('scaleY', options.scaleY);
-      object.skewX = options.skewX;
-      object.skewY = options.skewY;
-      object.angle = options.angle;
-      object.setPositionByOrigin(center, 'center', 'center');
+    realizeTransform: function(object, parentMatrix) {
+      fabric.util.addTransformToObject(object, parentMatrix);
       return object;
     },
 
-    /**
-     * Restores original state of a specified object in group
-     * @private
-     * @param {fabric.Object} object
-     * @return {fabric.Group} thisArg
-     */
-    _restoreObjectState: function(object) {
-      this.realizeTransform(object);
-      delete object.group;
-      object.setCoords();
-      return this;
-    },
-
     /**
      * Destroys a group (restoring state of its objects)
      * @return {fabric.Group} thisArg
@@ -474,19 +476,20 @@
     _calcBounds: function(onlyWidthHeight) {
       var aX = [],
           aY = [],
-          o, prop,
+          o, prop, coords,
           props = ['tr', 'br', 'bl', 'tl'],
           i = 0, iLen = this._objects.length,
           j, jLen = props.length;
 
       for ( ; i < iLen; ++i) {
         o = this._objects[i];
-        o.aCoords = o.calcACoords();
+        coords = o.calcACoords();
         for (j = 0; j < jLen; j++) {
           prop = props[j];
-          aX.push(o.aCoords[prop].x);
-          aY.push(o.aCoords[prop].y);
+          aX.push(coords[prop].x);
+          aY.push(coords[prop].y);
         }
+        o.aCoords = coords;
       }
 
       this._getBounds(aX, aY, onlyWidthHeight);

src/util/misc.js

@@ -969,6 +969,59 @@
       }).join(' ') + ')';
     },
 
+    /**
+     * given an object and a transform, apply the inverse transform to the object,
+     * this is equivalent to remove from that object that transformation, so that
+     * added in a space with the removed transform, the object will be the same as before.
+     * Removing from an object a transform that scale by 2 is like scaling it by 1/2.
+     * Removing from an object a transfrom that rotate by 30deg is like rotating by 30deg
+     * in the opposite direction.
+     * This util is used to add objects inside transformed groups or nested groups.
+     * @memberOf fabric.util
+     * @param {fabric.Object} object the object you want to transform
+     * @param {Array} transform the destination transform
+     */
+    removeTransformFromObject: function(object, transform) {
+      var inverted = fabric.util.invertTransform(transform),
+          finalTransform = fabric.util.multiplyTransformMatrices(inverted, object.calcOwnMatrix());
+      fabric.util.applyTransformToObject(object, finalTransform);
+    },
+
+    /**
+     * given an object and a transform, apply the transform to the object.
+     * this is equivalent to change the space where the object is drawn.
+     * Adding to an object a transform that scale by 2 is like scaling it by 2.
+     * This is used when removing an object from an active selection for example.
+     * @memberOf fabric.util
+     * @param {fabric.Object} object the object you want to transform
+     * @param {Array} transform the destination transform
+     */
+    addTransformToObject: function(object, transform) {
+      fabric.util.applyTransformToObject(
+        object,
+        fabric.util.multiplyTransformMatrices(transform, object.calcOwnMatrix())
+      );
+    },
+
+    /**
+     * discard an object transform state and apply the one from the matrix.
+     * @memberOf fabric.util
+     * @param {fabric.Object} object the object you want to transform
+     * @param {Array} transform the destination transform
+     */
+    applyTransformToObject: function(object, transform) {
+      var options = fabric.util.qrDecompose(transform),
+          center = new fabric.Point(options.translateX, options.translateY);
+      object.flipX = false;
+      object.flipY = false;
+      object.set('scaleX', options.scaleX);
+      object.set('scaleY', options.scaleY);
+      object.skewX = options.skewX;
+      object.skewY = options.skewY;
+      object.angle = options.angle;
+      object.setPositionByOrigin(center, 'center', 'center');
+    },
+
     /**
      * given a width and height, return the size of the bounding box
      * that can contains the box with width/height with applied transform

test/unit/group.js

@@ -818,6 +818,50 @@
     assert.equal(group._objects[1].canvas, canvas, 'canvas has been set on object 0');
   });
 
+  QUnit.test('addWithUpdate and coordinates', function(assert) {
+    var rect1 = new fabric.Rect({ top: 1, left: 1, width: 3, height: 2, strokeWidth: 0, fill: 'red' }),
+        rect2 = new fabric.Rect({ top: 5, left: 5, width: 2, height: 6, angle: 90, strokeWidth: 0, fill: 'red' }),
+        group = new fabric.Group([]);
+    group.addWithUpdate(rect1);
+    group.addWithUpdate(rect2);
+    group.left = 5;
+    group.top = 5;
+    group.scaleX = 3;
+    group.scaleY = 2;
+    group.destroy();
+    assert.equal(rect1.top, 5, 'top has been moved');
+    assert.equal(rect1.left, 11, 'left has been moved');
+    assert.equal(rect1.scaleX, 3, 'scaleX has been scaled');
+    assert.equal(rect1.scaleY, 2, 'scaleY has been scaled');
+    assert.equal(rect2.top, 13, 'top has been moved');
+    assert.equal(rect2.left, 23, 'left has been moved');
+    assert.equal(rect2.scaleX, 2, 'scaleX has been scaled inverted because of angle 90');
+    assert.equal(rect2.scaleY, 3, 'scaleY has been scaled inverted because of angle 90');
+  });
+
+  QUnit.test('addWithUpdate and coordinates with nested groups', function(assert) {
+    var rect1 = new fabric.Rect({ top: 1, left: 1, width: 3, height: 2, strokeWidth: 0, fill: 'red' }),
+        rect2 = new fabric.Rect({ top: 5, left: 5, width: 2, height: 6, angle: 90, strokeWidth: 0, fill: 'red' }),
+        group0 = new fabric.Group([rect1, rect2]),
+        rect3 = new fabric.Rect({ top: 2, left: 9, width: 3, height: 2, strokeWidth: 0, fill: 'red' }),
+        rect4 = new fabric.Rect({ top: 3, left: 5, width: 2, height: 6, angle: 90, strokeWidth: 0, fill: 'red' }),
+        group1 = new fabric.Group([rect3, rect4], { scaleX: 3, scaleY: 4 }),
+        group = new fabric.Group([group0, group1], { angle: 90, scaleX: 2, scaleY: 0.5 }),
+        rect5 = new fabric.Rect({ top: 1, left: 1, width: 3, height: 2, strokeWidth: 0, fill: 'red' });
+
+    group1.addWithUpdate(rect5);
+    assert.equal(rect5.top, -5.5, 'top has been moved');
+    assert.equal(rect5.left, -19.5, 'left has been moved');
+    assert.equal(rect5.scaleX, 2, 'scaleX has been scaled');
+    assert.equal(rect5.scaleY, 0.5, 'scaleY has been scaled');
+    group.destroy();
+    group1.destroy();
+    assert.equal(rect5.top, 1, 'top is back to original minus rounding errors');
+    assert.equal(rect5.left, 1, 'left is back to original');
+    assert.equal(rect5.scaleX, 1, 'scaleX is back to original');
+    assert.equal(rect5.scaleY, 1, 'scaleY is back to original');
+  });
+
   // QUnit.test('cloning group with image', function(assert) {
   //   var done = assert.async();
   //   var rect = new fabric.Rect({ top: 100, left: 100, width: 30, height: 10 }),