Media Editor: Enforce a minimum crop size in the image editor#78268
Merged
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ramonjd
added
[Type] Bug
ramonjd
commented
May 14, 2026
| * enough to allow tight crops (favicons, icons). Adjust here to tune the | ||
| * floor globally. | ||
| */ | ||
| export const MIN_CROP_PIXELS = 24; |
Member
Author
There was a problem hiding this comment.
Up for debate. 16... 48...
Later it can be overridden by consumer
Contributor
There was a problem hiding this comment.
I wouldn't go any smaller for now. If we were to go even smaller, I feel like we'd want some kind of snap interface with grid lines as we're delving into pixel art territory 😄
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Media Editor image cropper to enforce a minimum crop size based on a fixed pixel floor (MIN_CROP_PIXELS), converted into per-axis normalized-space constraints so that minimum crop output stays consistent across small/large images and at 90°-step rotations.
Changes:
- Add a new global constant
MIN_CROP_PIXELS = 24and derive a per-axis normalizedminCropSizeinCropper. - Extend stencil resize math helpers to accept an optional per-axis
minCropSize(defaulting to the prior 5% behavior to preserve existing callers/tests). - Thread the new
minCropSizethroughStencilPropsandRectangleStencilinto the resize computations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/media-editor/src/image-editor/react/components/stencils/rectangle-stencil.tsx | Accepts minCropSize and forwards it to resize math helpers. |
| packages/media-editor/src/image-editor/react/components/cropper.tsx | Computes minCropSize from MIN_CROP_PIXELS and snap-rotation bbox; passes to stencil. |
| packages/media-editor/src/image-editor/core/types.ts | Extends StencilProps with optional minCropSize?: Size. |
| packages/media-editor/src/image-editor/core/stencil-math.ts | Replaces single scalar min with per-axis minimum support (with backward-compatible defaults). |
| packages/media-editor/src/image-editor/core/constants.ts | Introduces MIN_CROP_PIXELS constant as the single tuning knob for the pixel floor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
There's a slight edge case here: when you crop down to the minimum (currently 24px x 24px) and then change to an aspect ratio like Tall 9:16, it'll push the crop area below the minimum on one side. Aspect-ratio-at-high-zoom is a real edge case but a separate concern, and any fix is either surprising (reset zoom) or invasive (custom maxZoom math) - I tried both. Can deal with it later. The real bug was being able to zoom infinitely inwards to an invalid crop. |
|
Size Change: +87 B (0%) Total Size: 7.97 MB 📦 View Changed
ℹ️ View Unchanged
|
78 tasks
|
Flaky tests detected in cdd0c24. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/26011923753
|
ramonjd
force-pushed
the
fix/cropper-min-crop-size
branch
from
a1fb2f1 to
6a5828e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
packages/media-editor/src/image-editor/core/stencil-math.ts:100
- When
minCropSizeis larger than the remaining space to the bound on the dragged side (e.g.minCropSize.width === 1butedgeLeft > bounds.minX), the currentMath.max(edgeLeft + minCropSize.width, Math.min(..., bounds.maxX))logic can returnedgeRight > bounds.maxX(and similarly for bottom). This violates the function contract that edges are clamped tobounds. Consider clamping the effective min size to the available span and/or shifting the anchored edge so the rect still satisfies both the min size and the bounds constraints.
This issue also appears on line 198 of the same file.
Math.min( s.y + dy, edgeBottom - minCropSize.height )
);
}
if ( handle === 's' || handle === 'sw' || handle === 'se' ) {
edgeBottom = Math.max(
edgeTop + minCropSize.height,
Math.min( s.y + s.height + dy, bounds.maxY )
);
}
if ( handle === 'w' || handle === 'nw' || handle === 'sw' ) {
edgeLeft = Math.max(
bounds.minX,
Math.min( s.x + dx, edgeRight - minCropSize.width )
);
}
if ( handle === 'e' || handle === 'ne' || handle === 'se' ) {
edgeRight = Math.max(
edgeLeft + minCropSize.width,
Math.min( s.x + s.width + dx, bounds.maxX )
);
}
return {
x: edgeLeft,
packages/media-editor/src/image-editor/core/stencil-math.ts:215
computeLockedResizeRectclampsdistW/distHtomaxW/maxH, but then re-applies the minimum (minDistW/minDistH) afterwards. If the minimum is larger than the available space from the anchor to the bounds (possible now thatminCropSizecan reach 1.0), this can push the rect back outside the bounds. The minimum enforcement likely needs to be applied in a way that can’t exceedmaxW/maxH(e.g. cap the effective minimum to what fits, or adjust the anchor/position while preserving ratio).
const maxH = dirY > 0 ? bounds.maxY - anchorY : anchorY - bounds.minY;
if ( distW > maxW ) {
distW = maxW;
distH = distW / normalizedRatio;
}
if ( distH > maxH ) {
distH = maxH;
distW = distH * normalizedRatio;
}
// Enforce minimum after clamping.
distW = Math.max( distW, minDistW );
distH = Math.max( distH, minDistH );
// Compute the final rect position from the anchor.
const newX = dirX > 0 ? anchorX : anchorX - distW;
const newY = dirY > 0 ? anchorY : anchorY - distH;
The cropper previously allowed sub-pixel output crops on small source images because the minimum was hardcoded to 5% of the image in normalized space. Replace that with a per-axis pixel-based floor (MIN_CROP_PIXELS, default 24) derived from the source image's snap-rotated bounding box, so the same output-pixel floor applies regardless of source size, zoom, or 90° rotation. For sources smaller than the floor on an axis the minimum is capped at the full image.
cropRect is normalized in the viewport's snap-rotation bbox, so the captured source-pixel width is `cropRect.width * bbox.width / zoom`. The previous floor ignored zoom and only held at zoom=1: after a drag, SETTLE_CROP zooms in proportional to the shrink, and the next drag could shrink the cropRect to the same normalized minimum again — capturing progressively fewer source pixels each cycle. Multiply the normalized minimum by `state.zoom` so the source-pixel floor stays constant across SETTLE_CROP cycles and user-driven zoom.
Switching to a tall/narrow aspect ratio (e.g. 9:16) reshaped the crop via computeInscribedRect, which picks the largest centered rect in [0,1] viewport space. At high zoom (after SETTLE_CROP) that viewport represents very few source pixels, so the inscribed rect could fall below MIN_CROP_PIXELS on the narrow axis. Pass minCropSize into computeInscribedRect and scale the rect up on either axis until both satisfy the source-pixel floor, preserving the target ratio. At extreme zoom where the ratio can't fit, the rect is capped at the viewport edges and the ratio gives way to the floor.
Switching aspect ratios in freeform mode reshapes the crop to the largest inscribed rect of the new ratio. At high zoom (e.g. after a SETTLE_CROP) that inscribed rect could fall below the source-pixel floor on the narrow axis, and the previous fix capped it at the viewport edges — which broke the target aspect ratio. Compute the largest zoom at which the new ratio's inscribed rect still satisfies MIN_CROP_PIXELS on both axes, and lower the zoom (resetting pan) when the current zoom exceeds it. The inscribed rect is then a natural fit for the new ratio with no axis below the floor.
The previous fix lowered zoom *just enough* for the new ratio's
inscribed rect to satisfy the source-pixel floor, with bespoke math
(computeMaxZoomForRatio) and a scale-up-to-floor pass inside
computeInscribedRect. Either way, the prior crop coords are lost on
aspect-ratio change — so the extra machinery doesn't preserve user
intent. Trade the complexity for a plain reset: setZoomAtPoint(1,
{0,0}) before re-inscribing the rect. At zoom 1 the inscribed rect
has the full image to work with and is comfortably above the floor.
Also adds per-axis floor coverage to core/test/stencil-math.ts so the
new minCropSize behavior (free, locked with ratio projection, and
shift-locked edge handles) is exercised explicitly.
Resetting zoom and pan on aspect-ratio change is too surprising — it silently loses the user's framing. Restore the original behavior (reshape the crop to the largest inscribed rect at the current view). The aspect-ratio-at-high-zoom edge case is left as-is for a separate discussion; the source-pixel floor enforced during handle drags is the load-bearing fix for the reported sub-pixel cropping.
ramonjd
force-pushed
the
fix/cropper-min-crop-size
branch
from
6a5828e to
00f322d
Compare
SETTLE_CROP multiplied state.zoom by 1/cropRect.height to keep the framed content visible after a resize drag. For tight crops the multiplier was unbounded — a 24-source-px crop on a 4000-px image produced state.zoom ≈ 167, well past the user-facing MAX_ZOOM cap of 10. Subsequent wheel and slider zoom interactions then clamped to MAX_ZOOM and snapped state.zoom down on the first input, breaking smooth zoom in either direction. Cap the settle scale so the resulting zoom stays ≤ MAX_ZOOM. When the cap binds, the new crop is smaller than the viewport instead of filling it, but the source region the user framed is preserved (pan and cropRect both scale by the same capped factor). Zoom remains within the user-facing range so wheel and slider behave normally.
Member
Author
|
Testing this I found a niggling inconsistency between mouse zooming and the zoom slider. The zoom slider makes the image translate toward the nearest corner of the viewport as it zooms out. I'd expect the image to zoom out around the centre of the crop (matching wheel-zoom behavior when the cursor sits over the crop). Kapture.2026-05-18.at.13.41.11.mp4I'll do a follow up since it's visible in trunk. Edit: will be a follow up to this PR |
7 tasks
andrewserong
approved these changes
May 18, 2026
Contributor
There was a problem hiding this comment.
Testing nicely for me! The only thing I'd noticed while testing was that it could zoom past 10 and then when you go to mousewheel zoom or use the slider, it snaps to 10, and then you fixed it via cdd0c24 😄
LGTM, and good catch re: the origin/anchor for the follow-up 👍
| * enough to allow tight crops (favicons, icons). Adjust here to tune the | ||
| * floor globally. | ||
| */ | ||
| export const MIN_CROP_PIXELS = 24; |
Contributor
There was a problem hiding this comment.
I wouldn't go any smaller for now. If we were to go even smaller, I feel like we'd want some kind of snap interface with grid lines as we're delving into pixel art territory 😄
Member
Author
|
Thanks again for all the confidence checking 🙇🏻 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Part of
Adds a per-axis minimum crop size to the image editor cropper, expressed in source-image pixels via a single
MIN_CROP_PIXELSconstant (default24).Why
The previous floor was a hardcoded
MIN_CROP_SIZE = 0.05in normalized space (5% of the image), which meant:A pixel-based floor solves both: 24×24 output minimum is small enough for icon/favicon workflows and large enough that fumbled handle drags can't produce a useless tiny crop.
How
MIN_CROP_PIXELS = 24incore/constants.ts— single knob to tune the floor.stencil-math.tsresize helpers (computeFreeResizeRect,computeLockedResizeRect,computeShiftLockedResizeRect) take a per-axisminCropSize: Sizeparameter in normalized space. Default kept at{ 0.05, 0.05 }so existing tests pass unchanged.RectangleStencilaccepts a newminCropSizeprop and threads it through.CropperderivesminCropSizefromMIN_CROP_PIXELSand the snap-rotated source bbox (getRotatedBBox(naturalWidth, naturalHeight, snapRotation)), so the floor applies in output pixels and remains correct at 0°/90°/180°/270°.Behavior matrix
Test plan
npm test packages/media-editor— existingstencil-mathtests still pass (defaults preserved).