fix(Controls): forbid scaling to avoid NaN issues on scaling zero sized objects. Issue 9475

[gloriousjob]

Motivation

@closes #9475

Description

0 height polygons have issues where they disappear when you scale them.

Changes

Adds guards to ensure divide by 0's do not happen to create these problems. This is done by preventing the scale when value is 0.

[gloriousjob]

Original PR here: #9496
I renamed the branch since I'm isolating to the issue number that the branch was not named to.

[ShaMan123]

what do you think? What about the scaling from the corner case?
But still I am not sure this is good enough
I think we should expect calculations to be safe in all cases.
However, that can be done in the future and doesn't have to be part of this PR

[ShaMan123]

Suggested change

	if (fabricObject.width === 0 && by == 'x') {
	return true;
	}
	if (fabricObject.height === 0 && by === 'y') {
	return true;
	}
	if (fabricObject.width === 0 && by !== 'y') {
	return true;
	}
	if (fabricObject.height === 0 && by !== 'x') {
	return true;
	}

[gloriousjob]

I tried this with a 0 height rectangle. It turned out that the bottom right corner is what was showing on the right side, so I couldn't extend the rectangle in the x direction. That said, I found out the diagonal was working because it used uniform scaling and the uniform scaling calculation uses a different calculation based on adding the scales and doesn't break as a result. But if the uniform scaling was off, the diagonal ends up with the issue (i.e., NaN and the polygon disappears).

So this is a good thought since non-uniform scaling will break without taking the 'both' case into consideration. But it almost begs for the diagonals to be hidden. Is there an easy way to do this (or would that be a bad idea since it could add confusion on how corners show)?

[ShaMan123]

Not sure I understand

[gloriousjob]

Basically, because it’s a line, the diagonal scalers stack on top of the edge scaler and making the both case forbidden means you can’t extend the line.

To make things confusing, when uniform scaling is on and both is allowed, the diagonals work but when uniform scaling is off, this diagonals are broken.

[asturur]

i m pretty much sure there was code for the simple Line object that was hiding the corners when one dimension was 0.

[ShaMan123]

seems I have found the source of these bugs

Should be !width || !height

[ShaMan123]

  isNotVisible() {
    return !this.opacity || !this.width || !this.height || !this.visible;
  }

[ShaMan123]

IMO it is weird the an object with height of 0 and strokeWidth >0 draws the stroke
Effectively there is nothing to stroke

[asturur]

that only skips rendering, it doesn't makes it unselectable. If you have padding the object is still selectable.

[asturur]

seems I have found the source of these bugs

Should be `!width || !height`

This is unrelated.
the boolean visible is for the developer, and determines rendering and selectability.
The 'isNotVisible' is an internal check to skip rendering code when will produce no output. It has nothing to do with our calculation blowing away because of a zero division.

[asturur]

i m ok with this, regarding the best way to do this, this was an issue we also had with line, and somehow we solved it by hiding the controls.
I do not have strong opinions about calculations being safe all the time, important is to not break.

[asturur]

Looking at this issue
#9475
and the code in general, i have the following comments:

• it is worth to be fixed even if the use case is really unlikely.
• The code fails because getTransformedDimensions has a zero dimension, so we should check on that.
• the problem of overlapping controls exists, but i m not sure what is the solution, if we should write code because a developer uses a zero sized rectangle to do something instead of a line object
• i think the code that was safeguarding line is either well hidden or lost during the custom control api

[asturur]

I think the test is a bit over complicated i think we can slim it down a bit

[gloriousjob]

@asturur Concerning

• The code fails because getTransformedDimensions has a zero dimension, so we should check on that.

What would you think of this change in the place of old lines 177/178? Basically it just verifies the non-zero value and defaults the scale to 1 if it is 0:
scaleX = dim.x ? Math.abs((newPoint.x * target.scaleX) / dim.x) : 1;
scaleY = dim.y ? Math.abs((newPoint.y * target.scaleY) / dim.y) : 1;

This might be a better fix than blocking the scale considering some of the concerns brought up. (Note: newPoint.x/y could be considered instead of 1.) (Note2: The original bugfix suggested giving the shape a minimal height so that the scaling functionality would actually scale as opposed to staying 0 height. I'm not sure which would be correct but the reporter seemed to think it made sense for a 0 height rectangle to be scaled to 2 height.)

[asturur]

It doesn't make sense to go from 0 to 2 with scaling, especially in a world in which you can just slap on a rectangle a change height control.
FabricJS is not a piece of JS to build a foolproof ux for editors covering all the possible programmers mistakes.
It supports interactivity but you have to know what are you doing.
Another example is string in place to numbers, i do not think we should check that a value is a string and in case parse an integer from it because some developers do not remember that any input form return a string even if is of a numeric kind.

setting scale to 1 is a bit dangerous, it would break the uniform scaling.

Imagine you have a poling scaled to 4x on both sizes.
then you move the points in a line, you obtain size 0 on height, and then you try to scale, you reset scale to 1.
then you go back to editing points and you move back a point, now your height is again different from 0 but your polygon is now scaled with a non uniform value.

Again this is a very weird case, we have just to avoid to fillup the console in red, so imho blocking it is good.
We do not have to trigger the forbid sign, we can also just return false from the action so scaling won't happen at all.

The part of showing the disallow cursor is optional if we think it makes more sense to warn the dev/user that something is being blocked rather than thinking their left mouse button is being defective

[ShaMan123]

I think we can use the logic concept @asturur implemented in #9575 to fix this for good
If we calculate the ratio between the 2 vectors that define the scale axis it should be safer/easier
We have the anchor point and the pointer.
Calculate the ref vector: A = controlPosition - anchorPoint
Calculate the current vector: A' = pointer - anchorPoint
Calculate the ratio: ratio = magnitude(A) > 0 ? A' / A : 0
Calculate new scale: scale *= ratio
We should use a dot product calculation to obtain the magnitude with respect to the direction so it supports flipping

[asturur]

I think we can use the logic concept @asturur implemented in #9575 to fix this for good If we calculate the ratio between the 2 vectors that define the scale axis it should be safer/easier We have the anchor point and the pointer. Calculate the ref vector: A = controlPosition - anchorPoint Calculate the current vector: A' = pointer - anchorPoint Calculate the ratio: ratio = magnitude(A) > 0 ? A' / A : 0 Calculate new scale: scale *= ratio We should use a dot product calculation to obtain the magnitude with respect to the direction so it supports flipping

but any scale you give to a zero size item will still give you zero.
the fix we implemented here imho is just fine, 0 scaling doesn't exist.
Just i would like to slim down the test that is too confusing

[asturur]

Merging this next, but i will try to make the test as simple as possible.

[asturur]

At the end i extended the test because i couldn't simplify it.