refactor(): getActiveControl return value (#9515)

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## [next]
 
+- refactor(): `getActiveControl` now returns the key, the corner and the coordinates [#9515](https://github.com/fabricjs/fabric.js/pull/9515)
 - fix(Controls): forbid scaling to avoid NaN issues on scaling zero sized objects. #9475 [#9563](https://github.com/fabricjs/fabric.js/pull/9563)
 - feat(LayoutManager): BREAKING remove `shouldResetTransform` handling from LayoutManager [#9581](https://github.com/fabricjs/fabric.js/pull/9581)
 - refactor(): rm active selection ref [#9561](https://github.com/fabricjs/fabric.js/pull/9561)

src/EventTypeDefs.ts

@@ -55,7 +55,7 @@ export type ControlCursorCallback = ControlCallback<string>;
  */
 export type Transform = {
   target: FabricObject;
-  action: string;
+  action?: string;
   actionHandler?: TransformActionHandler;
   corner: string;
   scaleX: number;
@@ -79,8 +79,6 @@ export type Transform = {
     originX: TOriginX;
     originY: TOriginY;
   };
-  // @TODO: investigate if this reset is really needed
-  reset?: boolean;
   actionPerformed: boolean;
 };
 
@@ -110,7 +108,7 @@ export interface ModifiedEvent<E extends Event = TPointerEvent>
   extends TEvent<E> {
   transform: Transform;
   target: FabricObject;
-  action: string;
+  action?: string;
 }
 
 type ModificationEventsSpec<

src/canvas/Canvas.ts

@@ -1101,15 +1101,14 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
           pointer = this.getScenePoint(e),
           mouseDownHandler =
             control && control.getMouseDownHandler(e, target, control);
-        if (mouseDownHandler) {
+        mouseDownHandler &&
           mouseDownHandler.call(
             control,
             e,
             this._currentTransform!,
             pointer.x,
             pointer.y
           );
-        }
       }
     }
     //  we clear `_objectsToRender` in case of a change in order to repopulate it at rendering
@@ -1149,17 +1148,6 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
       : this.findTarget(e);
   }
 
-  /**
-   * @private
-   */
-  _beforeTransform(e: TPointerEvent) {
-    const t = this._currentTransform!;
-    this.fire('before:transform', {
-      e,
-      transform: t,
-    });
-  }
-
   /**
    * Method that defines the actions when mouse is hovering the canvas.
    * The currentTransform parameter will define whether the user is rotating/scaling/translating
@@ -1343,9 +1331,6 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
             target.group.calcTransformMatrix()
           )
         : scenePoint;
-    // seems used only here.
-    // @TODO: investigate;
-    transform.reset = false;
     transform.shiftKey = e.shiftKey;
     transform.altKey = !!this.centeredKey && e[this.centeredKey];
 

src/canvas/SelectableCanvas.spec.ts

@@ -302,4 +302,52 @@ describe('Selectable Canvas', () => {
       }
     });
   });
+
+  describe('setupCurrentTransform', () => {
+    test.each(
+      ['tl', 'mt', 'tr', 'mr', 'br', 'mb', 'bl', 'ml', 'mtr']
+        .map((controlKey) => [
+          { controlKey, zoom: false },
+          { controlKey, zoom: true },
+        ])
+        .flat()
+    )('should fire before:transform event %p', ({ controlKey, zoom }) => {
+      const canvas = new Canvas();
+      const canvasOffset = canvas.calcOffset();
+      const object = new FabricObject({
+        left: 50,
+        top: 50,
+        width: 50,
+        height: 50,
+      });
+      canvas.add(object);
+      canvas.setActiveObject(object);
+      zoom && canvas.zoomToPoint(new Point(25, 25), 2);
+      expect(canvas._currentTransform).toBeFalsy();
+
+      const spy = jest.fn();
+      canvas.on('before:transform', spy);
+      const setupCurrentTransformSpy = jest.spyOn(
+        canvas,
+        '_setupCurrentTransform'
+      );
+
+      const {
+        corner: { tl, tr, bl },
+      } = object.oCoords[controlKey];
+      canvas.getSelectionElement().dispatchEvent(
+        new MouseEvent('mousedown', {
+          clientX: canvasOffset.left + (tl.x + tr.x) / 2,
+          clientY: canvasOffset.top + (tl.y + bl.y) / 2,
+          which: 1,
+        })
+      );
+
+      expect(setupCurrentTransformSpy).toHaveBeenCalledTimes(1);
+      expect(canvas._currentTransform).toBeDefined();
+      expect(canvas._currentTransform).toHaveProperty('target', object);
+      expect(canvas._currentTransform).toHaveProperty('corner', controlKey);
+      expect(spy).toHaveBeenCalledTimes(1);
+    });
+  });
 });

src/canvas/SelectableCanvas.ts

@@ -545,6 +545,11 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
       x: target.originX,
       y: target.originY,
     };
+
+    if (!controlName) {
+      return origin;
+    }
+
     // is a left control ?
     if (['ml', 'tl', 'bl'].includes(controlName)) {
       origin.x = RIGHT;
@@ -565,16 +570,14 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
   /**
    * @private
    * @param {Event} e Event object
-   * @param {FaricObject} target
+   * @param {FabricObject} target
+   * @param {boolean} [alreadySelected] pass true to setup the active control
    */
   _setupCurrentTransform(
     e: TPointerEvent,
     target: FabricObject,
     alreadySelected: boolean
   ): void {
-    if (!target) {
-      return;
-    }
     const pointer = target.group
       ? // transform pointer to target's containing coordinate plane
         sendPointToPlane(
@@ -583,22 +586,23 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
           target.group.calcTransformMatrix()
         )
       : this.getScenePoint(e);
-    const corner = target.getActiveControl() || '',
-      control = !!corner && target.controls[corner],
+    const { key: corner = '', control } = target.getActiveControl() || {},
       actionHandler =
         alreadySelected && control
           ? control.getActionHandler(e, target, control)?.bind(control)
           : dragHandler,
       action = getActionFromCorner(alreadySelected, corner, e, target),
-      origin = this._getOriginFromCorner(target, corner),
       altKey = e[this.centeredKey as ModifierKey],
+      origin = this._shouldCenterTransform(target, action, altKey)
+        ? ({ x: CENTER, y: CENTER } as const)
+        : this._getOriginFromCorner(target, corner),
       /**
        * relative to target's containing coordinate plane
        * both agree on every point
        **/
       transform: Transform = {
         target: target,
-        action: action,
+        action,
         actionHandler,
         actionPerformed: false,
         corner,
@@ -618,21 +622,20 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
         width: target.width,
         height: target.height,
         shiftKey: e.shiftKey,
-        altKey: altKey,
+        altKey,
         original: {
           ...saveObjectTransform(target),
           originX: origin.x,
           originY: origin.y,
         },
       };
 
-    if (this._shouldCenterTransform(target, action, altKey)) {
-      transform.originX = CENTER;
-      transform.originY = CENTER;
-    }
     this._currentTransform = transform;
-    // @ts-expect-error this method exists in the subclass - should be moved or declared as abstract
-    this._beforeTransform(e);
+
+    this.fire('before:transform', {
+      e,
+      transform,
+    });
   }
 
   /**

src/controls/Control.spec.ts

@@ -16,9 +16,16 @@ describe('Controls', () => {
 
     const target = new FabricObject({
       controls: { test: control, test2: control },
+      canvas: new Canvas(),
     });
-    jest.spyOn(target, '_findTargetCorner').mockReturnValue('test');
-    jest.spyOn(target, 'getActiveControl').mockReturnValue('test');
+
+    target.setCoords();
+
+    jest
+      .spyOn(target, '_findTargetCorner')
+      .mockImplementation(function (this: FabricObject) {
+        return (this.__corner = 'test');
+      });
 
     const canvas = new Canvas();
     canvas.setActiveObject(target);

src/controls/util.ts

@@ -25,7 +25,7 @@ export const NOT_ALLOWED_CURSOR = 'not-allowed';
  */
 export const getActionFromCorner = (
   alreadySelected: boolean,
-  corner: string,
+  corner: string | undefined,
   e: TPointerEvent,
   target: FabricObject
 ) => {

src/shapes/Object/InteractiveObject.spec.ts

@@ -1,6 +1,7 @@
+import { Canvas } from '../../canvas/Canvas';
+import { Control } from '../../controls/Control';
 import { radiansToDegrees } from '../../util';
 import { Group } from '../Group';
-import { Canvas } from '../../canvas/Canvas';
 import { FabricObject } from './FabricObject';
 import { InteractiveFabricObject, type TOCoord } from './InteractiveObject';
 
@@ -14,6 +15,7 @@ describe('InteractiveObject', () => {
     const { controls, ...defaults } = FabricObject.getDefaults();
     expect(defaults).toMatchSnapshot();
   });
+
   describe('setCoords for objects inside group with rotation', () => {
     it('all corners are rotated as much as the object total angle', () => {
       const canvas = new Canvas();
@@ -49,4 +51,18 @@ describe('InteractiveObject', () => {
       });
     });
   });
+
+  test('getActiveControl', () => {
+    const object = new FabricObject({ canvas: new Canvas() });
+    const control = new Control();
+    object.controls = { control };
+    object.setCoords();
+    expect(object.getActiveControl()).toBeUndefined();
+    object.__corner = 'control';
+    expect(object.getActiveControl()).toEqual({
+      key: 'control',
+      control,
+      coord: object.oCoords.control,
+    });
+  });
 });

src/shapes/Object/InteractiveObject.ts

@@ -153,17 +153,29 @@ export class InteractiveFabricObject<
   _updateCacheCanvas() {
     const targetCanvas = this.canvas;
     if (this.noScaleCache && targetCanvas && targetCanvas._currentTransform) {
-      const target = targetCanvas._currentTransform.target,
-        action = targetCanvas._currentTransform.action;
-      if (this === (target as unknown as this) && action.startsWith('scale')) {
+      const transform = targetCanvas._currentTransform,
+        target = transform.target,
+        action = transform.action;
+      if (
+        this === (target as unknown as this) &&
+        action &&
+        action.startsWith('scale')
+      ) {
         return false;
       }
     }
     return super._updateCacheCanvas();
   }
 
   getActiveControl() {
-    return this.__corner;
+    const key = this.__corner;
+    return key
+      ? {
+          key,
+          control: this.controls[key],
+          coord: this.oCoords[key],
+        }
+      : undefined;
   }
 
   /**

test/unit/canvas_events.js

@@ -26,55 +26,6 @@
     }
   });
 
-  QUnit.test('_beforeTransform', function (assert) {
-    assert.ok(typeof canvas._beforeTransform === 'function');
-
-    var canvasOffset = canvas.calcOffset();
-    var rect = new fabric.Rect({ left: 50, top: 50, width: 50, height: 50 });
-    canvas.add(rect);
-    canvas.setActiveObject(rect);
-
-    var t, counter = 0;
-    canvas.on('before:transform', function (options) {
-      t = options.transform.target;
-      counter++;
-    });
-
-    var corners = ['tl', 'mt', 'tr', 'mr', 'br', 'mb', 'bl', 'ml', 'mtr'];
-    for (var i = 0; i < corners.length; i++) {
-      var co = rect.oCoords[corners[i]].corner;
-      var e = {
-        clientX: canvasOffset.left + (co.tl.x + co.tr.x) / 2,
-        clientY: canvasOffset.top + (co.tl.y + co.bl.y) / 2,
-        which: 1,
-        target: canvas.upperCanvasEl
-      };
-      canvas._setupCurrentTransform(e, rect);
-    }
-    assert.equal(counter, corners.length, 'before:transform should trigger onBeforeScaleRotate for all corners');
-    assert.equal(t, rect, 'before:transform should receive correct target');
-
-    canvas.zoomToPoint({ x: 25, y: 25 }, 2);
-
-    t = null;
-    counter = 0;
-    for (var i = 0; i < corners.length; i++) {
-      var c = corners[i];
-      var co = rect.oCoords[c].corner;
-      var e = {
-        clientX: canvasOffset.left + (co.tl.x + co.tr.x) / 2,
-        clientY: canvasOffset.top + (co.tl.y + co.bl.y) / 2,
-        which: 1,
-        target: canvas.upperCanvasEl
-      };
-      canvas._beforeTransform(e, rect);
-    }
-    assert.equal(counter, corners.length, 'before:transform should trigger onBeforeScaleRotate when canvas is zoomed');
-    assert.equal(t, rect, 'before:transform should receive correct target when canvas is zoomed');
-
-    canvas.zoomToPoint({ x: 0, y: 0 }, 1);
-  });
-
   QUnit.test('cache and reset event properties', function(assert) {
     var e = { clientX: 30, clientY: 30, which: 1, target: canvas.upperCanvasEl };
     var rect = new fabric.Rect({ width: 60, height: 60 });

test/unit/itext_click_behaviour.js

@@ -243,6 +243,7 @@
       iText.selected = true;
       iText.__lastSelected = true;
       iText.__corner = 'mt';
+      iText.setCoords();
       iText.mouseUpHandler({ e: {} });
       assert.equal(iText.isEditing, false, 'iText should not enter editing');
       iText.exitEditing();