fix(): set/discard active object return value

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## [next]
 
+- fix(): BREAKING set/discard active object return value, discard active object now return false if no discard happened [#8672](https://github.com/fabricjs/fabric.js/issues/8672)
 - fix(): object dispose removes canvas/event refs [#8673](https://github.com/fabricjs/fabric.js/issues/8673)
 - fix(test): Textbox `fromObject` test is incorrectly trying to restore an instance [#8686](https://github.com/fabricjs/fabric.js/pull/8686)
 - TS(): Moved cache properties to static properties on classes [#xxxx](https://github.com/fabricjs/fabric.js/pull/xxxx)

src/canvas/SelectableCanvas.ts

@@ -22,7 +22,7 @@ import {
 } from '../util/types';
 import { invertTransform, transformPoint } from '../util/misc/matrix';
 import { isTransparent } from '../util/misc/isTransparent';
-import { TMat2D, TOriginX, TOriginY, TSize } from '../typedefs';
+import { AssertKeys, TMat2D, TOriginX, TOriginY, TSize } from '../typedefs';
 import { degreesToRadians } from '../util/misc/radiansDegreesConversion';
 import { getPointer, isTouchEvent } from '../util/dom_event';
 import type { IText } from '../shapes/IText/IText';
@@ -1383,30 +1383,36 @@ export class SelectableCanvas<
    * Sets given object as the only active object on canvas
    * @param {FabricObject} object Object to set as an active one
    * @param {TPointerEvent} [e] Event (passed along when firing "object:selected")
-   * @chainable
+   * @return {Boolean} true if the object has been selected
    */
-  setActiveObject(object: FabricObject, e?: TPointerEvent) {
+  setActiveObject(
+    object: FabricObject,
+    e?: TPointerEvent
+  ): this is AssertKeys<this, '_activeObject'> {
     // we can't inline this, since _setActiveObject will change what getActiveObjects returns
     const currentActives = this.getActiveObjects();
-    this._setActiveObject(object, e);
+    const selected = this._setActiveObject(object, e);
     this._fireSelectionEvents(currentActives, e);
+    return selected;
   }
 
   /**
-   * This is a private method for now.
    * This is supposed to be equivalent to setActiveObject but without firing
    * any event. There is commitment to have this stay this way.
    * This is the functional part of setActiveObject.
-   * @private
    * @param {Object} object to set as active
    * @param {Event} [e] Event (passed along when firing "object:selected")
-   * @return {Boolean} true if the selection happened
+   * @return {Boolean} true if the object has been selected
    */
-  _setActiveObject(object: FabricObject, e?: TPointerEvent) {
+  _setActiveObject(
+    object: FabricObject,
+    e?: TPointerEvent
+  ): this is AssertKeys<this, '_activeObject'> {
     if (this._activeObject === object) {
       return false;
     }
-    if (!this._discardActiveObject(e, object)) {
+    if (!this._discardActiveObject(e, object) && this._activeObject) {
+      // refused to deselect
       return false;
     }
     if (object.onSelect({ e })) {
@@ -1417,16 +1423,17 @@ export class SelectableCanvas<
   }
 
   /**
-   * This is a private method for now.
    * This is supposed to be equivalent to discardActiveObject but without firing
    * any selection events ( can still fire object transformation events ). There is commitment to have this stay this way.
    * This is the functional part of discardActiveObject.
    * @param {Event} [e] Event (passed along when firing "object:deselected")
    * @param {Object} object the next object to set as active, reason why we are discarding this
-   * @return {Boolean} true if the selection happened
-   * @private
+   * @return {Boolean} true if the active object has been discarded
    */
-  _discardActiveObject(e?: TPointerEvent, object?: FabricObject) {
+  _discardActiveObject(
+    e?: TPointerEvent,
+    object?: FabricObject
+  ): this is { _activeObject: undefined } {
     const obj = this._activeObject;
     if (obj) {
       // onDeselect return TRUE to cancel selection;
@@ -1438,8 +1445,9 @@ export class SelectableCanvas<
         this.endCurrentTransform(e);
       }
       this._activeObject = undefined;
+      return true;
     }
-    return true;
+    return false;
   }
 
   /**
@@ -1448,9 +1456,9 @@ export class SelectableCanvas<
    * sent to the fire function for the custom events. When used as a method the
    * e param does not have any application.
    * @param {event} e
-   * @chainable
+   * @return {Boolean} true if the active object has been discarded
    */
-  discardActiveObject(e?: TPointerEvent) {
+  discardActiveObject(e?: TPointerEvent): this is { _activeObject: undefined } {
     const currentActives = this.getActiveObjects(),
       activeObject = this.getActiveObject();
     if (currentActives.length) {
@@ -1459,8 +1467,9 @@ export class SelectableCanvas<
         deselected: [activeObject!],
       });
     }
-    this._discardActiveObject(e);
+    const discarded = this._discardActiveObject(e);
     this._fireSelectionEvents(currentActives, e);
+    return discarded;
   }
 
   /**
@@ -1514,8 +1523,10 @@ export class SelectableCanvas<
    * Clears all contexts (background, main, top) of an instance
    */
   clear() {
-    // this.discardActiveGroup();
+    // discard active object and fire events
     this.discardActiveObject();
+    // make sure we clear the active object in case it refused to be discarded
+    this._activeObject = undefined;
     this.clearContext(this.contextTop);
     super.clear();
   }

test/unit/canvas.js

@@ -1790,10 +1790,11 @@
 
     canvas.add(rect1, rect2);
 
-    canvas.setActiveObject(rect1);
+    assert.ok(canvas.setActiveObject(rect1), 'selected');
     assert.ok(rect1 === canvas._activeObject);
 
-    canvas.setActiveObject(rect2);
+    assert.ok(canvas.setActiveObject(rect2), 'selected');
+    assert.ok(!canvas.setActiveObject(rect2), 'no effect');
     assert.ok(rect2 === canvas._activeObject);
   });
 
@@ -1852,7 +1853,8 @@
     canvas.add(makeRect());
     canvas.setActiveObject(canvas.item(0));
 
-    canvas._discardActiveObject();
+    assert.ok(canvas._discardActiveObject(), 'discarded');
+    assert.ok(!canvas._discardActiveObject(), 'no effect');
     assert.equal(canvas.getActiveObject(), null);
   });
 
@@ -1891,15 +1893,26 @@
       eventsFired.selectionCleared = true;
     });
 
-    canvas.discardActiveObject();
-    assert.equal(canvas.getActiveObject(), null);
+    assert.ok(canvas.discardActiveObject(), 'deselected');
+    assert.ok(!canvas.getActiveObject(), 'no active object');
+    assert.ok(!canvas.discardActiveObject(), 'no effect');
     assert.equal(canvas.getActiveObject(), null);
 
     for (var prop in eventsFired) {
       assert.ok(eventsFired[prop]);
     }
   });
 
+  QUnit.test('refuse discarding active object', function (assert) {
+    const rect = makeRect();
+    rect.onDeselect = () => true;
+    canvas.setActiveObject(rect);
+    assert.ok(!canvas.discardActiveObject(), 'no effect');
+    assert.ok(canvas.getActiveObject() === rect, 'active object');
+    canvas.clear();
+    assert.ok(!canvas.getActiveObject(), 'cleared the stubborn ref');
+  })
+
   QUnit.test('complexity', function(assert) {
     assert.ok(typeof canvas.complexity === 'function');
     assert.equal(canvas.complexity(), 0);
@@ -1948,7 +1961,7 @@
     assert.equal(aGroup._objects[1], circle2);
     assert.equal(aGroup._objects[2], rect1);
     assert.equal(aGroup._objects[3], circle1);
-    canvas.setActiveObject(aGroup)
+    assert.ok(canvas.setActiveObject(aGroup));
     canvas.renderAll();
     // after rendering objects are ordered in canvas stack order
     assert.equal(aGroup._objects[0], rect1);