fix(): Avoid firing twice in subselection #9329

jiayihu · 2023-09-12T13:40:52Z

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 · 2023-09-13T01:50:28Z

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 should subTargets be the result or an empty array if not?

Currently, I think it will contain itself and its children in case it is flagged with subTargetCheck.

ShaMan123 · 2023-09-13T02:30:07Z

This is an opportunity to define what should happen and refactor _searchPossibleTargets so we stop using Canvas#targets

ShaMan123 · 2023-09-13T02:36:39Z

this helped find another severe bug

ShaMan123

_searchPossibleTargets => fintTargetsTraversal, now returns an array of hits
fixed searchPossibleTargets bug cause d by confusion of subTargetCheck and interactive
added tests

ShaMan123 · 2023-09-13T03:51:45Z

src/canvas/SelectableCanvas.ts

-    return target &&
-      isCollection(target) &&
-      target.interactive &&
-      this.targets[0]
-      ? this.targets[0]
-      : target;


this was wrong, added a test that fails on master

Can you define in which way this is wrong?

@asturur this.
If there is a target under a subTargetChec group under and interactive group the middle group should be selected but actually the target will be wrongly selected
see the test

src/canvas/SelectableCanvas.ts

ShaMan123

OK
This is stabilizing.
I now think that subTargets should be renamed into path or hits or something since target can indeed be part of it, and the name doesn't reflect the fact that target's parent are part of the array as well.
The fix is done.

ShaMan123 · 2023-09-13T10:04:18Z

src/canvas/__tests__/eventData.test.ts

dreadful work
but at least now when these tests fail it will have ameaning for a human being and it will take a second to understand what failed instead of trying to find in the massive dump something worthwhile

ShaMan123 · 2023-09-13T10:04:49Z

test/unit/canvas.js

moved all findTarget tests to jest

ShaMan123 · 2023-09-13T10:04:58Z

test/unit/canvas_events.js

moved to jest

ShaMan123

refactored _hovreredTarget, _hovreredTargets => hoveringState
squashed _fireEnterLeaveEvents, _fireOverOutEvents => handleSyntheticInOutEvents

ShaMan123 · 2023-09-13T11:52:06Z

src/canvas/Canvas.ts

-   * @type FabricObject
-   * @private
-   */
-  private declare _draggedoverTarget?: FabricObject;


very weird artifact

I found where it was set, thought it was only read. but still redundant

ShaMan123 · 2023-09-13T11:52:59Z

src/canvas/Canvas.ts

-      // TODO: verify if those should loop in inverse order then?
-      // what is the order of subtargets?


targets from the search are in correct order

ShaMan123 · 2023-09-13T13:06:06Z

src/canvas/Canvas.ts

-        // ISSUE 4115: should we consider subTargets here?
-        // this._hoveredTargets = [];
-        // this._hoveredTargets = this.targets.concat();


what is this?

asturur · 2023-09-13T21:56:10Z

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 · 2023-09-13T21:56:53Z

this helped find another severe bug

Which one?

ShaMan123 · 2023-09-13T22:01:01Z

I think first I will extract the migrated tests to a PR because it is impossible working with the old draggable text tests

asturur · 2023-09-13T22:07:06Z

I would like to understand why this one:
d64f256
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 · 2023-09-13T22:21:50Z

You can if you want to monkey patch and leave the real bug in the code base

asturur · 2023-09-13T22:28:29Z

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 · 2023-09-13T22:29:42Z

it can

ShaMan123 · 2023-09-13T22:33:06Z

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 · 2023-09-13T22:48:40Z

Why can't we talk about the problem?

It is exactly what i was asking, what the other bug is.

asturur · 2023-09-13T22:50:16Z

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 · 2023-09-13T23:03:27Z

e2e/tests/text/drag&drop/index.spec.ts-snapshots/disabling-drop-A-events.json

look here at the events recorded by the test
dragenter is called too much and dragleave too little - might be it is caused by this PR.

I couldn't know what was wrong because of the draggable_text test that just fails without outputting something decent

ShaMan123 · 2023-09-13T23:25:34Z

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 · 2023-09-13T23:25:43Z

I reverted everything

Update eventData.test.ts

ShaMan123

restored

jiayihu · 2023-09-14T09:17:20Z

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 · 2023-09-15T00:55:23Z

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 ancestors and if subTargetCheck is true the descendants of the found target. That is why I made it overridable in the changes I reverted.

In other words I find subTargets useless as it is.

ShaMan123 · 2023-09-15T01:57:07Z

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 · 2023-09-15T06:13:30Z

Take a look at #9343
It is a good and small fix for the underlying issue.

jiayihu · 2023-09-15T13:28:53Z

I have found more bugs.
That is why I dived in.

I'll ask it for @asturur: what are the bugs?

ShaMan123 · 2023-09-15T15:19:11Z

where events are fired on subtargets this will happen (e.g. drag events)
synthetic events are wrong, firing incosistentantly because it relays on targets
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

Avoid firing twice

d64f256

ShaMan123 added group selection labels Sep 12, 2023

ShaMan123 previously approved these changes Sep 13, 2023

View reviewed changes

ShaMan123 changed the title ~~Avoid firing twice in subselection~~ fix(): Avoid firing twice in subselection + return correct subTarget Sep 13, 2023

ShaMan123 dismissed their stale review via 779e80c September 13, 2023 03:58

jiayihu commented Sep 13, 2023

View reviewed changes

src/canvas/SelectableCanvas.ts Outdated Show resolved Hide resolved

ShaMan123 force-pushed the fired-twice branch 4 times, most recently from 6867240 to 6368234 Compare September 13, 2023 10:00

ShaMan123 reviewed Sep 13, 2023

View reviewed changes

ShaMan123 force-pushed the fired-twice branch 3 times, most recently from c5d4275 to b1e5144 Compare September 13, 2023 13:08

ShaMan123 reviewed Sep 13, 2023

View reviewed changes

ShaMan123 requested a review from asturur September 13, 2023 14:45

ShaMan123 reviewed Sep 13, 2023

View reviewed changes

ShaMan123 force-pushed the fired-twice branch 2 times, most recently from fc3ba50 to 8fae21f Compare September 13, 2023 23:23

add test

2a95437

Update eventData.test.ts

ShaMan123 force-pushed the fired-twice branch from 8fae21f to 2a95437 Compare September 13, 2023 23:26

ShaMan123 reviewed Sep 13, 2023

View reviewed changes

ShaMan123 mentioned this pull request Sep 13, 2023

test(): migrate tests to playwright/jest #9333

Merged

ShaMan123 changed the title ~~fix(): Avoid firing twice in subselection + return correct subTarget~~ fix(): Avoid firing twice in subselection Sep 15, 2023

ShaMan123 mentioned this pull request Sep 15, 2023

fix(): searchPossibleTargets targets value #9343

Closed

asturur added 2 commits September 26, 2023 10:54

Merge branch 'master' into fired-twice

25f6e3c

added changelog

aac8e2f

asturur approved these changes Sep 26, 2023

View reviewed changes

asturur merged commit 678b79c into fabricjs:master Sep 26, 2023
21 of 23 checks passed

ShaMan123 mentioned this pull request May 7, 2024

WIP fix(): searchPossibleTargets targets value ShaMan123/fabric.js#5

Open

		// TODO: verify if those should loop in inverse order then?
		// what is the order of subtargets?

fix(): Avoid firing twice in subselection #9329

fix(): Avoid firing twice in subselection #9329

Conversation

jiayihu commented Sep 12, 2023 • edited by ShaMan123 Loading

ShaMan123 commented Sep 13, 2023 • edited Loading

ShaMan123 commented Sep 13, 2023

ShaMan123 commented Sep 13, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Sep 13, 2023

asturur commented Sep 13, 2023

ShaMan123 commented Sep 13, 2023

asturur commented Sep 13, 2023

ShaMan123 commented Sep 13, 2023

asturur commented Sep 13, 2023

ShaMan123 commented Sep 13, 2023

ShaMan123 commented Sep 13, 2023

asturur commented Sep 13, 2023

asturur commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 commented Sep 13, 2023

ShaMan123 commented Sep 13, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

jiayihu commented Sep 14, 2023

ShaMan123 commented Sep 15, 2023 • edited Loading

ShaMan123 commented Sep 15, 2023

Rethinking and Reconsidering subTargets

ShaMan123 commented Sep 15, 2023

jiayihu commented Sep 15, 2023

ShaMan123 commented Sep 15, 2023 • edited Loading

jiayihu commented Sep 12, 2023 •

edited by ShaMan123

Loading

ShaMan123 commented Sep 13, 2023 •

edited

Loading

ShaMan123 commented Sep 15, 2023 •

edited

Loading

Rethinking and Reconsidering `subTargets`

ShaMan123 commented Sep 15, 2023 •

edited

Loading