Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUGFIX: fix bug with pixelSnapping on certain aspectRatios and image dimensions #3578

Open
wants to merge 1 commit into
base: 8.2
Choose a base branch
from
Open

BUGFIX: fix bug with pixelSnapping on certain aspectRatios and image dimensions #3578

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/neos-ui-editors/src/SecondaryEditors/ImageCropper/index.js
View file
Open in desktop
Expand Up @@ -159,8 +159,14 @@ export default class ImageCropper extends PureComponent {
const normalizedAspectRatioHeight = currentAspectRatioStrategy.height / aspectRatioGcd;

// pixel perfect calculations
const naturalCropWidth = Math.floor(imageWidth * (cropArea.width / 100) / normalizedAspectRatioWidth) * normalizedAspectRatioWidth;
const naturalCropHeight = naturalCropWidth / normalizedAspectRatioWidth * normalizedAspectRatioHeight;
let naturalCropWidth = Math.floor(imageWidth * (cropArea.width / 100) / normalizedAspectRatioWidth) * normalizedAspectRatioWidth;
let naturalCropHeight = naturalCropWidth / normalizedAspectRatioWidth * normalizedAspectRatioHeight;

while (naturalCropHeight > imageHeight) {
// can't crop area larger than image itself, so keep subtracting normalized aspect ratio values until area is valid
naturalCropHeight -= normalizedAspectRatioHeight;
naturalCropWidth -= normalizedAspectRatioWidth;
}
Comment on lines +165 to +169
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be sure that this while loop never causes the ui to freeze (eg. are negative values at any time possible? - if so we need to guard against this ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assumption being made in the following statements is that imageHeight will be positive or 0.

The loop then terminates when one of the following conditions is met:

normalizedAspectRatioHeight > 0 or naturalCropHeight < 0

If we actually substitute the variables in naturalCropHeight variable declaration and ignore the floor function since it wouldn't change the sign , you eventually get a minimized form like

let naturalCropHeightWithoutFloor = cropArea.width * imageWidth * (currentAspectRatioStrategy.height /( 100 * currentAspectRatioStrategy.width)) (see Wolframalpha)

From that we can see that if either passed variable in crop configuration is negative, naturalCropHeight will be smaller than zero and the loop will never start.

If both configuration dimension variables are positive the aspectRatioGcd will be positive and therefore normalizedAspectRatioHeight will be positive, hence the loop with exit eventually.

If both are negative, naturalCropHeight will be greater than 0 and normalizedAspectRatioHeight will be smaller than 0. In that case the loop will not exit. Math.abs() on aspectRatioGcd would cover that edge case (I mean who passes negative values in the aspectRatio configuration ?)


// modify cropArea with pixel snapping values
cropArea.width = (naturalCropWidth / imageWidth) * 100;
Expand Down