refactor(Group): parent + fix(ActiveSelection): transferring object

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ## [next]
 
+- refactor(Group): `parent` + fix(ActiveSelection): transferring object [#9349](https://github.com/fabricjs/fabric.js/pull/9349)
+  **BREAKING beta**:
+  - rm(): `getParent` => `FabricObject#parent`
+
+## [6.0.0-beta13]
+
 - fix(Object): fixes centeredScaling prop type [#9401](https://github.com/fabricjs/fabric.js/pull/9401)
 - CI(): fix build caching + tests when merging to master [#9404](https://github.com/fabricjs/fabric.js/pull/9404)
 - chore(): export poly control utils [#9400](https://github.com/fabricjs/fabric.js/pull/9400)

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

@@ -89,7 +89,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);
@@ -106,18 +106,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

@@ -79,19 +79,18 @@ export class ActiveSelection extends Group {
    * @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);
+    // make sure we exit the parent only once
+    if (object.parent && object.parent === object.group) {
+      // keep the object part of the parent
+      object.parent._exitGroup(object);
+    } else if (object.group && object.parent !== object.group) {
+      // in case `object` is transferred between active selections we remove it from the prev
+      object.group.remove(object);
     }
+
     this._enterGroup(object, removeParentTransform);
-    return true;
   }
 
   /**
@@ -102,12 +101,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?._enterGroup(object, true);
   }
 
   /**
@@ -117,11 +112,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

@@ -401,14 +401,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?.remove(object);
+    object._set('parent', this);
     this._enterGroup(object, removeParentTransform);
-    return true;
   }
 
   /**
@@ -451,6 +448,7 @@ export class Group extends createCollectionMixin(
    */
   exitGroup(object: FabricObject, removeParentTransform?: boolean) {
     this._exitGroup(object, removeParentTransform);
+    object._set('parent', undefined);
     object._set('canvas', undefined);
   }
 

src/shapes/Object/StackedObject.ts

@@ -3,7 +3,6 @@ import type { Group } from '../Group';
 import type { Canvas } from '../../canvas/Canvas';
 import type { StaticCanvas } from '../../canvas/StaticCanvas';
 import { ObjectGeometry } from './ObjectGeometry';
-import { isActiveSelection } from '../../util/typeAssertions';
 
 type TAncestor = StackedObject | Canvas | StaticCanvas;
 type TCollection = Group | Canvas | StaticCanvas;
@@ -42,18 +41,7 @@ export class StackedObject<
    * A reference to the parent of the object
    * Used to keep the original parent ref when the object has been added to an ActiveSelection, hence loosing the `group` ref
    */
-  declare __owningGroup?: Group;
-
-  /**
-   * Returns instance's parent **EXCLUDING** `ActiveSelection`
-   * @param {boolean} [strict] exclude canvas as well
-   */
-  getParent<T extends boolean>(strict?: T): TAncestor | undefined {
-    return (
-      (isActiveSelection(this.group) ? this.__owningGroup : this.group) ||
-      (strict ? undefined : this.canvas)
-    );
-  }
+  declare parent?: Group;
 
   /**
    * Checks if object is descendant of target
@@ -63,11 +51,11 @@ export class StackedObject<
    */
   isDescendantOf(target: TAncestor): boolean {
     return (
-      this.__owningGroup === target ||
+      this.parent === target ||
       this.group === target ||
       this.canvas === target ||
       // walk up
-      (!!this.__owningGroup && this.__owningGroup.isDescendantOf(target)) ||
+      (!!this.parent && this.parent.isDescendantOf(target)) ||
       (!!this.group && this.group.isDescendantOf(target))
     );
   }
@@ -83,7 +71,9 @@ export class StackedObject<
     let parent: TAncestor | undefined = this;
     do {
       parent =
-        parent instanceof StackedObject ? parent.getParent(strict) : undefined;
+        parent instanceof StackedObject
+          ? parent.parent ?? (!strict ? parent.canvas : undefined)
+          : undefined;
       parent && ancestors.push(parent);
     } while (parent);
     return ancestors as Ancestors<T>;