fix(Canvas): control down/up pointer

[ShaMan123]

Motivation

Description

The pointer for mouseDownHandler/mouseUpHandler should be from the viewport

https://github.com/fabricjs/fabric.js/pull/8519/files#diff-72332330d8e00b2fda6e57d000a93e4e8dd8a46fdc8bcbdb3320c23b1509f8b5L748-L752

the bug exists on v5:
https://github.com/fabricjs/fabric.js/blob/5.x/src/mixins/canvas_events.mixin.js#L468-L472

Changes

Gist

In Action

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	911.792 (+0.018)	305.472 (+0.009)

[github-actions]

Coverage after merging fix-control-down/up into master will be

82.98%

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%	119, 130, 139, 32, 95
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	78.87%	76.67%	82.76%	79.88%	1000–1001, 1001, 1001, 1008–1009, 1017–1018, 1018, 1018–1019, 1025, 1027, 1055–1057, 1060–1061, 1065–1066, 1185–1187, 1190–1191, 1264, 1383, 148, 1505, 173, 282–283, 286–290, 295, 318–319, 32, 324–329, 349, 349, 349–350, 350, 350–351, 359, 36, 364–365, 365, 365–366, 368, 377, 383–384, 384, 384, 427, 435, 439, 439, 439–440, 442, 525–526, 526, 526–527, 533, 533, 533–535, 555, 557, 557, 557–558, 558, 558, 561, 561, 561–562, 565, 574–575, 577–578, 580, 580–581, 583–584, 596–597, 597, 597–598, 600–605, 611, 618, 655, 655, 655, 657, 659–664, 670, 676, 676, 676–677, 679, 682, 687, 700, 728, 728, 789–790, 790, 790, 790, 790, 790, 793–794, 797, 797–799, 802–803, 879, 891, 898, 898, 898, 911, 944, 965–966, 982–983, 983, 983–985, 988–989, 989, 989, 991, 999, 999, 999
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	94.35%	92.06%	94.23%	96.20%	1080, 1082, 1084–1085, 301, 471–472, 474–475, 475, 475, 524–525, 586–587, 600, 640–642, 674, 679–680, 707–708, 880, 880–881, 884, 904, 904, 953, 961
StaticCanvas.ts	96.78%	93.09%	100%	98.53%	1031, 1041, 1093–1094, 1097, 1132–1133, 1209, 1218, 1218, 1222, 1222, 1269–1270, 187–188, 204, 571, 583–584, 914–915, 915, 915–916
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	97.50%	88.89%	100%	100%	33
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	94.44%	93.10%	91.67%	96.77%	183, 249, 354
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	7.25%	0%	0%	13.51%	103, 108, 120, 120, 120, 120, 120, 122–125, 125, 128, 135, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–84, 84, 86, 89–90, 90, 90, 90, 90, 92, 97
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%

[asturur]

Eventually it would be the same thing we send to the actionHandler for consistency.
From the code it looks like we send to the transform action a normal point,
(look into _transformObject(e: TPointerEvent) {).
But then if we are in a group we transform it in the group plane that from the object that are inside it still acts as a normal point since it maps 1:1 with their aCoord.

Why do you think this is a bug? which is the backstory?

[ShaMan123]

It is another control confusion
Controls work with oCoords, but then the down/up handler receive a pointer in the scene, why?

But then if we are in a group we transform it in the group plane that from the object that are inside it still acts as a normal point since it maps 1:1 with their aCoord.

Also a confusion. Actually a patch/hack I did to fix controls under group without a lot of refactoring. But it makes no sense. All should be in the viewport as controls are

[asturur]

yeah i don't know.
Don't we want to go away from the duality of coordinates?

your patch/hack (as you call it), could be less hacky by moving that plane shift inside the action handler with a wrapper, called 'support nested object'
That would make the api less confusing and leaving the developer the choice to use it or not during a custom action.
That is probably a good thing to think about

Going back to controls, so for you since those works with oCoords they should work viewport coords.

Since we don't have any custom existing mousup/down coordinates why we don't offer a wrapper for actions like the withFixedPosition, and we call it withViewportCoordinates? so that we can change what the custom action see and who writes the custom action can pick up a wrapper or not.
I think what are you doing with your control dictates what in that moment is considered normal and correct.

One example is that i want to check where i m dropping a control compared to other objects and since all geometry function are getting the viewport version removed, i would then need the version with the scene pointer.

if i want to know if i m overing another control, for example to connect an arrow between 2 controls of 2 objects, the viewport one seems more usable.

[ShaMan123]

That is why I promote sendPointToPlane and that is why I think we should send viewport coords and let the dev use sendPointToPlane.
Simple and easy and standardized

[ShaMan123]

Don't we want to go away from the duality of coordinates?

Indeed go away

[ShaMan123]

In fact this the control API has ambiguous values for x, y passed args.
In actionHandler x, y are in the parent plane, in up/down handlers they are in the scene plane
That is clearly a bug but this PR is stale