V7.0.0 crop in pixel

[Dallas62]

Hi!

I'm just testing the new version of react-image-crop and I think there is a issue with the coords/size of crop. In the previous version I was using pixelCrop from onComplete to get the real coords/size of selection (regardless resize due to CSS). But in the new version the crop coords/size seems to be relative to a scale due to resize. This make incompatible pixelCrop from te previous version and crop of the new one. Also, the old pixelCrop is not available.

In my opinion, we should always return the real crop size, regardless a resize from the browser.

[sekoyo]

Hi is this when you are showing the crop preview in the browser or the crop itself? See this screengrab of a non-natural size image with crop showing correct values:

Are you able to replicate the issue here?: https://codesandbox.io/s/72py4jlll6

I'm not sure what you mean with "In my opinion, we should always return the real crop size, regardless a resize from the browser." as the crop dimensions should reflect what is the browser.

[Dallas62]

Hi,

On the User-side, every thing works fine, but data displayed in the console are strange. I took an example with the same point on the same picture (just resized the frame). The position is different for the same point and same image. And the selection display the same value in the console but looks larger on the image.

(You have to reload the page between resize)

[Dallas62]

I think I understand what it's going on, On react-image-crop, crop value is relative to the browser position/bounding box. In my opinion, it would be more natural to have the value relative to the image.

x: 10, y: 10 should be at the same place even if the image doesn't have the same size after browser render.

[sekoyo]

Thanks for looking into this I think it's related to using transform for x + y instead of top and left which I was doing before. I'll check and publish a fix later (blocked at work)

[sekoyo]

Please try 7.0.1 when you get a chance

[Dallas62]

Hi,

Thanks for your time, unfortunately, I think there is a misunderstanding on the problem.

I took an other exemple to illustrate:

In this exemple, I should have see the crop position on the same place AND with the same selection on the image (bottom of the shelf). But this in this exemple, the crop area is going up and down, do not resize as the image resize.

I reproduced this on the previous version:

This use case is an issue when you have a mobile or a desktop. You cannot select the same area on an picture on mobile/desktop, specialy if you have 'minHeight' or 'minWidth'. You have to calculate a scale size (width / naturalWidth) to know how to crop the selected area on the server. If you have an object detection on server side, you have to calculate the crop area based on (width / naturalWidth).

In my opinion, "pixels are better than percent", because minHeight=50%, minWidth=50% will depend on the aspect ratio of the image. Actually, minHeight=1000px, minWidth=1000px, will render a square but it's not the same crop area if you are on mobile or desktop. In the previous version (percent) the render was similar.

[sekoyo]

This is a tricky problem, since the library can't convert the pixels on load according to the image size unless it knows the size of the image when those pixel values were set.

I think the library might need to pass a percentCrop like it used to for pixel crops to handle this case, and be able to convert percentages to pixels in the crop object.

[Dallas62]

The previous version is able to do this thing, isn't it possible to wait for the image size then apply the scale ? (width /naturalWidth)

I will try to look into the library ASAP what we can do.

[sekoyo]

That would only be possible if the pixel values passed in were from a full size image, as it needs a size to convert them from. If you went from e.g. a squashed mobile view to a full size desktop view then we wouldn't be able to calculate the values as we wouldn't know what size the image was when the crop was made.

[frontshift]

It took me a while to figure out that V7 is now pixel value based since the example is still based one the previous version. 🤣 The way I solved this is to update the crop in onImageLoaded, returning false so that onCropComplete / onCropChange don't fire (documentation needs to be updated). I added a resize listener and re-calculate the crop whenever the browser size changes When I pass an updated crop back to the parent component, I convert the crop back to its original values. I think it should be relatively straightforward to support this out of the box (optional), perhaps passing a scale adjusted crop and a crop based on the natural dimensions of the image

  public componentDidMount(): void {
   this.throttledOnResize = throttle(this.updateCrop, 300, { trailing: true });
   window.addEventListener('resize', this.throttledOnResize);
  }

  private calculateCrop(adjustForImageScale = true, crop = this.props.crop): Crop {
    let scaleFactorX;
    let scaleFactorY;
    if (adjustForImageScale) {
      scaleFactorX = this.image.width / this.image.naturalWidth;
      scaleFactorY = this.image.height / this.image.naturalHeight;
    } else {
      scaleFactorX = this.image.naturalWidth / this.image.width;
      scaleFactorY = this.image.naturalHeight / this.image.height;
    }

    return {
      aspect: crop.aspect,
      width: Math.round(crop.width * scaleFactorX),
      height: Math.round(crop.height * scaleFactorY),
      x: Math.round(crop.x * scaleFactorX),
      y: Math.round(crop.y * scaleFactorY)
    };
  }

 private updateCrop = () => {
    this.setState({ crop: this.calculateCrop() });
  }

  private onImageLoaded = (image: HTMLImageElement): boolean => {
    this.image = image;
    this.updateCrop();
    // we are returning false so that ReactCrop doesn't call onCropComplete() callback
    return false;
  }

  private onCropComplete = (crop: Crop) => {
    // convert crop to original scale
    const newCrop = this.calculateCrop(false, crop);
    this.props.onCropChange(newCrop);
  }

 private onCropChange = (crop) => {
    this.setState({ crop });
  }

[sekoyo]

Thanks @frontshift, unfortunately there is a trade off both ways. I think what I'll do is add a unit prop to specify if the crop object is using pixels or percent, and adjust these to pixels before rendering. Also I will pass a percentCrop to onChange and onComplete in case the user wants to use that instead of pixels.

That way a user can still use percentages for the crop if they choose to for situations like easily restoring the crop on different devices. Or they can use the default pixels for simplicity. Also that way max/min dimensions can still be specified in pixels which is generally what people want.

As for resizing, a resize HOC could be added, and so long as the user is using the percent version, if they re-render then it should re-draw the crop correctly.

PS what do you mean by the example is still using percentages? On codesandbox? (it should be updated there)

[frontshift]

@DominicTobias you are right, the example is using v7.0.1. Not sure what link I was looking at... A HoC / render prop sounds like a good idea. Should keep the library tidy without having to account for too many scenarios.

[Inveth]

Is this using pixels now instead of percents? Updated it just today and it seems to be using pixels by default.

[frontshift]

@Inveth Yes, see https://github.com/DominicTobias/react-image-crop/releases/tag/7.0.0 But it looks like in 8.0 you can set a unit prop to use percentage or pixel values https://github.com/DominicTobias/react-image-crop/releases/tag/8.0.0.beta-0

[Inveth]

@frontshift thanks - I switched back to previous version and probably move with 8.0+ later on!

[sekoyo]

@Inveth v8 is released you can now use percentages if you choose to 🎉. Closing but feel free to comment if you have any issues.

See - https://github.com/DominicTobias/react-image-crop/releases/tag/8.0.0

[Dallas62]

@DominicTobias Sorry for the delay, the project is blocked so I didn't dig more into it.

But I think there is a misunderstanding of the problem. The problem is not to have percentage or pixels, it's that pixels are handled with navigator's perspective. (it's 200px in the navigator-inspector like in the library). In my point of view, we should have the pixel on picture's view.

If a picture of 2400x1600 is displayed in 600x400 by the navigator (like max-width 600, img-fluid of Twitter Bootstrap), react-image-crop will use the second resolution. In my point of view it's incorrect because a frontend may have to interact with a backend (server-side crop). If the backend do not know the rendered size, or if the frontend do not convert the resolution, it will cause pain to debug.

And it also cause problem when you use the component on mobile, like I said:

In my opinion, "pixels are better than percent", because minHeight=50%, minWidth=50% will depend on the aspect ratio of the image. Actually, minHeight=1000px, minWidth=1000px, will render a square but it's not the same crop area if you are on mobile or desktop. In the previous version (percent) the render was similar.

Finally, minHeight=1000px, minWidth=1000px, may block the usage of the library on mobile.

[sekoyo]

@Dallas62 np thanks I think I get what you mean, however it would be a bit strange if the crop that came out of onChange is not what you are seeing or pass into the component.

But cropping on the server shouldn't be hard, you can either a) use percentages and send those to the server, the server can use those regardless of what size the image is on the client, or b) scale the pixels using image.naturalWidth and image.naturalHeight and image.width and image.height before sending them.

I think sending percentages is the easiest option as the server can convert those to pixels using the image dimensions and there is no scaling you need to do on the client (or you could do it on the client if you prefer).