Repository navigation
Conversation
|
Size Change: +2.96 kB (+0.04%) Total Size: 7.93 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in bf2183b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25420708931
|
ramonjd
force-pushed
the
try/image-editor-geometry-api
branch
from
May 1, 2026 07:59
4e43e85 to
cfd2c49
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a crop-geometry surface to the media editor image cropper so external controls can read current crop pixels + allowed bounds and validate/clamp proposed crop rectangles against those bounds.
Changes:
- Introduces core crop-geometry helpers (
getCropPixelRect,getCropPixelBounds,validateCropPixelRect, etc.) plus unit tests. - Publishes measured cropper layout geometry from
CropperviaCropperProvidercontext and exposes a newuseCropGeometry()hook. - Adds/adjusts Media Editor UI to include an Advanced crop panel and updates modifier-building to use the shared pixel-rect helper.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/media-editor/src/image-editor/react/hooks/use-crop-geometry.ts | New hook exposing a computed crop-geometry snapshot to consumers. |
| packages/media-editor/src/image-editor/react/hooks/test/use-crop-geometry.tsx | Hook tests for readiness + snapshot outputs. |
| packages/media-editor/src/image-editor/react/hooks/index.ts | Exports the new hook and related types. |
| packages/media-editor/src/image-editor/react/components/cropper.tsx | Publishes measured layout geometry/bounds via optional provider setter. |
| packages/media-editor/src/image-editor/react/components/cropper-provider.tsx | Adds geometry context + hooks to consume/publish layout geometry. |
| packages/media-editor/src/image-editor/index.ts | Exposes crop-geometry helpers and hook from the package root. |
| packages/media-editor/src/image-editor/core/crop-geometry.ts | New pure helpers for pixel rect conversion, bounds, clamp/validate, snapshot. |
| packages/media-editor/src/image-editor/core/test/crop-geometry.ts | Core tests for geometry conversion, bounds, and validation/clamping. |
| packages/media-editor/src/image-editor/core/index.ts | Re-exports crop-geometry helpers/types from core. |
| packages/media-editor/src/image-editor/README.md | Documents the new crop-geometry APIs. |
| packages/media-editor/src/components/media-editor-modal/build-modifiers.ts | Refactors to reuse getCropPixelRect for modifier math. |
| packages/media-editor/src/components/media-editor-modal/style.scss | Styles the new “Advanced” crop panel layout. |
| packages/media-editor/src/components/media-editor-crop-panel/index.tsx | Wires CropAdvancedPanel into the crop sidebar. |
| packages/media-editor/src/components/media-editor-crop-panel/crop-advanced-panel.tsx | Adds Advanced panel inputs driven by useCropGeometry() + validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramonjd
added
[Feature] Media
ramonjd
force-pushed
the
try/image-editor-geometry-api
branch
from
May 3, 2026 09:04
cfd2c49 to
b37a43e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ramonjd
force-pushed
the
try/image-editor-geometry-api
branch
from
May 4, 2026 01:20
e9f731e to
8d829a8
Compare
ramonjd
changed the title
ramonjd
marked this pull request as ready for review
May 4, 2026 01:26
|
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
commented
May 4, 2026
ramonjd
force-pushed
the
try/image-editor-geometry-api
branch
2 times, most recently
from
May 6, 2026 02:32
7781761 to
5a66057
Compare
andrewserong
reviewed
May 6, 2026
andrewserong
left a comment
andrewserong
left a comment
Contributor
There was a problem hiding this comment.
I'm liking the overall direction of this! Just a drive-by comment as I haven't dived deep into the code yet, but I noticed that I think there's a stale maximum or calculation value happening somewhere? If I change values fairly quickly, I can get the width control confused about what the maximum value can be. In the following screengrab the real maximum pixel size is 2560px, but I can get it to think the maximum is 2790px:
2026-05-06.13.03.12.mp4
Also, there seems to be a rounding issue possibly somewhere as sometimes it thinks the maximum is 2559px and left sometimes goes up to 1px. Hard to describe when it happens, though! 😅 If you can't reproduce this, I'll see if I can figure out when it occurs.
And unrelated to that logic, but it looks like Undo/Redo isn't covered yet. Do we need to add a useCropGestureHandlers(); call somewhere and apply it to the panel to ensure history state is covered?
Member
Author
Thanks for testing. I'm just keeping this PR up to date as we refine the main features mostly. Thanks for raising those bugs. Right now, I'm getting more test coverage in, so I'll track them down. I might have overstated this PR's usefulness to the new interaction modal (image bounds), but, more modestly put: #77899 creates the image-bounds constraint model and this PR exposes that model to consumers. 😃
I just wired that in in the last commit, but yeah, you need to click out of the control first, so that's another one 😄 |
ramonjd
force-pushed
the
try/image-editor-geometry-api
branch
from
May 6, 2026 04:55
5cff684 to
3accd16
Compare
Member
Author
|
Latest state of things: Kapture.2026-05-06.at.16.50.17.mp4Ran into a couple of issues: 1. Getting the right balance of interaction and commit history steps I would expect the canvas to update when I change the values manually. This works okay. However, we need to control when the undo state is updated. For example, onChange, or onBlur. It matters because we don't want to add I think I've reached a balance here, but it might need some further testing. 2. Combining fine rotation into the crop can display some non-intuitive bounds in the controls Example, when an image is fine rotated, it's possible that the left value is a minus number. It could be After fine rotation, the actual rotated image is no longer aligned to default "box". Its bounds can stick out to the left/top. So 
|
ramonjd
commented
May 6, 2026
| const [ focused, setFocused ] = useState( false ); | ||
| const [ draft, setDraft ] = useState( '' ); | ||
| const skipBlurCommitRef = useRef( false ); | ||
| const previewDelayRef = useRef< ReturnType< typeof setTimeout > >(); |
Member
Author
There was a problem hiding this comment.
previewDelayRef lets the component:
- cancel the previous pending commit when the user keeps typing,
- cancel the pending commit on blur or Enter before committing immediately,
- cancel the pending commit on Escape,
- clear the timer on unmount.
Member
Author
There was a problem hiding this comment.
The name is slightly stale now. It used to describe "previewing" the value after a delay, but with the current API it' is' really a delayed commit timer. A clearer name would be something like commitDelayRef or delayedCommitRef.
Contributor
For this one, rather than having to manage commit delays in the controls, I'm wondering if we could use the Would that then use the history's debounce instead of us needing to manage it in the advanced controls? If something like that works, we could retain immediate feedback in the canvas while the history state entries could still be debounced. You might have already tried that, so apologies if I'm missing something! It's mostly that it looks like we're doing a bunch of extra work in the advanced controls, and if we need to do that for them, then I'm wondering if it'll be hard for any custom extensions further down the track, too 🤔 |
andrewserong
reviewed
May 7, 2026
andrewserong
left a comment
andrewserong
left a comment
Contributor
There was a problem hiding this comment.
Nice progress! I've left a few non-urgent comments while I read through the code. Overall I think the additional utility functions are a good idea. In places it feels pretty complex for the consumer, so I was wondering if there's an opportunity to make some things internal.
In terms of UX, the three main things I ran into were:
- The delay when updating values, which might be avoided if we rely on a debounce in the history rather than in the UI components (the earlier comment I left)
- Sometimes I'm noticing a delay before the disabled state is applied on the X and Y fields, etc. Maybe related to the delayed commit?
- I'm still sometimes encountering the issue of the max value that's applied to the width field being incorrect. I wonder if that's a race condition due to the delays 🤔
Something I'm also wondering about is that the scope of this work has expanded a little from just adding the advanced controls. I suppose we could always break it down into separate parts if it proves tricky to coordinate all the changes at once. Just a thought in case this starts feeling challenging to wrangle!
Member
Author
You're right. It's definitely broader than advanced controls. The math bounds stuff might be useful, but I agree that we could focus a bit more here. The current state is working well, so I'll see if I can target a smaller PR using that logic and reduce the API impact. |
…d allowed crop bounds, then safely move or resize crops within constraints.
- Introduced new types and functions for managing crop pixel bounds and validation. - Updated `useCropGeometry` to return crop bounds and a snapshot of the current crop state. - Enhanced `CropAdvancedPanel` to utilize new crop geometry features, improving the user experience for resizing and moving crops. - Removed deprecated capabilities and operations, streamlining the crop geometry API. This refactor aims to provide a more robust and flexible cropping experience while ensuring that all operations respect the defined constraints.
- Introduced `CropPixelRectInput` type to represent consumer-proposed crop rectangles. - Updated `CropAdvancedPanel` to improve input handling and validation, ensuring better user experience during crop adjustments. - Refined the logic for committing crop values, allowing for more accurate clamping within defined bounds. - Enhanced documentation to clarify the usage of new types and functions related to crop geometry. These changes aim to streamline the cropping process and provide clearer guidelines for consumers interacting with crop geometry.
…ithout requiring raw layout sizes. Updated documentation for clarity and added a test case to verify this behavior.
- Removed `CropGeometryInput`, `getCropGeometrySnapshot`, and `getCropPixelLayoutBounds` from the API to streamline crop geometry handling. - Updated documentation to reflect these changes and clarify the current crop geometry functionality. These modifications aim to enhance the clarity and usability of the crop geometry API.
- Updated `CropAdvancedPanel` to utilize `applyOperation` for crop adjustments, enhancing the clarity of crop operations. - Replaced deprecated `setCropRect` with `applyOperation` to streamline the cropping process. - Adjusted the handling of crop geometry to use `imageBounds` instead of `bounds`, improving consistency across the API. - Enhanced documentation to reflect changes in crop geometry handling and clarify the usage of new types. These modifications aim to improve the user experience and maintainability of the crop geometry API.
- Introduced a new function to snap input values to the nearest integer, improving precision in crop adjustments. - Updated input bounds calculations to utilize the new snapping function, ensuring more accurate handling of near-integer values. - Refactored the commit logic to use refs for bounds and commit functions, optimizing performance during input changes. - Added tests to validate the behavior of crop bounds with near-integer values and ensure correct handling during delayed previews. These changes aim to improve the accuracy and responsiveness of the crop input system in the media editor.
- Added fine rotation input handling to allow users to make precise rotation adjustments. - Introduced functions to calculate and clamp fine rotation offsets, ensuring values stay within defined limits. - Updated the CropInput component to support a degree suffix for the fine rotation input. - Enhanced tests to validate the application of fine rotation changes and ensure clamping behavior works as expected. These changes improve the user experience by providing more control over rotation adjustments in the media editor.
- Introduced step and commitStep properties to CropInput for improved precision in value adjustments. - Added functions to snap input values to defined steps, ensuring consistent behavior during input changes. - Updated getInputBounds and getInputCommitValue functions to utilize the new snapping logic, enhancing input validation. - Enhanced tests to cover scenarios for out-of-range values and half-degree increments for fine rotation. These changes improve the accuracy and user experience of the crop input system in the media editor.
- Renamed debounce constants for clarity, changing INPUT_PREVIEW_DEBOUNCE_MS to INPUT_COMMIT_DEBOUNCE_MS. - Updated CropInput to manage delayed commits instead of previews, enhancing user experience during input changes. - Refactored related state management to track last committed draft values, ensuring accurate commit behavior. - Adjusted tests to reflect changes in input labeling for better clarity. These changes streamline the input handling process in the media editor, improving responsiveness and user interaction.
ramonjd
force-pushed
the
try/image-editor-geometry-api
branch
from
May 11, 2026 04:56
97c13e2 to
e31c3a4
Compare
ramonjd
commented
May 11, 2026
Comment on lines
+104
to
+123
| const leftRange = makeRange( | ||
| imageBounds.minLeft, | ||
| imageBounds.maxRight - rect.width | ||
| ); | ||
| const topRange = makeRange( | ||
| imageBounds.minTop, | ||
| imageBounds.maxBottom - rect.height | ||
| ); | ||
| const widthRange = getWidthRange( | ||
| rect, | ||
| imageBounds, | ||
| aspectRatio, | ||
| freeformCrop | ||
| ); | ||
| const heightRange = getHeightRange( | ||
| rect, | ||
| imageBounds, | ||
| aspectRatio, | ||
| freeformCrop | ||
| ); |
Member
Author
There was a problem hiding this comment.
Those dependencies are real because the controls are not generic number fields but bounded crop operations.
Future advanced controls don't need to know about all the logic inside CropInput, but they do need to provide their own range.
If we wanted to reduce panel noise, we could build in a hook, e.g.,
useCropAdvancedControlRanges( { rect, imageBounds, aspectRatio, freeformCrop, rotation, flip } ):
{
leftRange
topRange
widthRange
heightRange
fineRotationRange
fineRotationOffset
canMoveCropRect
}
Member
Author
|
The UX I've demanded here is deliberately tricky. The combo of preview-on-type, idle commit/clamp, single undo step per edit, Escape key rollback comes with an implementation cost. I think it'd be helpful to work backwards: forgetting the code for a moment, is the UX in this PR right, or close to it? Also is there anything we could do without that would simplify things? For example, the commit/settle If so, then we have a benchmark and try to refactor the concerns accordingly I reckon. |
Contributor
This is feeling really good to me. Nice work! The main things I like:
The things that sometimes feel slightly awkward:
2026-05-11.15.39.17.mp4But other than that this is feeling very nice to me. And I wonder if the above behaviour can be mitigated by upping the debounce window?
Great question. The UX here is feeling very polished, so I find it hard to pick something we could do without as it all feels very powerful. But if the values are updated via a debounce, then in principle it sounds like we mightn't need the If we find we need it after the fact, we could always look at adding it back in further down the track, too 🤔 Other than that, when it comes to the history/undo stack, I think the main thing we want to guard against is that if a user presses and holds a key we don't wind up with entries for
But yes, I think you've landed on a really good benchmark in terms of usability, I'm excited about this one! |
Member
Author
|
Firstly, thanks for the feedback @andrewserong Great to know we're heading in the right direction. I have a refactor PR in the oven which will tidy up the provider chain, and move things to testable hooks. Basically try to rebalance the code quality vs UX trade offs and remove 2 x structural smells: derived-state write-back from The hooks might be a lot different, so apologies in advance for that, but the general idea will be preserved.
Let me test that out! Worth a shot. |
Member
Author
I've been looking at this and I think you've uncovered a bug-in-waiting here. Here's one scenario:
Would bumping the debounce help? Kinda, but not really. Let's take 750ms or even 1s. We'd have a longer window to finish typing through the boundary but slow typers and people backspacing then thinking still hit it. Also a longer debounce makes the "I'm done" signal less responsive. I'm working on a new paradigm in (still testing) The cropper doesn't re-center mid-edit anymore, but will only re-center when you blur or press the Enter key. This lets you key in (or delete) numbers for precise values. Whatever you type in remains there. Out of range values will be ignored by the canvas, and the control values will self-update and the canvas will resettle when you leave the control. Same feature, same UX but the code lives in different places, plus it shifts the values into the one, existing provider instead of creating two. Sorry for the code fire hose here, it's a tricky thing to get right. Also, I don't think this feature is mission critical for the block editor version if it's getting too gnarly! |
Member
Author
If this works I can merge back to this branch or just retire this PR |
Contributor
UX-wise that feels just about perfect to me. It's a nice balance between optimistic rendering (really update the canvas when you can) and preserving the state of what the user is typing. Feel free to either merge into this branch or retire this PR, whichever is easier for you. I can follow along wherever you're doing the work 😄
Agreed. I think of it as a nice-to-have for the block editor / modal version 👍 |
Member
Author
I'll move over to #78187 for a clean slate Thanks for all the help. |
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:
Alternative to:
Adds an Advanced crop controls panel to the media editor crop sidebar.
The panel exposes manual controls for:
The controls update the crop preview immediately, while history commits are grouped so typing does not create one undo step per keystroke.
Kapture.2026-05-08.at.16.59.59.mp4
Why?
These controls provide precise crop adjustment for users who need pixel-level control, and give us a more reliable path for validating programmatic crop changes, including AI-generated crop instructions.
The bounds logic follows the current image-bounds constraint model, so values are clamped against the transformed image rather than the crop viewport.
Implementation notes
-45degto45deg.Testing
Known note
With fine rotation, X/Y values can sometimes be negative. These values are still valid: they are measured against the transformed image bounds, not the visible viewport.