fix: make px crops export at a deterministic size #35
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
| import { ReactAdapterElement, RenderHooks } from 'Frontend/generated/flow/ReactAdapter'; | ||
| import { JSXElementConstructor, ReactElement, useRef, useEffect } from "react"; | ||
| import React from 'react'; | ||
| import { type Crop, ReactCrop, PixelCrop, PercentCrop, makeAspectCrop, centerCrop, convertToPixelCrop, convertToPercentCrop } from "react-image-crop"; | ||
|
|
||
| // MIME types that HTMLCanvasElement.toDataURL can actually encode across browsers. | ||
| // Anything else silently falls back to image/png, so we never emit it. | ||
|
|
@@ -106,82 +106,88 @@ class ImageCropElement extends ReactAdapterElement { | |
| const [outputMimeType] = hooks.useState<string>("outputMimeType"); | ||
| const [outputQuality] = hooks.useState<number>("outputQuality", 1.0); | ||
|
|
||
| // Skip the first run of the output-format effect (initial encoding is handled on image load) | ||
| const didMountRef = useRef(false); | ||
|
|
||
| /** | ||
| * Normalizes a configured crop to a "%" crop of the image's natural size: a | ||
| * "px" crop is interpreted as source (natural) pixels, while a "%" crop is | ||
| * already resolution-independent and is returned unchanged. | ||
| * | ||
| * Returns null while the image has no intrinsic size (naturalWidth / | ||
| * naturalHeight are 0 before it loads, and stay 0 for a source without an | ||
| * intrinsic size), since every conversion against a zero dimension is | ||
| * meaningless. | ||
| */ | ||
| const toPercentCrop = (configured: Crop, img: HTMLImageElement): PercentCrop | null => { | ||
| const { naturalWidth, naturalHeight } = img; | ||
| if (!naturalWidth || !naturalHeight) { | ||
| return null; | ||
| } | ||
| return convertToPercentCrop(configured, naturalWidth, naturalHeight); | ||
| }; | ||
|
|
||
| /** | ||
| * Enforces the configured aspect ratio on a "%" crop by deriving its height | ||
| * from its width, leaving the position untouched. | ||
| * | ||
| * No-op when no aspect is configured: makeAspectCrop divides the width by the | ||
| * aspect, so passing an undefined one collapses the crop to zero. | ||
| */ | ||
| const applyAspect = (percentCrop: PercentCrop, img: HTMLImageElement): PercentCrop => | ||
| aspect | ||
| ? makeAspectCrop( | ||
| { unit: "%", width: percentCrop.width, x: percentCrop.x, y: percentCrop.y }, | ||
| aspect, | ||
| img.naturalWidth, | ||
| img.naturalHeight | ||
| ) | ||
| : percentCrop; | ||
|
|
||
| /** | ||
| * 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 | ||
|
Member
There was a problem hiding this comment. This px→percent conversion is hand-rolled here and duplicated again in the
Member
Author
There was a problem hiding this comment. Agreed, done in |
||
| * 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 onImageLoad = () => { | ||
| const img = imgRef.current; | ||
| if (!img || !crop) { | ||
| return; | ||
| } | ||
| // Work in "%": a "px" crop is treated as source pixels and converted. | ||
| let normalized = toPercentCrop(crop, img); | ||
| if (!normalized) { | ||
| return; | ||
| } | ||
| // Enforce the aspect ratio when configured, then center the selection. | ||
| normalized = applyAspect(normalized, img); | ||
| normalized = centerCrop(normalized, img.naturalWidth, img.naturalHeight); | ||
|
|
||
| setCrop(normalized); | ||
| this._updateCroppedImage(normalized); | ||
| }; | ||
|
|
||
| /** | ||
| * 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 | ||
|
Member
There was a problem hiding this comment. Unlike Repro: configure
Member
Author
There was a problem hiding this comment. Confirmed with that exact repro, and fixed in
Member
There was a problem hiding this comment. This normalization always rewrites the crop's unit to On
Member
Author
There was a problem hiding this comment. Confirmed: after this PR, One correction on the premise, though — What I do think is worth acting on is precision rather than the unit. Fixing that means changing |
||
| * explicitly positioned, but the aspect ratio is enforced just as it is on | ||
| * load, so a crop that does not match the configured aspect is corrected | ||
| * instead of staying off-ratio until the user drags a handle. | ||
| */ | ||
| useEffect(() => { | ||
| const img = imgRef.current; | ||
| if (!crop || crop.unit === "%" || !img) { | ||
| return; | ||
| } | ||
| const normalized = toPercentCrop(crop, img); | ||
| if (normalized) { | ||
| setCrop(applyAspect(normalized, img)); | ||
| } | ||
| }, [crop]); | ||
|
|
||
| /** | ||
|
|
@@ -199,19 +205,22 @@ class ImageCropElement extends ReactAdapterElement { | |
| } | ||
| }, [outputMimeType, outputQuality]); | ||
|
|
||
| // 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 = (_pixelCrop: PixelCrop, percentCrop: PercentCrop) => { | ||
| this._updateCroppedImage(percentCrop); | ||
| }; | ||
|
|
||
| return ( | ||
| <ReactCrop | ||
| crop={crop} | ||
| onChange={(c: PixelCrop, pc: PercentCrop) => onChange(c, pc)} | ||
| onComplete={(c: PixelCrop, pc: PercentCrop) => onComplete(c, pc)} | ||
| circularCrop={circularCrop} | ||
| aspect={aspect} | ||
| keepSelection={keepSelection} | ||
|
|
@@ -246,43 +255,44 @@ class ImageCropElement extends ReactAdapterElement { | |
| * 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 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: 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) { | ||
|
|
||
| // 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"); | ||
| const ctx = canvas.getContext("2d"); | ||
|
|
||
| // 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); | ||
| const outHeight = Math.round(ccrop.height); | ||
|
|
||
| // Nothing to export: the image has no intrinsic size (naturalWidth / | ||
| // naturalHeight are still 0 before it loads, and stay 0 for a source | ||
| // without an intrinsic size, which collapses any crop to zero) or the | ||
| // selection itself is empty. Bail out instead of drawing a 0x0 canvas | ||
| // and firing an event with a blank "data:," URI. | ||
| if (!Number.isFinite(outWidth) || !Number.isFinite(outHeight) | ||
| || outWidth <= 0 || outHeight <= 0) { | ||
| return; | ||
| } | ||
|
|
||
| // Setting canvas dimensions resets the 2D context, so it must happen | ||
| // before any drawing/clipping state is configured below. | ||
|
|
@@ -302,10 +312,10 @@ class ImageCropElement extends ReactAdapterElement { | |
|
|
||
| ctx.drawImage( | ||
| image, | ||
| ccrop.x, | ||
| ccrop.y, | ||
| ccrop.width, | ||
| ccrop.height, | ||
| 0, | ||
| 0, | ||
| outWidth, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toPercentreturns0whendimensionis falsy:For an image whose
naturalWidth/naturalHeightis0even afterloadfires (e.g. an SVG source withoutwidth/height/viewBox), all fourtoPercentcalls inonImageLoadcollapse to0, producing a zero-size crop that gets fed into_updateCroppedImage— an empty/invalid exported image instead of the configured crop, with no error surfaced.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid, fixed — though the guard belongs further down: with a zero
naturalWidth/naturalHeightany crop maps to zero,%included, so patchingtoPercentalone wouldn't have covered it._updateCroppedImagenow bails out instead of drawing a 0×0 canvas and firing a blankdata:,URI (4ec6120), and the normalization helper returns null while the image has no intrinsic size (4bed3ba).