fix(Group): remove added object from canvas #8034
Conversation
Code Coverage Summary |
|
It's very nice that tests simply pass (without firefox failing) |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This allows objects on active selection to enter I want to change that related #7882
src/static_canvas.class.ts
Outdated
| else if (object.canvas) { | ||
| /* _DEV_MODE_START_ */ | ||
| console.warn('fabric.Canvas: removing object from canvas before entering another', object, object.canvas, this); | ||
| /* _DEV_MODE_END_ */ | ||
| object.canvas.remove(object); | ||
| } |
There was a problem hiding this comment.
in my opinion this should be executed always, not just with an else if.
What if it was in a group, on another canvas?
In that case the older canvas has a reference to an invalid object.
if we are safeguarding, we should remove from group and from canvas.
There was a problem hiding this comment.
What if it was in a group, on another canvas?
I that case the first if block will catch it and Group#remove will clear refs.
Else if is necessary since we are checking for a parent and because an object always has a ref to canvas (even if not a direct child of canvas) we call tell if canvas is the parent or not only by checking if the group ref isn't empty.
I introduced getParent in a pending group PR for this purpose exactly.
Then it can become
object.getParent()?.remove(object)
src/shapes/group.class.ts
Outdated
| else if (object.canvas) { | ||
| /* _DEV_MODE_START_ */ | ||
| console.warn('fabric.Group: removing object from canvas before entering group', object, object.canvas, this); | ||
| /* _DEV_MODE_END_ */ | ||
| object.canvas.remove(object); | ||
| } |
There was a problem hiding this comment.
as the comment below, this check seems necessary in every case
|
Forgot about this small pr. |
|
@asturur up |
|
Build Stats
*inaccurate, see link |
|
those fix where committed with other PRs, i see them inside the codebase |
| } else if (object.canvas) { | ||
| console.warn( | ||
| 'fabric.Group: removing object from canvas before entering group', | ||
| object, | ||
| object.canvas, | ||
| this | ||
| ); | ||
| object.canvas.remove(object); |
There was a problem hiding this comment.
This is not in master @asturur
|
This Pr needs porting but should enter rc ASAP |
This PR safeguards
group.addfrom a case which the added object belongs to a canvas and vice versaActiveSelection should conform to this logic IMO, though currently this bug is prevented because the selected objects are filtered out from the rendered objects in the render cycle.
Meaning a case in which an object is shared between 2 canvases and is selected by one is a hole in the logic. I can code something to patch this but I don't think it's needed 'cause were talking about a super edge case (which I think states the dev is handling heavy customization so tough luck for them)EDITED OK that's wrong. An object won't be selected if it doesn't belong to canvas or a group in it (only programmatically) . So this isn't really a case