Skip to content

fix: make px crops export at a deterministic size #35

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

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

fix: make px crops export at a deterministic size #35

Show file tree
Hide file tree
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/main/java/com/flowingcode/vaadin/addons/imagecrop/Crop.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@
* The crop dimensions are defined by the unit, x and y coordinates, width, and
* height.
*
* @param unit the unit of the crop dimensions, can be 'px' (pixels) or '%'
* (percentage).
* @param unit the unit of the crop dimensions, can be 'px' or '%'. A '%' crop
* is resolution-independent; a 'px' crop is interpreted in the
* image's source (natural) pixels (see issue #33).
* @param x the x-coordinate of the cropped area.
* @param y the y-coordinate of the cropped area.
* @param width the width of the cropped area
Expand Down
19 changes: 16 additions & 3 deletions src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,15 @@ public String getImageAlt() {

/**
* Defines the crop dimensions.
*
*
* <p>
* A {@code %} crop is resolution-independent and is recommended for
* programmatic use. A {@code px} crop is interpreted in the image's
* <em>source</em> (natural) pixels, so the cropped output has a deterministic
* size regardless of how the image is scaled on screen (see issue #33). Note
* that this differs from {@link #setCropMinWidth(Integer) the min/max crop
* constraints}, which are expressed in rendered (on-screen) pixels.
*
* @param crop the crop dimensions
*/
public void setCrop(Crop crop) {
Expand Down Expand Up @@ -247,8 +255,13 @@ public boolean isLocked() {
}

/**
* Sets a minimum crop width, in pixels.
*
* Sets a minimum crop width, in rendered (on-screen) pixels.
*
* <p>
* This constraint is applied by react-image-crop in the image's displayed
* pixels, unlike a {@code px} {@link #setCrop(Crop) crop}, which is expressed in
* source (natural) pixels.
*
* @param minWidth the minimum crop width
*/
public void setCropMinWidth(Integer minWidth) {
Expand Down
181 changes: 84 additions & 97 deletions src/main/resources/META-INF/resources/frontend/src/image-crop.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,82 +106,75 @@
const [outputMimeType] = hooks.useState<string>("outputMimeType");
const [outputQuality] = hooks.useState<number>("outputQuality", 1.0);

// Track previous image dimensions to adjust crop proportionally when resizing
const prevImgSize = useRef<{ width: number; height: number } | null>(null);
// Skip the first run of the output-format effect (initial encoding is handled on image load)
const didMountRef = useRef(false);

/**
* Handles intial calculations on image load.
* Converts a value expressed in source (natural) pixels to a percentage of
* the given dimension.
*/
const onImageLoad = () => {
if (imgRef.current) {
const { width, height } = imgRef.current;
prevImgSize.current = { width, height };
if (crop) {
const newcrop = centerCrop(
makeAspectCrop(
{
unit: crop.unit,
width: crop.width,
height: crop.height,
x: crop.x,
y: crop.y
},
aspect,
width,
height
),
width,
height
)
setCrop(newcrop);
this._updateCroppedImage(newcrop);
}
}
};
const toPercent = (value: number, dimension: number) =>
dimension ? (value / dimension) * 100 : 0;

/**
* Adjusts the crop size proportionally when the image is resized.
* Normalizes the configured crop when the image loads. The crop is kept as a
* percentage of the image's natural size, so both the on-screen selection and

@javier-godoy javier-godoy Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

toPercent returns 0 when dimension is falsy:

const toPercent = (value: number, dimension: number) =>
    dimension ? (value / dimension) * 100 : 0;

For an image whose naturalWidth/naturalHeight is 0 even after load fires (e.g. an SVG source without width/height/viewBox), all four toPercent calls in onImageLoad collapse to 0, producing a zero-size crop that gets fed into _updateCroppedImage — an empty/invalid exported image instead of the configured crop, with no error surfaced.

@javier-godoy javier-godoy Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This px→percent conversion is hand-rolled here and duplicated again in the useEffect below (normalizing a late setCrop). Consider sharing one helper (or using react-image-crop's own convertToPercentCrop, already available alongside convertToPixelCrop) so the two normalization paths can't drift out of sync — which is exactly what happened with the missing aspect-ratio step noted in the other comment.

* the exported image are independent of how the browser scales the image on
* screen. A configured "px" crop is interpreted as source (natural) pixels,
* which makes the exported size deterministic (see issue #33).
*/
const resizeCrop = (newWidth: number, newHeight: number) => {
if (!crop || !prevImgSize.current) return;
const { width: oldWidth, height: oldHeight } = prevImgSize.current;

const scaleX = newWidth / oldWidth;
const scaleY = newHeight / oldHeight;

const resizedCrop: Crop = {
unit: crop.unit,
width: crop.width * scaleX,
height: crop.height * scaleY,
x: crop.x * scaleX,
y: crop.y * scaleY,
};
const onImageLoad = () => {
const img = imgRef.current;
if (!img || !crop) {
return;
}
const { naturalWidth, naturalHeight } = img;

// Work in "%": a "px" crop is treated as source pixels and converted.
let normalized: PercentCrop = crop.unit === "%"
? { unit: "%", x: crop.x, y: crop.y, width: crop.width, height: crop.height }
: {
unit: "%",
x: toPercent(crop.x, naturalWidth),
y: toPercent(crop.y, naturalHeight),
width: toPercent(crop.width, naturalWidth),
height: toPercent(crop.height, naturalHeight),
};

// Enforce the aspect ratio when configured, then center the selection.
if (aspect) {
normalized = makeAspectCrop(
{ unit: "%", width: normalized.width, x: normalized.x, y: normalized.y },
aspect,
naturalWidth,
naturalHeight
);
}
normalized = centerCrop(normalized, naturalWidth, naturalHeight);

setCrop(resizedCrop);
prevImgSize.current = { width: newWidth, height: newHeight };
setCrop(normalized);
this._updateCroppedImage(normalized);
};
Comment thread
paodb marked this conversation as resolved.

/**
* Observes image resizing and updates crop size dynamically.
* Normalizes a programmatic "px" crop that the server sets after the image
* has loaded. onImageLoad only runs on the initial load, so without this a
* later setCrop("px", ...) would be rendered by ReactCrop as on-screen pixels
* and the selection box would diverge from the natural-pixel export (issue
* #33). The configured x/y are preserved (no centering) since the crop is

@javier-godoy javier-godoy Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unlike onImageLoad (which calls makeAspectCrop before centerCrop), this effect never re-applies the configured aspect when normalizing a late programmatic px crop.

Repro: configure aspect={1} with the image already loaded, then call setCrop(new Crop("px", 10, 10, 200, 50)) (non-square). onImageLoad won't re-run since the image is already loaded, so only this effect fires — it converts x/y/width/height to percent verbatim with no aspect enforcement, leaving the crop box (and exported image) non-square despite aspect=1, until the user manually drags a handle.

@javier-godoy javier-godoy Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This normalization always rewrites the crop's unit to "%", so getCrop() on the Java side can now return a different unit and different numeric values than what was passed to setCrop().

On master, onImageLoad preserved crop.unit verbatim, so a px crop stayed px after load. With this change, setCrop(new Crop("px", 100, 100, 300, 300)) followed by getCrop() (after load or any interaction) returns unit % with fractional values instead of the original px values — silently breaking any caller code that persists getCrop() and re-applies it later, or that branches on crop.unit().equals("px").

* explicitly positioned.
*/
useEffect(() => {
if (!imgRef.current) return;

const resizeObserver = new ResizeObserver(() => {
if (imgRef.current && prevImgSize.current) {
const { width, height } = imgRef.current;
if (width != prevImgSize.current.width &&
height != prevImgSize.current.height) {
resizeCrop(width, height);
}
}
});

resizeObserver.observe(imgRef.current);

return () => resizeObserver.disconnect();
const img = imgRef.current;
if (crop && crop.unit !== "%" && img && img.naturalWidth) {

Check warning on line 169 in src/main/resources/META-INF/resources/frontend/src/image-crop.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer using an optional chain expression instead, as it's more concise and easier to read. 


See more on https://sonarcloud.io/project/issues?id=FlowingCode_ImageCrop&issues=AZ-BSw0fLLrGSLJk3pd2&open=AZ-BSw0fLLrGSLJk3pd2&pullRequest=35
setCrop({
unit: "%",
x: toPercent(crop.x, img.naturalWidth),
y: toPercent(crop.y, img.naturalHeight),
width: toPercent(crop.width, img.naturalWidth),
height: toPercent(crop.height, img.naturalHeight),
});
}
}, [crop]);

/**
Expand All @@ -199,19 +192,22 @@
}
}, [outputMimeType, outputQuality]);

const onChange = (c: Crop) => {
setCrop(c);
// Keep the crop state in "%" so it stays valid regardless of the image's
// on-screen size; that scale-invariance is why no ResizeObserver is needed
// to rescale it on layout changes (see issue #33).
const onChange = (_pixelCrop: PixelCrop, percentCrop: PercentCrop) => {
setCrop(percentCrop);
};

const onComplete = (c: PixelCrop) => {
this._updateCroppedImage(c);
const onComplete = (_pixelCrop: PixelCrop, percentCrop: PercentCrop) => {
this._updateCroppedImage(percentCrop);
};

return (
<ReactCrop
crop={crop}
onChange={(c: Crop) => onChange(c)}
onComplete={(c: PixelCrop) => onComplete(c)}
onChange={(c: PixelCrop, pc: PercentCrop) => onChange(c, pc)}
onComplete={(c: PixelCrop, pc: PercentCrop) => onComplete(c, pc)}
circularCrop={circularCrop}
aspect={aspect}
keepSelection={keepSelection}
Expand Down Expand Up @@ -246,43 +242,34 @@
* Draws the selected crop region onto an off-screen canvas and dispatches the
* resulting data URI through a {@code cropped-image} event.
*
* <p>The crop rectangle reported by react-image-crop is expressed in the
* image's <em>displayed</em> (rendered) pixels, which can be smaller or larger
* than the image's intrinsic resolution when the browser scales it to fit the
* layout. The selected region is mapped back to the source's <em>natural</em>
* pixels using {@code scaleX}/{@code scaleY} for both the source rectangle and
* the output canvas, so the cropped image keeps the original resolution of the
* selected area rather than the (smaller or larger) on-screen size (see issue
* #26).</p>
* <p>The crop is mapped to the image's <em>natural</em> (intrinsic) pixels:
* {@code convertToPixelCrop} scales a {@code %} crop against
* {@code naturalWidth}/{@code naturalHeight}, while a {@code px} crop is taken
* as source (natural) pixels directly. Because the mapping never depends on the
* image's on-screen size, the exported dimensions are deterministic regardless
* of how the browser scaled the image when the crop was set (see issues #26 and
* #33).</p>
*
* <p>Note: a {@code px} crop is measured in rendered pixels, so the exported
* size is the rendered crop scaled to natural resolution, not necessarily the
* configured pixel value. The output is not multiplied by
* {@code window.devicePixelRatio}, so the original pixels are used verbatim
* instead of being upsampled on high-density displays (see issue #21).</p>
* <p>Note: the output is not multiplied by {@code window.devicePixelRatio}, so
* the original pixels are used verbatim instead of being upsampled on
* high-density displays (see issue #21).</p>
*/
public _updateCroppedImage(crop: PixelCrop|PercentCrop) {
const image = this.querySelector("img");
if (crop && image) {

crop = convertToPixelCrop(crop, image.width, image.height);
// Map the crop to the image's natural pixels. A "%" crop scales to the
// natural resolution; a "px" crop is interpreted as source pixels.
const ccrop = convertToPixelCrop(crop, image.naturalWidth, image.naturalHeight);

// create a canvas element to draw the cropped image
const canvas = document.createElement("canvas");

// draw the image on the canvas
const ccrop = crop;

// Ratio between the image's natural resolution and its displayed size.
// Greater than 1 when the image is scaled down to fit the screen.
const scaleX = image.naturalWidth / image.width;
const scaleY = image.naturalHeight / image.height;
const ctx = canvas.getContext("2d");

// Size the output in the crop region's natural pixels so the cropped
// The output is sized in the crop region's natural pixels, so the cropped
// image keeps the source's resolution rather than the on-screen size.
const outWidth = Math.round(ccrop.width * scaleX);
const outHeight = Math.round(ccrop.height * scaleY);
const outWidth = Math.round(ccrop.width);
const outHeight = Math.round(ccrop.height);

// Setting canvas dimensions resets the 2D context, so it must happen
// before any drawing/clipping state is configured below.
Expand All @@ -302,10 +289,10 @@

ctx.drawImage(
image,
ccrop.x * scaleX,
ccrop.y * scaleY,
ccrop.width * scaleX,
ccrop.height * scaleY,
ccrop.x,
ccrop.y,
ccrop.width,
ccrop.height,
0,
0,
outWidth,
Expand Down
Loading