From 66980289d11223232ff84206e23fae0f1ae9a2a2 Mon Sep 17 00:00:00 2001 From: zhe-he Date: Thu, 14 Sep 2023 14:26:25 +0800 Subject: [PATCH 01/13] fix: getActiveObjects --- src/canvas/SelectableCanvas.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/canvas/SelectableCanvas.ts b/src/canvas/SelectableCanvas.ts index d9a783d63cf..ad498c65ba6 100644 --- a/src/canvas/SelectableCanvas.ts +++ b/src/canvas/SelectableCanvas.ts @@ -293,7 +293,7 @@ export class SelectableCanvas protected declare _isCurrentlyDrawing: boolean; declare freeDrawingBrush?: BaseBrush; declare _activeObject?: FabricObject; - protected readonly _activeSelection: ActiveSelection; + protected _activeSelection: ActiveSelection; constructor( el: string | HTMLCanvasElement, @@ -1104,6 +1104,11 @@ export class SelectableCanvas return false; } this._activeObject = object; + if (object instanceof ActiveSelection) { + this._activeSelection = object; + this._activeSelection.set('canvas', this); + this._activeSelection.setCoords(); + } return true; } From ad1ab0f985864243aef27ca1c6097430745bb696 Mon Sep 17 00:00:00 2001 From: zhe-he Date: Thu, 14 Sep 2023 14:36:01 +0800 Subject: [PATCH 02/13] update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4c5a1f1e9a..6fbba939b40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # Changelog +- fix(ActiveSelection): fix getActiveObjects [#9336](https://github.com/fabricjs/fabric.js/pull/9336) + ## [next] - Fix(Control) fix a regression in `wrap with fixed anchor`, regression from #8400 [#9326](https://github.com/fabricjs/fabric.js/pull/9326) From a60f0564ae553f6c9477c040f0d4e97dbdfec79b Mon Sep 17 00:00:00 2001 From: zhe-he Date: Fri, 15 Sep 2023 10:17:59 +0800 Subject: [PATCH 03/13] add: test getactiveObjects --- src/shapes/ActiveSelection.spec.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/shapes/ActiveSelection.spec.ts b/src/shapes/ActiveSelection.spec.ts index c7f6e393404..389ce2ae869 100644 --- a/src/shapes/ActiveSelection.spec.ts +++ b/src/shapes/ActiveSelection.spec.ts @@ -86,4 +86,16 @@ describe('ActiveSelection', () => { expect(canvas.getActiveSelection().lineCoords).toMatchSnapshot(); expect(canvas.getActiveSelection().aCoords).toMatchSnapshot(); }); + + it('setting and getting are the same', () => { + const dom = document.createElement('canvas'); + const canvas = new Canvas(dom); + const obj1 = new FabricObject(); + const obj2 = new FabricObject(); + canvas.add(obj1, obj2); + const activeSelection = new ActiveSelection([obj1, obj2]); + canvas.setActiveObject(activeSelection); + expect(canvas.getActiveSelection()).toEqual(activeSelection); + expect(canvas.getActiveObjects()).toEqual([obj1, obj2]); + }); }); From 5add9f1140b451ecb6364a458622966c5fadbfeb Mon Sep 17 00:00:00 2001 From: zhe-he Date: Fri, 15 Sep 2023 10:22:02 +0800 Subject: [PATCH 04/13] add: test getactiveObjects --- src/shapes/ActiveSelection.spec.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/shapes/ActiveSelection.spec.ts b/src/shapes/ActiveSelection.spec.ts index 389ce2ae869..5edb6894d75 100644 --- a/src/shapes/ActiveSelection.spec.ts +++ b/src/shapes/ActiveSelection.spec.ts @@ -88,8 +88,7 @@ describe('ActiveSelection', () => { }); it('setting and getting are the same', () => { - const dom = document.createElement('canvas'); - const canvas = new Canvas(dom); + const canvas = new Canvas(null); const obj1 = new FabricObject(); const obj2 = new FabricObject(); canvas.add(obj1, obj2); From 0a31b6b6d75005656f3049570b7783032284492a Mon Sep 17 00:00:00 2001 From: zhe he Date: Fri, 15 Sep 2023 13:41:05 +0800 Subject: [PATCH 05/13] Update src/shapes/ActiveSelection.spec.ts Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com> --- src/shapes/ActiveSelection.spec.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/shapes/ActiveSelection.spec.ts b/src/shapes/ActiveSelection.spec.ts index 5edb6894d75..86e8478dfc5 100644 --- a/src/shapes/ActiveSelection.spec.ts +++ b/src/shapes/ActiveSelection.spec.ts @@ -87,14 +87,17 @@ describe('ActiveSelection', () => { expect(canvas.getActiveSelection().aCoords).toMatchSnapshot(); }); - it('setting and getting are the same', () => { + it(`setActiveObject` should update the active selection ref on canvas, () => { const canvas = new Canvas(null); const obj1 = new FabricObject(); const obj2 = new FabricObject(); canvas.add(obj1, obj2); const activeSelection = new ActiveSelection([obj1, obj2]); + const spy = jest.spyOn(activeSelection, 'setCoords'); canvas.setActiveObject(activeSelection); - expect(canvas.getActiveSelection()).toEqual(activeSelection); + expect(canvas.getActiveSelection()).toBe(activeSelection); expect(canvas.getActiveObjects()).toEqual([obj1, obj2]); + expect(spy).toHaveBeenCalled(); + expect(activeSelection.canvas).toBe(canvas); }); }); From 51bcf36f14d5fdab0179f4a734915ddfb6659423 Mon Sep 17 00:00:00 2001 From: zhe he Date: Fri, 15 Sep 2023 13:41:31 +0800 Subject: [PATCH 06/13] Update CHANGELOG.md Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fbba939b40..b7ac3534fb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -- fix(ActiveSelection): fix getActiveObjects [#9336](https://github.com/fabricjs/fabric.js/pull/9336) +- fix(Canvas): `setActiveObject` should update `canvas#_activeSelection` [#9336](https://github.com/fabricjs/fabric.js/pull/9336) ## [next] From 6fac998f03c234e1407d63cf1404b162393e8779 Mon Sep 17 00:00:00 2001 From: zhe-he Date: Fri, 15 Sep 2023 13:44:21 +0800 Subject: [PATCH 07/13] update test --- src/shapes/ActiveSelection.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shapes/ActiveSelection.spec.ts b/src/shapes/ActiveSelection.spec.ts index 86e8478dfc5..9fb1b384913 100644 --- a/src/shapes/ActiveSelection.spec.ts +++ b/src/shapes/ActiveSelection.spec.ts @@ -87,7 +87,7 @@ describe('ActiveSelection', () => { expect(canvas.getActiveSelection().aCoords).toMatchSnapshot(); }); - it(`setActiveObject` should update the active selection ref on canvas, () => { + it('`setActiveObject` should update the active selection ref on canvas', () => { const canvas = new Canvas(null); const obj1 = new FabricObject(); const obj2 = new FabricObject(); From 2ccfbde30eafa955c3d3366dbe3c41516f256381 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Fri, 15 Sep 2023 14:25:25 +0530 Subject: [PATCH 08/13] fix(): transferring object between active selections --- src/shapes/ActiveSelection.ts | 6 +++--- src/shapes/Object/StackedObject.ts | 11 ++++++----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/shapes/ActiveSelection.ts b/src/shapes/ActiveSelection.ts index 71bc0eadabe..f72f361af8b 100644 --- a/src/shapes/ActiveSelection.ts +++ b/src/shapes/ActiveSelection.ts @@ -82,9 +82,9 @@ export class ActiveSelection extends Group { * @returns {boolean} true if object entered group */ enterGroup(object: FabricObject, removeParentTransform?: boolean) { - if (object.group) { + const parent = object.getParent(true); + if (parent) { // save ref to group for later in order to return to it - const parent = object.group; parent._exitGroup(object); object.__owningGroup = parent; } @@ -100,7 +100,7 @@ export class ActiveSelection extends Group { */ exitGroup(object: FabricObject, removeParentTransform?: boolean) { this._exitGroup(object, removeParentTransform); - const parent = object.__owningGroup; + const parent = object.getParent(true); if (parent) { // return to owning group parent._enterGroup(object, true); diff --git a/src/shapes/Object/StackedObject.ts b/src/shapes/Object/StackedObject.ts index ca7876a1891..fbde7fb1d0e 100644 --- a/src/shapes/Object/StackedObject.ts +++ b/src/shapes/Object/StackedObject.ts @@ -48,11 +48,12 @@ export class StackedObject< * Returns instance's parent **EXCLUDING** `ActiveSelection` * @param {boolean} [strict] exclude canvas as well */ - getParent(strict?: T): TAncestor | undefined { - return ( - (isActiveSelection(this.group) ? this.__owningGroup : this.group) || - (strict ? undefined : this.canvas) - ); + getParent< + T extends boolean, + R = (T extends true ? Group : TCollection) | undefined + >(strict?: T): R { + return ((isActiveSelection(this.group) ? this.__owningGroup : this.group) || + (strict ? undefined : this.canvas)) as R; } /** From 8f67b964ef5d25cc41a3eb5e253c6e439a9aeb2a Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Fri, 15 Sep 2023 21:13:22 +0530 Subject: [PATCH 09/13] fix(): transferring object between active selections 2 This reverts commit 2ccfbde30eafa955c3d3366dbe3c41516f256381. fix --- src/shapes/ActiveSelection.spec.ts | 16 ++++++++++++++++ src/shapes/ActiveSelection.ts | 10 ++++++---- src/shapes/Object/StackedObject.ts | 11 +++++------ 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/shapes/ActiveSelection.spec.ts b/src/shapes/ActiveSelection.spec.ts index 9fb1b384913..ee5a9588fba 100644 --- a/src/shapes/ActiveSelection.spec.ts +++ b/src/shapes/ActiveSelection.spec.ts @@ -1,5 +1,6 @@ import { Canvas } from '../canvas/Canvas'; import { ActiveSelection } from './ActiveSelection'; +import { Group } from './Group'; import { FabricObject } from './Object/FabricObject'; describe('ActiveSelection', () => { @@ -100,4 +101,19 @@ describe('ActiveSelection', () => { expect(spy).toHaveBeenCalled(); expect(activeSelection.canvas).toBe(canvas); }); + + it('transferring an object between active selections keeps its owning group', () => { + const object = new FabricObject(); + const group = new Group([object]); + const activeSelection1 = new ActiveSelection([object]); + const activeSelection2 = new ActiveSelection(); + expect(object.group).toBe(activeSelection1); + expect(object.getParent(true)).toBe(group); + activeSelection2.add(object); + expect(object.group).toBe(activeSelection2); + expect(object.getParent(true)).toBe(group); + activeSelection2.removeAll(); + expect(object.group).toBe(group); + expect(object.getParent(true)).toBe(group); + }); }); diff --git a/src/shapes/ActiveSelection.ts b/src/shapes/ActiveSelection.ts index f72f361af8b..cd5adfde41a 100644 --- a/src/shapes/ActiveSelection.ts +++ b/src/shapes/ActiveSelection.ts @@ -82,11 +82,13 @@ export class ActiveSelection extends Group { * @returns {boolean} true if object entered group */ enterGroup(object: FabricObject, removeParentTransform?: boolean) { - const parent = object.getParent(true); - if (parent) { + if (object.group) { // save ref to group for later in order to return to it + const parent = object.group; parent._exitGroup(object); - object.__owningGroup = parent; + // make sure we are setting the correct owning group + // in case `object` is transferred between active selections + !(parent instanceof ActiveSelection) && (object.__owningGroup = parent); } this._enterGroup(object, removeParentTransform); return true; @@ -100,7 +102,7 @@ export class ActiveSelection extends Group { */ exitGroup(object: FabricObject, removeParentTransform?: boolean) { this._exitGroup(object, removeParentTransform); - const parent = object.getParent(true); + const parent = object.__owningGroup; if (parent) { // return to owning group parent._enterGroup(object, true); diff --git a/src/shapes/Object/StackedObject.ts b/src/shapes/Object/StackedObject.ts index fbde7fb1d0e..ca7876a1891 100644 --- a/src/shapes/Object/StackedObject.ts +++ b/src/shapes/Object/StackedObject.ts @@ -48,12 +48,11 @@ export class StackedObject< * Returns instance's parent **EXCLUDING** `ActiveSelection` * @param {boolean} [strict] exclude canvas as well */ - getParent< - T extends boolean, - R = (T extends true ? Group : TCollection) | undefined - >(strict?: T): R { - return ((isActiveSelection(this.group) ? this.__owningGroup : this.group) || - (strict ? undefined : this.canvas)) as R; + getParent(strict?: T): TAncestor | undefined { + return ( + (isActiveSelection(this.group) ? this.__owningGroup : this.group) || + (strict ? undefined : this.canvas) + ); } /** From 5fc9171b231824ab80117227c9c1bb02b86e2ba9 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Fri, 15 Sep 2023 21:39:59 +0530 Subject: [PATCH 10/13] only if changed Update SelectableCanvas.ts --- src/canvas/SelectableCanvas.ts | 4 +++- src/shapes/ActiveSelection.spec.ts | 11 ++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/canvas/SelectableCanvas.ts b/src/canvas/SelectableCanvas.ts index ad498c65ba6..5710781e84e 100644 --- a/src/canvas/SelectableCanvas.ts +++ b/src/canvas/SelectableCanvas.ts @@ -1104,7 +1104,9 @@ export class SelectableCanvas return false; } this._activeObject = object; - if (object instanceof ActiveSelection) { + + if (object instanceof ActiveSelection && this._activeSelection !== object) { + this._activeSelection.dispose(); this._activeSelection = object; this._activeSelection.set('canvas', this); this._activeSelection.setCoords(); diff --git a/src/shapes/ActiveSelection.spec.ts b/src/shapes/ActiveSelection.spec.ts index ee5a9588fba..d6f74bea367 100644 --- a/src/shapes/ActiveSelection.spec.ts +++ b/src/shapes/ActiveSelection.spec.ts @@ -88,18 +88,27 @@ describe('ActiveSelection', () => { expect(canvas.getActiveSelection().aCoords).toMatchSnapshot(); }); - it('`setActiveObject` should update the active selection ref on canvas', () => { + it('`setActiveObject` should update the active selection ref on canvas if it changed', () => { const canvas = new Canvas(null); const obj1 = new FabricObject(); const obj2 = new FabricObject(); canvas.add(obj1, obj2); + const existingActiveSelection = canvas.getActiveSelection(); + const disposeSpy = jest.spyOn(existingActiveSelection, 'dispose'); const activeSelection = new ActiveSelection([obj1, obj2]); const spy = jest.spyOn(activeSelection, 'setCoords'); canvas.setActiveObject(activeSelection); expect(canvas.getActiveSelection()).toBe(activeSelection); expect(canvas.getActiveObjects()).toEqual([obj1, obj2]); + expect(disposeSpy).toHaveBeenCalled(); expect(spy).toHaveBeenCalled(); expect(activeSelection.canvas).toBe(canvas); + + spy.mockClear(); + const disposeSpy2 = jest.spyOn(activeSelection, 'dispose'); + canvas.setActiveObject(activeSelection); + expect(disposeSpy2).not.toHaveBeenCalled(); + expect(spy).not.toHaveBeenCalled(); }); it('transferring an object between active selections keeps its owning group', () => { From ca0b97e096c6f4ef5b50960ed5611ba8cd52b488 Mon Sep 17 00:00:00 2001 From: zhe-he Date: Mon, 18 Sep 2023 11:09:46 +0800 Subject: [PATCH 11/13] update(src/canvas/SelectableCanvas.ts): _setActiveObject --- src/canvas/SelectableCanvas.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/canvas/SelectableCanvas.ts b/src/canvas/SelectableCanvas.ts index 5710781e84e..920655ba2b3 100644 --- a/src/canvas/SelectableCanvas.ts +++ b/src/canvas/SelectableCanvas.ts @@ -1108,8 +1108,8 @@ export class SelectableCanvas if (object instanceof ActiveSelection && this._activeSelection !== object) { this._activeSelection.dispose(); this._activeSelection = object; - this._activeSelection.set('canvas', this); - this._activeSelection.setCoords(); + object.set('canvas', this); + object.setCoords(); } return true; From c2340aef52d8649aad5b9605f71e8672ceeee21b Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Sat, 23 Sep 2023 19:31:19 +0530 Subject: [PATCH 12/13] no dispose it is bad indeed to dispose anyways it is safe to assume that the prev active selection has no children because it has been discarded and onDelect removed all the children --- src/canvas/SelectableCanvas.ts | 1 - src/shapes/ActiveSelection.spec.ts | 5 ----- 2 files changed, 6 deletions(-) diff --git a/src/canvas/SelectableCanvas.ts b/src/canvas/SelectableCanvas.ts index 920655ba2b3..999a782c0c5 100644 --- a/src/canvas/SelectableCanvas.ts +++ b/src/canvas/SelectableCanvas.ts @@ -1106,7 +1106,6 @@ export class SelectableCanvas this._activeObject = object; if (object instanceof ActiveSelection && this._activeSelection !== object) { - this._activeSelection.dispose(); this._activeSelection = object; object.set('canvas', this); object.setCoords(); diff --git a/src/shapes/ActiveSelection.spec.ts b/src/shapes/ActiveSelection.spec.ts index d6f74bea367..f3b4fd0c5f7 100644 --- a/src/shapes/ActiveSelection.spec.ts +++ b/src/shapes/ActiveSelection.spec.ts @@ -93,21 +93,16 @@ describe('ActiveSelection', () => { const obj1 = new FabricObject(); const obj2 = new FabricObject(); canvas.add(obj1, obj2); - const existingActiveSelection = canvas.getActiveSelection(); - const disposeSpy = jest.spyOn(existingActiveSelection, 'dispose'); const activeSelection = new ActiveSelection([obj1, obj2]); const spy = jest.spyOn(activeSelection, 'setCoords'); canvas.setActiveObject(activeSelection); expect(canvas.getActiveSelection()).toBe(activeSelection); expect(canvas.getActiveObjects()).toEqual([obj1, obj2]); - expect(disposeSpy).toHaveBeenCalled(); expect(spy).toHaveBeenCalled(); expect(activeSelection.canvas).toBe(canvas); spy.mockClear(); - const disposeSpy2 = jest.spyOn(activeSelection, 'dispose'); canvas.setActiveObject(activeSelection); - expect(disposeSpy2).not.toHaveBeenCalled(); expect(spy).not.toHaveBeenCalled(); }); From 5f0b6274ebd6b8aa3af7539c427f166d825b9669 Mon Sep 17 00:00:00 2001 From: ShaMan123 Date: Thu, 28 Sep 2023 12:49:20 +0530 Subject: [PATCH 13/13] Update SelectableCanvas.ts --- src/canvas/SelectableCanvas.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/canvas/SelectableCanvas.ts b/src/canvas/SelectableCanvas.ts index a85549aecf4..bef54ab6c65 100644 --- a/src/canvas/SelectableCanvas.ts +++ b/src/canvas/SelectableCanvas.ts @@ -1186,7 +1186,7 @@ export class SelectableCanvas */ destroy() { // dispose of active selection - const activeSelection = this._activeSelection!; + const activeSelection = this._activeSelection; activeSelection.removeAll(); // @ts-expect-error disposing this._activeSelection = undefined;