feat(Group): 2nd Patch of New Group! 🎉 (#7859)

Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>

src/brushes/spray_brush.class.js

@@ -112,7 +112,12 @@ fabric.SprayBrush = fabric.util.createClass( fabric.BaseBrush, /** @lends fabric
       rects = this._getOptimizedRects(rects);
     }
 
-    var group = new fabric.Group(rects);
+    var group = new fabric.Group(rects, {
+      objectCaching: true,
+      layout: 'fixed',
+      subTargetCheck: false,
+      interactive: false
+    });
     this.shadow && group.set('shadow', new fabric.Shadow(this.shadow));
     this.canvas.fire('before:path:created', { path: group });
     this.canvas.add(group);

src/canvas.class.js

@@ -441,7 +441,7 @@
         }
         objsToRender.push.apply(objsToRender, activeGroupObjects);
       }
-      //  in case a single object is selected render it's entire above the other objects
+      //  in case a single object is selected render it's entire parent above the other objects
       else if (!this.preserveObjectStacking && activeObjects.length === 1) {
         var target = activeObjects[0], ancestors = target.getAncestors(true);
         var topAncestor = ancestors.length === 0 ? target : ancestors.pop();
@@ -892,7 +892,7 @@
      */
     searchPossibleTargets: function (objects, pointer) {
       var target = this._searchPossibleTargets(objects, pointer);
-      return target;
+      return target && target.interactive && this.targets[0] ? this.targets[0] : target;
     },
 
     /**

src/mixins/animation.mixin.js

@@ -25,11 +25,11 @@ fabric.util.object.extend(fabric.StaticCanvas.prototype, /** @lends fabric.Stati
 
     return fabric.util.animate({
       target: this,
-      startValue: object.left,
+      startValue: object.getX(),
       endValue: this.getCenterPoint().x,
       duration: this.FX_DURATION,
       onChange: function(value) {
-        object.set('left', value);
+        object.setX(value);
         _this.requestRenderAll();
         onChange();
       },
@@ -58,11 +58,11 @@ fabric.util.object.extend(fabric.StaticCanvas.prototype, /** @lends fabric.Stati
 
     return fabric.util.animate({
       target: this,
-      startValue: object.top,
+      startValue: object.getY(),
       endValue: this.getCenterPoint().y,
       duration: this.FX_DURATION,
       onChange: function(value) {
-        object.set('top', value);
+        object.setY(value);
         _this.requestRenderAll();
         onChange();
       },

src/mixins/canvas_events.mixin.js

@@ -712,9 +712,13 @@
           }
         }
       }
+      var invalidate = shouldRender || shouldGroup;
+      //  we clear `_objectsToRender` in case of a change in order to repopulate it at rendering
+      //  run before firing the `down` event to give the dev a chance to populate it themselves
+      invalidate && (this._objectsToRender = undefined);
       this._handleEvent(e, 'down');
       // we must renderAll so that we update the visuals
-      (shouldRender || shouldGroup) && this.requestRenderAll();
+      invalidate && this.requestRenderAll();
     },
 
     /**
@@ -900,13 +904,19 @@
      */
     _transformObject: function(e) {
       var pointer = this.getPointer(e),
-          transform = this._currentTransform;
+          transform = this._currentTransform,
+          target = transform.target,
+          //  transform pointer to target's containing coordinate plane
+          //  both pointer and object should agree on every point
+          localPointer = target.group ?
+            fabric.util.sendPointToPlane(pointer, null, target.group.calcTransformMatrix()) :
+            pointer;
 
       transform.reset = false;
       transform.shiftKey = e.shiftKey;
       transform.altKey = e[this.centeredKey];
 
-      this._performTransformAction(e, transform, pointer);
+      this._performTransformAction(e, transform, localPointer);
       transform.actionPerformed && this.requestRenderAll();
     },
 

src/mixins/canvas_grouping.mixin.js

@@ -13,8 +13,19 @@
      */
     _shouldGroup: function(e, target) {
       var activeObject = this._activeObject;
-      return activeObject && this._isSelectionKeyPressed(e) && target && target.selectable && this.selection &&
-            (activeObject !== target || activeObject.type === 'activeSelection') && !target.onSelect({ e: e });
+      // check if an active object exists on canvas and if the user is pressing the `selectionKey` while canvas supports multi selection.
+      return !!activeObject && this._isSelectionKeyPressed(e) && this.selection
+        // on top of that the user also has to hit a target that is selectable.
+        && !!target && target.selectable
+        // if all pre-requisite pass, the target is either something different from the current
+        // activeObject or if an activeSelection already exists
+        // TODO at time of writing why `activeObject.type === 'activeSelection'` matter is unclear.
+        // is a very old condition uncertain if still valid.
+        && (activeObject !== target || activeObject.type === 'activeSelection')
+        //  make sure `activeObject` and `target` aren't ancestors of each other
+        && !target.isDescendantOf(activeObject) && !activeObject.isDescendantOf(target)
+        //  target accepts selection
+        && !target.onSelect({ e: e });
     },
 
     /**
@@ -50,7 +61,7 @@
     _updateActiveSelection: function(target, e) {
       var activeSelection = this._activeObject,
           currentActiveObjects = activeSelection._objects.slice(0);
-      if (activeSelection.contains(target)) {
+      if (target.group === activeSelection) {
         activeSelection.remove(target);
         this._hoveredTarget = target;
         this._hoveredTargets = this.targets.concat();
@@ -80,17 +91,19 @@
       this._fireSelectionEvents(currentActives, e);
     },
 
+
     /**
      * @private
      * @param {Object} target
+     * @returns {fabric.ActiveSelection}
      */
     _createGroup: function(target) {
-      var objects = this._objects,
-          isActiveLower = objects.indexOf(this._activeObject) < objects.indexOf(target),
-          groupObjects = isActiveLower
-            ? [this._activeObject, target]
-            : [target, this._activeObject];
-      this._activeObject.isEditing && this._activeObject.exitEditing();
+      var activeObject = this._activeObject;
+      var groupObjects = target.isInFrontOf(activeObject) ?
+        [activeObject, target] :
+        [target, activeObject];
+      activeObject.isEditing && activeObject.exitEditing();
+      //  handle case: target is nested
       return new fabric.ActiveSelection(groupObjects, {
         canvas: this
       });

src/mixins/collection.mixin.js

@@ -20,7 +20,7 @@ fabric.Collection = {
   add: function (objects, callback) {
     var size = this._objects.push.apply(this._objects, objects);
     if (callback) {
-      for (var i = 0, length = objects.length; i < length; i++) {
+      for (var i = 0; i < objects.length; i++) {
         callback.call(this, objects[i]);
       }
     }
@@ -39,7 +39,7 @@ fabric.Collection = {
     var args = [index, 0].concat(objects);
     this._objects.splice.apply(this._objects, args);
     if (callback) {
-      for (var i = 2, length = args.length; i < length; i++) {
+      for (var i = 2; i < args.length; i++) {
         callback.call(this, args[i]);
       }
     }
@@ -50,22 +50,21 @@ fabric.Collection = {
    * @private
    * @param {fabric.Object[]} objectsToRemove objects to remove
    * @param {(object:fabric.Object) => any} [callback] function to call for each object removed
-   * @returns {boolean} true if objects were removed
+   * @returns {fabric.Object[]} removed objects
    */
   remove: function(objectsToRemove, callback) {
-    var objects = this._objects,
-        index, somethingRemoved = false;
-
-    for (var i = 0, length = objectsToRemove.length; i < length; i++) {
-      index = objects.indexOf(objectsToRemove[i]);
+    var objects = this._objects, removed = [];
+    for (var i = 0, object, index; i < objectsToRemove.length; i++) {
+      object = objectsToRemove[i];
+      index = objects.indexOf(object);
       // only call onObjectRemoved if an object was actually removed
       if (index !== -1) {
-        somethingRemoved = true;
         objects.splice(index, 1);
-        callback && callback.call(this, objectsToRemove[i]);
+        removed.push(object);
+        callback && callback.call(this, object);
       }
     }
-    return somethingRemoved;
+    return removed;
   },
 
   /**
@@ -82,7 +81,7 @@ fabric.Collection = {
    */
   forEachObject: function(callback, context) {
     var objects = this.getObjects();
-    for (var i = 0, len = objects.length; i < len; i++) {
+    for (var i = 0; i < objects.length; i++) {
       callback.call(context, objects[i], i, objects);
     }
     return this;
@@ -131,6 +130,7 @@ fabric.Collection = {
   /**
    * Returns true if collection contains an object.\
    * **Prefer using {@link `fabric.Object#isDescendantOf`} for performance reasons**
+   * instead of a.contains(b) use b.isDescendantOf(a)
    * @param {Object} object Object to check against
    * @param {Boolean} [deep=false] `true` to check all descendants, `false` to check only `_objects`
    * @return {Boolean} `true` if collection contains an object

src/mixins/eraser_brush.mixin.js

@@ -125,7 +125,6 @@
     /* _TO_SVG_END_ */
   });
 
-  var __restoreObjectsState = fabric.Group.prototype._restoreObjectsState;
   fabric.util.object.extend(fabric.Group.prototype, {
     /**
      * @private
@@ -181,15 +180,6 @@
               });
           }
         });
-    },
-
-    /**
-     * Propagate the group's eraser to its objects, crucial for proper functionality of the eraser within the group and nested objects.
-     * @private
-     */
-    _restoreObjectsState: function () {
-      this.erasable === true && this.applyEraserToObjects();
-      return __restoreObjectsState.call(this);
     }
   });
 
@@ -217,23 +207,21 @@
      */
     originY: 'center',
 
-    drawObject: function (ctx) {
-      ctx.save();
-      ctx.fillStyle = 'black';
-      ctx.fillRect(-this.width / 2, -this.height / 2, this.width, this.height);
-      ctx.restore();
-      this.callSuper('drawObject', ctx);
-    },
-
     /**
      * eraser should retain size
      * dimensions should not change when paths are added or removed
      * handled by {@link fabric.Object#_drawClipPath}
      * @override
      * @private
      */
-    _getBounds: function () {
-      //  noop
+    layout: 'fixed',
+
+    drawObject: function (ctx) {
+      ctx.save();
+      ctx.fillStyle = 'black';
+      ctx.fillRect(-this.width / 2, -this.height / 2, this.width, this.height);
+      ctx.restore();
+      this.callSuper('drawObject', ctx);
     },
 
     /* _TO_SVG_START_ */

src/mixins/itext_behavior.mixin.js

@@ -25,8 +25,9 @@
      */
     initAddedHandler: function() {
       var _this = this;
-      this.on('added', function() {
-        var canvas = _this.canvas;
+      this.on('added', function (opt) {
+        //  make sure we listen to the canvas added event
+        var canvas = opt.target;
         if (canvas) {
           if (!canvas._hasITextHandlers) {
             canvas._hasITextHandlers = true;
@@ -40,8 +41,9 @@
 
     initRemovedHandler: function() {
       var _this = this;
-      this.on('removed', function() {
-        var canvas = _this.canvas;
+      this.on('removed', function (opt) {
+        //  make sure we listen to the canvas removed event
+        var canvas = opt.target;
         if (canvas) {
           canvas._iTextInstances = canvas._iTextInstances || [];
           fabric.util.removeFromArray(canvas._iTextInstances, _this);

src/mixins/itext_click_behavior.mixin.js

@@ -152,7 +152,8 @@ fabric.util.object.extend(fabric.IText.prototype, /** @lends fabric.IText.protot
    */
   mouseUpHandler: function(options) {
     this.__isMousedown = false;
-    if (!this.editable || this.group ||
+    if (!this.editable ||
+      (this.group && !this.group.interactive) ||
       (options.transform && options.transform.actionPerformed) ||
       (options.e.button && options.e.button !== 1)) {
       return;