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

feat(perPixelTargetFind): support not-selected option #9167

Closed
wants to merge 11 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]

- feat(`perPixelTargetFind`): support `not-selected` option [#9167](https://github.com/fabricjs/fabric.js/pull/9167)
- CD(): website submodule [#9165](https://github.com/fabricjs/fabric.js/pull/9165)

## [6.0.0-beta12]
Expand Down
4 changes: 2 additions & 2 deletions src/canvas/CanvasOptions.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { ModifierKey, TOptionalModifierKey } from '../EventTypeDefs';
import type { ActiveSelection } from '../shapes/ActiveSelection';
import type { TOptions } from '../typedefs';
import type { PerPixelTargetFind, TOptions } from '../typedefs';
import type { StaticCanvasOptions } from './StaticCanvasOptions';

export interface CanvasTransformOptions {
Expand Down Expand Up @@ -182,7 +182,7 @@ export interface TargetFindOptions {
* @type Boolean
* @default
*/
perPixelTargetFind: boolean;
perPixelTargetFind: PerPixelTargetFind;

/**
* Number of pixels around target pixel to tolerate (consider active) during object detection
Expand Down
34 changes: 15 additions & 19 deletions src/canvas/SelectableCanvas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { isCollection } from '../util/typeAssertions';
import { invertTransform, transformPoint } from '../util/misc/matrix';
import { isTransparent } from '../util/misc/isTransparent';
import type {
PerPixelTargetFind,
TMat2D,
TOriginX,
TOriginY,
Expand Down Expand Up @@ -167,7 +168,7 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
declare containerClass: string;

// target find config
declare perPixelTargetFind: boolean;
declare perPixelTargetFind: PerPixelTargetFind;
declare targetFindTolerance: number;
declare skipTargetFind: boolean;

Expand Down Expand Up @@ -772,25 +773,20 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
obj: FabricObject,
globalPointer: Point
): boolean {
if (
obj &&
obj.visible &&
obj.evented &&
// http://www.geog.ubc.ca/courses/klink/gis.notes/ncgia/u32.html
// http://idav.ucdavis.edu/~okreylos/TAship/Spring2000/PointInPolygon.html
Comment on lines -779 to -780
Copy link
Member

Choose a reason for hiding this comment

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

we should preserve this comment maybe over the containsPoint function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both are dead, and also the impl has moved forward more than a decade

obj.containsPoint(pointer)
) {
if (
(this.perPixelTargetFind || obj.perPixelTargetFind) &&
!(obj as unknown as IText).isEditing
) {
if (!this.isTargetTransparent(obj, globalPointer.x, globalPointer.y)) {
return true;
}
} else {
return true;
}
if (obj && obj.visible && obj.evented && obj.containsPoint(pointer)) {
const shouldPerformPixelFind =
!(obj as unknown as IText).isEditing &&
(this.perPixelTargetFind === true ||
obj.perPixelTargetFind === true ||
((this.perPixelTargetFind === 'not-selected' ||
obj.perPixelTargetFind === 'not-selected') &&
Comment on lines +779 to +782
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 am not sure about combinations between canvas.perPixelTargetFind and obj.perPixelTargetFind
Shouldn't the object take presedence?
I will update tests once we decide

Copy link
Member

Choose a reason for hiding this comment

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

We should collect the current rules around the code.
But there shouldn't be such precedence is just confusing.
There must a be a documented way to handle that for all objects or just some.
If you can set it on a single object or an entire subclass through default values is good enough imho

!this.getActiveObjects().includes(obj)));

return shouldPerformPixelFind
? !this.isTargetTransparent(obj, globalPointer.x, globalPointer.y)
: true;
}

return false;
}

Expand Down
57 changes: 57 additions & 0 deletions src/canvas/__tests__/perPixelTargetFind.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { Point } from '../../Point';
import type { IText } from '../../shapes/IText/IText';
import { FabricObject } from '../../shapes/Object/FabricObject';
import { Canvas } from '../Canvas';

describe('perPixelTargetFind', () => {
let canvas: Canvas;
let object: FabricObject;
let isTargetTransparent: jest.SpyInstance;

beforeEach(() => {
canvas = new Canvas(null);
isTargetTransparent = jest
.spyOn(canvas, 'isTargetTransparent')
.mockReturnValue(true);
object = new FabricObject();
jest.spyOn(object, 'containsPoint').mockReturnValue(true);
});

afterEach(() => {
return canvas.dispose();
});

const checkTarget = () =>
canvas._checkTarget(new Point(), object, new Point());

test('perPixelTargetFind === false', () => {
expect(checkTarget()).toBe(true);
expect(isTargetTransparent).not.toBeCalled();
});

test('perPixelTargetFind === true', () => {
object.perPixelTargetFind = true;
expect(checkTarget()).toBe(false);
expect(isTargetTransparent).toBeCalled();
});

test('perPixelTargetFind === true, object is editing', () => {
object.perPixelTargetFind = true;
(object as IText).isEditing = true;
expect(checkTarget()).toBe(true);
expect(isTargetTransparent).not.toBeCalled();
});

test('perPixelTargetFind === "not-selected", object is not selected', () => {
object.perPixelTargetFind = 'not-selected';
expect(checkTarget()).toBe(false);
expect(isTargetTransparent).toBeCalled();
});

test('perPixelTargetFind === "not-selected", object is selected', () => {
object.perPixelTargetFind = 'not-selected';
canvas.setActiveObject(object);
expect(checkTarget()).toBe(true);
expect(isTargetTransparent).not.toBeCalled();
});
});
4 changes: 2 additions & 2 deletions src/shapes/Object/InteractiveObject.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Point } from '../../Point';
import type { TCornerPoint, TDegree } from '../../typedefs';
import type { PerPixelTargetFind, TCornerPoint, TDegree } from '../../typedefs';
import { FabricObject } from './Object';
import { degreesToRadians } from '../../util/misc/radiansDegreesConversion';
import type { TQrDecomposeOut } from '../../util/misc/matrix';
Expand Down Expand Up @@ -87,7 +87,7 @@ export class InteractiveFabricObject<

declare selectable: boolean;
declare evented: boolean;
declare perPixelTargetFind: boolean;
declare perPixelTargetFind: PerPixelTargetFind;
declare activeOn: 'down' | 'up';

declare hoverCursor: CSSStyleDeclaration['cursor'] | null;
Expand Down
4 changes: 2 additions & 2 deletions src/shapes/Object/types/FabricObjectProps.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { TDegree } from '../../../typedefs';
import type { PerPixelTargetFind, TDegree } from '../../../typedefs';
import type { BorderProps } from './BorderProps';
import type { ControlProps } from './ControlProps';
import type { LockInteractionProps } from './LockInteractionProps';
Expand Down Expand Up @@ -81,7 +81,7 @@ export interface FabricObjectProps
* @type Boolean
* @default
*/
perPixelTargetFind: boolean;
perPixelTargetFind: PerPixelTargetFind;

/**
* When set to `false`, an object can not be selected for modification (using either point-click-based or group-based selection).
Expand Down
2 changes: 2 additions & 0 deletions src/typedefs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,5 @@ export type Abortable = {
};

export type TOptions<T> = Partial<T> & Record<string, any>;

export type PerPixelTargetFind = boolean | 'not-selected';
Loading