diff --git a/CHANGELOG.md b/CHANGELOG.md index 53684702f8e..6da9ff3f2c0 100644 --- a/CHANGELOG.md +++ b/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) 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 cd5adfde41a..b3e46590737 100644 --- a/src/shapes/ActiveSelection.ts +++ b/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(); 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 15ae6d704d1..c1d58f8200b 100644 --- a/src/shapes/Group.ts +++ b/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 */ diff --git a/src/shapes/Image.ts b/src/shapes/Image.ts index 3d46ce4623f..f3e699792e8 100644 --- a/src/shapes/Image.ts +++ b/src/shapes/Image.ts @@ -821,7 +821,7 @@ export class FabricImage< static fromURL>( url: string, { crossOrigin = null, signal }: LoadImageOptions = {}, - imageOptions: T + imageOptions?: T ): Promise { return loadImage(url, { crossOrigin, signal }).then( (img) => new this(img, imageOptions) diff --git a/src/shapes/Object/StackedObject.ts b/src/shapes/Object/StackedObject.ts index ca7876a1891..165e9287d8d 100644 --- a/src/shapes/Object/StackedObject.ts +++ b/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(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 @@ -62,13 +50,14 @@ export class StackedObject< * @returns {boolean} */ isDescendantOf(target: TAncestor): boolean { + const { parent, group } = this; return ( - this.__owningGroup === target || - this.group === target || + parent === target || + group === target || this.canvas === target || // walk up - (!!this.__owningGroup && this.__owningGroup.isDescendantOf(target)) || - (!!this.group && this.group.isDescendantOf(target)) + (!!parent && parent.isDescendantOf(target)) || + (!!group && group !== parent && group.isDescendantOf(target)) ); } @@ -83,7 +72,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/src/util/typeAssertions.ts b/src/util/typeAssertions.ts index 1970eb61c95..6c7a8a54316 100644 --- a/src/util/typeAssertions.ts +++ b/src/util/typeAssertions.ts @@ -2,7 +2,6 @@ import type { FabricObject } from '../shapes/Object/Object'; import type { TFiller } from '../typedefs'; import type { FabricText } from '../shapes/Text/Text'; import type { Pattern } from '../Pattern'; -import type { ActiveSelection } from '../shapes/ActiveSelection'; import type { Path } from '../shapes/Path'; export const isFiller = ( @@ -25,12 +24,6 @@ export const isPattern = (filler: TFiller): filler is Pattern => { ); }; -export const isActiveSelection = ( - fabricObject?: FabricObject -): fabricObject is ActiveSelection => { - return !!fabricObject && fabricObject.isType('ActiveSelection'); -}; - export const isTextObject = ( fabricObject?: FabricObject ): fabricObject is FabricText => { diff --git a/test/unit/activeselection.js b/test/unit/activeselection.js index fdd993f83fc..b5f0b81847c 100644 --- a/test/unit/activeselection.js +++ b/test/unit/activeselection.js @@ -38,6 +38,7 @@ assert.ok(group); assert.ok(group instanceof fabric.ActiveSelection, 'should be instance of fabric.ActiveSelection'); + assert.ok(!group.item(0).parent, 'parent ref is undefined'); }); QUnit.test('toString', function(assert) { diff --git a/test/unit/group.js b/test/unit/group.js index d1983cacaf3..7b419d854fd 100644 --- a/test/unit/group.js +++ b/test/unit/group.js @@ -88,12 +88,14 @@ assert.ok(typeof group.remove === 'function'); assert.ok(rect1.group === group, 'group should be referenced'); + assert.ok(rect1.parent === group, 'parent should be referenced'); group.on('object:removed', (opt) => { targets.push(opt.target); }); rect1.on('removed', (opt) => { assert.equal(opt.target, group); assert.ok(rect1.group === undefined, 'group should not be referenced'); + assert.ok(rect1.parent === undefined, 'parent should not be referenced'); fired = true; }); var removed = group.remove(rect2); @@ -595,9 +597,12 @@ assert.equal(firstObjInGroup.group, group); assert.equal(secondObjInGroup.group, group); + assert.equal(firstObjInGroup.parent, group); + assert.equal(secondObjInGroup.parent, group); group.remove(firstObjInGroup); assert.ok(typeof firstObjInGroup.group === 'undefined'); + assert.ok(typeof firstObjInGroup.parent === 'undefined'); }); QUnit.test('insertAt', function (assert) { diff --git a/test/unit/object.js b/test/unit/object.js index fcd66e55750..f9ec160a50a 100644 --- a/test/unit/object.js +++ b/test/unit/object.js @@ -628,63 +628,33 @@ 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.group = parent; - assert.equal(object.getParent(), parent); - assert.equal(parent.getParent(), canvas); - const another = new fabric.Object(); - object.group = another; - object.group.group = parent; - assert.equal(object.getParent(), another); - assert.equal(another.getParent(), parent); - object.group = undefined; - assert.equal(object.getParent(), undefined); - object.canvas = canvas; - assert.equal(object.getParent(), canvas); - object.group = parent; - assert.equal(object.getParent(), parent); - const activeSelection = new fabric.ActiveSelection([object], { canvas }); - assert.equal(object.group, activeSelection); - assert.equal(object.__owningGroup, parent); - assert.equal(object.canvas, canvas); - assert.equal(object.getParent(), parent); - object.__owningGroup = undefined; - assert.equal(object.getParent(), canvas); - }); - QUnit.test('isDescendantOf', function (assert) { const object = new fabric.Object(); const parent = new fabric.Object(); parent._exitGroup = () => { }; assert.ok(typeof object.isDescendantOf === 'function'); parent.canvas = canvas; - object.group = parent; + object.parent = parent; assert.ok(object.isDescendantOf(parent)); - object.group = new fabric.Object(); - object.group.group = parent; + object.parent = new fabric.Object(); + object.parent.parent = parent; assert.ok(object.isDescendantOf(parent)); assert.ok(object.isDescendantOf(canvas)); - object.group = undefined; + object.parent = undefined; assert.ok(object.isDescendantOf(parent) === false); assert.ok(object.isDescendantOf(canvas) === false); object.canvas = canvas; assert.ok(object.isDescendantOf(canvas)); assert.ok(object.isDescendantOf(object) === false); - object.group = parent; - assert.equal(object.getParent(), parent); + object.parent = parent; const activeSelection = new fabric.ActiveSelection([object], { canvas }); assert.equal(object.group, activeSelection); - assert.equal(object.__owningGroup, parent); + 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.__owningGroup = 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'); @@ -696,15 +666,15 @@ var other = new fabric.Object(); assert.ok(typeof object.getAncestors === 'function'); assert.deepEqual(object.getAncestors(), []); - object.group = parent; + object.parent = parent; assert.deepEqual(object.getAncestors(), [parent]); parent.canvas = canvas; assert.deepEqual(object.getAncestors(), [parent, canvas]); - parent.group = other; + parent.parent = other; assert.deepEqual(object.getAncestors(), [parent, other]); other.canvas = canvas; assert.deepEqual(object.getAncestors(), [parent, other, canvas]); - delete object.group; + delete object.parent; assert.deepEqual(object.getAncestors(), []); }); @@ -730,9 +700,11 @@ } _onObjectAdded(object) { object.group = this; + object.parent = this; } _onObjectRemoved(object) { delete object.group; + delete object.parent; } removeAll() { this.remove(...this._objects);