fix(Group, ActiveSelection): regression, wrong bounding box position when activeSelection or Group has originX/originY that are not default left/top #9649
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Build Stats
|
| .subtract(bboxSize) | ||
| .multiply(originFactor); | ||
| // translate the layout origin from left top to target's origin | ||
| const center = bboxLeftTop.add(bboxSize.multiply(originFactor)); |
There was a problem hiding this comment.
Seems to me this is accounted for here:
fabric.js/src/LayoutManager/LayoutManager.ts
Line 207 in 0547c05
and some line lower for the case of non initialization
|
Ok all layout visual tests are failing at least i have something to check why is failing |
|
So far i think the visual snapshot are bad indeed: This is a top,left no rotation this a bottom right no rotation When the group is created by the test code: function createObjectsForOriginTests(originX, originY, options) {
var rect1 = new fabric.Rect({ left: 150, top: 100, width: 30, height: 10, strokeWidth: 0 }),
rect2 = new fabric.Rect({ left: 200, top: 120, width: 10, height: 40, strokeWidth: 0 }),
controlPoint = new fabric.Circle({ radius: 5, fill: 'blue', left: 150, top: 100, originX: 'center', originY: 'center' }),
tlControlPoint = new fabric.Circle({ radius: 5, fill: 'red', left: 150, top: 100, strokeWidth: 0 });
var g = new fabric.Group([rect1, rect2, tlControlPoint], Object.assign({}, options, {
originX, originY, backgroundColor: 'pink'
}));
return [g, controlPoint];
}It applies the originX/originY to the group at creation. I didn't check the cases with strategy different from fit-content yet. |
|
I understand how this is not consistent with the other properties like angle or scaling, since if i create a group with those already set they taken in consideration during layout and so the group will be bigger than the objects or the objects will appear rotated. But originX/originY are kind of worse because if you don't adapt for those every group that is not top/left will be always moved by an offset compared to the original objects. |
|
I can try to help |
|
I don't understand why the test snapshot you provided are wrong |
@Shaman in the snapshot the end result of the group is moved on the canvas depending the group orginX/originY. |
|
simplifed example, if i have 2 rects at 0,0 and 100,100 both with a 100 size, |
|
So those tests have been added 1.5 years ago. |
|
If i run this code in the old fabricJS: The group stays put when i add it 
And this wasn't supposed to change imho. |
|
While if i specify a position that overrides the result of my group and is calculated with the origin 
|
|
now if do the same with current master the second use case is working ( the one that specify the postion ) the other isn't. |
|
This is a refinement of the LayoutStrategies. |
Please find a time you are comfortable with and make up a calendar event with a fallback option. I m usually free for meeting up to 2PM central europe time |
asturur
changed the title
|
@ShaMan123 is useful to have some comment on this before tomorrow |
|
@ShaMan123 remember to keep some time to gets your thougts on this. Mine so far are:
|
| WARNING needs fix: | ||
| The behaviour of this layout is currently broken if we want. | ||
| The center of the group is the center of the clipPath visually but the position of the clipPath compared to the object on the canvas is hard to predict as it is now | ||
|
|
There was a problem hiding this comment.
@ShaMan123 I verified those behaviors in master and in the PR.
The differences are that now originX and originY do not count anymore for group layouting.
So for example in master with the fixed strategy the final group bounding box would be aligned with the top,left corner or the center or the bottom,right corner depending of the group origin.
Object displacement would happen in any case apart the topleft.
The clipPath has a similar behaviour in master with the clipPath ending not in the center if is not using top,left and object being displaced.
Now the final viewport of the group is always aligned with the center of the initial bounding box, but never displaces anything.
The Clippath has an added behaviour ( both here and in master ) that i do not understand, in particular that if you specify a position for the clippath is not clear how that relates with the objects. but this is out of scope for me to fix now, i m fine to know is known bug and move to other things
There was a problem hiding this comment.
What you describe is correct and proves to me that this PR is wrong
And that is why I want to block it until you remove originX/Y
|
There isn't more to do in this PR from my sides.
|
|
The veto is NOT to merge this PR until you remove originX/Y because this PR is clearly related. |
| @@ -858,7 +858,7 @@ describe('Layout Manager', () => { | |||
| }); | |||
| expect(group).toMatchObject({ width: 100, height: 300 }); | |||
| expect(child.getCenterPoint()).toMatchObject({ x: 100, y: 100 }); | |||
| expect(group.getCenterPoint()).toMatchObject({ x: 50, y: 150 }); | |||
| expect(group.getCenterPoint()).toMatchObject({ x: 100, y: 100 }); | |||
There was a problem hiding this comment.
You pass width and height and a fixed layout and no left and top
So the group's tl should be positioned at 0, 0
Anything else is not fixed layout
There was a problem hiding this comment.
i updated the readme since there was no explanation.
The group should not move the object that is wrapping unless a position is passed.
This is how it was before even the group rewrite.
If you artificially crafted a group that had smaller bbox than the objects, the bounding box would be centered around the group center cutting equally from all the sides.
The same issue here with origin, if you specify for this group center/center, it won't be positioned at 0,0 and also the object will be misplaced.
| If the developer specifies both dimensions of the box, the bounding box is still calculated to obtain the center. | ||
| Once the group is initialized with a fixed layout, adding new objects is not changing the center anymore. | ||
| The layout is fixed, is cropped by the bounding box itself. | ||
| When larger or smaller than the actual boundingbox the group is centered on the initial center of the initial set of objects and the fixed bounding box extends from the center. |
There was a problem hiding this comment.
wrong
it used to position by the passed origin and that is what you missed
There was a problem hiding this comment.
that is not a feature. we don't do that.
originX and originY represent the which point of the object is located at the position described by the properties top and left. They don't represent anything else. They can't also be used to say that since a bounding box is smaller the bbox left/top corner will be aligned wit the top/left corner of the elements, or in another case bottom/right. That is not originX/originY.
There was a problem hiding this comment.
I wrote group layout to fix v5 behavior not to organize it
Centering it makes no sense and again this is why this PR should be blocked until you remove originX/Y
If you do not agree with me go ahead
I don't approve this direction.
There was a problem hiding this comment.
Well you didn't comunicate that clearly when you organize the group rewrite, at all.
Please try to explain what issue the centering causes apart solving the issue of shifting objects.
| WARNING needs fix: | ||
| The behaviour of this layout is currently broken if we want. | ||
| The center of the group is the center of the clipPath visually but the position of the clipPath compared to the object on the canvas is hard to predict as it is now | ||
|
|
There was a problem hiding this comment.
What you describe is correct and proves to me that this PR is wrong
And that is why I want to block it until you remove originX/Y
That behaviour is broken in master and in this PR in the same way. |
|
Your test has been moved and not reverted because is not correct to change the code and the test together. |
|
The proposal of removing originX and originY is still a proposal and won't be pushed in the middle of a process already started of closing a release and doing the website, it will have to wait and is not as related as you think. |
|
You say let's plan. |
This is not planned no. Take your time to understand that or to prove the contrary, is not about frustration or anything. |
|
Keep in mind my time for fabric today is probably finished, i used it to try to refine those PR. The new bug that is discovered about multi selection in interactive groups and that can be considered an edge case and that is monitored by a jest test in the other pr for me can be fixed later, is not immediate to trigger compared to the generic activeselection in group not working at all. |
|
@ShaMan123 @asturur |
|
@ritali4912 this PR and the other marked as bug v6 are in theory the last 2 prs to draw a line and call master 6.0 ( or at least release candidate ). |
|
@ShaMan123 Attaching to this thread again trying to clarify this. If you don't have a better way to handle this bug or if you have been too buys to handle it in the last 2 weeks i will just have to move forward. I do not think anyone will miss a feature that was never announced or described anywhere outside that comment i linked. |
|
This is not a feature.
Incorrect. This PR seems fine but I do not understand why you are keeping the old tests, they are wrong as you pointed out and are useless. Keep them if that is important for you. |
I m keeping old and new tests because in this situation only the combination of both cover the all possible combinations of properties of the group, only with both we an successfully catch an unwanted regression from a code change that seems safe. |
Description
close #9640
I can't find the exact commit because testing the old code is complicated especially when we go back to the first group patch.
It seems to me that the target origin is already taken in consideration in the method 'commitLayout' when setting top and left of the active selection, and that his one internal here is a duplicate of that operation.
When we do:
We need to consider that:
bboxLeftTop is already top/left independently of the origin of the objects, that has been already ammortized in the previous function. originFactor is 0,0 when the the target origin is set to 'center', so this is moving the center to bboxLeftTop.
The center of the bounding box, regardless of objects origin seems to be 'bboxCenter' as calculated in this same function, and so we can trust that.
I built a test case in the vanila template to quickly test different combinations, but i would need your help @ShaMan123 because i m not 100% confident of why every single operation is there
Intended behaviour
Actual behaviour