BRAEKING BETA revert(): rm active selection ref (#9561)

**BRAEKING beta** --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
fabricjs · Jan 9, 2024 · 4979eec · 4979eec
1 parent 2dda471
commit 4979eec
Show file tree

Hide file tree

Showing 19 changed files with 157 additions and 170 deletions.
diff --git a/.codesandbox/templates/next/components/Canvas.tsx b/.codesandbox/templates/next/components/Canvas.tsx
@@ -28,6 +28,8 @@ export const Canvas = React.forwardRef<
       ref.current = canvas;
     }
 
+    // it is crucial `onLoad` is a dependency of this effect
+    // to ensure the canvas is disposed and re-created if it changes
     onLoad?.(canvas);
 
     return () => {

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## [next]
 
+- refactor(): rm active selection ref [#9561](https://github.com/fabricjs/fabric.js/pull/9561)
 - feat(Next.js sandbox): simpler canvas hook [#9577](https://github.com/fabricjs/fabric.js/pull/9577)
 - fix(): fix modify polygon points with zero sized polygons ( particular case of axis oriented lines ) [#9575](https://github.com/fabricjs/fabric.js/pull/9575)
 - fix(Polyline, Polygon): Fix wrong pathOffset for polyline with the normal bounding box calculation. [#9460](https://github.com/fabricjs/fabric.js/pull/9460)

diff --git a/src/ClassRegistry.ts b/src/ClassRegistry.ts
@@ -24,7 +24,7 @@ export class ClassRegistry {
     this[SVG] = new Map();
   }
 
-  getClass(classType: string): any {
+  getClass<T>(classType: string): T {
     const constructor = this[JSON].get(classType);
     if (!constructor) {
       throw new FabricError(`No class registered for ${classType}`);

diff --git a/src/canvas/Canvas.ts b/src/canvas/Canvas.ts
@@ -1,3 +1,4 @@
+import { classRegistry } from '../ClassRegistry';
 import { NONE } from '../constants';
 import type {
   CanvasEvents,
@@ -8,12 +9,14 @@ import type {
   Transform,
 } from '../EventTypeDefs';
 import { Point } from '../Point';
+import type { ActiveSelection } from '../shapes/ActiveSelection';
 import type { Group } from '../shapes/Group';
 import type { IText } from '../shapes/IText/IText';
 import type { FabricObject } from '../shapes/Object/FabricObject';
 import { isTouchEvent, stopEvent } from '../util/dom_event';
 import { getDocumentFromElement, getWindowFromElement } from '../util/dom_misc';
 import { sendPointToPlane } from '../util/misc/planeChange';
+import { isActiveSelection } from '../util/typeAssertions';
 import type { CanvasOptions, TCanvasOptions } from './CanvasOptions';
 import { SelectableCanvas } from './SelectableCanvas';
 import { TextEditingManager } from './TextEditingManager';
@@ -1387,10 +1390,9 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
       return;
     }
     let hoverCursor = target.hoverCursor || this.hoverCursor;
-    const activeSelection =
-        this._activeObject === this._activeSelection
-          ? this._activeObject
-          : null,
+    const activeSelection = isActiveSelection(this._activeObject)
+        ? this._activeObject
+        : null,
       // only show proper corner when group selection is not active
       corner =
         (!activeSelection || target.group !== activeSelection) &&
@@ -1431,8 +1433,7 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
    */
   protected handleMultiSelection(e: TPointerEvent, target?: FabricObject) {
     const activeObject = this._activeObject;
-    const activeSelection = this._activeSelection;
-    const isAS = activeObject === activeSelection;
+    const isAS = isActiveSelection(activeObject);
     if (
       // check if an active object exists on canvas and if the user is pressing the `selectionKey` while canvas supports multi selection.
       !!activeObject &&
@@ -1455,9 +1456,8 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
       !activeObject.getActiveControl()
     ) {
       if (isAS) {
-        const prevActiveObjects =
-          activeSelection.getObjects() as FabricObject[];
-        if (target === activeSelection) {
+        const prevActiveObjects = activeObject.getObjects();
+        if (target === activeObject) {
           const pointer = this.getViewportPoint(e);
           target =
             // first search active objects for a target to remove
@@ -1470,32 +1470,43 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
             return false;
           }
         }
-        if (target.group === activeSelection) {
+        if (target.group === activeObject) {
           // `target` is part of active selection => remove it
-          activeSelection.remove(target);
+          activeObject.remove(target);
           this._hoveredTarget = target;
           this._hoveredTargets = [...this.targets];
-          if (activeSelection.size() === 1) {
+          // if after removing an object we are left with one only...
+          if (activeObject.size() === 1) {
             // activate last remaining object
-            this._setActiveObject(activeSelection.item(0) as FabricObject, e);
+            // deselecting the active selection will remove the remaining object from it
+            this._setActiveObject(activeObject.item(0), e);
           }
         } else {
-          //  `target` isn't part of active selection => add it
-          activeSelection.multiSelectAdd(target);
-          this._hoveredTarget = activeSelection;
+          // `target` isn't part of active selection => add it
+          activeObject.multiSelectAdd(target);
+          this._hoveredTarget = activeObject;
           this._hoveredTargets = [...this.targets];
         }
         this._fireSelectionEvents(prevActiveObjects, e);
       } else {
         (activeObject as IText).exitEditing &&
           (activeObject as IText).exitEditing();
         // add the active object and the target to the active selection and set it as the active object
-        activeSelection.multiSelectAdd(activeObject, target);
-        this._hoveredTarget = activeSelection;
+        const klass =
+          classRegistry.getClass<typeof ActiveSelection>('ActiveSelection');
+        const newActiveSelection = new klass([], {
+          /**
+           * it is crucial to pass the canvas ref before calling {@link ActiveSelection#multiSelectAdd}
+           * since it uses {@link FabricObject#isInFrontOf} which relies on the canvas ref
+           */
+          canvas: this,
+        });
+        newActiveSelection.multiSelectAdd(activeObject, target);
+        this._hoveredTarget = newActiveSelection;
         // ISSUE 4115: should we consider subTargets here?
         // this._hoveredTargets = [];
         // this._hoveredTargets = this.targets.concat();
-        this._setActiveObject(activeSelection, e);
+        this._setActiveObject(newActiveSelection, e);
         this._fireSelectionEvents([activeObject], e);
       }
       return true;
@@ -1549,8 +1560,9 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
       this.setActiveObject(objects[0], e);
     } else if (objects.length > 1) {
       // add to active selection and make it the active object
-      this._activeSelection.add(...objects);
-      this.setActiveObject(this._activeSelection, e);
+      const klass =
+        classRegistry.getClass<typeof ActiveSelection>('ActiveSelection');
+      this.setActiveObject(new klass(objects, { canvas: this }), e);
     }
 
     // cleanup

diff --git a/src/canvas/CanvasOptions.ts b/src/canvas/CanvasOptions.ts
@@ -1,5 +1,4 @@
 import type { ModifierKey, TOptionalModifierKey } from '../EventTypeDefs';
-import type { ActiveSelection } from '../shapes/ActiveSelection';
 import type { TOptions } from '../typedefs';
 import type { StaticCanvasOptions } from './StaticCanvasOptions';
 
@@ -260,9 +259,7 @@ export interface CanvasOptions
   preserveObjectStacking: boolean;
 }
 
-export type TCanvasOptions = TOptions<
-  CanvasOptions & { activeSelection: ActiveSelection }
->;
+export type TCanvasOptions = TOptions<CanvasOptions>;
 
 export const canvasDefaults: TOptions<CanvasOptions> = {
   uniformScaling: true,

diff --git a/src/canvas/SelectableCanvas.ts b/src/canvas/SelectableCanvas.ts
@@ -11,7 +11,6 @@ import type {
 } from '../EventTypeDefs';
 import {
   addTransformToObject,
-  resetObjectTransform,
   saveObjectTransform,
 } from '../util/misc/objectTransforms';
 import type { TCanvasSizeOptions } from './StaticCanvas';
@@ -31,13 +30,13 @@ import type { IText } from '../shapes/IText/IText';
 import type { BaseBrush } from '../brushes/BaseBrush';
 import { pick } from '../util/misc/pick';
 import { sendPointToPlane } from '../util/misc/planeChange';
-import { ActiveSelection } from '../shapes/ActiveSelection';
 import { cos, createCanvasElement, sin } from '../util';
 import { CanvasDOMManager } from './DOMManagers/CanvasDOMManager';
 import { BOTTOM, CENTER, LEFT, RIGHT, TOP } from '../constants';
-import type { CanvasOptions, TCanvasOptions } from './CanvasOptions';
+import type { CanvasOptions } from './CanvasOptions';
 import { canvasDefaults } from './CanvasOptions';
 import { Intersection } from '../Intersection';
+import { isActiveSelection } from '../util/typeAssertions';
 
 /**
  * Canvas class
@@ -294,17 +293,6 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
   protected declare _isCurrentlyDrawing: boolean;
   declare freeDrawingBrush?: BaseBrush;
   declare _activeObject?: FabricObject;
-  protected _activeSelection: ActiveSelection;
-
-  constructor(
-    el?: string | HTMLCanvasElement,
-    { activeSelection = new ActiveSelection(), ...options }: TCanvasOptions = {}
-  ) {
-    super(el, options);
-    this._activeSelection = activeSelection;
-    this._activeSelection.set('canvas', this);
-    this._activeSelection.setCoords();
-  }
 
   protected initElements(el?: string | HTMLCanvasElement) {
     this.elements = new CanvasDOMManager(el, {
@@ -1029,27 +1017,17 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
     return this._activeObject;
   }
 
-  /**
-   * Returns instance's active selection
-   */
-  getActiveSelection() {
-    return this._activeSelection;
-  }
-
   /**
    * Returns an array with the current selected objects
    * @return {FabricObject[]} active objects array
    */
   getActiveObjects(): FabricObject[] {
     const active = this._activeObject;
-    if (active) {
-      if (active === this._activeSelection) {
-        return [...(active as ActiveSelection)._objects];
-      } else {
-        return [active];
-      }
-    }
-    return [];
+    return isActiveSelection(active)
+      ? active.getObjects()
+      : active
+      ? [active]
+      : [];
   }
 
   /**
@@ -1134,20 +1112,22 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
    * @return {Boolean} true if the object has been selected
    */
   _setActiveObject(object: FabricObject, e?: TPointerEvent) {
-    if (this._activeObject === object) {
+    const prevActiveObject = this._activeObject;
+    if (prevActiveObject === object) {
       return false;
     }
+    // after calling this._discardActiveObject, this,_activeObject could be undefined
     if (!this._discardActiveObject(e, object) && this._activeObject) {
       // refused to deselect
       return false;
     }
     if (object.onSelect({ e })) {
       return false;
     }
+
     this._activeObject = object;
 
-    if (object instanceof ActiveSelection && this._activeSelection !== object) {
-      this._activeSelection = object;
+    if (isActiveSelection(object) && prevActiveObject !== object) {
       object.set('canvas', this);
       object.setCoords();
     }
@@ -1173,11 +1153,6 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
       if (obj.onDeselect({ e, object })) {
         return false;
       }
-      // clear active selection
-      if (obj === this._activeSelection) {
-        this._activeSelection.removeAll();
-        resetObjectTransform(this._activeSelection);
-      }
       if (this._currentTransform && this._currentTransform.target === obj) {
         // @ts-expect-error this method exists in the subclass - should be moved or declared as abstract
         this.endCurrentTransform(e);
@@ -1227,11 +1202,13 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
    */
   destroy() {
     // dispose of active selection
-    const activeSelection = this._activeSelection;
-    activeSelection.removeAll();
-    // @ts-expect-error disposing
-    this._activeSelection = undefined;
-    activeSelection.dispose();
+    const activeObject = this._activeObject;
+    if (isActiveSelection(activeObject)) {
+      activeObject.removeAll();
+      activeObject.dispose();
+    }
+
+    delete this._activeObject;
 
     super.destroy();
 
@@ -1271,7 +1248,7 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
   /**
    * @private
    */
-  _toObject(
+  protected _toObject(
     instance: FabricObject,
     methodName: 'toObject' | 'toDatalessObject',
     propertiesToInclude: string[]
@@ -1293,14 +1270,11 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
    * @param {FabricObject} [instance] the object to transform (gets mutated)
    * @returns the original values of instance which were changed
    */
-  _realizeGroupTransformOnObject(
+  private _realizeGroupTransformOnObject(
     instance: FabricObject
   ): Partial<typeof instance> {
-    if (
-      instance.group &&
-      instance.group === this._activeSelection &&
-      this._activeObject === instance.group
-    ) {
+    const { group } = instance;
+    if (group && isActiveSelection(group) && this._activeObject === group) {
       const layoutProps = [
         'angle',
         'flipX',
@@ -1313,7 +1287,7 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
         TOP,
       ] as (keyof typeof instance)[];
       const originalValues = pick<typeof instance>(instance, layoutProps);
-      addTransformToObject(instance, this._activeObject.calcOwnMatrix());
+      addTransformToObject(instance, group.calcOwnMatrix());
       return originalValues;
     } else {
       return {};

diff --git a/src/canvas/StaticCanvas.ts b/src/canvas/StaticCanvas.ts
@@ -859,7 +859,7 @@ export class StaticCanvas<
   /**
    * @private
    */
-  _toObject(
+  protected _toObject(
     instance: FabricObject,
     methodName: TValidToObjectMethod,
     propertiesToInclude?: string[]

diff --git a/src/filters/Composed.ts b/src/filters/Composed.ts
@@ -67,7 +67,9 @@ export class Composed extends BaseFilter {
   ) {
     return Promise.all(
       ((object.subFilters || []) as BaseFilter[]).map((filter) =>
-        classRegistry.getClass(filter.type).fromObject(filter, options)
+        classRegistry
+          .getClass<typeof BaseFilter>(filter.type)
+          .fromObject(filter, options)
       )
     ).then(
       (enlivedFilters) => new this({ subFilters: enlivedFilters }) as BaseFilter