Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perPixelTargetFind not working in nested group. #5287

Merged
merged 12 commits into from
Oct 13, 2018
Merged

perPixelTargetFind not working in nested group. #5287

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
34afb37
fix perPixelTargetFind in nested object
doouding Oct 1, 2018
02cfb33
fix: lint
doouding Oct 1, 2018
5985429
add methods doc
doouding Oct 2, 2018
940a9f2
doc: update _serachPossibleTarget doc
doouding Oct 2, 2018
b0b0eb0
test: add test case
doouding Oct 2, 2018
f453fd3
fix: remove pointer check
doouding Oct 3, 2018
cc81ea5
fix: object in nested group should normalize just once
doouding Oct 3, 2018
9865efd
test: add test case
doouding Oct 3, 2018
4c92474
restore to previous code style
doouding Oct 8, 2018
b7b8a26
fix: update test case descriptions
doouding Oct 8, 2018
1d22391
fix: test case
doouding Oct 8, 2018
9bb43a5
test: add skew and angle test case
doouding Oct 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions src/canvas.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1158,6 +1158,7 @@

var ignoreZoom = true,
pointer = this.getPointer(e, ignoreZoom),
globalPointer = pointer,
activeObject = this._activeObject,
aObjects = this.getActiveObjects(),
activeTarget, activeTargetSubs;
Expand All @@ -1167,15 +1168,17 @@
// 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.
if (aObjects.length === 1 && activeObject._findTargetCorner(pointer)) {
return activeObject;
}
if (aObjects.length === 1 &&
activeObject === this._searchPossibleTargets([activeObject], pointer)) {
activeObject === this._searchPossibleTargets([activeObject], pointer, globalPointer)) {
if (!this.preserveObjectStacking) {
return activeObject;
}
Expand All @@ -1185,7 +1188,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;
Expand All @@ -1196,13 +1199,14 @@
/**
* @private
*/
_checkTarget: function(pointer, obj) {
_checkTarget: function(pointer, obj, globalPointer) {
globalPointer = globalPointer || pointer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this check necessary? it looks like that we always pass in globalPointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if someone might call this private method directly.
Should I remove this check or just document this method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the method is enough, is a private method.
If someone call it directly is gonna have problems anyway since those methods get changed with no information to devs.
We can put this check in searchPossibleTargets that while being private too, is at least one level up of this.

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That make sense to me.

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;
}
Expand All @@ -1216,18 +1220,18 @@
/**
* @private
*/
_searchPossibleTargets: function(objects, pointer) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if those 2 methods are private, we could still document them

Function used to search inside objects an object that contains pointer in bounding box or that contains pointerOnCanvas when painted
@param [fabric.Object] objects array to look into
...

can you complete the JSDOC information for _searchPossibleTargets and checkTarget ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

_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;
Expand Down