fix(MultiSelection): add target from behind active selection #8744
Conversation
|
Build Stats
|
in case group and all it's objects are selected we deselect group
ShaMan123
changed the title
|
@ShaMan123 can you explain the bug described as |
src/shapes/ActiveSelection.ts
Outdated
| ) { | ||
| // in case group and all it's objects are selected we deselect group | ||
| this.remove(parent); | ||
| } |
There was a problem hiding this comment.
this kind of interactivity with groups opens for a lot of edge cases.
I don't think automatically deselecting things outside user clicks is correct since we are assuming which is the correct UX here.
Why is deselecting the parent or the group or anything correct over the other option?
Does it create an interaction issue or just feel strange?
There was a problem hiding this comment.
This fixes a bug
I assume, though I don't really understand, that since group doesn't layout when its children select/deselect something happens causing:
fabric.js.sandbox.-.Google.Chrome.2023-02-27.15-24-53_Trim.mp4
both textboxes and poly are grouped
active selection at the beginning is circle and group
There was a problem hiding this comment.
with interactive groups it will be possible to deselect them in a multi selection interaction if only one object remains unselected in group since the object becomes the deselection target and the group will hang around, SELECTED
There was a problem hiding this comment.
I agree maybe it should be moved to multiSelectAdd
There was a problem hiding this comment.
I remember you said groups are simply containers.
What meaning does an empty selected container have in a selection context?
There was a problem hiding this comment.
Groups were simple containers, yes, meaning the would just contain object and render them.
Now they are interactive, and the surface edge cases.
Most of them should be left to the developer, and we should patch the ones that can be caused by the user alone.
Probably is enough to avoid rendering the control boxes, i m not sure at this point since it doesn't make sense to select a group and the objects inside.
What i would answer quickly would be that if the group is selected, you can't select objects inside.
And probably is the solution i would adapt if it simplifies our life.
There was a problem hiding this comment.
What i would answer quickly would be that if the group is selected, you can't select objects inside.
And probably is the solution i would adapt if it simplifies our life.
Not bad
There was a problem hiding this comment.
Your suggestion makes sense. If we agree on it I can try a patch
I am not sure. Looking into it. But 5.x supports this Kitchensink._.Fabric.js.Demos.-.Google.Chrome.2023-02-27.15-21-49_Trim.mp4 |
|
Found it. findTarget: function (e, skipGroup) {
if (this.skipTargetFind) {
return;
}
var ignoreZoom = true,
pointer = this.getPointer(e, ignoreZoom),
activeObject = this._activeObject,
aObjects = this.getActiveObjects(),
activeTarget, activeTargetSubs,
isTouch = isTouchEvent(e),
+ shouldLookForActive = false;
// first check current group (if one exists)
// active group does not check sub targets like normal groups.
// if active group just exits.
this.targets = [];
+ // skipGroup skips all logic that I commented out
// if we hit the corner of an activeObject, let's return that.
// if (shouldLookForActive && activeObject._findTargetCorner(pointer, isTouch)) {
// return activeObject;
// }
// if (aObjects.length > 1 && !skipGroup && activeObject === this._searchPossibleTargets([activeObject], pointer)) {
// return activeObject;
// }
// if (aObjects.length === 1 &&
// activeObject === this._searchPossibleTargets([activeObject], pointer)) {
// if (!this.preserveObjectStacking) {
// return activeObject;
// }
// else {
// activeTarget = activeObject;
// activeTargetSubs = this.targets;
// this.targets = [];
// }
// }
+ var target = this._searchPossibleTargets(this._objects, pointer); // I missed here
if (e[this.altSelectionKey] && target && activeTarget && target !== activeTarget) {
target = activeTarget;
this.targets = activeTargetSubs;
}
return target;
}, |
Yes this was made possible in findTarget, i think at the end of the function where we would say that if there wasn't a sub target in the activeSelection, but there was eventually a target under it we would take that one |
src/canvas/Canvas.ts
Outdated
| (object) => !prevActiveObjects.includes(object) | ||
| ), | ||
| ...prevActiveObjects, | ||
| ], |
There was a problem hiding this comment.
I m trying to wrap my head around why we need to change the stack for searchPossibleTarget.
I understand you want the activeSelectionObject searched first, but i think that is job for altSelectionKey.
Isn'it?
Shouldn't we use the natural stack order here?
There was a problem hiding this comment.
Yes this was made possible in findTarget, i think at the end of the function where we would say that if there wasn't a sub target in the activeSelection, but there was eventually a target under it we would take that one
I followed this logic and:
fabric.js/src/canvas/SelectableCanvas.ts
Line 989 in 26ed225
There was a problem hiding this comment.
I guess we can achieve it in a different way.
If !target => search the rest of the object on canvas to see if there is an object behind the selection that we can add
|
@asturur ? |
|
@asturur what about this? |
|
@asturur this is blocking me at work, can we merge it? |
| target = | ||
| // first search active objects for a target to remove | ||
| this.searchPossibleTargets(prevActiveObjects, pointer) || | ||
| // if not found, search under active selection for a target to add | ||
| // `prevActiveObjects` will be searched but we already know they will not be found | ||
| this.searchPossibleTargets(this._objects, pointer); |
There was a problem hiding this comment.
I have made this simpler perhaps in 347ff7f - revert it to see the prev revision
I belive this is more performant also
There was a problem hiding this comment.
ShaMan123
changed the title
Motivation
Adding a target from behind active selection was not possible
Description
Changes
Gist
In Action