Squashed parent branch #9349

commit 1395602
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Wed Oct 18 18:23:35 2023 +0700

    prettier

commit d7fe84b
Merge: d1f9b4d 7a71097
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Wed Oct 18 18:19:27 2023 +0700

    Merge branch 'master' into parent

commit d1f9b4d
Merge: 0b43b72 ef27fcd
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Sep 23 14:00:34 2023 +0530

    Merge branch 'master' into parent

commit 0b43b72
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Sep 23 13:33:25 2023 +0530

    another parent/group fix

commit 89e4c74
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Sep 16 08:56:50 2023 +0530

    e2e test

    Update index.spec.ts

commit c2fc529
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Sep 16 07:51:06 2023 +0530

    comments

commit a09f840
Author: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Date:   Sat Sep 16 02:18:07 2023 +0000

    update CHANGELOG.md

commit b3bc476
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Sep 16 07:37:01 2023 +0530

    refactor(): `parent` + fix active selection edge case

CHANGELOG.md

@@ -20,6 +20,11 @@
 - fix(#9172): dep export `Object`, `Text`, `Image` [#9433](https://github.com/fabricjs/fabric.js/pull/9433)
 
 ## [6.0.0-beta14]
+- 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]
 
 - refactor(Group): `parent` + fix(ActiveSelection): transferring object [#9349](https://github.com/fabricjs/fabric.js/pull/9349)
   **BREAKING beta**:

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

@@ -79,12 +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) {
-    object.group?._exitGroup(object);
+    // 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 previous one
+      object.group.remove(object);
+    }
+
     this._enterGroup(object, removeParentTransform);
-    return true;
   }
 
   /**
@@ -95,6 +101,7 @@ export class ActiveSelection extends Group {
    */
   exitGroup(object: FabricObject, removeParentTransform?: boolean) {
     this._exitGroup(object, removeParentTransform);
+    //  return to parent
     object.parent?._enterGroup(object, true);
   }
 
@@ -105,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,15 +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);
-    }
-    this._enterGroup(object, removeParentTransform);
+    object.group?.remove(object);
     object._set('parent', this);
-    return true;
+    this._enterGroup(object, removeParentTransform);
   }
 
   /**

src/shapes/Object/StackedObject.ts

@@ -81,7 +81,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>;

test/unit/object.js

@@ -628,35 +628,6 @@
     assert.ok(removedEventFired);
   });
 
-  QUnit.test('getParent', function (assert) {
-    const object = new fabric.Object();
-    const parent = new fabric.Object();
-    parent._exitGroup = () => { };
-    assert.ok(typeof object.getParent === 'function');
-    parent.canvas = canvas;
-    object.parent = parent;
-    assert.equal(object.getParent(), parent);
-    assert.equal(parent.getParent(), canvas);
-    const another = new fabric.Object();
-    object.parent = another;
-    object.parent.parent = parent;
-    assert.equal(object.getParent(), another);
-    assert.equal(another.getParent(), parent);
-    object.parent = undefined;
-    assert.equal(object.getParent(), undefined);
-    object.canvas = canvas;
-    assert.equal(object.getParent(), canvas);
-    object.parent = parent;
-    assert.equal(object.getParent(), parent);
-    const activeSelection = new fabric.ActiveSelection([object], { canvas });
-    assert.equal(object.group, activeSelection);
-    assert.equal(object.parent, parent);
-    assert.equal(object.canvas, canvas);
-    assert.equal(object.getParent(), parent);
-    object.parent = undefined;
-    assert.equal(object.getParent(), canvas);
-  });
-
   QUnit.test('isDescendantOf', function (assert) {
     const object = new fabric.Object();
     const parent = new fabric.Object();
@@ -681,10 +652,10 @@
     assert.equal(object.group, activeSelection);
     assert.equal(object.parent, parent);
     assert.equal(object.canvas, canvas);
-    assert.ok(object.isDescendantOf(parent), 'should recognize owning group');
+    assert.ok(object.isDescendantOf(parent), 'should recognize parent');
     assert.ok(object.isDescendantOf(activeSelection), 'should recognize active selection');
     assert.ok(object.isDescendantOf(canvas), 'should recognize canvas');
-    object.parent = undefined;
+    delete object.parent;
     assert.ok(!object.isDescendantOf(parent));
     assert.ok(object.isDescendantOf(activeSelection), 'should recognize active selection');
     assert.ok(object.isDescendantOf(canvas), 'should recognize canvas');