From 34afb379a73b50374c719601278aa7a489de4b30 Mon Sep 17 00:00:00 2001 From: codert Date: Tue, 2 Oct 2018 00:00:54 +0800 Subject: [PATCH 01/12] fix perPixelTargetFind in nested object --- src/canvas.class.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/canvas.class.js b/src/canvas.class.js index 1389ae38211..49e518879f1 100644 --- a/src/canvas.class.js +++ b/src/canvas.class.js @@ -1158,6 +1158,7 @@ var ignoreZoom = true, pointer = this.getPointer(e, ignoreZoom), + globalPointer = pointer, activeObject = this._activeObject, aObjects = this.getActiveObjects(), activeTarget, activeTargetSubs; @@ -1167,7 +1168,7 @@ // if active group just exits. this.targets = []; - if (aObjects.length > 1 && !skipGroup && activeObject === this._searchPossibleTargets([activeObject], pointer)) { + if (aObjects.length > 1 && !skipGroup && activeObject === this._searchPossibleTargets([activeObject], pointer, globalPointer)) { return activeObject; } // if we hit the corner of an activeObject, let's return that. @@ -1175,7 +1176,7 @@ return activeObject; } if (aObjects.length === 1 && - activeObject === this._searchPossibleTargets([activeObject], pointer)) { + activeObject === this._searchPossibleTargets([activeObject], pointer, globalPointer)) { if (!this.preserveObjectStacking) { return activeObject; } @@ -1185,7 +1186,7 @@ this.targets = []; } } - var target = this._searchPossibleTargets(this._objects, pointer); + var target = this._searchPossibleTargets(this._objects, pointer, globalPointer); if (e[this.altSelectionKey] && target && activeTarget && target !== activeTarget) { target = activeTarget; this.targets = activeTargetSubs; @@ -1196,13 +1197,14 @@ /** * @private */ - _checkTarget: function(pointer, obj) { + _checkTarget: function(pointer, obj, globalPointer) { + globalPointer = globalPointer || pointer; if (obj && obj.visible && obj.evented && this.containsPoint(null, obj, pointer)){ if ((this.perPixelTargetFind || obj.perPixelTargetFind) && !obj.isEditing) { - var isTransparent = this.isTargetTransparent(obj, pointer.x, pointer.y); + var isTransparent = this.isTargetTransparent(obj, globalPointer.x, globalPointer.y); if (!isTransparent) { return true; } @@ -1216,18 +1218,18 @@ /** * @private */ - _searchPossibleTargets: function(objects, pointer) { + _searchPossibleTargets: function(objects, pointer, pointerOnCanvas) { // Cache all targets where their bounding box contains point. var target, i = objects.length, normalizedPointer, subTarget; // Do not check for currently grouped objects, since we check the parent group itself. // until we call this function specifically to search inside the activeGroup while (i--) { - if (this._checkTarget(pointer, objects[i])) { + if (this._checkTarget(pointer, objects[i], pointerOnCanvas)) { target = objects[i]; if (target.subTargetCheck && target instanceof fabric.Group) { normalizedPointer = this._normalizePointer(target, pointer); - subTarget = this._searchPossibleTargets(target._objects, normalizedPointer); + subTarget = this._searchPossibleTargets(target._objects, normalizedPointer, pointerOnCanvas); subTarget && this.targets.push(subTarget); } break; From 02cfb332a1f3acdfccc93b142f0b79251d9ce8a8 Mon Sep 17 00:00:00 2001 From: codert Date: Tue, 2 Oct 2018 00:10:58 +0800 Subject: [PATCH 02/12] fix: lint --- src/canvas.class.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/canvas.class.js b/src/canvas.class.js index 49e518879f1..34dac6eb3a3 100644 --- a/src/canvas.class.js +++ b/src/canvas.class.js @@ -1168,7 +1168,9 @@ // if active group just exits. this.targets = []; - if (aObjects.length > 1 && !skipGroup && activeObject === this._searchPossibleTargets([activeObject], pointer, globalPointer)) { + if (aObjects.length > 1 && + !skipGroup && + activeObject === this._searchPossibleTargets([activeObject],pointer, globalPointer)) { return activeObject; } // if we hit the corner of an activeObject, let's return that. From 5985429003d4e28473314cf31e16b43418e49fa9 Mon Sep 17 00:00:00 2001 From: codert Date: Tue, 2 Oct 2018 13:16:54 +0800 Subject: [PATCH 03/12] add methods doc --- src/canvas.class.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/canvas.class.js b/src/canvas.class.js index 34dac6eb3a3..6d0f6031a77 100644 --- a/src/canvas.class.js +++ b/src/canvas.class.js @@ -1197,6 +1197,11 @@ }, /** + * Checks point is inside the object. + * @param {Object} [pointer] x,y object of point coordinates we want to check. + * @param {fabric.Object} obj Object to test against + * @param {Object} [globalPointer] x,y object of point coordinates relative to canvas used to search per pixel target. + * @return {Boolean} true if point is contained within an area of given object * @private */ _checkTarget: function(pointer, obj, globalPointer) { @@ -1218,6 +1223,11 @@ }, /** + * Method that determines what object we are interacting with. + * @param {Array} objects array of objects to test agains. + * @param {Object} [pointer] x,y object of point coordinates we want to check. + * @param {Object} [pointerOnCanvas] x,y object of point coordinates relative to canvas used to search per pixel target. + * @return {fabric.Object} object that contains pointer * @private */ _searchPossibleTargets: function(objects, pointer, pointerOnCanvas) { From 940a9f2593162875038bbf8354ef72c64fe5917d Mon Sep 17 00:00:00 2001 From: codert Date: Tue, 2 Oct 2018 13:31:43 +0800 Subject: [PATCH 04/12] doc: update _serachPossibleTarget doc --- src/canvas.class.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/canvas.class.js b/src/canvas.class.js index 6d0f6031a77..9fb98df958e 100644 --- a/src/canvas.class.js +++ b/src/canvas.class.js @@ -1223,8 +1223,8 @@ }, /** - * Method that determines what object we are interacting with. - * @param {Array} objects array of objects to test agains. + * Function used to search inside objects an object that contains pointer in bounding box or that contains pointerOnCanvas when painted + * @param {Array} [objects] objects array to look into * @param {Object} [pointer] x,y object of point coordinates we want to check. * @param {Object} [pointerOnCanvas] x,y object of point coordinates relative to canvas used to search per pixel target. * @return {fabric.Object} object that contains pointer From b0b0eb06b2c63a6fb9fe04bc16983ef0a5d7c658 Mon Sep 17 00:00:00 2001 From: codert Date: Tue, 2 Oct 2018 22:39:08 +0800 Subject: [PATCH 05/12] test: add test case --- test/unit/canvas.js | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/unit/canvas.js b/test/unit/canvas.js index 040b5c14ed2..99d86806d0e 100644 --- a/test/unit/canvas.js +++ b/test/unit/canvas.js @@ -790,6 +790,36 @@ canvas.remove(triangle); }); + QUnit.test('findTarget with perPixelTargetFind in nested group', function(assert) { + assert.ok(typeof canvas.findTarget === 'function'); + var triangle = makeTriangle({ left: 0, top: 0 }), + target, + group1 = new fabric.Group([triangle], { + subTargetCheck: true + }), + group2 = new fabric.Group([group1], { + subTargetCheck: true + }); + + canvas.add(group2); + target = canvas.findTarget({ + clientX: 5, clientY: 5 + }); + assert.equal(target, group2, 'Should return the triangle by bounding box'); + canvas.perPixelTargetFind = true; + target = canvas.findTarget({ + clientX: 5, clientY: 5 + }); + assert.equal(target, null, 'Should return null because of transparency checks'); + target = canvas.findTarget({ + clientX: 15, clientY: 15 + }); + assert.equal(target, group2, 'Should return the group2 now'); + assert.equal(canvas.targets[0], group1, 'First subTargets should be group1'); + canvas.perPixelTargetFind = false; + canvas.remove(triangle); + }); + QUnit.test('findTarget on activegroup', function(assert) { var rect1 = makeRect({ left: 0, top: 0 }), target; var rect2 = makeRect({ left: 20, top: 20 }); From f453fd320ca31303e56196ca70f02c6917f9b845 Mon Sep 17 00:00:00 2001 From: codert Date: Wed, 3 Oct 2018 15:39:26 +0800 Subject: [PATCH 06/12] fix: remove pointer check --- src/canvas.class.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/canvas.class.js b/src/canvas.class.js index 9fb98df958e..9063eeb8676 100644 --- a/src/canvas.class.js +++ b/src/canvas.class.js @@ -1205,7 +1205,6 @@ * @private */ _checkTarget: function(pointer, obj, globalPointer) { - globalPointer = globalPointer || pointer; if (obj && obj.visible && obj.evented && @@ -1231,7 +1230,7 @@ * @private */ _searchPossibleTargets: function(objects, pointer, pointerOnCanvas) { - + pointerOnCanvas = pointerOnCanvas || pointer; // Cache all targets where their bounding box contains point. var target, i = objects.length, normalizedPointer, subTarget; // Do not check for currently grouped objects, since we check the parent group itself. From cc81ea538c55c0fcdd2dbf35124915ec1a273c94 Mon Sep 17 00:00:00 2001 From: codert Date: Thu, 4 Oct 2018 00:40:17 +0800 Subject: [PATCH 07/12] fix: object in nested group should normalize just once --- src/canvas.class.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/canvas.class.js b/src/canvas.class.js index 9063eeb8676..2f8ddea7284 100644 --- a/src/canvas.class.js +++ b/src/canvas.class.js @@ -1158,7 +1158,6 @@ var ignoreZoom = true, pointer = this.getPointer(e, ignoreZoom), - globalPointer = pointer, activeObject = this._activeObject, aObjects = this.getActiveObjects(), activeTarget, activeTargetSubs; @@ -1170,7 +1169,7 @@ if (aObjects.length > 1 && !skipGroup && - activeObject === this._searchPossibleTargets([activeObject],pointer, globalPointer)) { + activeObject === this._searchPossibleTargets([activeObject], pointer)) { return activeObject; } // if we hit the corner of an activeObject, let's return that. @@ -1178,7 +1177,7 @@ return activeObject; } if (aObjects.length === 1 && - activeObject === this._searchPossibleTargets([activeObject], pointer, globalPointer)) { + activeObject === this._searchPossibleTargets([activeObject], pointer)) { if (!this.preserveObjectStacking) { return activeObject; } @@ -1188,7 +1187,7 @@ this.targets = []; } } - var target = this._searchPossibleTargets(this._objects, pointer, globalPointer); + var target = this._searchPossibleTargets(this._objects, pointer); if (e[this.altSelectionKey] && target && activeTarget && target !== activeTarget) { target = activeTarget; this.targets = activeTargetSubs; @@ -1225,22 +1224,23 @@ * Function used to search inside objects an object that contains pointer in bounding box or that contains pointerOnCanvas when painted * @param {Array} [objects] objects array to look into * @param {Object} [pointer] x,y object of point coordinates we want to check. - * @param {Object} [pointerOnCanvas] x,y object of point coordinates relative to canvas used to search per pixel target. * @return {fabric.Object} object that contains pointer * @private */ - _searchPossibleTargets: function(objects, pointer, pointerOnCanvas) { - pointerOnCanvas = pointerOnCanvas || pointer; + _searchPossibleTargets: function(objects, pointer) { // Cache all targets where their bounding box contains point. - var target, i = objects.length, normalizedPointer, subTarget; + var target, i = objects.length, subTarget; // Do not check for currently grouped objects, since we check the parent group itself. // until we call this function specifically to search inside the activeGroup while (i--) { - if (this._checkTarget(pointer, objects[i], pointerOnCanvas)) { + var objToCheck = objects[i]; + if (this._checkTarget(objToCheck.group && objToCheck.group.type !== 'activeSelection' + ? this._normalizePointer(objToCheck.group, pointer) + : pointer, + objToCheck, pointer)) { target = objects[i]; if (target.subTargetCheck && target instanceof fabric.Group) { - normalizedPointer = this._normalizePointer(target, pointer); - subTarget = this._searchPossibleTargets(target._objects, normalizedPointer, pointerOnCanvas); + subTarget = this._searchPossibleTargets(target._objects, pointer); subTarget && this.targets.push(subTarget); } break; From 9865efd54f11956d66a190f5c7f667e97f9940c8 Mon Sep 17 00:00:00 2001 From: codert Date: Thu, 4 Oct 2018 00:40:27 +0800 Subject: [PATCH 08/12] test: add test case --- test/unit/canvas.js | 63 +++++++++++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 16 deletions(-) diff --git a/test/unit/canvas.js b/test/unit/canvas.js index 99d86806d0e..38ec0986361 100644 --- a/test/unit/canvas.js +++ b/test/unit/canvas.js @@ -792,30 +792,61 @@ QUnit.test('findTarget with perPixelTargetFind in nested group', function(assert) { assert.ok(typeof canvas.findTarget === 'function'); - var triangle = makeTriangle({ left: 0, top: 0 }), - target, - group1 = new fabric.Group([triangle], { - subTargetCheck: true - }), - group2 = new fabric.Group([group1], { - subTargetCheck: true - }); - - canvas.add(group2); + var triangle = makeTriangle({ left: 0, top: 0, width: 30, height: 30, fill: 'yellow' }), + circle = new fabric.Circle({ radius: 30, top: 0, left: 30, fill: 'blue' }), + circle2 = new fabric.Circle({ scaleX: 2, scaleY: 2, radius: 10, top: 120, left: -20, fill: 'purple' }), + rect = new fabric.Rect({ width: 100, height: 80, top: 50, left: 60, fill: 'green' }), + group1 = new fabric.Group([triangle, circle], { subTargetCheck: true }), + group2 = new fabric.Group([group1, circle2, rect], { subTargetCheck: true }), + group3 = new fabric.Group([group2], { subTargetCheck: true }), + target; + + canvas.add(group3); + canvas.perPixelTargetFind = true; target = canvas.findTarget({ clientX: 5, clientY: 5 }); - assert.equal(target, group2, 'Should return the triangle by bounding box'); - canvas.perPixelTargetFind = true; + assert.equal(target, null, 'Should return null because of transparency checks on coordinates 1'); target = canvas.findTarget({ - clientX: 5, clientY: 5 + clientX: 21, clientY: 9 }); - assert.equal(target, null, 'Should return null because of transparency checks'); + assert.equal(target, null, 'Should return null because of transparency checks on coordinates 2'); + target = canvas.findTarget({ + clientX: 37, clientY: 7 + }); + assert.equal(target, null, 'Should return null because of transparency checks on coordinates 3'); + target = canvas.findTarget({ + clientX: 89, clientY: 47 + }); + assert.equal(target, null, 'Should return null because of transparency checks on coordinates 4'); + target = canvas.findTarget({ + clientX: 16, clientY: 122 + }); + assert.equal(target, null, 'Should return null because of transparency checks on coordinates 4'); target = canvas.findTarget({ clientX: 15, clientY: 15 }); - assert.equal(target, group2, 'Should return the group2 now'); - assert.equal(canvas.targets[0], group1, 'First subTargets should be group1'); + assert.equal(target, group3, 'Should return the group3 now'); + assert.equal(canvas.targets.length, 3, 'Subtargets length should be 3'); + assert.equal(canvas.targets[0], triangle, 'The deepest target should be triangle'); + target = canvas.findTarget({ + clientX: 50, clientY: 20 + }); + assert.equal(target, group3, 'Should return the group3 now'); + assert.equal(canvas.targets.length, 3, 'Subtargets length should be 3'); + assert.equal(canvas.targets[0], circle, 'The deepest target should be circle'); + target = canvas.findTarget({ + clientX: 100, clientY: 90 + }); + assert.equal(target, group3, 'Should return the group3 now'); + assert.equal(canvas.targets.length, 2, 'Subtargets length should be 2'); + assert.equal(canvas.targets[0], rect, 'The deepest target should be rect'); + target = canvas.findTarget({ + clientX: 9, clientY: 145 + }); + assert.equal(target, group3, 'Should return the group3 now'); + assert.equal(canvas.targets.length, 2, 'Subtargets length should be 2'); + assert.equal(canvas.targets[0], circle2, 'The deepest target should be circle2'); canvas.perPixelTargetFind = false; canvas.remove(triangle); }); From 4c9247406121e4c97e52c5151c0b63d4475e8714 Mon Sep 17 00:00:00 2001 From: codert Date: Mon, 8 Oct 2018 16:09:09 +0800 Subject: [PATCH 09/12] restore to previous code style --- src/canvas.class.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/canvas.class.js b/src/canvas.class.js index 2f8ddea7284..46e668ad2b0 100644 --- a/src/canvas.class.js +++ b/src/canvas.class.js @@ -1167,9 +1167,7 @@ // if active group just exits. this.targets = []; - if (aObjects.length > 1 && - !skipGroup && - activeObject === this._searchPossibleTargets([activeObject], pointer)) { + if (aObjects.length > 1 && !skipGroup && activeObject === this._searchPossibleTargets([activeObject], pointer)) { return activeObject; } // if we hit the corner of an activeObject, let's return that. From b7b8a26eecc035cd847d3f11e6c103540ea916e3 Mon Sep 17 00:00:00 2001 From: codert Date: Mon, 8 Oct 2018 16:09:44 +0800 Subject: [PATCH 10/12] fix: update test case descriptions --- test/unit/canvas.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/unit/canvas.js b/test/unit/canvas.js index 38ec0986361..8741d3d47e0 100644 --- a/test/unit/canvas.js +++ b/test/unit/canvas.js @@ -806,23 +806,23 @@ target = canvas.findTarget({ clientX: 5, clientY: 5 }); - assert.equal(target, null, 'Should return null because of transparency checks on coordinates 1'); + assert.equal(target, null, 'Should return null because of transparency checks case 1'); target = canvas.findTarget({ clientX: 21, clientY: 9 }); - assert.equal(target, null, 'Should return null because of transparency checks on coordinates 2'); + assert.equal(target, null, 'Should return null because of transparency checks case 2'); target = canvas.findTarget({ clientX: 37, clientY: 7 }); - assert.equal(target, null, 'Should return null because of transparency checks on coordinates 3'); + assert.equal(target, null, 'Should return null because of transparency checks case 3'); target = canvas.findTarget({ clientX: 89, clientY: 47 }); - assert.equal(target, null, 'Should return null because of transparency checks on coordinates 4'); + assert.equal(target, null, 'Should return null because of transparency checks case 4'); target = canvas.findTarget({ clientX: 16, clientY: 122 }); - assert.equal(target, null, 'Should return null because of transparency checks on coordinates 4'); + assert.equal(target, null, 'Should return null because of transparency checks case 5'); target = canvas.findTarget({ clientX: 15, clientY: 15 }); From 1d223911adcde8d93df4253b0f5fc74c0696c3ec Mon Sep 17 00:00:00 2001 From: codert Date: Mon, 8 Oct 2018 20:12:21 +0800 Subject: [PATCH 11/12] fix: test case --- test/unit/canvas.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/canvas.js b/test/unit/canvas.js index 8741d3d47e0..d773a4f2874 100644 --- a/test/unit/canvas.js +++ b/test/unit/canvas.js @@ -848,7 +848,7 @@ assert.equal(canvas.targets.length, 2, 'Subtargets length should be 2'); assert.equal(canvas.targets[0], circle2, 'The deepest target should be circle2'); canvas.perPixelTargetFind = false; - canvas.remove(triangle); + canvas.remove(group3); }); QUnit.test('findTarget on activegroup', function(assert) { From 9bb43a54a86d1efaa19b88eaeb94b8cf422a63fd Mon Sep 17 00:00:00 2001 From: codert Date: Mon, 8 Oct 2018 20:48:28 +0800 Subject: [PATCH 12/12] test: add skew and angle test case --- test/unit/canvas.js | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/test/unit/canvas.js b/test/unit/canvas.js index d773a4f2874..d7391316d08 100644 --- a/test/unit/canvas.js +++ b/test/unit/canvas.js @@ -793,11 +793,13 @@ QUnit.test('findTarget with perPixelTargetFind in nested group', function(assert) { assert.ok(typeof canvas.findTarget === 'function'); var triangle = makeTriangle({ left: 0, top: 0, width: 30, height: 30, fill: 'yellow' }), + triangle2 = makeTriangle({ left: 100, top: 120, width: 30, height: 30, angle: 100, fill: 'pink' }), circle = new fabric.Circle({ radius: 30, top: 0, left: 30, fill: 'blue' }), circle2 = new fabric.Circle({ scaleX: 2, scaleY: 2, radius: 10, top: 120, left: -20, fill: 'purple' }), rect = new fabric.Rect({ width: 100, height: 80, top: 50, left: 60, fill: 'green' }), - group1 = new fabric.Group([triangle, circle], { subTargetCheck: true }), - group2 = new fabric.Group([group1, circle2, rect], { subTargetCheck: true }), + rect2 = new fabric.Rect({ width: 50, height: 30, top: 10, left: 110, fill: 'red', skewX: 40, skewY: 20 }), + group1 = new fabric.Group([triangle, circle, rect2], { subTargetCheck: true }), + group2 = new fabric.Group([group1, circle2, rect, triangle2], { subTargetCheck: true }), group3 = new fabric.Group([group2], { subTargetCheck: true }), target; @@ -823,6 +825,14 @@ clientX: 16, clientY: 122 }); assert.equal(target, null, 'Should return null because of transparency checks case 5'); + target = canvas.findTarget({ + clientX: 127, clientY: 37 + }); + assert.equal(target, null, 'Should return null because of transparency checks case 6'); + target = canvas.findTarget({ + clientX: 87, clientY: 139 + }); + assert.equal(target, null, 'Should return null because of transparency checks case 7'); target = canvas.findTarget({ clientX: 15, clientY: 15 }); @@ -835,6 +845,12 @@ assert.equal(target, group3, 'Should return the group3 now'); assert.equal(canvas.targets.length, 3, 'Subtargets length should be 3'); assert.equal(canvas.targets[0], circle, 'The deepest target should be circle'); + target = canvas.findTarget({ + clientX: 117, clientY: 16 + }); + assert.equal(target, group3, 'Should return the group3 now'); + assert.equal(canvas.targets.length, 3, 'Subtargets length should be 2'); + assert.equal(canvas.targets[0], rect2, 'The deepest target should be rect2'); target = canvas.findTarget({ clientX: 100, clientY: 90 }); @@ -847,6 +863,12 @@ assert.equal(target, group3, 'Should return the group3 now'); assert.equal(canvas.targets.length, 2, 'Subtargets length should be 2'); assert.equal(canvas.targets[0], circle2, 'The deepest target should be circle2'); + target = canvas.findTarget({ + clientX: 66, clientY: 143 + }); + assert.equal(target, group3, 'Should return the group3 now'); + assert.equal(canvas.targets.length, 2, 'Subtargets length should be 2'); + assert.equal(canvas.targets[0], triangle2, 'The deepest target should be triangle2'); canvas.perPixelTargetFind = false; canvas.remove(group3); });