refactor(): _findTargetCorner now returns the key and the control

[jiayihu]

Just like getActiveControl, this PR refactors _findTargetCorner to return the Control as well. This allows to override the returned control, for instance allowing a Group to return children controls to allow "pass-through" interactions.

Honestly, I believe Control should have a key/name property. It's very common to need that information and having getActiveControl and _findTargetCorner return { key: string, control: Control } instead of just Control is a clear sign of that.
For the Control itself it's very convenient to know the key as this.key, for instance you may have several instances of the same Control class, but with different keys, e.g. resize controls.

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[asturur]

Control had a name property, at least all my fabric 5.x apps have one.
Correction: is a custom thing i did by passing a 'name' property when the controls were happy plain objects.

[asturur]

The constructor of Control uses Object.assign, so you can probably just pass in the name and get it in the control.
I wonder if with the interfaces those properties can be added easily without the need of subclassing.

[jiayihu]

The constructor of Control uses Object.assign, so you can probably just pass in the name and get it in the control.

That’s true. Didn’t think about it.
Anyway if we make it built-in in fabric it means we can drop the object return type in getActiveControl and _findTargerCorner.

[ShaMan123]

Question:
Does it make sense for the controls object to be an array?
Then we force the control class to accept a key/name?
IMO it makes sense because control hit detection depends on the control stack order and an array makes that very clear.

[jiayihu]

Might be a good idea but out of the scope of this PR. I guess it'd be similar to findTarget returning an array of targets in the stack order, something that you tried already IIRC

[asturur]

im sure the name was in at some point. I think i removed it because i realized it was duplicate of the key.
If the issue is a name i m glad to add a name at this point rather than changing another function return style.

Changing the return of the function of the internal function is not terrible anyway.
Doing both maybe is unnecessary.

[asturur]

this return type changed like 4 times in 2023

[jiayihu]

Sign of bad API. If name is re-introduced, the return type could just be : Control, which is much better IMO.

[asturur]

But it should also be called findTargetControl, not findTargetCorner. But anyway.
Can you add a sentence in perfect english on top of _findTargetCorner that explains in simple words your use case for overriding.
Something like:

If you are using interactive groups, it may happen to you that the control of your outer group is preferred to the overlapping control of the inner object, if you find this annoying please consider that you can override this method in order to return the control you prefer.

If this is your use case, can you add it there that will serve as an hint when documenting but also as an hint to be careful with changes that this method is half internal half not.
I overrided it as well in the past

[asturur]

Or add your personal use case whichever it is if is not secret

[jiayihu]

But it should also be called findTargetControl

I thought about that as well, I'd honestly prefer renaming but did not want to break the API, although it's internal. But if you're okay with it I think it would make more sense.

[jiayihu]

im sure the name was in at some point. I think i removed it because i realized it was duplicate of the key.
If the issue is a name i m glad to add a name at this point rather than changing another function return style.

The name is not the issue but I'd welcome the change.

Changing the return of the function of the internal function is not terrible anyway.
Doing both maybe is unnecessary.

The change of returned value is still necessary for me to allow overriding the method to return a Group's subTarget control. There is no reason why fabric should first call _findTargetCorner then do target.controls[corner]. By returning the control directly, the fabric code is less verbose while also giving more flexibility to devs.

I'm not suggesting fabric should start caring about devs overriding internal methods, it's just that this seems a win-win for both. Fabric gets a simpler code, I get an overridable method "by chance" 😄 All thanks to improve return value

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	910.079 (+0.143)	304.771 (+0.033)

[asturur]

I fixed the action for the changelog, first try!

JJhiRdcYfcokU.mp4

[asturur]

@jiayihu and @ShaMan123 if you want to merge this PR fix the unit tests or tell me you can't but you would and i ll fix the unit tests.

[asturur]

@ShaMan123 do you have opinions here?

[asturur]

pending @ShaMan123 review

[ShaMan123]

I also am for renaming the method

[jiayihu]

I also am for renaming the method

Yeah, maybe we should call it _findTargetControlHandle and keep _findTargetCorner: (pointer) => _findTargetControlHandle(pointer)?.key basically

[ShaMan123]

How about findControl?

[jiayihu]

findControl is okay for me, @asturur ?

[asturur]

At this point changing what the method returns or what returns and how is called is the same.
I need some hours to think if at this point it should also get a scene point rather than a viewport point since it would become a public method.

[asturur]

i m good with findControl but probably i don't want to touch anything else.
I ll update the PR and if we have second thoughts tomorrow we just can revert and fix it

[ShaMan123]

I am now migrating this breaking change and I have noticed that the only usage left of FabricObject#__corner is by getActiveControl.
So I suggest removing FabricObject#__corner and getActiveControl and not exposing it as part of v6

The dev should be in charge of understanding what the active control is, they can look at canvas._currentTransform.
To begin with I exposed getActiveControl because there was no simple way to hook into findControl but now there is

[ShaMan123]

updated comment

[asturur]

The point of __corner, name apart, is that is precalculated by your last mouse movement and is used by developers too.
so is a good way to find it right away when you don't have a transform going on. __corner is a long standing feature, we could remove getActiveControl but not __corner.

[jiayihu]

Funny for something called __corner, thus "private private corner" to be used by developers.

[asturur]

it is like that, funny or not is exposed through the object during events
It can be upgraded to official event property but removed seems removing something useful.