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.

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

Rounding issue in renderCropBoxData causing it to set to the old left value #1057

Closed
JamieAtherton opened this issue Mar 21, 2023 · 8 comments

Comments

@JamieAtherton
Copy link

JamieAtherton commented Mar 21, 2023

Describe the bug
When cropping a specific image (https://images.pexels.com/photos/3802510/pexels-photo-3802510.jpeg) using
setData({
width: 2362,
height: 2362,
x: 0,
y: 0
});
The x value is not set to 0 and instead it reverts it back to the original x value before the call to setData().
We have looked at the source code and traced the issue to the function renderCropBoxData, which doesn't seem to round the width value when checking it against the maxWidth value. i.e width is something like 260.00000000006 (after being scaled down based on the aspect ratio) and maxWidth is 260 and so this check evaluates to false and then it revers the x value. See screenshot below.

To Reproduce
See above

Expected behavior
I expect the x value to be honoured and not reset.

Screenshots
1

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser: Edge and chrome
  • Version: Latest

Smartphone (please complete the following information):
N/A

Additional context
N/A

@fengyuanchen
Copy link
Owner

fengyuanchen commented Mar 23, 2023

Maybe you have to use cropper.setCropBoxData(cropBoxData).setCanvasData(canvasData) instead of cropper.setData(data).

To restore a crop box exactly, you can reference this example.

@JamieAtherton
Copy link
Author

JamieAtherton commented Mar 23, 2023

Maybe you have to use cropper.setCropBoxData(cropBoxData).setCanvasData(canvasData) instead of cropper.setData(data).

To restore a crop box exactly, you can reference this example.

The link provided gives me a 404.

Why can't we just round the width and height when scaling the crop box? Pretty sure that would fix the issue
image

We are allowing users to switch between images by calling cropper.replace(imageUrl), so to restore the crop box for that image we need to call setData() with the last known crop bounds. Otherwise, if we have to call setCropBoxData then we are basically going to have to do all the same scaling calculations that cropperjs does.
image

@clay-lenhart-jelly
Copy link

From a user's point of view, cropperjs seems buggy. We set the crop that the user specified previously and cropperjs ignores it.

How can we fix cropperjs? Should we submit a PR?

@fengyuanchen
Copy link
Owner

You might try the v2 version.

@clay-lenhart-jelly
Copy link

You might try the v2 version.

Ah ok, makes sense what is going on. We were puzzled why you didn't see this as a bug.

@fengyuanchen
Copy link
Owner

@clay-lenhart-jelly It is a design problem of v1, but not bug. To "fix" this design problem, I refactor all things in v2.

@13twelve
Copy link

13twelve commented Jun 23, 2023

@JamieAtherton @fengyuanchen it seems if you check the x position after setting and reset just the x position on its own, this works:

let crop = {
  width: 2362,
  height: 2362,
  x: 0,
  y: 0
};

cropper.setData(crop)

if (crop.x !== cropper.getData(true).x) {
  cropper.setData({ x: crop.x })
}

Or, even just setting all the props individually:

let crop = {
  width: 2362,
  height: 2362,
  x: 0,
  y: 0
};

cropper.setData({ x: crop.x })
cropper.setData({ y: crop.y })
cropper.setData({ width: crop.width })
cropper.setData({ height: crop.height })

13twelve added a commit to area17/twill that referenced this issue Jun 23, 2023
attempting to fix #1194

which seems to be an issue with cropper.js
(fengyuanchen/cropperjs#1057)

and ultimately is likely caused by rounding errors

It seems setting individual crop settings instead
of attempting to set them all in one go fixes it,
at least within Twill
@JamieAtherton
Copy link
Author

@JamieAtherton @fengyuanchen it seems if you check the x position after setting and reset just the x position on its own, this works:

let crop = {
  width: 2362,
  height: 2362,
  x: 0,
  y: 0
};

cropper.setData(crop)

if (crop.x !== cropper.getData(true).x) {
  cropper.setData({ x: crop.x })
}

Or, even just setting all the props individually:

let crop = {
  width: 2362,
  height: 2362,
  x: 0,
  y: 0
};

cropper.setData({ x: crop.x })
cropper.setData({ y: crop.y })
cropper.setData({ width: crop.width })
cropper.setData({ height: crop.height })

Thanks, I ended up just npm patching the node module as a short term solution with this fix:
image

ifox pushed a commit to area17/twill that referenced this issue Nov 29, 2023
attempting to fix #1194

which seems to be an issue with cropper.js
(fengyuanchen/cropperjs#1057)

and ultimately is likely caused by rounding errors

It seems setting individual crop settings instead
of attempting to set them all in one go fixes it,
at least within Twill
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants