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): reset positioning when cleared #9088

Merged
merged 14 commits into from
Aug 30, 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): reset positioning when cleared [#9088](https://github.com/fabricjs/fabric.js/pull/9088)
- ci(): generate docs [#9169](https://github.com/fabricjs/fabric.js/pull/9169)
- fix(utils) Fixes the code for the anchor point in point controls for polygons [#9178](https://github.com/fabricjs/fabric.js/pull/9178)
- CD(): website submodule [#9165](https://github.com/fabricjs/fabric.js/pull/9165)
Expand Down
31 changes: 31 additions & 0 deletions e2e/tests/selection/stale-state/index.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { expect, test } from '@playwright/test';
import { CanvasUtil } from '../../../utils/CanvasUtil';

import '../../../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();
});
expect(await new CanvasUtil(page).screenshot()).toMatchSnapshot();
});
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
33 changes: 33 additions & 0 deletions e2e/tests/selection/stale-state/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { Rect } from 'fabric';
import { beforeAll } from '../../test';

beforeAll(
(canvas) => {
const centerPoint = canvas.getCenterPoint();
const rect1 = new Rect({
width: 75,
height: 75,
top: centerPoint.y - 75,
left: centerPoint.x - 75,
originX: 'center',
originY: 'center',
});
const rect2 = new Rect({
width: 75,
height: 75,
top: centerPoint.y + 75,
left: centerPoint.x + 75,
originX: 'center',
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 }
);
58 changes: 58 additions & 0 deletions src/shapes/ActiveSelection.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { ActiveSelection } from './ActiveSelection';
import { FabricObject } from './Object/FabricObject';

describe('ActiveSelection', () => {
it('clearing active selection objects resets transform', () => {
const obj = new FabricObject({
left: 100,
top: 100,
width: 100,
height: 100,
});
const selection = new ActiveSelection([obj], {
left: 200,
top: 200,
angle: 45,
skewX: 0.5,
skewY: -0.5,
});
selection.remove(obj);
expect(selection).toMatchObject({
left: 0,
top: 0,
angle: 0,
scaleX: 1,
scaleY: 1,
skewX: 0,
skewY: 0,
flipX: false,
flipY: false,
_objects: [],
});
});

it('deselect removes all objects and resets transform', () => {
const selection = new ActiveSelection([], {
left: 200,
top: 100,
angle: 45,
});
const spy = jest.spyOn(selection, 'removeAll');
selection.onDeselect();
expect(spy).toHaveBeenCalled();
expect(selection).toMatchObject({
left: 0,
top: 0,
angle: 0,
scaleX: 1,
scaleY: 1,
skewX: 0,
skewY: 0,
flipX: false,
flipY: false,
_objects: [],
});
selection.add(new FabricObject({ left: 50, top: 50, strokeWidth: 0 }));
expect(selection.item(0).getCenterPoint()).toEqual({ x: 50, y: 50 });
});
});
21 changes: 20 additions & 1 deletion src/shapes/ActiveSelection.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { ControlRenderingStyleOverride } from '../controls/controlRendering';
import { classRegistry } from '../ClassRegistry';
import type { GroupProps } from './Group';
import type { GroupProps, LayoutContext } from './Group';
import { Group } from './Group';
import type { FabricObject } from './Object/FabricObject';
import type { TOptions } from '../typedefs';
Expand Down Expand Up @@ -141,6 +141,25 @@ export class ActiveSelection extends Group {
return false;
}

_applyLayoutStrategy(context: LayoutContext): void {
super._applyLayoutStrategy(context);
if (this._objects.length === 0) {
// in this case layout was skipped
// we reset transform for the next selection
Object.assign(this, {
left: 0,
top: 0,
angle: 0,
scaleX: 1,
scaleY: 1,
skewX: 0,
skewY: 0,
flipX: false,
flipY: false,
});
}
}
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason why resetting and not doing new ActiveSelection([]) ?

In this way the reset becomes an ActiveSelection issue, while creating it is a canvas issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To keep the ref so we can listen to events etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also if somrone subclasses they can set the ref and it will remain as is

Copy link
Contributor

@jiayihu jiayihu Jul 28, 2023

Choose a reason for hiding this comment

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

I think that this is an evidence that keeping the same reference for ActiveSelection is a bad breaking change and design. It causes bugs and workarounds for things that were more natural behaviour, e.g. #9066.

To keep the ref so we can listen to events etc.

What's an use case for listening events on the ActiveSelection? Is there an alternative that doesn't require keeping the same reference?

Also if somrone subclasses they can set the ref and it will remain as is

Not clear to me, can you make an example of use case + code?

Copy link
Contributor Author

@ShaMan123 ShaMan123 Jul 29, 2023

Choose a reason for hiding this comment

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

  1. Bug Scope: I have ben thinking of this specific fix. Could it be that it belongs to Group? I need to test this. Might be this is a bug in enterGroup when group is rotated.
  2. Events: There are many requests from devs needing to listen to active selection events. Many speak of customizing the stack order in accordance to some business logic (which can now be accomplished using multiSelectAdd). Also toggling selectablility of objects, responding visually to stuff etc.
  3. Subclassing: In our project I will subclass active selection to override multiSelectAdd to block objects that should not be selected from being added to the selection (formerly _createActiveSelection, _updateActiveSelection)
export class ActiveSelection extends fabric.ActiveSelection {
  multiSelectAdd(...targets: fabric.Object[]): void {
    super.multiSelectAdd(...targets.filter((object) => !isReadOnly(object)));
  }
}
  • The only place that was creating a new ActiveSelection was _createActiveSelection.
  • I want group to be robost, it seems to me this bug is a group bug.
  • I also think canvas should accept the active selection ref in options or create a default one if non was passed. We can make the constructor of Activeselection protected - that will help devs see the change.
  • Devs can also add logic to setActiveObject if they must continue creating active selections:
setActiveObject(object) {
  if (object instaceof ActiveSelection) {
    this._activeSelection = object;
    object.set('canvas', this);
  }
  super.setActiveObject(object);
}

I do think it makes sense holding the ref but I am not fixed on it.

Copy link
Contributor Author

@ShaMan123 ShaMan123 Jul 30, 2023

Choose a reason for hiding this comment

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

Legit point of view
I am agnostic
It is a breaking change, and the worst kind because it is silent
However we want to publish with as less changes as possible at this point
@asturur I know you didn't like the const ref either. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

i forgot to update myself on this. i ll do and let's clear this out soon

Copy link
Member

Choose a reason for hiding this comment

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

First question, is there any reason why here we do this manual change while in _discardActiveObject we call the util resetObjectTransform?
We could extend resetObjectTransform to handle top and left since seems clear to me that in both cases we use resetObjectTransform we don't care for survival of top/left.

My general idea is that is late to remove this reference activeSelection even if i don't like it at all, it would be nice to reuse resetObjectTransform to save code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as fabric does not create instances of active selection I am ok with dropping the ref.
A bit late, but fine. Lets take advantage of beta.
Second, the resetting should occur in group in terms of expectation, not only active selection.
I do not remeber why I didn't use resetObjectTransform but I do remember considering it.

Copy link
Member

Choose a reason for hiding this comment

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

The only reason why i don't want to remove the ref is because i don't like go back and forth on choices.
We made this, let's move forward with this. If some issue will arise we will have to reconsider and we will do a breaking change.
i will try to reuse resetObject trasform in a separate PR, i can't see any reason for it to be a problem.


/**
* Returns string representation of a group
* @return {String}
Expand Down
Loading