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

Cropped area not accounting for zoom of containing elements properly #591

Open
c0d3ster opened this issue May 29, 2024 · 6 comments
Open

Comments

@c0d3ster
Copy link

Bug Description:
I am using ReactCrop as an element/node on a React Flow canvas. The React Flow library allows the entire canvas area to be zoomed on scroll. Whenever the zoom !== 1, it leaves me unable to crop the larger than zoom (when < 1) or able to crop larger than the image (when > 1). I am also scaling the image down from naturalWidth to a set max width with CSS, but the completed crop is properly being calculated so shouldn't be any issues there.

Screenshot 2024-05-29 at 11 02 44 AM

Expected Behavior:
The croppable area selection should always be a maximum of the width and height of the image.

Actual Behavior:
The croppable area selection is only able to go to a maximum of width * zoom or height * zoom. If I try to manually divide by the zoom value during setCrop it causes another issue where the crop area expands while moving the selection.

Minimum Reproducible Example
A minimum reproducible example can be created by adding transform: scale(50%) to the div containing the ReactCrop component then trying to move the crop selection. the selection will only be able to crop up to 50% in the top left of the image

Possible Solutions
There seems to be an issue with the containCrop or clamping logic within the resizeCrop function, so I'm wondering if we can either calculate any zoom value internally using window/document properties or if we can pass in a prop for zoom. This would be different from the scale prop because I still want the image to display without any whitespace and at 1.0 scale.

@Ericxgao
Copy link

I am also seeing this issue.

@sekoyo
Copy link
Owner

sekoyo commented Jun 3, 2024

I used to have a zoom prop, perhaps something like that can fix it 🤔

/** A non-visual prop to keep pointer coords accurate when a parent element is scaled.
Not to be confused with the `scale` prop which scales the image itself. Defaults to 1. */
zoom?: number

https://github.com/sekoyo/react-image-crop/blob/9.1.1/src/ReactCrop.tsx#L358

https://github.com/sekoyo/react-image-crop/blob/9.1.1/src/ReactCrop.tsx#L436

Are you able to make an example of the issue on React Flow (haven't used or heard of it before)?

@c0d3ster
Copy link
Author

c0d3ster commented Jun 5, 2024

React Flow is a library used for node based workflows. The way the zoom works under the hood is utilizing transform: scale with CSS, so you can just wrap a ReactCrop element in a scaled div to see the same effect. Here is the result of doing that with the CodeSandbox example:
https://codesandbox.io/p/sandbox/react-image-crop-demo-with-react-hooks-forked-with-zoomed-out-container-gqjwpg

It would be possible to make a CodeSandbox with React Flow in particular, but that would require more boilerplate since you need to define all your node types. In my current project, the behavior is slightly different than the example above as the cropped output is accurate to the crop area visually. It just won't let you set the crop area to larger than the zoom amount. In other words, the screenshot I attached above is the maximum selectable area.
I believe bringing that zoom prop back would do the trick in both cases though.

@sekoyo
Copy link
Owner

sekoyo commented Jun 5, 2024

Sorry I misread - looks like it's only an issue with the crop preview.

So you can modify it like:

async function canvasPreview(
  image: HTMLImageElement,
  canvas: HTMLCanvasElement,
  crop: PixelCrop,
  scale = 1,
  rotate = 0,
  appScalePc = 100,
) {
  const ctx = canvas.getContext('2d')

  if (!ctx) {
    throw new Error('No 2d context')
  }

  const scaleX = (image.naturalWidth / image.width) * (100 / appScalePc)
  const scaleY = (image.naturalHeight / image.height) * (100 / appScalePc)
  
  // ....
}

And if your transform: 'scale(50%)' is 50%, then pass 50 to appScalePc

@Ericxgao
Copy link

Would it still be possible to expose that zoom prop?

@Ericxgao
Copy link

I fixed this with the following subclass - would it be possible to expose a scale or zoom parameter as it seems like it does this?

interface ScaleCompatibleReactCropProps {
  scale?: number
}

class ScaleCompatibleReactCrop extends ReactCrop {
  props: ReactCropProps & ScaleCompatibleReactCropProps

  getBox() {
    const box = super.getBox()
    const { scale } = this.props

    if (scale !== undefined) {
      box.width /= scale
      box.height /= scale
    }

    return box
  }
}

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

3 participants