Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Canvas): setActiveObject should update canvas#_activeSelection #9336

Merged
merged 15 commits into from
Oct 2, 2023
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Changelog

- fix(Canvas): `setActiveObject` should update `canvas#_activeSelection` [#9336](https://github.com/fabricjs/fabric.js/pull/9336)

## [next]

- fix(Canvas): invalidate `_objectsToRender` on stack change [#9387](https://github.com/fabricjs/fabric.js/pull/9387)
Expand Down
10 changes: 8 additions & 2 deletions src/canvas/SelectableCanvas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
protected declare _isCurrentlyDrawing: boolean;
declare freeDrawingBrush?: BaseBrush;
declare _activeObject?: FabricObject;
protected readonly _activeSelection: ActiveSelection;
protected _activeSelection: ActiveSelection;

constructor(
el?: string | HTMLCanvasElement,
Expand Down Expand Up @@ -1086,6 +1086,12 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
}
this._activeObject = object;

if (object instanceof ActiveSelection && this._activeSelection !== object) {
this._activeSelection = object;
object.set('canvas', this);
object.setCoords();
}

return true;
}

Expand Down Expand Up @@ -1161,7 +1167,7 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
*/
destroy() {
// dispose of active selection
const activeSelection = this._activeSelection!;
const activeSelection = this._activeSelection;
activeSelection.removeAll();
// @ts-expect-error disposing
this._activeSelection = undefined;
Expand Down
34 changes: 34 additions & 0 deletions src/shapes/ActiveSelection.spec.ts
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Canvas } from '../canvas/Canvas';
import { ActiveSelection } from './ActiveSelection';
import { Group } from './Group';
import { FabricObject } from './Object/FabricObject';

describe('ActiveSelection', () => {
Expand Down Expand Up @@ -86,4 +87,37 @@ describe('ActiveSelection', () => {
expect(canvas.getActiveSelection().lineCoords).toMatchSnapshot();
expect(canvas.getActiveSelection().aCoords).toMatchSnapshot();
});

it('`setActiveObject` should update the active selection ref on canvas if it changed', () => {
const canvas = new Canvas(null);
const obj1 = new FabricObject();
const obj2 = new FabricObject();
canvas.add(obj1, obj2);
const activeSelection = new ActiveSelection([obj1, obj2]);
const spy = jest.spyOn(activeSelection, 'setCoords');
canvas.setActiveObject(activeSelection);
expect(canvas.getActiveSelection()).toBe(activeSelection);
expect(canvas.getActiveObjects()).toEqual([obj1, obj2]);
expect(spy).toHaveBeenCalled();
expect(activeSelection.canvas).toBe(canvas);

spy.mockClear();
canvas.setActiveObject(activeSelection);
expect(spy).not.toHaveBeenCalled();
});

it('transferring an object between active selections keeps its owning group', () => {
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);
activeSelection2.add(object);
expect(object.group).toBe(activeSelection2);
expect(object.getParent(true)).toBe(group);
activeSelection2.removeAll();
expect(object.group).toBe(group);
expect(object.getParent(true)).toBe(group);
});
});
4 changes: 3 additions & 1 deletion src/shapes/ActiveSelection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ export class ActiveSelection extends Group {
// save ref to group for later in order to return to it
const parent = object.group;
parent._exitGroup(object);
object.__owningGroup = parent;
// make sure we are setting the correct owning group
// in case `object` is transferred between active selections
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there shouldn't be transferring between active selections.
How that happens?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you can do:

new ActiveSelection(canvas.getActiveObjects())

And that will hit this code

!(parent instanceof ActiveSelection) && (object.__owningGroup = parent);
}
this._enterGroup(object, removeParentTransform);
return true;
Expand Down
Loading