refactor(Canvas): _setupCurrentTransform => setupCurrentTransform fixing control mousedown unexpected behavior

[ShaMan123]

Description

I found a minor bug during work.

If an object is not selected _setupCurrentTransform sets a drag action.
However, if findControl returned a control the down handler would get called.
This PR fixes that.

• dep(): _setupCurrentTransform in favor of setupCurrentTransform

As part of the PR I refactored setupCurrentTransform to accept the control it should setup or the drag action (which was the default switch if no control was found or if the object wasn't already selected before the mousedown) and a new option of undefined.
I did that to make the code simpler, now we just pass down args instead of letting setupCurrentTransform make decisions. Passing down undefined is also an important option that enables an app to control if/how the drag action is setup. I have done a lot to workaround the default drag transform action and this refactor makes it really simple.

In Action

Run the following on master and see it fail

 it.only('control mousedown handler should be called only if current transform uses it', () => {

     const rectOptions = {
        left: 75,
        top: 75,
        width: 50,
        height: 50,
      };
      const eAt100 = new MouseEvent('mousedown', {
        clientX: 100,
        clientY: 100,
      });
      const eOverTLControl = new MouseEvent('mousedown', {
        clientX: 69,
        clientY: 69,
      });
      const eOverMLControl = new MouseEvent('mousedown', {
        clientX: 69,
        clientY: 95,
        shiftKey: true,
      });

        const canvas = new Canvas();
        const rect = new Rect(rectOptions);
        jest.spyOn(rect, 'toJSON').mockReturnValue('rect');
        canvas.add(rect);
        canvas.setActiveObject(rect);
        const hit = rect.findControl(canvas.getViewportPoint(eOverTLControl));

        expect(hit).toBeDefined();
        expect(hit?.key).toBe('tl');
        const controlDownSpy = jest.spyOn(hit!.control, 'getMouseDownHandler');
        const spy = jest.spyOn(canvas, '_setupCurrentTransform');
        jest.spyOn(canvas, 'findTarget').mockReturnValue(rect);

        canvas.getSelectionElement().dispatchEvent(eOverTLControl);
        expect(spy).toHaveBeenCalledTimes(1);
        expect(spy).toHaveBeenCalledWith(eOverTLControl, rect, true);
        expect(controlDownSpy).toHaveBeenCalledTimes(1);

        spy.mockClear();
        controlDownSpy.mockClear();

        canvas.discardActiveObject();

        canvas.getSelectionElement().dispatchEvent(eOverTLControl);
        expect(spy).toHaveBeenCalledTimes(1);
        expect(spy).toHaveBeenCalledWith(eOverTLControl, rect, false);
        expect(controlDownSpy).not.toHaveBeenCalled();
      });

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

• Run Next Template
• Run Node Template
• Run Vanilla Template
• Run Vue Template

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	916.914 (+0.664)	305.848 (+0.201)

[github-actions]

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

83.85%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	14.29%	100%	0%	25%	23, 26, 29, 41, 44, 47
src
ClassRegistry.ts	90.32%	61.54%	100%	97.73%	31, 52
Collection.ts	78.47%	42.62%	87.10%	85.82%	130, 138, 153, 155–157, 159, 169–170, 181, 197, 215, 217, 228, 243, 254, 265, 270, 279, 281, 286–287, 302, 304, 309–310, 329, 333–334, 338–344, 346–348, 350
CommonMethods.ts	89.86%	69.23%	100%	94%	50, 52, 61
Intersection.ts	85.25%	48.91%	100%	97.30%	184–188, 190, 228, 237, 239, 289, 297, 297
Observable.ts	79.89%	54.55%	93.75%	87.10%	136, 145, 148, 160, 162, 167, 68–70, 72, 76, 80, 84–85, 87–91
Point.ts	90.27%	61.22%	100%	93.60%	104, 117, 148, 157, 179, 197, 206, 216, 225, 236–239, 259, 285, 297, 317, 328, 341, 349, 359, 95
Shadow.ts	87.45%	73.91%	100%	88.37%	147, 150, 152–157, 166, 203, 206, 213, 230–237, 241–242, 38–41
cache.ts	84.88%	45.45%	100%	90.14%	57, 59, 71–72, 74–77
config.ts	87.73%	55%	66.67%	94.03%	132, 134–137, 139, 142–143, 147, 152
constants.ts	100%	100%	100%	100%
src/LayoutManager
ActiveSelectionLayoutManager.ts	93.33%	76.92%	85.71%	100%
LayoutManager.ts	90.54%	65.06%	76.92%	99.29%	269, 333, 344
constants.ts	100%	100%	100%	100%
index.ts	48.57%	37.50%	80%	66.67%	1, 1, 1–2, 2, 2–3, 3, 3–4, 4, 4–5, 5, 5, 5–6
types.ts	100%	100%	100%	100%
src/LayoutManager/LayoutStrategies
ClipPathLayout.ts	73.08%	50%	100%	78.95%	39, 41–44, 46–48, 57–58, 66–69
FitContentLayout.ts	100%	100%	100%	100%
FixedLayout.ts	85.71%	20%	100%	100%	23, 23
LayoutStrategy.ts	89.26%	59.26%	100%	97.70%	54, 72, 74
utils.ts	72.58%	50%	100%	78.72%	29–32, 34–35, 40–44
src/Pattern
Pattern.ts	70.18%	90.91%	80%	65.95%	105–107, 114, 118–119, 119–122, 130–138, 140–141, 143, 153–164, 174, 176–181, 183–188, 190–199, 204–205, 207–209, 211, 33, 37
src/brushes
BaseBrush.ts	89.33%	91.67%	100%	88.55%	110, 115, 124–125, 130, 135, 143, 146, 155–160, 99
CircleBrush.ts	52.10%	12.50%	12.50%	58.25%	100–108, 108–118, 122, 130–139, 55, 67, 69, 76, 76, 78–79, 79, 83, 85–86, 92–98
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, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	74.80%	40.59%	92.86%	83.71%	1004, 1007–1008, 1012–1013, 1018, 1083–1089, 1125, 1146, 1150, 1157, 1160, 1223–1227, 1306–1310, 1340, 1357, 1403–1420, 1426–1431, 1434–1435, 1437, 1441, 1443–1444, 1446–1448, 1452, 1454, 1456–1458, 1461–1466, 1469–1471, 1474, 1476, 1490, 1497, 1499–1510, 1512–1515, 1515, 1517, 1521–1522, 1525–1526, 1529–1531, 1534, 1542, 354, 369, 388, 443, 559–563, 566–567, 569, 579, 582–583, 585, 588–590, 602, 609–613, 615–620, 622–626, 659, 661, 668–672, 674–679, 681, 683–684, 686–689, 691–692, 747, 783–786, 789, 791, 794, 796, 798, 824, 833, 843, 845, 847–851, 854–861, 887, 932–933, 936, 938, 947–953, 956, 963–964, 966–970, 972, 974, 995–996, 999
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	84.55%	53.49%	100%	93.62%	1014, 1016–1017, 1034, 1043, 1111–1115, 1161–1162, 1164–1165, 1167, 1194, 1196, 1258, 1260, 1264, 1267, 1288, 1291, 1310, 1312, 1316, 1334–1341, 1344, 1347, 1356–1357, 1361–1370, 1374, 1376–1377, 1383–1388, 1392, 362, 384, 462, 513, 515, 549, 551, 584, 648–651, 653–659, 661, 695, 723
StaticCanvas.ts	75.93%	70.11%	98.91%	75.43%	1003, 1009, 1012, 1014, 1016, 1024, 1026–1032, 1042, 1045, 1048–1054, 1056–1057, 1059–1080, 1087–1089, 1091, 1099, 1105, 1107, 1107–1108, 1108–1109, 1111, 1111–1116, 1129, 1134–1136, 1140, 1144, 1146, 1148, 1153–1157, 1159, 1161–1164, 1167, 1169, 1178, 1180–1181, 1188–1191, 1193, 1199–1202, 1206–1207, 1217, 1223–1225, 1227–1232, 1232, 1232–1236, 1236, 1236–1241, 1243–1250, 1279–1280, 1282–1283, 1285–1287, 1289–1295, 1299, 1301–1314, 1322–1323, 1333, 1344, 1385–1389, 1391, 1393, 1395–1399, 1418, 1433, 1448, 1468,

[ShaMan123]

Ready to merge

[ShaMan123]

@asturur if this can merge into the rc I would appreciate that

[asturur]

The issue with this PR is that on top of fixing a bug you go into a refactor of setupCurrentTransform without explaining why that refactor is necessary or if is just a preference.
What is the goal of the PR? fix the bug? or change something else?

[asturur]

When we add ci() build() feat() on the title of the PR i think we are looking at this convetion: https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#-commit-message-guidelines
sometimes you use some i don't recognize or i do not understand what you mean.

In general this is a refactor, not sure what 'dep' means.
Did you mean deprecation?

[ShaMan123]

Yes, I meant deprecation, refactor is more suiting I guess

[ShaMan123]

updated description

As part of the PR I refactored setupCurrentTransform to accept the control it should setup or the drag action (which was the default switch if no control was found or if the object wasn't already selected before the mousedown) and a new option of undefined.
I did that to make the code simpler, now we just pass down args instead of letting setupCurrentTransform make decisions. Passing down undefined is also an important option that enables an app to control if/how the drag action is setup. I have done a lot to workaround that and this refactor makes it really simple.