From eeb26436b0805022ab20dc3a869f6fb2b7965670 Mon Sep 17 00:00:00 2001
From: sasensi <asensi.samuel@gmail.com>
Date: Sat, 6 Oct 2018 15:37:44 +0200
Subject: [PATCH] Fix bounds error with nested empty items

Closes #1467
---
 src/item/Item.js    | 22 ++++++++++++++++++----
 test/tests/Group.js | 15 +++++++++++++++
 test/tests/Layer.js |  6 ++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/src/item/Item.js b/src/item/Item.js
index 71448a25..dddb4068 100644
--- a/src/item/Item.js
+++ b/src/item/Item.js
@@ -1072,7 +1072,9 @@ new function() { // Injection scope for various item event handlers
             options = options || {};
             for (var i = 0, l = items.length; i < l; i++) {
                 var item = items[i];
-                if (item._visible && !item.isEmpty()) {
+                // Item is handled if it is visible and not recursively empty.
+                // This avoid errors with nested empty groups (#1467).
+                if (item._visible && !item.isEmpty(true)) {
                     // Pass true for noInternal, since even when getting
                     // internal bounds for this item, we need to apply the
                     // matrices to its children.
@@ -2836,11 +2838,23 @@ new function() { // Injection scope for hit-test functions shared with project
      * no children, a {@link TextItem} with no text content and a {@link Path}
      * with no segments all are considered empty.
      *
-     * @return {Boolean}
+     * @param {Boolean} [recursively=false] whether an item with children should be
+     * considered empty if all its descendants are empty
+     * @return Boolean
      */
-    isEmpty: function() {
+    isEmpty: function(recursively) {
         var children = this._children;
-        return !children || !children.length;
+        var numChildren = children ? children.length : 0;
+        if (recursively) {
+            // In recursive check, item is empty if all its children are empty.
+            for (var i = 0; i < numChildren; i++) {
+                if (!children[i].isEmpty(recursively)) {
+                    return false;
+                }
+            }
+            return true;
+        }
+        return !numChildren;
     },
 
     /**
diff --git a/test/tests/Group.js b/test/tests/Group.js
index 52d7589d..17c59d06 100644
--- a/test/tests/Group.js
+++ b/test/tests/Group.js
@@ -151,3 +151,18 @@ test('group.setSelectedColor() with selected bound and position', function() {
     group2.selectedColor = 'black';
     comparePixels(group1, group2);
 });
+
+test('Group#isEmpty(recursively)', function() {
+    var group = new Group();
+    equals(true, group.isEmpty());
+    equals(true, group.isEmpty(true));
+    var group = new Group(new Group());
+    equals(false, group.isEmpty());
+    equals(true, group.isEmpty(true));
+    var group = new Group(new Path());
+    equals(false, group.isEmpty());
+    equals(true, group.isEmpty(true));
+    var group = new Group(new PointText());
+    equals(false, group.isEmpty());
+    equals(true, group.isEmpty(true));
+});
diff --git a/test/tests/Layer.js b/test/tests/Layer.js
index 2b73b52e..947f0ab4 100644
--- a/test/tests/Layer.js
+++ b/test/tests/Layer.js
@@ -139,3 +139,9 @@ test('#remove() with named layers', function(){
     equals(removeCount, 2,
             'project.layers[name].remove(); should be called twice');
 });
+
+test('#bounds with nested empty items', function() {
+    var item = new Path.Rectangle(new Point(10,10), new Size(10));
+    new Group(new Group());
+    equals(item.bounds, project.activeLayer.bounds);
+});