Repository: fabricjs/fabric.js
Pull request #9329 (2023-09-12 13:40:52)

fix(): Avoid firing twice in subselection
In Group subselection, mouse events are fired twice because the target is both `target` and in `subTargets`. This is the quick fix, the proper one should probably be evaluating whether it's intended for the target to appear in both states.

cc @ShaMan123 


ShaMan123: Great catch, thanks! (saying it again)
In fabric terms this happens when group is  flagged with `interaction` and `subTargetCheck`.
It is part of the group rewrite I did not consider.
I will add a failing test so we have something to refer to.

Now question is how we should handle this.
IMO the target should be removed from `subTargets`.
I want to understand how` subTargets` is used.
In this case  `subTargets` wouldn't `subTargets contain only `target`? 
Running `searchPossibleTargets` on `target` might make sense. Then we align to current behavior.
If `subTargetCheck` is true `subTargets` will be the result or an empty array if not.


ShaMan123: This is an opportunity to define what should happen and refactor `_searchPossibleTargets` so we stop using `Canvas#targets` 
ShaMan123: this helped find another severe bug
asturur: For me this is on hold until the layout manager refactor, and after considering a dumb fix to avoid the double firing before considering other changes.
asturur: > this helped find another severe bug

Which one?
ShaMan123: I think first I will extract the migrated tests to a PR because it is impossible working with the old draggable text tests
asturur: I would like to understand why this one:
https://github.com/fabricjs/fabric.js/pull/9329/commits/d64f256968d15aa1eba9e06f03a24b1bd54d18a7
the perfect commit, wasn't good enough.
Was this solving the bug or not?
Can't we have this and a test for this?
ShaMan123: You can if you want to monkey patch and leave the real bug in the code base
asturur: There are no real bugs and fake bugs.
@jiayihu described a bug and proposed a fix.
The other bug which is it? and why cannot be fixed in a different PR?

ShaMan123: it can
ShaMan123: Why can't we talk about the problem?
`searchPossibleTargets` populates `targets` and that was never considered with the group rewrite.
So now it returns targets including possibly the `target` that will be returned by the method and that is not a 2 line fix.
It is all across the event logic and there are no tests for it

asturur: > Why can't we talk about the problem? 

It is exactly what i was asking, what the other bug is.
asturur: So what is the impact of the bug and why you consider it severe? One is a double event firing, the other is an incorrect target? another double firing? Really isn't clear.
ShaMan123: Whatever relies on `targets` (e.g. synthetic events) is incompatible with v6.
I asked what is the definition of `subTargets`, can you answer that?
ShaMan123: I reverted everything
jiayihu: Maybe we should go ahead with my simple and harmless fix, then take time to understand the implications of `subTargets` being populated with the target. That is the root logical inconsistency, that could provoke other bugs. At the same time it's better to avoid risky refactors until the situation is clear.

Once we all have a clear picture of what `subTargets` should and should not contain, then we can address properly. Spoken simply, we can wait to see if there are further bugs. The more bugs come in, the more we understand.
ShaMan123: I have found more bugs.
That is why I dived in.
I am against monkey patching, especially in large repos like this.
This is a symptom, not the problem.
We need to fix the problem.
You say justly that chasing bugs is not the right way forward.
Or else as a dev I can't trust fabric. It is a nightmare to dev like this. Not being sure you can trust a simple method or an event behaving as expected.

As I said, this is part of the group rewrite. (Redoing coords is as well).
Since changes must be incremental this is the nature of changing. 

I never understood this part until now because how it is written. Scatterred changes in so many place I could not wrap my head around it.

I am now convinced subTargets should be removed in favor of a different namre first of all, hits explains best what that thing is.

Also take into account that since the group rewrite there is a valid case where you will want to hit a sibling of what was found. Currently, subTargets will contain only the parents. That is why I made it overridable.
I other words I find subTargets useless as it is.
ShaMan123: ## Rethinking and Reconsidering `subTargets`

If I go by the name and from what I understand was the original intention I believe `subTargets` should populate the sub targets of the found target.
Parents of the found target should not be part of `subTargets`, that is what makes the name confusing. They should not be part mainly because it is clear that they are in the hit region or else the found target would not be found. If anyone including fabric needs the parents it is very simple to walk up the tree.

The main use in fabric of `subTargets` is for events. So we need to fix that.
I think it is unclear what is expected from the synthetic events.
Should enter/exit events fire on parents as well and on descendants? I think they should.
That is an easy fix.
ShaMan123: Take a look at #9343 
It is a good and small fix for the underlying issue.
jiayihu: > I have found more bugs.
> That is why I dived in.

I'll ask it for @asturur: what are the bugs?
ShaMan123: - where events are fired on subtargets this will happen (e.g. drag events)
- synthetic events are wrong, firing incosistentantly
- event data using `subTargets` is wrong - who knows how apps you it - the name itself it wrong the way it is currently. I would expect subTargets to be the subTargets of the event target and they are not, so why `subTargets`. Call them `targetsFoundWhileSearching`, `path, `hits`
- `_setCursorFromEvent` uses it

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index bd604fc67..5c5edb529 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## [next]
 
+- fix(Canvas): avoid firing event twice when working with nested objects [#9329](https://github.com/fabricjs/fabric.js/pull/9329)
 - fix(Control): `calcCornerCoords` angle + calculation [#9377](https://github.com/fabricjs/fabric.js/pull/9377)
 - patch(): dep findCrossPoints in favor of `isPointInPolygon` [#9374](https://github.com/fabricjs/fabric.js/pull/9374)
 - docs() enable typedocs to run again [#9356](https://github.com/fabricjs/fabric.js/pull/9356)
diff --git a/src/canvas/Canvas.ts b/src/canvas/Canvas.ts
index 19693880b..27df5efd1 100644
--- a/src/canvas/Canvas.ts
+++ b/src/canvas/Canvas.ts
@@ -903,7 +903,7 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
     this.fire(eventType, options);
     target && target.fire(eventType, options);
     for (let i = 0; i < subTargets.length; i++) {
-      subTargets[i].fire(eventType, options);
+      subTargets[i] !== target && subTargets[i].fire(eventType, options);
     }
     return options;
   }
@@ -943,7 +943,7 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
     // this may be a little be more complicated of what we want to handle
     target && target.fire(`mouse${eventType}`, options);
     for (let i = 0; i < targets.length; i++) {
-      targets[i].fire(`mouse${eventType}`, options);
+      targets[i] !== target && targets[i].fire(`mouse${eventType}`, options);
     }
   }
 


Test patch:
diff --git a/src/canvas/__tests__/eventData.test.ts b/src/canvas/__tests__/eventData.test.ts
index 4b1ba7a1e..58c5d04a4 100644
--- a/src/canvas/__tests__/eventData.test.ts
+++ b/src/canvas/__tests__/eventData.test.ts
@@ -1,7 +1,10 @@
 /* eslint-disable no-restricted-globals */
 import '../../../jest.extend';
+import type { TPointerEvent } from '../../EventTypeDefs';
 import { Point } from '../../Point';
+import { Group } from '../../shapes/Group';
 import { IText } from '../../shapes/IText/IText';
+import { FabricObject } from '../../shapes/Object/FabricObject';
 import type { TMat2D } from '../../typedefs';
 import { Canvas } from '../Canvas';
 
@@ -100,3 +103,22 @@ describe('Canvas event data', () => {
     }
   );
 });
+
+it('A selected subtarget should not fire an event twice', () => {
+  const target = new FabricObject();
+  const group = new Group([target], {
+    subTargetCheck: true,
+    interactive: true,
+  });
+  const canvas = new Canvas(null);
+  canvas.add(group);
+  const targetSpy = jest.fn();
+  target.on('mousedown', targetSpy);
+  jest.spyOn(canvas, '_checkTarget').mockReturnValue(true);
+  canvas.__onMouseDown({
+    target: canvas.getSelectionElement(),
+    clientX: 0,
+    clientY: 0,
+  } as unknown as TPointerEvent);
+  expect(targetSpy).toHaveBeenCalledTimes(1);
+});