feat(perPixelTargetFind): support not-selected option

[ShaMan123]

Motivation

I was looking into this and it doesn't make sense IMO.

If an object is selected I think the user expectation is to iteract with the bbox, whereas if it is not selected perPixelTargetFind should be respected

Description

added an option not-selected to perPixelTargetFind

Changes

Gist

In Action

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	915.120 (-0.020)	305.692 (+0.112)

[jiayihu]

This sounds a strong assumption on the desired UX. Fabric should probably be agnostic about it, leaving the decision to the application. Maybe fabric should just tell that both the children and the activeObject are targets of the event.

[ShaMan123]

Maybe fabric should just tell that both the children and the activeObject are targets of the event.

Don't get it

[ShaMan123]

if we turn the usage from a flag check to a method it can be easily overridden

object.isPerPixelTargetFind()

[jiayihu]

Don't get it

I mean that _searchPossibleTargets maybe should return a list of targets, instead of only one. Then depending on the case, evaluate what to do if multiple targets are detected, leaving possibly some decisions to the application

if we turn the usage from a flag check to a method it can be easily overridden

In that case you can already override the property with a getter

[ShaMan123]

We talked about refactoring _searchPossibleTargets to return context instead of setting targets on canvas etc. => that should be done

[asturur]

i remember when we needed it years ago, was mostly for text, people would see things behind text objects and they were angry they couldn't be selected if text was selected.

It could be an option in 6.1, now this shouldn't be touched.

[asturur]

we should preserve this comment maybe over the containsPoint function

[ShaMan123]

both are dead, and also the impl has moved forward more than a decade

[asturur]

i can see both ways being reasonable.
this will be a boolean option maybe called preferBoundingBoxWhenActive: boolean to add.

[ShaMan123]

IMO options less booleans more deleagtes/managers

[asturur]

We talked about refactoring _searchPossibleTargets to return context instead of setting targets on canvas etc. => that should be done

In this way we take away from simplicity for the purpose of trying to write the perfect flexible code that doesn't exist.

You were about to change an IF only, that means that a boolean option is fine.

[ShaMan123]

So if going in the direction I would change perPixelTargetFind: boolean | 'not-selected'

[github-actions]

Coverage after merging patch-selected-pptf into master will be

82.97%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	9
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.76%	85.29%	87.50%	89.58%	134–135, 160–161, 32–33, 41, 50, 78, 89
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.31%	91.89%	90%	93.55%	118, 129, 138, 31, 94
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.06%	82.35%	100%	93.81%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	79.12%	77.68%	83.05%	79.61%	1001–1002, 1002, 1002, 1004, 1012, 1012, 1012–1014, 1014, 1014, 1020–1021, 1029–1030, 1030, 1030–1031, 1036, 1038, 1069–1071, 1074–1075, 1079–1080, 1193–1195, 1198–1199, 1272, 1391, 1513, 157, 182, 292–293, 296–300, 305, 328–329, 334–339, 359, 359, 359–360, 360, 360–361, 369, 37, 374–375, 375, 375–376, 378, 387, 393–394, 394, 394, 41, 437, 445, 449, 449, 449–450, 452, 534–535, 535, 535–536, 542, 542, 542–544, 564, 566, 566, 566–567, 567, 567, 570, 570, 570–571, 574, 583–584, 586–587, 589, 589–590, 592–593, 605–606, 606, 606–607, 609–614, 620, 627, 664, 664, 664, 666, 668–673, 679, 685, 685, 685–686, 688, 691, 696, 709, 737, 737, 795–796, 796, 796–797, 799, 802–803, 803, 803–804, 806–807, 810, 810–812, 815–816, 886, 898, 905, 926, 958, 979–980, 996–997, 997, 997–999
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	94.26%	91.90%	94.23%	96.14%	1094, 1096, 1098–1099, 302, 478–479, 481–482, 482, 482, 531–532, 593–594, 647–649, 681, 686–687, 714–715, 778, 894, 894–895, 898, 918, 918, 967, 975
StaticCanvas.ts	96.78%	93.09%	100%	98.53%	1030, 1040, 1092–1093, 1096, 1131–1132, 1208, 1217, 1217, 1221, 1221, 1268–1269, 185–186, 202, 570, 582–583, 913–914, 914, 914–915
StaticCanvasOptions.ts	100%	100%	100%	100%
TextEditingManager.ts	84.31%	71.43%	91.67%	88%	17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	100%	100%	100%	100%
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	93.33%	87.88%	91.67%	97.78%	175, 240, 327, 327, 362
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	5.97%	0%	0%	11.11%	100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	19.57%	12.50%	50%	21.43%	41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
scale.ts	93.57%	92.94%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	131–132, 163–164, 171, 177, 179
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env

[ShaMan123]

added not-selected option

[ShaMan123]

I am not sure about combinations between canvas.perPixelTargetFind and obj.perPixelTargetFind
Shouldn't the object take presedence?
I will update tests once we decide

[asturur]

We should collect the current rules around the code.
But there shouldn't be such precedence is just confusing.
There must a be a documented way to handle that for all objects or just some.
If you can set it on a single object or an entire subclass through default values is good enough imho

[ShaMan123]

I am not sure about this PR.
Perhaps it is best to expose a method on object instead of the new option.

[asturur]

how this would behave when you have selected an object inside a group with not-selected? what is the ux like?

[ShaMan123]

I think we should close this pr.
And jnstead move the logic to containsPoint.
Then each object can override at will and no confusing ux.
Thoughts?

[asturur]

my question on the ux is still useful to understand what this features would do.

contains point won't be the center of the selection anymore since it doesn't take padding in account. containsPoint can be used for selection with a pointer, but is not a selection function so it should not have selection logic inside. If a point is contained it has to return true, regardless of selection ux.

[asturur]

i still want to know this if you know:

how this would behave when you have selected an object inside a group with 'not-selected'? what is the ux like?