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

Avoid resetting Group transform by default #9525

Closed
wants to merge 10 commits into from
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(): avoid resetting Group transform by default [#9525](https://github.com/fabricjs/fabric.js/pull/9525)
- refactor(): Change how LayoutManager handles restoring groups [#9522](https://github.com/fabricjs/fabric.js/pull/9522)
- fix(BaseConfiguration): set `devicePixelRatio` from window [#9470](https://github.com/fabricjs/fabric.js/pull/9470)
- fix(): bubble dirty flag for group only when true [#9540](https://github.com/fabricjs/fabric.js/pull/9540)
Expand Down
29 changes: 13 additions & 16 deletions src/LayoutManager/LayoutManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -474,10 +474,10 @@ describe('Layout Manager', () => {
const canvasFire = jest.fn();
target.canvas = { fire: canvasFire };

const shouldResetTransform = jest
.spyOn(manager.strategy, 'shouldResetTransform')
const onAfterLayout = jest
.spyOn(manager.strategy, 'onAfterLayout')
.mockImplementation(() => {
lifecycle.push(shouldResetTransform);
lifecycle.push(onAfterLayout);
});

const context: StrictLayoutContext = {
Expand All @@ -500,11 +500,11 @@ describe('Layout Manager', () => {
manager['onAfterLayout'](context, layoutResult);

expect(lifecycle).toEqual([
shouldResetTransform,
onAfterLayout,
targetFire,
...(bubbles ? [parentPerformLayout] : []),
]);
expect(shouldResetTransform).toBeCalledWith(context);
expect(onAfterLayout).toBeCalledWith(context);
expect(targetFire).toBeCalledWith('layout:after', {
context,
result: layoutResult,
Expand All @@ -526,32 +526,29 @@ describe('Layout Manager', () => {
}
);

test.each([true, false])('reset target transform %s', (reset) => {
const targets = [new Group([new FabricObject()]), new FabricObject()];
const target = new Group(targets);
test.each([
{ strategy: new FitContentLayout(), shouldReset: true },
{ strategy: new FixedLayout(), shouldReset: false },
])('reset target transform %s', ({ strategy, shouldReset }) => {
const target = new Group([]);
target.left = 50;

const manager = new LayoutManager();
jest
.spyOn(manager.strategy, 'shouldResetTransform')
.mockImplementation(() => {
return reset;
});
const manager = new LayoutManager(strategy);

const context: StrictLayoutContext = {
bubbles: true,
strategy: manager.strategy,
type: LAYOUT_TYPE_REMOVED,
target,
targets,
targets: [],
prevStrategy: undefined,
stopPropagation() {
this.bubbles = false;
},
};
manager['onAfterLayout'](context);

expect(target.left).toBe(reset ? 0 : 50);
expect(target.left).toBe(shouldReset ? 0 : 50);
});

test('bubbling', () => {
Expand Down
12 changes: 5 additions & 7 deletions src/LayoutManager/LayoutManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { CENTER, iMatrix } from '../constants';
import type { Group } from '../shapes/Group';
import type { FabricObject } from '../shapes/Object/FabricObject';
import { invertTransform } from '../util/misc/matrix';
import { resetObjectTransform } from '../util/misc/objectTransforms';
import { resolveOrigin } from '../util/misc/resolveOrigin';
import { FitContentLayout } from './LayoutStrategies/FitContentLayout';
import type { LayoutStrategy } from './LayoutStrategies/LayoutStrategy';
Expand Down Expand Up @@ -101,8 +100,11 @@ export class LayoutManager {
}

protected onBeforeLayout(context: StrictLayoutContext) {
const { target } = context;
const { target, strategy } = context;
const { canvas } = target;

strategy.onBeforeLayout(context);

// handle layout triggers subscription
if (
// subscribe only if instance is the target's `layoutManager`
Expand Down Expand Up @@ -253,11 +255,7 @@ export class LayoutManager {
...bubblingContext
} = context;
const { canvas } = target;
if (strategy.shouldResetTransform(context)) {
resetObjectTransform(target);
target.left = 0;
target.top = 0;
}
strategy.onAfterLayout(context);

// fire layout event (event will fire only for layouts after initialization layout)
target.fire('layout:after', {
Expand Down
9 changes: 9 additions & 0 deletions src/LayoutManager/LayoutStrategies/FitContentLayout.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { resetObjectTransform } from '../../util/misc/objectTransforms';
import type { StrictLayoutContext } from '../types';
import { LayoutStrategy } from './LayoutStrategy';

Expand All @@ -15,4 +16,12 @@ export class FitContentLayout extends LayoutStrategy {
shouldPerformLayout(context: StrictLayoutContext) {
return true;
}

onAfterLayout({ target }: StrictLayoutContext): void {
if (target.size() === 0) {
resetObjectTransform(target);
target.left = 0;
target.top = 0;
}
}
}
13 changes: 11 additions & 2 deletions src/LayoutManager/LayoutStrategies/LayoutStrategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,20 @@ export abstract class LayoutStrategy {
return result.size;
}

/**
* called from the `onBeforeLayout` hook
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
onBeforeLayout(context: StrictLayoutContext) {
// Noop
}

/**
* called from the `onAfterLayout` hook
*/
shouldResetTransform(context: StrictLayoutContext) {
return context.target.size() === 0;
// eslint-disable-next-line @typescript-eslint/no-unused-vars
onAfterLayout(context: StrictLayoutContext) {
// Noop
}

/**
Expand Down
Loading