refactor(): expose Object#parent instead of getParent` (#8951)

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## [next]
 
+- fix(): transferring object between active selections, expose `FabricObject#parent`, rm `isActiveSelection` [#8951](https://github.com/fabricjs/fabric.js/pull/8951)
+  **BREAKING beta**:
+  - rm(): `getParent` => `FabricObject#parent`
 - fix(): fire Poly control events [#9504](https://github.com/fabricjs/fabric.js/pull/9504)
 - test(FabricObject): add a snapshot of the default values so that reordering and shuffling is verified. [#9492](https://github.com/fabricjs/fabric.js/pull/9492)
 - feat(FabricObject, Canvas) BREAKING: remove calculate true/false from the api. [#9483](https://github.com/fabricjs/fabric.js/pull/9483)

e2e/tests/selection/stale-state/index.spec.ts

@@ -1,32 +1,55 @@
+import type { Page } from '@playwright/test';
 import { expect, test } from '@playwright/test';
 import setup from '../../../setup';
 import { CanvasUtil } from '../../../utils/CanvasUtil';
 
 setup();
 
-test('selection stale state #9087', async ({ page }) => {
-  await test.step('select', async () => {
-    await page.mouse.move(20, 20);
-    await page.mouse.down();
-    await page.mouse.move(600, 600, { steps: 20 });
-    await page.mouse.up();
-  });
-  await test.step('rotate', async () => {
-    await page.mouse.move(400, 150);
-    await page.mouse.down();
-    await page.mouse.move(570, 150, { steps: 20 });
-    await page.mouse.up();
-  });
-  await test.step('deselect', async () => {
-    await page.mouse.move(20, 20);
-    await page.mouse.down();
-    await page.mouse.up();
-  });
-  await test.step('select', async () => {
-    await page.mouse.move(20, 20);
-    await page.mouse.down();
-    await page.mouse.move(600, 600, { steps: 20 });
-    await page.mouse.up();
+const tests = [
+  {
+    name: 'selection stale state #9087',
+    step: (page: Page) =>
+      test.step('deselect', async () => {
+        await page.mouse.move(20, 20);
+        await page.mouse.down();
+        await page.mouse.up();
+      }),
+  },
+  {
+    name: 'replacing selection',
+    step: (page: Page) =>
+      test.step('replace selection', () =>
+        new CanvasUtil(page).executeInBrowser((canvas) => {
+          canvas.setActiveObject(
+            new fabric.ActiveSelection(canvas.getActiveObjects())
+          );
+        })),
+  },
+];
+
+for (const { name, step } of tests) {
+  test(name, async ({ page }) => {
+    await test.step('select', async () => {
+      await page.mouse.move(20, 20);
+      await page.mouse.down();
+      await page.mouse.move(600, 600, { steps: 20 });
+      await page.mouse.up();
+    });
+    await test.step('rotate', async () => {
+      await page.mouse.move(400, 150);
+      await page.mouse.down();
+      await page.mouse.move(570, 150, { steps: 20 });
+      await page.mouse.up();
+    });
+    await step(page);
+    await test.step('select', async () => {
+      await page.mouse.move(20, 20);
+      await page.mouse.down();
+      await page.mouse.move(600, 600, { steps: 20 });
+      await page.mouse.up();
+    });
+    expect(await new CanvasUtil(page).screenshot()).toMatchSnapshot({
+      name: 'selection-stale-state.png',
+    });
   });
-  expect(await new CanvasUtil(page).screenshot()).toMatchSnapshot();
-});
+}

e2e/tests/selection/stale-state/index.ts

@@ -21,12 +21,6 @@ beforeAll(
       originY: 'center',
     });
     canvas.add(rect1, rect2);
-    canvas.on('mouse:down', ({ pointer, absolutePointer }) =>
-      console.log(pointer, absolutePointer)
-    );
-    canvas.on('mouse:up', ({ pointer, absolutePointer }) =>
-      console.log(pointer, absolutePointer)
-    );
     return { rect1, rect2 };
   },
   { enableRetinaScaling: false }

src/shapes/ActiveSelection.spec.ts

@@ -88,7 +88,7 @@ describe('ActiveSelection', () => {
   });
 
   it('`setActiveObject` should update the active selection ref on canvas if it changed', () => {
-    const canvas = new Canvas(null);
+    const canvas = new Canvas();
     const obj1 = new FabricObject();
     const obj2 = new FabricObject();
     canvas.add(obj1, obj2);
@@ -105,18 +105,76 @@ describe('ActiveSelection', () => {
     expect(spy).not.toHaveBeenCalled();
   });
 
-  it('transferring an object between active selections keeps its owning group', () => {
+  test('adding and removing an object belonging to a group', () => {
+    const object = new FabricObject();
+    const group = new Group([object]);
+    const canvas = new Canvas();
+    const activeSelection = canvas.getActiveSelection();
+
+    const eventsSpy = jest.spyOn(object, 'fire');
+    const removeSpy = jest.spyOn(group, 'remove');
+    const exitSpy = jest.spyOn(group, '_exitGroup');
+    const enterSpy = jest.spyOn(activeSelection, 'enterGroup');
+
+    expect(object.group).toBe(group);
+    expect(object.parent).toBe(group);
+    expect(object.canvas).toBeUndefined();
+
+    activeSelection.add(object);
+    expect(object.group).toBe(activeSelection);
+    expect(object.parent).toBe(group);
+    expect(object.canvas).toBe(canvas);
+    expect(removeSpy).not.toBeCalled();
+    expect(exitSpy).toBeCalledWith(object);
+    expect(enterSpy).toBeCalledWith(object, true);
+    expect(eventsSpy).toHaveBeenNthCalledWith(1, 'added', {
+      target: activeSelection,
+    });
+
+    activeSelection.remove(object);
+    expect(eventsSpy).toHaveBeenNthCalledWith(2, 'removed', {
+      target: activeSelection,
+    });
+    expect(object.group).toBe(group);
+    expect(object.parent).toBe(group);
+    expect(object.canvas).toBeUndefined();
+  });
+
+  test('transferring an object between active selections', () => {
     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);
+    expect(object.parent).toBe(group);
+
+    const eventsSpy = jest.spyOn(object, 'fire');
+    const removeSpy = jest.spyOn(activeSelection1, 'remove');
+
+    Object.entries({
+      object,
+      group,
+      activeSelection1,
+      activeSelection2,
+    }).forEach(([key, obj]) => jest.spyOn(obj, 'toJSON').mockReturnValue(key));
+
     activeSelection2.add(object);
     expect(object.group).toBe(activeSelection2);
-    expect(object.getParent(true)).toBe(group);
+    expect(object.parent).toBe(group);
+    expect(removeSpy).toBeCalledWith(object);
+    expect(eventsSpy).toHaveBeenNthCalledWith(1, 'removed', {
+      target: activeSelection1,
+    });
+    expect(eventsSpy).toHaveBeenNthCalledWith(2, 'added', {
+      target: activeSelection2,
+    });
+
     activeSelection2.removeAll();
     expect(object.group).toBe(group);
-    expect(object.getParent(true)).toBe(group);
+    expect(object.parent).toBe(group);
+    expect(eventsSpy).toHaveBeenNthCalledWith(3, 'removed', {
+      target: activeSelection2,
+    });
+    expect(eventsSpy).toBeCalledTimes(3);
   });
 });

src/shapes/ActiveSelection.ts

@@ -76,22 +76,29 @@ export class ActiveSelection extends Group {
   }
 
   /**
+   * Change an object so that it can be part of an active selection.
+   * this method is called by multiselectAdd from canvas code.
    * @private
    * @param {FabricObject} object
    * @param {boolean} [removeParentTransform] true if object is in canvas coordinate plane
-   * @returns {boolean} true if object entered group
    */
   enterGroup(object: FabricObject, removeParentTransform?: boolean) {
-    if (object.group) {
-      //  save ref to group for later in order to return to it
-      const parent = object.group;
-      parent._exitGroup(object);
-      // make sure we are setting the correct owning group
-      // in case `object` is transferred between active selections
-      !(parent instanceof ActiveSelection) && (object.__owningGroup = parent);
+    // This condition check that the object has currently a group, and the group
+    // is also its parent, meaning that is not in an active selection, but is
+    // in a normal group.
+    if (object.parent && object.parent === object.group) {
+      // Disconnect the object from the group functionalities, but keep the ref parent intact
+      // for later re-enter
+      object.parent._exitGroup(object);
+      // in this case the object is probably inside an active selection.
+    } else if (object.group && object.parent !== object.group) {
+      // in this case group.remove will also clear the old parent reference.
+      object.group.remove(object);
     }
+    // enter the active selection from a render perspective
+    // the object will be in the objects array of both the ActiveSelection and the Group
+    // but referenced in the group's _activeObjects so that it won't be rendered twice.
     this._enterGroup(object, removeParentTransform);
-    return true;
   }
 
   /**
@@ -102,12 +109,8 @@ export class ActiveSelection extends Group {
    */
   exitGroup(object: FabricObject, removeParentTransform?: boolean) {
     this._exitGroup(object, removeParentTransform);
-    const parent = object.__owningGroup;
-    if (parent) {
-      //  return to owning group
-      parent._enterGroup(object, true);
-      delete object.__owningGroup;
-    }
+    // return to parent
+    object.parent && object.parent._enterGroup(object, true);
   }
 
   /**
@@ -117,11 +120,10 @@ export class ActiveSelection extends Group {
    */
   _onAfterObjectsChange(type: 'added' | 'removed', targets: FabricObject[]) {
     super._onAfterObjectsChange(type, targets);
-    const groups: Group[] = [];
+    const groups = new Set<Group>();
     targets.forEach((object) => {
-      object.group &&
-        !groups.includes(object.group) &&
-        groups.push(object.group);
+      const { parent } = object;
+      parent && groups.add(parent);
     });
     if (type === 'removed') {
       //  invalidate groups' layout and mark as dirty

src/shapes/Group.spec.ts

@@ -1,3 +1,4 @@
+import { Canvas } from '../canvas/Canvas';
 import { Group } from './Group';
 import { FabricObject } from './Object/FabricObject';
 
@@ -12,4 +13,41 @@ describe('Group', () => {
     expect(objs).toHaveLength(2);
     expect(group._objects).toHaveLength(3);
   });
+
+  test('adding and removing an object', () => {
+    const object = new FabricObject();
+    const group = new Group([object]);
+    const group2 = new Group();
+    const canvas = new Canvas();
+
+    const eventsSpy = jest.spyOn(object, 'fire');
+    const removeSpy = jest.spyOn(group, 'remove');
+    const exitSpy = jest.spyOn(group, 'exitGroup');
+    const enterSpy = jest.spyOn(group2, 'enterGroup');
+
+    expect(object.group).toBe(group);
+    expect(object.parent).toBe(group);
+    expect(object.canvas).toBeUndefined();
+
+    canvas.add(group, group2);
+    expect(object.canvas).toBe(canvas);
+
+    group2.add(object);
+    expect(object.group).toBe(group2);
+    expect(object.parent).toBe(group2);
+    expect(object.canvas).toBe(canvas);
+    expect(removeSpy).toBeCalledWith(object);
+    expect(exitSpy).toBeCalledWith(object, undefined);
+    expect(enterSpy).toBeCalledWith(object, true);
+    expect(eventsSpy).toHaveBeenNthCalledWith(1, 'removed', { target: group });
+    expect(eventsSpy).toHaveBeenNthCalledWith(2, 'added', { target: group2 });
+
+    group2.remove(object);
+    expect(eventsSpy).toHaveBeenNthCalledWith(3, 'removed', { target: group2 });
+    expect(object.group).toBeUndefined();
+    expect(object.parent).toBeUndefined();
+    expect(object.canvas).toBeUndefined();
+
+    expect(eventsSpy).toBeCalledTimes(3);
+  });
 });

src/shapes/Group.ts

@@ -273,12 +273,6 @@ export class Group extends createCollectionMixin(
     object.fire('added', { target: this });
   }
 
-  _onRelativeObjectAdded(object: FabricObject) {
-    this.enterGroup(object, false);
-    this.fire('object:added', { target: object });
-    object.fire('added', { target: this });
-  }
-
   /**
    * @private
    * @param {FabricObject} object
@@ -401,14 +395,11 @@ export class Group extends createCollectionMixin(
    * @private
    * @param {FabricObject} object
    * @param {boolean} [removeParentTransform] true if object is in canvas coordinate plane
-   * @returns {boolean} true if object entered group
    */
   enterGroup(object: FabricObject, removeParentTransform?: boolean) {
-    if (object.group) {
-      object.group.remove(object);
-    }
+    object.group && object.group.remove(object);
+    object._set('parent', this);
     this._enterGroup(object, removeParentTransform);
-    return true;
   }
 
   /**
@@ -451,11 +442,16 @@ export class Group extends createCollectionMixin(
    */
   exitGroup(object: FabricObject, removeParentTransform?: boolean) {
     this._exitGroup(object, removeParentTransform);
+    object._set('parent', undefined);
     object._set('canvas', undefined);
   }
 
   /**
-   * @private
+   * Executes the inner fabric logic of exiting a group.
+   * - Stop watching the object
+   * - Remove the object from the optimization map this._activeObjects
+   * - unset the group property of the object
+   * @protected
    * @param {FabricObject} object
    * @param {boolean} [removeParentTransform] true if object should exit group without applying group's transform to it
    */

src/shapes/Image.ts

@@ -821,7 +821,7 @@ export class FabricImage<
   static fromURL<T extends TOptions<ImageProps>>(
     url: string,
     { crossOrigin = null, signal }: LoadImageOptions = {},
-    imageOptions: T
+    imageOptions?: T
   ): Promise<FabricImage> {
     return loadImage(url, { crossOrigin, signal }).then(
       (img) => new this(img, imageOptions)