From b53aefe5786cd39f3914a530dd083e6e68f97532 Mon Sep 17 00:00:00 2001 From: Shachar <34343793+ShaMan123@users.noreply.github.com> Date: Sat, 13 Jan 2024 16:08:12 +0200 Subject: [PATCH] refactor(): `getActiveControl` return value (#9515) --- CHANGELOG.md | 1 + src/EventTypeDefs.ts | 6 +-- src/canvas/Canvas.ts | 17 +------ src/canvas/SelectableCanvas.spec.ts | 48 ++++++++++++++++++++ src/canvas/SelectableCanvas.ts | 33 +++++++------- src/controls/Control.spec.ts | 11 ++++- src/controls/util.ts | 2 +- src/shapes/Object/InteractiveObject.spec.ts | 18 +++++++- src/shapes/Object/InteractiveObject.ts | 20 +++++++-- test/unit/canvas_events.js | 49 --------------------- test/unit/itext_click_behaviour.js | 1 + 11 files changed, 114 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa38725e3c2..6962b9c656b 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/EventTypeDefs.ts b/src/EventTypeDefs.ts index a4191e3c612..10ba08cc596 100644 --- a/src/EventTypeDefs.ts +++ b/src/EventTypeDefs.ts @@ -55,7 +55,7 @@ export type ControlCursorCallback = ControlCallback; */ 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 extends TEvent { transform: Transform; target: FabricObject; - action: string; + action?: string; } type ModificationEventsSpec< diff --git a/src/canvas/Canvas.ts b/src/canvas/Canvas.ts index 81eb115d4ac..e4fb091ff76 100644 --- a/src/canvas/Canvas.ts +++ b/src/canvas/Canvas.ts @@ -1101,7 +1101,7 @@ 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, @@ -1109,7 +1109,6 @@ export class Canvas extends SelectableCanvas implements CanvasOptions { 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]; diff --git a/src/canvas/SelectableCanvas.spec.ts b/src/canvas/SelectableCanvas.spec.ts index 866c3cd1140..05030452ecb 100644 --- a/src/canvas/SelectableCanvas.spec.ts +++ b/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); + }); + }); }); diff --git a/src/canvas/SelectableCanvas.ts b/src/canvas/SelectableCanvas.ts index 396595c4f4e..1ef63a39cde 100644 --- a/src/canvas/SelectableCanvas.ts +++ b/src/canvas/SelectableCanvas.ts @@ -545,6 +545,11 @@ export class SelectableCanvas 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 /** * @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 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,7 +622,7 @@ export class SelectableCanvas width: target.width, height: target.height, shiftKey: e.shiftKey, - altKey: altKey, + altKey, original: { ...saveObjectTransform(target), originX: origin.x, @@ -626,13 +630,12 @@ export class SelectableCanvas }, }; - 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, + }); } /** diff --git a/src/controls/Control.spec.ts b/src/controls/Control.spec.ts index 6ddc3922449..f666be06f9d 100644 --- a/src/controls/Control.spec.ts +++ b/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); diff --git a/src/controls/util.ts b/src/controls/util.ts index bffe24c90e9..c372c1a2612 100644 --- a/src/controls/util.ts +++ b/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 ) => { diff --git a/src/shapes/Object/InteractiveObject.spec.ts b/src/shapes/Object/InteractiveObject.spec.ts index cd1e56b5d03..1d83e7690af 100644 --- a/src/shapes/Object/InteractiveObject.spec.ts +++ b/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, + }); + }); }); diff --git a/src/shapes/Object/InteractiveObject.ts b/src/shapes/Object/InteractiveObject.ts index 2c3bcb283c9..eb7735d738c 100644 --- a/src/shapes/Object/InteractiveObject.ts +++ b/src/shapes/Object/InteractiveObject.ts @@ -153,9 +153,14 @@ 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; } } @@ -163,7 +168,14 @@ export class InteractiveFabricObject< } getActiveControl() { - return this.__corner; + const key = this.__corner; + return key + ? { + key, + control: this.controls[key], + coord: this.oCoords[key], + } + : undefined; } /** diff --git a/test/unit/canvas_events.js b/test/unit/canvas_events.js index ada6c5c9c37..48c14d79b34 100644 --- a/test/unit/canvas_events.js +++ b/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 }); diff --git a/test/unit/itext_click_behaviour.js b/test/unit/itext_click_behaviour.js index a47de28b91f..7e0d66e55cf 100644 --- a/test/unit/itext_click_behaviour.js +++ b/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();