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.

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

isTargetTransparent fails on the control borders for objects #2601

Closed
jhundley9109 opened this issue Nov 2, 2015 · 8 comments
Closed

isTargetTransparent fails on the control borders for objects #2601

jhundley9109 opened this issue Nov 2, 2015 · 8 comments
Assignees

Comments

@jhundley9109
Copy link

See this fiddle I created here:

http://jsfiddle.net/jhundley9109/co6ncoyo/

You can see the green circle on the left has 12 pixels that fail the isTargetTransparent test. However, the yellow circle on the right has a 100% isTransparent pass rate on the right. This is due to the controls being on for the circle on the left. It looks like when fabric runs this test and draws the object in the contextCache it is not turning off the controls but it is turning off the borders and corners.

I simply fixed this by also turning off the controls too and the test passed (as I expected at least)

Here is my modified code:

isTargetTransparent: function (target, x, y) {
  var hasBorders = target.hasBorders,
      transparentCorners = target.transparentCorners,
      hasControls = target.hasControls;

  target.hasBorders = target.transparentCorners = target.hasControls = false;

  this._draw(this.contextCache, target);

  target.hasBorders = hasBorders;
  target.transparentCorners = transparentCorners;
  target.hasControls = hasControls;

  var isTransparent = fabric.util.isTransparent(
    this.contextCache, x, y, this.targetFindTolerance);

  this.clearContext(this.contextCache);

  return isTransparent;
},

Let me know what you think!

@asturur
Copy link
Member

asturur commented Nov 2, 2015

if you turn off controls, it will not be possible to select controls with the "perPixelFind" true.
Am i wrong?
Can you resize the object by corner if you do per pixelTargetFind?

@jhundley9109
Copy link
Author

This modified code will only turn the corners off while testing the isTargetTransparent. I don't think this code will affect re-sizing the objects since it only turns it off on the object being drawn on to the contextCache. Did I misunderstand your question?

@asturur
Copy link
Member

asturur commented Nov 3, 2015

canvas.isTargetTransparent is a part of the perPixelTargetFind procedure.

If you check the code target.transparentCorners is set to false on porpouse.

Example:
First fabricjs check if target contains point. This contain point take in consideration bounding box and corners. Then if perPixelTargetFind is true, it validates with transparency.
If in that moment corners are transparent it cannot validate them and the target is false.

At least this is the idea i get from the code.

Other solution is that this.transparentCorners = false is wrong and must be changed with target.hasControls = false as you say.

@jhundley9109
Copy link
Author

Ah I understand now. Just testing it with my fiddle provided, I added perPixelTargetFind: true to the green circle on the left. It appears just by that test the corner resize is still working as expected. However, I do not know if this could have negative implications in other uses. Logically it seems more correct to not check the corners of an object when testing if it is transparent. For me that is the desired behavior. Is this fabricjs desired behavior though?

I can do a work around where I put 'hasControls' as false on the object before I test it for transparency and then reset hasControls to true after running my test.

@asturur
Copy link
Member

asturur commented Nov 3, 2015

The problem is that actually the code is:

if (obj &&
obj.visible &&
obj.evented &&
this.containsPoint(e, obj)){
if ((this.perPixelTargetFind || obj.perPixelTargetFind) && !obj.isEditing) {
var isTransparent = this.isTargetTransparent(obj, pointer.x, pointer.y);
if (!isTransparent) {
return true;
}
}
else {
return true;
}
}

so:
if we are in the bouding box or on a corner and we are checking for transparency, return true just if it is not transparent.

I would make a deep analysis before changing this behaviour.
Maybe this thing is corrected by other code somewhere else.

What mainly makes me doubt is that "this.transparentCorner = false" put there. It can't be a typo but it looks like intentional.

@jhundley9109
Copy link
Author

You are much more qualified to solve this problem than I am!

Hopefully this will be updated in a not so distant future release.

Thanks for your help!

@asturur
Copy link
Member

asturur commented Nov 3, 2015

@kangax should remember

@asturur asturur self-assigned this Apr 8, 2016
@asturur
Copy link
Member

asturur commented Apr 25, 2016

I finally understand this issue.
it happens just when hovering a non active object.
The mouse cursor changes on drag mode when hovering over the not visible control.
We should not make check the control during transparency check if the object is not active.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants