Media Editor Modal: Add an Advanced Panel with top/left and width/height controls#77778
Draft
andrewserong wants to merge 8 commits intotrunkfrom
Draft
Media Editor Modal: Add an Advanced Panel with top/left and width/height controls#77778andrewserong wants to merge 8 commits intotrunkfrom
andrewserong wants to merge 8 commits intotrunkfrom
Conversation
…crop position and width and height
andrewserong
added
[Feature] Media
Contributor
Author
|
@ramonjd just giving you an early ping while this is a draft in case you have any thoughts or concerns. It occurred to me that we might want to share some of the pixel conversion logic / try to make sure that the buildModifiers and advanced area are as close as possible to each other to avoid any surprises... Happy for any UX or technical feedback, this was pretty quickly thrown together 😄 |
andrewserong
changed the title
|
Size Change: +1.43 kB (+0.02%) Total Size: 7.82 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in a82212b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25144120833
|
Member
|
Looking good so far. Controls are in the right place IMO. I'm still testing, but one thing stood out: strange things happen when I input impossible numbers, like widths far beyond the available left constraint: 
I'm wondering what we need to be able to hook into the constraint here, or trigger a center and recalculation |
Contributor
Author
Oh, good catch! Yes, we should guard / constrain that... I'll have a play |
…y to the user, so that clamping appears to happen on blur
ramonjd
reviewed
Apr 29, 2026
Member
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
Do you think we need to expose any tools from the image editor?
Something like, answers to the questions "where are the crop boundaries?", "what's the current crop geometry in pixels?"
| checked={ freeformCrop } | ||
| onChange={ onFreeformChange } | ||
| /> | ||
| <CropAdvancedPanel |
Member
There was a problem hiding this comment.
Not for this PR, but do you think eventually the default crop controls should be in a collapsible component too (default open)?
Contributor
Author
There was a problem hiding this comment.
Yeah, I think once we've got this PR in, it'd be good to put our heads together on what other controls are missing, and how the overall area should look and feel 👍
| const handleChange = | ||
| ( field: 'x' | 'y' | 'width' | 'height' ) => | ||
| ( nextValue: string | undefined ) => { | ||
| const parsed = parseInt( nextValue ?? '', 10 ); |
Member
There was a problem hiding this comment.
I was also inputting non finite values like 100.22. Even with step={ 1 } it screws with the positioning a little.
Maybe we could use Number instead as the check and then reject non-finite:
const next = Number( nextValue );
if ( ! Number.isFinite( next ) ) {
return;
}|
|
||
| const pxSuffix = <InputControlSuffixWrapper>px</InputControlSuffixWrapper>; | ||
|
|
||
| export default function CropAdvancedPanel( { |
Member
There was a problem hiding this comment.
Dumb question: are we talking only about the cropping box for these values? E.g., the width of the rectangle (rather than the width of the underlying cropped area as a fraction of the original image width)
What should happen when freeform crop is off? Or when an aspect ratio is selected?
For the former and latter, I guess width/height don't make much sense. Should they be disabled? (?)
I guess I'm not sure what should happen - right now I can enter a value manually when freeform is off
Kapture.2026-04-29.at.14.57.29.mp4
Contributor
Author
There was a problem hiding this comment.
What should happen when freeform crop is off? Or when an aspect ratio is selected?
For the former and latter, I guess width/height don't make much sense. Should they be disabled? (?)
Or, maybe the input controls need to be clamped to the aspect ratio in that case. So if I update width, height automatically updates too, or if I update height, then width updates... I'll hack around.
Comment on lines
+70
to
+79
| const newRect = pixelsToCropRect( | ||
| { | ||
| x: field === 'x' ? parsed : x, | ||
| y: field === 'y' ? parsed : y, | ||
| width: field === 'width' ? parsed : width, | ||
| height: field === 'height' ? parsed : height, | ||
| }, | ||
| state, | ||
| imageSize | ||
| ); |
Member
There was a problem hiding this comment.
I wonder if the cropper API needs something like setCropPixelField
Contributor
Author
There was a problem hiding this comment.
Good question. For now, I was trying to see how far I can get without updating the cropper API... but if this is a common problem for consumers to deal with, then yeah maybe adding it the API might make sense 🤔
Contributor
Author
|
Alrighty, I've pushed some updates, and tried to not go down too many rabbitholes. The main changes:
2026-04-29.16.20.39.mp4I'll probably leave it here for today, as I've been staring at this one for too long, but will continue on with this tomorrow. Let me know if there's a particular path forward you think I should take! Separately to this PR but at some point I'd like to play around with the idea of the zoom control (that is, the one in the sidebar) controlling the zoom of the camera/world without adjusting the crop area. Similarly, to allow panning the camera/world separately from moving the crop area. Essentially allowing a way to browse the area a bit like we might in Figma/Photoshop/Affinity. Well out of scope for this PR, but if we manage to do that, then it would unlock more granular input controls. I think this is lower priority, though, and potentially a larger refactor, but just throwing it out there as an idea for the future Thanks again for all the suggestions! |
Member
|
Just a drive by smoke test. I can see the improvements! The values are more constrained and, though I can edit, they "snap" back if the crop area can't be widened or the aspect ratio is engaged. 👍🏻 The scroll wheel is a nice way to edit things too 😄 I just had to mentioned a couple of things for later -
Kapture.2026-04-29.at.16.50.03.mp4
Sounds like a neat feature to explore 👍🏻 Thanks again for persisting with this one! |
Contributor
Author
|
Thanks again! I'll dig into those points tomorrow 👍 |
…scroll zoom when input fields are focused, and ensure toggling freeform crop doesn't break the custom dimensions
…op was switched off
Contributor
Author
|
Alrighty, I've had a go at fixing some bugs and doing some updates in 1029031 and a82212b. They're slightly tentative fixes, but what I've tried to do is:
As always, happy to change course if this looks off (especially since the fixes are a little broader in scope than originally intended). I might switch over to the Undo/Redo PR for a bit after lunch as my eyes are a little tired from looking at this one this morning. Funny how these things are always more complex than you think they'll be! |
ramonjd
reviewed
Apr 30, 2026
| label={ __( 'Width' ) } | ||
| value={ pixels.width } | ||
| min={ 1 } | ||
| max={ Math.max( 1, maxRight - pixels.x ) } |
Member
There was a problem hiding this comment.
This is a rabbit hole! Thanks for going deep.
Manual resize has an implicit anchor because the user grabs a specific handle. But it looks like the advanced control's width field considers the left edge to be fixed.
That can be more restrictive than what the user can do manually - at the end of the vid, I'm trying to widen the crop area with the controls, then end up pulling it over.
2026-04-30.13.49.10.mp4
I'm wondering if it's worth time-boxing an API to provide answers to the question: "Given the current cropper state plus current canvas dimensions, tell me which crop operations I can perform and what ranges they can use."
It might be relevant for AI suggestions as well, e.g., a suggested crop can be validated/clamped against the same rules as manual interaction.
🤔
I might spend 20 mins taking a look - you've been dealing with the issue longer than I, so also happy to wait for your thoughts.
Contributor
Author
There was a problem hiding this comment.
I might spend 20 mins taking a look
Feel free to have a look if you think it's worth it! I don't have a strong opinion on it, but one of the reasons I think it'd be worth decoupling the limits from the current view of the canvas (the idea of being able to pan/zoom separately from cropping per se) is that it could (at least theoretically) simplify some of this.
So I'd be slightly hesitant about adding an API for what's allowed if it winds up baking in more restrictive logic than we want to have... on the other hand, encapsulating making changes via an API method would also make things simpler, so your idea has merit!
Contributor
Author
There was a problem hiding this comment.
Manual resize has an implicit anchor because the user grabs a specific handle. But it looks like the advanced control's width field considers the left edge to be fixed.
On this point, one idea I had was to nudge "Left" and "Top" lower if the values entered for width and height wind up being too high. When I tried hacking that in, I quickly got tangled in other bugs, so leaned toward trying to keep it as simple as I could in the shorter-term. I don't think I've succeeded in the "simple" part though 😄
Member
There was a problem hiding this comment.
adding an API for what's allowed if it winds up baking in more restrictive logic than we want to have
Good point. In my mind, the constraints would be dynamic according to the current state of the canvas, so it would only reflect what's possible at that point in time.
I'm not actually sure it'll be useful. I'll let you know soon :D
Member
There was a problem hiding this comment.
Seems to behave. Here's what's possible (built on top of your work)
Kapture.2026-04-30.at.14.46.21.mp4
In terms of API + consumer interaction, most of the magic is done via hook output values
value={ rect.left }
range={ leftRange }
disabled={ ! capabilities.canMoveX }
It does expand the image editor API with a "describe a crop operation" surface though 🤔
I guess we need to ask: should consumers need to know how crop bounds are calculated? Or should we just open up the values so they can do it themselves. I'm quite attracted to that idea since we don't know how users will want to use the values.
For consumers to do it reliably themselves (and do all the math), this is the minimum of what the image editor would need to expose:
type ConsumerCropGeometryInternals = {
state: {
cropRect: NormalizedRect;
pan: NormalizedPoint;
zoom: number;
rotation: number;
flip: {
horizontal: boolean;
vertical: boolean;
};
image: {
src: string;
naturalWidth: number;
naturalHeight: number;
} | null;
};
layout: {
canvasSize: Size;
elementSize: Size;
visualSize: Size;
cropBounds: {
minX: number;
minY: number;
maxX: number;
maxY: number;
} | null;
};
constraints: {
freeformCrop: boolean;
aspectRatio?: number;
minCropSize: number;
minZoom: number;
maxZoom: number;
};
coordinateSpace: {
rect: CropPixelRect;
sourceRegion: SourceRegion;
};
};
Contributor
Author
There was a problem hiding this comment.
Thanks for putting that up! What are your thoughts on the approach? While it abstracts away the math, at face value useCropGeometry seems a fairly complex API and means we have an additional API surface.
But on the other hand, it also gives us quite a bit of flexibility to control the cropper programatically. In particular, I really like how simple CropAdvancedPanel becomes. It feels much easier to read.
If we think of the root of media-editor as the consumer and image-editor as the cropping library, I do like the idea of the math being tucked away inside the library, so I'm liking the direction!
Member
There was a problem hiding this comment.
at face value useCropGeometry seems a fairly complex API and means we have an additional API surface.
This is my concern. Also I'm not yet sure about its scalability.
Do you think there could be a middle path? E.g., the base package exposes the values in the type above, and we abstract the math and associated hooks for consumption. Kinda like a "plugin".
I don't know! I'm just waffling.
I do think there should be a way for a human or robot to know if values they propose are valid within a certain range.
Member
There was a problem hiding this comment.
By the way, whichever way we go, I think what we've come up with so far has the potential to be more user friendly than Core's manual controls.
Contributor
Author
There was a problem hiding this comment.
Good questions, all of them.
Do you think there could be a middle path? E.g., the base package exposes the values in the type #77778 (comment), and we abstract the math and associated hooks for consumption. Kinda like a "plugin".
I think there could be. I guess a challenge if we're exposing more of the cropper's values is that that too becomes part of the API. Not necessarily a bad thing, especially if we want to add navigator-like controls in the future (free zoom / pan unrelated to cropping). Definitely worth exploring / playing with.
I do think there should be a way for a human or robot to know if values they propose are valid within a certain range.
Same here. And the advanced controls are an interesting case here. Because what we're finding in this discussion is that it isn't just about the user giving some pixel dimensions, it's also a means of programatic control of the overall cropper.
I think that's why this particular feature has grown some tentacles 😄
Definitely worth playing with some more. If we need to get something in quicker, I'd vote for going with something like the current state of this PR in the shorter-term as it doesn't really affect the API of what we're exposing from the image-cropper. But I do think that these questions of exposing the range / constraints of the cropper itself are worth solving either now or in follow-ups.
Happy to continue playing with this tomorrow!
Member
There was a problem hiding this comment.
If we need to get something in quicker, I'd vote for going with something like the current state of this PR in the shorter-term as it doesn't really affect the API of what we're exposing from the image-cropper.
That's a good point. We've got plenty of time to play around with things and test in order to get it right 👍🏻
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
2 participants
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:
To the Media Editor Modal, add an Advanced Panel with left/top and width/height input fields for granular control over the crop.
Why?
For both keyboard accessibility and fine-grained technical control over the crop. At the same time, these should be live values (controlled components) that also inform the user of the eventual pixel size of their crop.
How?
Add the controls, but also add some logic to determine the crop pixels and share it with
buildModifiers. The reason for consolidation is that we want to make sure that the values we're showing a user match as closely as possible to the actual pixel dimensions of the resulting cropped image.Testing Instructions
Screenshots or screencast
Use of AI Tools
Claude Code