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): initial coords [#9322](https://github.com/fabricjs/fabric.js/pull/9322)
- docs(): add link to contributing guide [#8393](https://github.com/fabricjs/fabric.js/pull/8393)
- test(e2e): Drag&Drop tests [#9112](https://github.com/fabricjs/fabric.js/pull/9112)
- fix(CanvasEvents): regression of `getPointer` usages + BREAKING: drop event data [#9186](https://github.com/fabricjs/fabric.js/pull/9186)
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
17 changes: 17 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,20 @@ describe('ActiveSelection', () => {
selection.add(new FabricObject({ left: 50, top: 50, strokeWidth: 0 }));
expect(selection.item(0).getCenterPoint()).toEqual({ x: 50, y: 50 });
});

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().getCoords()).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

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

let's test by picking aCoords and lineCoords that is what are we testing today.
We are testing that those are correct, without the need of being recalculated.
getCoords return an array and is mostly for internal usage of calculations, if we snapshot aCoords and lineCoords we also see tl,tr,bl,br.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are testing that those are correct, without the need of being recalculated.

They are not recalcuated.

I just have in mind #8767 so I avoid lineCoords already but whatever

Copy link
Member

@asturur asturur Sep 10, 2023

Choose a reason for hiding this comment

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

they are not recalculated no, but no one knows it, is not written the test is not explcit.
if getCoords change or recalculation becomes default, this test doesn't break but also becomes uneffective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not follow
What should be changed?

Copy link
Member

Choose a reason for hiding this comment

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

oh i thought you changed because i have seen outdated tag on the discussion
i ll change it never mind

expect(canvas.getActiveSelection().getCoords(true)).toMatchSnapshot();
});
});
10 changes: 0 additions & 10 deletions src/shapes/ActiveSelection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ 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';

Expand All @@ -26,15 +25,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`] = `
[
Point {
"x": 600,
"y": 200,
},
Point {
"x": 802,
"y": 200,
},
Point {
"x": 802,
"y": 250.5,
},
Point {
"x": 600,
"y": 250.5,
},
]
`;

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