-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Media Editor Modal: Add an Advanced Panel with top/left and width/height controls #77778
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
7a585da
40acd1f
3888a23
75e346c
d74a860
b471032
1029031
a82212b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,245 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { | ||
| __experimentalInputControlSuffixWrapper as InputControlSuffixWrapper, | ||
| __experimentalNumberControl as NumberControl, | ||
| Flex, | ||
| FlexItem, | ||
| PanelBody, | ||
| } from '@wordpress/components'; | ||
| import { useMemo, useState } from '@wordpress/element'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { Stack } from '@wordpress/ui'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { useCropper } from '../../image-editor'; | ||
| import { | ||
| getCropPixels, | ||
| getReachableCropBoundsInPixels, | ||
| pixelsToCropRect, | ||
| } from '../../utils/crop-pixels'; | ||
|
|
||
| interface CropAdvancedPanelProps { | ||
| /** Resolved aspect ratio (width / height). When set, Width and Height inputs are linked. */ | ||
| aspectRatio?: number; | ||
| onPlacementControlInteraction?: () => void; | ||
| } | ||
|
|
||
| const pxSuffix = <InputControlSuffixWrapper>px</InputControlSuffixWrapper>; | ||
|
|
||
| interface CropInputProps { | ||
| label: string; | ||
| 'aria-label'?: string; | ||
| value: number; | ||
| min: number; | ||
| max: number; | ||
| onCommit: ( value: number ) => void; | ||
| } | ||
|
|
||
| // Shows a live draft while the user types, then snaps to the committed | ||
| // (enforced) value on blur or Enter — canvas updates in real-time without | ||
| // the input jumping to a constrained value on each keystroke. | ||
| function CropInput( { | ||
| label, | ||
| 'aria-label': ariaLabel, | ||
| value, | ||
| min, | ||
| max, | ||
| onCommit, | ||
| }: CropInputProps ) { | ||
| const [ focused, setFocused ] = useState( false ); | ||
| const [ draft, setDraft ] = useState( '' ); | ||
|
|
||
| const handleFocus = () => { | ||
| setFocused( true ); | ||
| setDraft( String( value ) ); | ||
| }; | ||
|
|
||
| const handleChange = ( v: string | undefined ) => { | ||
| setDraft( v ?? '' ); | ||
| const parsed = parseInt( v ?? '', 10 ); | ||
| if ( ! isNaN( parsed ) ) { | ||
| onCommit( Math.max( min, Math.min( parsed, max ) ) ); | ||
| } | ||
| }; | ||
|
|
||
| const commit = () => { | ||
| const parsed = parseInt( draft, 10 ); | ||
| if ( ! isNaN( parsed ) ) { | ||
| onCommit( Math.max( min, Math.min( parsed, max ) ) ); | ||
| } | ||
| }; | ||
|
|
||
| const handleBlur = () => { | ||
| setFocused( false ); | ||
| commit(); | ||
| }; | ||
|
|
||
| const handleKeyDown = ( | ||
| event: React.KeyboardEvent< HTMLInputElement > | ||
| ) => { | ||
| if ( event.key === 'Enter' ) { | ||
| setFocused( false ); | ||
| commit(); | ||
| event.currentTarget.blur(); | ||
| } else if ( event.key === 'Escape' ) { | ||
| setFocused( false ); | ||
| event.currentTarget.blur(); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <NumberControl | ||
| __next40pxDefaultSize | ||
| label={ label } | ||
| aria-label={ ariaLabel } | ||
| value={ focused ? draft : String( value ) } | ||
| min={ min } | ||
| max={ max } | ||
| step={ 1 } | ||
| onChange={ handleChange } | ||
| onFocus={ handleFocus } | ||
| onBlur={ handleBlur } | ||
| onKeyDown={ handleKeyDown } | ||
| suffix={ pxSuffix } | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| export default function CropAdvancedPanel( { | ||
| aspectRatio, | ||
| onPlacementControlInteraction, | ||
| }: CropAdvancedPanelProps ) { | ||
| const { state, setCropRect } = useCropper(); | ||
|
|
||
| const pixels = useMemo( () => { | ||
| if ( ! state.image ) { | ||
| return null; | ||
| } | ||
| const imageSize = { | ||
| width: state.image.naturalWidth, | ||
| height: state.image.naturalHeight, | ||
| }; | ||
| const raw = getCropPixels( state, imageSize ); | ||
| const reach = getReachableCropBoundsInPixels( state, imageSize ); | ||
| return { | ||
| x: Math.round( raw.x ), | ||
| y: Math.round( raw.y ), | ||
| width: Math.round( raw.width ), | ||
| height: Math.round( raw.height ), | ||
| minLeft: Math.max( 0, Math.floor( reach.minLeft - 0.5 ) ), | ||
| minTop: Math.max( 0, Math.floor( reach.minTop - 0.5 ) ), | ||
| maxRight: Math.floor( reach.maxRight ), | ||
| maxBottom: Math.floor( reach.maxBottom ), | ||
| }; | ||
| }, [ state ] ); | ||
|
|
||
| if ( ! pixels ) { | ||
| return null; | ||
| } | ||
|
|
||
| const { minLeft, minTop, maxRight, maxBottom } = pixels; | ||
|
|
||
| const handleApply = | ||
| ( field: 'x' | 'y' | 'width' | 'height' ) => ( clamped: number ) => { | ||
| if ( ! state.image ) { | ||
| return; | ||
| } | ||
| const imageSize = { | ||
| width: state.image.naturalWidth, | ||
| height: state.image.naturalHeight, | ||
| }; | ||
|
|
||
| // When an aspect ratio is locked, derive the paired dimension so | ||
| // both stay consistent. enforceContainment will clamp the result | ||
| // if it falls outside the valid crop bounds. | ||
| let newWidth = field === 'width' ? clamped : pixels.width; | ||
| let newHeight = field === 'height' ? clamped : pixels.height; | ||
| if ( aspectRatio && aspectRatio > 0 ) { | ||
| if ( field === 'width' ) { | ||
| newHeight = Math.max( | ||
| 1, | ||
| Math.round( clamped / aspectRatio ) | ||
| ); | ||
| } else if ( field === 'height' ) { | ||
| newWidth = Math.max( | ||
| 1, | ||
| Math.round( clamped * aspectRatio ) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| setCropRect( | ||
| pixelsToCropRect( | ||
| { | ||
| x: field === 'x' ? clamped : pixels.x, | ||
| y: field === 'y' ? clamped : pixels.y, | ||
| width: newWidth, | ||
| height: newHeight, | ||
| }, | ||
| state, | ||
| imageSize | ||
| ) | ||
| ); | ||
| onPlacementControlInteraction?.(); | ||
| }; | ||
|
|
||
| return ( | ||
| <PanelBody | ||
| title={ __( 'Advanced' ) } | ||
| initialOpen={ false } | ||
| className="media-editor-crop-advanced-panel" | ||
| > | ||
| <Stack direction="column" gap="sm"> | ||
| <Flex gap={ 2 } align="flex-start"> | ||
| <FlexItem isBlock> | ||
| <CropInput | ||
| label={ __( 'Left' ) } | ||
| aria-label={ __( 'Crop left position' ) } | ||
| value={ pixels.x } | ||
| min={ minLeft } | ||
| max={ Math.max( minLeft, maxRight - pixels.width ) } | ||
| onCommit={ handleApply( 'x' ) } | ||
| /> | ||
| </FlexItem> | ||
| <FlexItem isBlock> | ||
| <CropInput | ||
| label={ __( 'Top' ) } | ||
| aria-label={ __( 'Crop top position' ) } | ||
| value={ pixels.y } | ||
| min={ minTop } | ||
| max={ Math.max( | ||
| minTop, | ||
| maxBottom - pixels.height | ||
| ) } | ||
| onCommit={ handleApply( 'y' ) } | ||
| /> | ||
| </FlexItem> | ||
| </Flex> | ||
| <Flex gap={ 2 } align="flex-start"> | ||
| <FlexItem isBlock> | ||
| <CropInput | ||
| label={ __( 'Width' ) } | ||
| value={ pixels.width } | ||
| min={ 1 } | ||
| max={ Math.max( 1, maxRight - pixels.x ) } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.mp4I'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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems to behave. Here's what's possible (built on top of your work) Kapture.2026-04-30.at.14.46.21.mp4In terms of API + consumer interaction, most of the magic is done via hook output values 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for putting that up! What are your thoughts on the approach? While it abstracts away the math, at face value 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 If we think of the root of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good questions, all of them.
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.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's a good point. We've got plenty of time to play around with things and test in order to get it right 👍🏻 |
||
| onCommit={ handleApply( 'width' ) } | ||
| /> | ||
| </FlexItem> | ||
| <FlexItem isBlock> | ||
| <CropInput | ||
| label={ __( 'Height' ) } | ||
| value={ pixels.height } | ||
| min={ 1 } | ||
| max={ Math.max( 1, maxBottom - pixels.y ) } | ||
| onCommit={ handleApply( 'height' ) } | ||
| /> | ||
| </FlexItem> | ||
| </Flex> | ||
| </Stack> | ||
| </PanelBody> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import { | |
| ORIGINAL_ASPECT_RATIO, | ||
| } from '../../image-editor/core/constants'; | ||
| import type { AspectRatioPreset } from '../../image-editor/core/constants'; | ||
| import CropAdvancedPanel from './crop-advanced-panel'; | ||
|
|
||
| export interface MediaEditorCropPanelProps { | ||
| /** | ||
|
|
@@ -89,6 +90,15 @@ export default function MediaEditorCropPanel( { | |
| aspectRatioPresets, | ||
| }: MediaEditorCropPanelProps ) { | ||
| const { state, setZoom } = useCropper(); | ||
|
|
||
| const imageAspectRatio = state.image | ||
| ? state.image.naturalWidth / state.image.naturalHeight | ||
| : null; | ||
| const resolvedAspectRatio = resolveAspectRatio( | ||
| aspectRatioValue, | ||
| imageAspectRatio | ||
| ); | ||
|
|
||
| const aspectRatioOptions = [ | ||
| ...DEFAULT_ASPECT_RATIOS.filter( ( preset ) => preset.value <= 0 ), | ||
| ...( aspectRatioPresets ?? | ||
|
|
@@ -138,6 +148,10 @@ export default function MediaEditorCropPanel( { | |
| checked={ freeformCrop } | ||
| onChange={ onFreeformChange } | ||
| /> | ||
| <CropAdvancedPanel | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 👍 |
||
| aspectRatio={ resolvedAspectRatio } | ||
| onPlacementControlInteraction={ onPlacementControlInteraction } | ||
| /> | ||
| </Stack> | ||
| ); | ||
| } | ||

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.
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
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.
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.