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(ActiveSelection): make sure canvas is in charge of setting initial coords #9322

Merged
merged 11 commits into from
Sep 10, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [next]

- fix(ActiveSelection): make sure canvas is in charge of setting initial coords [#9322](https://github.com/fabricjs/fabric.js/pull/9322)
- test(): Migrate json control tests [#9323](https://github.com/fabricjs/fabric.js/pull/9323)
- fix() Textbox inputs with new lines, regression from #9097 [#9192](https://github.com/fabricjs/fabric.js/pull/9192)
- docs(): add link to contributing guide [#8393](https://github.com/fabricjs/fabric.js/pull/8393)
Expand Down
1 change: 1 addition & 0 deletions src/canvas/SelectableCanvas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
super(el, options);
this._activeSelection = activeSelection;
this._activeSelection.set('canvas', this);
this._activeSelection.setCoords();
}

protected initElements(el: string | HTMLCanvasElement) {
Expand Down
31 changes: 31 additions & 0 deletions src/shapes/ActiveSelection.spec.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Canvas } from '../canvas/Canvas';
import { ActiveSelection } from './ActiveSelection';
import { FabricObject } from './Object/FabricObject';

Expand Down Expand Up @@ -55,4 +56,34 @@ describe('ActiveSelection', () => {
selection.add(new FabricObject({ left: 50, top: 50, strokeWidth: 0 }));
expect(selection.item(0).getCenterPoint()).toEqual({ x: 50, y: 50 });
});

// remove skip once #9152 is merged
it.skip('should not set coords in the constructor', () => {
const spy = jest.spyOn(ActiveSelection.prototype, 'setCoords');
new ActiveSelection([
new FabricObject({
left: 100,
top: 100,
width: 100,
height: 100,
}),
]);
expect(spy).not.toHaveBeenCalled();
});

it('sets coords after attaching to canvas', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a canvas test? is the canvas executing the new code no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, I have no idea where it should belong

const canvas = new Canvas(null, {
activeSelection: new ActiveSelection([
new FabricObject({
left: 100,
top: 100,
width: 100,
height: 100,
}),
]),
viewportTransform: [2, 0, 0, 0.5, 400, 150],
});
expect(canvas.getActiveSelection().lineCoords).toMatchSnapshot();
expect(canvas.getActiveSelection().aCoords).toMatchSnapshot();
});
});
23 changes: 13 additions & 10 deletions src/shapes/ActiveSelection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,26 @@ import { classRegistry } from '../ClassRegistry';
import type { GroupProps, LayoutContext } from './Group';
import { Group } from './Group';
import type { FabricObject } from './Object/FabricObject';
import type { TOptions } from '../typedefs';

export type MultiSelectionStacking = 'canvas-stacking' | 'selection-order';

export interface ActiveSelectionOptions extends GroupProps {
multiSelectionStacking: MultiSelectionStacking;
}

/**
* Used by Canvas to manage selection.
* Canvas accepts an `activeSelection` option allowing overriding and customization.
*
* @example
* class MyActiveSelection extends ActiveSelection {
* ...
* }
*
* const canvas = new Canvas(el, {
* activeSelection: new MyActiveSelection()
* })
*/
export class ActiveSelection extends Group {
declare _objects: FabricObject[];

Expand All @@ -26,15 +38,6 @@ export class ActiveSelection extends Group {

static type = 'ActiveSelection';

constructor(
objects?: FabricObject[],
options?: TOptions<ActiveSelectionOptions>,
objectsRelativeToGroup?: boolean
) {
super(objects, options, objectsRelativeToGroup);
this.setCoords();
}

/**
* @private
*/
Expand Down
43 changes: 43 additions & 0 deletions src/shapes/__snapshots__/ActiveSelection.spec.ts.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`ActiveSelection sets coords after attaching to canvas 1`] = `
{
"bl": Point {
"x": 600,
"y": 250.5,
},
"br": Point {
"x": 802,
"y": 250.5,
},
"tl": Point {
"x": 600,
"y": 200,
},
"tr": Point {
"x": 802,
"y": 200,
},
}
`;

exports[`ActiveSelection sets coords after attaching to canvas 2`] = `
{
"bl": Point {
"x": 100,
"y": 201,
},
"br": Point {
"x": 201,
"y": 201,
},
"tl": Point {
"x": 100,
"y": 100,
},
"tr": Point {
"x": 201,
"y": 100,
},
}
`;
Loading