diff --git a/CHANGELOG.md b/CHANGELOG.md index 710a0e5bdc3..ac860c8a237 100644 --- a/CHANGELOG.md +++ b/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**: diff --git a/e2e/tests/selection/stale-state/index.spec.ts b/e2e/tests/selection/stale-state/index.spec.ts index 2bf714b8bb0..fb388e8675a 100644 --- a/e2e/tests/selection/stale-state/index.spec.ts +++ b/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(); -}); +} diff --git a/e2e/tests/selection/stale-state/index.spec.ts-snapshots/selection-stale-state-9087-1.png b/e2e/tests/selection/stale-state/index.spec.ts-snapshots/selection-stale-state-9087-1.png deleted file mode 100644 index b0eedcc3ac7..00000000000 Binary files a/e2e/tests/selection/stale-state/index.spec.ts-snapshots/selection-stale-state-9087-1.png and /dev/null differ diff --git a/e2e/tests/selection/stale-state/index.spec.ts-snapshots/selection-stale-state.png b/e2e/tests/selection/stale-state/index.spec.ts-snapshots/selection-stale-state.png new file mode 100644 index 00000000000..3337823837e Binary files /dev/null and b/e2e/tests/selection/stale-state/index.spec.ts-snapshots/selection-stale-state.png differ diff --git a/e2e/tests/selection/stale-state/index.ts b/e2e/tests/selection/stale-state/index.ts index e7566061802..fff6c47902f 100644 --- a/e2e/tests/selection/stale-state/index.ts +++ b/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 } diff --git a/src/shapes/ActiveSelection.spec.ts b/src/shapes/ActiveSelection.spec.ts index ab53cafa300..a7a9dc191a8 100644 --- a/src/shapes/ActiveSelection.spec.ts +++ b/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); }); }); diff --git a/src/shapes/ActiveSelection.ts b/src/shapes/ActiveSelection.ts index 5f46a28dab0..de7c8aca8f3 100644 --- a/src/shapes/ActiveSelection.ts +++ b/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(); 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 diff --git a/src/shapes/Group.spec.ts b/src/shapes/Group.spec.ts index af57f825dd6..95099a6c925 100644 --- a/src/shapes/Group.spec.ts +++ b/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); + }); }); diff --git a/src/shapes/Group.ts b/src/shapes/Group.ts index 3ada40404ef..6cdaaed58be 100644 --- a/src/shapes/Group.ts +++ b/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); } /** diff --git a/src/shapes/Object/StackedObject.ts b/src/shapes/Object/StackedObject.ts index 10d8151fc7c..bd0515bca68 100644 --- a/src/shapes/Object/StackedObject.ts +++ b/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; diff --git a/test/unit/object.js b/test/unit/object.js index c526a85fb3b..32a8db5f4db 100644 --- a/test/unit/object.js +++ b/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');