Media Editor Modal: Try adding undo/redo for the image cropper#77782
Merged
andrewserong merged 10 commits intoMay 1, 2026
Conversation
andrewserong
added
[Feature] Media
|
Size Change: +909 B (+0.01%) Total Size: 7.87 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in 7f087c2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25152074602
|
ramonjd
reviewed
Apr 30, 2026
Member
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
This is a power feature, thanks for working on it. I love the direction - controls look good and just fit in ≤ 480px. I think they're important to show in mobile at least 👍🏻
I left some observations, hopefully they don't muddy the waters!
Comment on lines
+473
to
+476
| const isTextField = | ||
| target.tagName === 'INPUT' || | ||
| target.tagName === 'TEXTAREA' || | ||
| target.isContentEditable; |
Member
There was a problem hiding this comment.
Do you think this condition is related to what I'm seeing with the rangeslider? When the handle is focussed undo is blocked.
2026-04-30.11.39.12.mp4
Is there a way to limit it to text-editable inputs, and not input[type="range"] for example?
I'm seeing the same for the zoom control in the RHS panel, though I think the panel seems to be an exception as undo isn't happening at all from there.
Member
There was a problem hiding this comment.
I'm seeing the same for the zoom control in the RHS panel
Ah I think setZoom doesn't push history I think? Same with aspect ratio changes.
Technically, these are cropper edits, so should undo/redo should cover them?
Contributor
Author
There was a problem hiding this comment.
Technically, these are cropper edits, so should undo/redo should cover them?
Yes, they should. I think my change here was a little too naive (and vibe coded). The intention was to exclude undo while folks are editing metadata fields that are unrelated to cropping. As we add more fields that are related to cropping we'll need a better way to determine this. I'll have a play around.
| const pushToHistory = useCallback( () => { | ||
| const current = stateRef.current; | ||
| // Skip duplicate pushes (e.g. rapid discrete action on unchanged state). | ||
| if ( historyRef.current[ historyRef.current.length - 1 ] === current ) { |
Member
There was a problem hiding this comment.
This checks object references, but would some actions return new objects? E.g., Zoom slider sends the current zoom again. Wondering if we need something like areCropperStatesEqual
Contributor
Author
Not muddied at all, my push yesterday was really just a first pass "I think this direction might just work" so it's all useful feedback, thanks! |
… for undo/redo, improve keyboard handling and undo/redo from within input fields
ramonjd
reviewed
Apr 30, 2026
| ); | ||
|
|
||
| const settleCrop = useCallback( () => { | ||
| pushToHistory(); |
Member
There was a problem hiding this comment.
I'm wondering if we need pushToHistory in settleCrop at all. Settle is part of the gesture. Theoretically the gesture boundary already records the snapshot.
I'm still seeing some strange behaviour rolling back from a manual crop.
I had a look at my past research/commits, and compared what we're doing here and it's the right approach definitely. There was one suggestion that might preserve history integrity:
Drop eager pushToHistory() calls from setFlip, snapRotate90, settleCrop, applyOperation in use-cropper-state.ts and create one recording rule for everyone: anything that changes state lives between beginGesture and commitHistory
I'm just testing that small change locally to see if it's not lying to me 😄
Contributor
Author
There was a problem hiding this comment.
Oh, I should have said, this isn't quite ready for a re-review just yet! I found some bugs and am trying an approach that uses a debounced check against state, which I think might simplify things a bit.
Contributor
Author
There was a problem hiding this comment.
anything that changes state lives between beginGesture and commitHistory
But in principle, yes, I'm trying something in that general direction 😄
Member
There was a problem hiding this comment.
Oh okay, sorry for the premature testing! I'll back off 😅 I tried those suggestions out and it's working better locally.
2026-04-30.16.33.14.mp4
It also had to connect toolbar actions (Rotate Left, Rotate Right, Flip H, Flip V) with a recordedAction helper that wraps beginGesture > action > commitHistory.
Slider already uses useCropGestureHandlers so it was good.
Member
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment.
Oh okay, sorry for the premature testing! I'll back off 😅 I tried those suggestions out and it's working better locally.
No, no, I appreciate the testing and extra eyes! I just didn't want to waste your time, potentially 😄
And this could come in handy if the approach I'm trying winds up not working right!
|
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. |
Contributor
Author
|
Alrighty, I've switched this over to ready for review and updated the How section in the PR description. Thanks again as always for all the back and forth @ramonjd, much appreciated! What I've tried now is to have the main way we tell if we should add to the undo stack be a debounced checking of state changes. I think this simplifies things a fair bit, but we can still use gestureEnd or pointer up to short circuit the debounce and commit history immediately. I believe this is working okay for the rotation and flip buttons, and we've got the range controls available for the undo/redo stack (and cmd+z inside their fields) while still excluding metadata fields from it. All that said, I'm not 100% sure of the approach. It might feel brittle, and maybe some of the changes need to be grouped or combined? In any case, I'm generally happier with the current state of the PR than what I had previously as it feels like there's a bit less manual fiddling around to make sure the beginning of gestures, etc, are covered, and it was all getting a bit too buggy previously. But I'll give this another look with fresh eyes tomorrow 😄 |
Contributor
Author
|
The resize settle behaviour currently creates an extra entry in the undo stack, which is a bit unfortunate. I'll dig in today — ideally, that'd be covered by the debounced approach rather than immediately committing history after resizing. I'll continue hacking! |
Member
No worries! I was just testing that. What fixed it for me was flushing the pending debounce in |
…ips, add a function to check equality, and make Reset no longer clear the undo stack
ramonjd
reviewed
May 1, 2026
| ( direction: 1 | -1 ) => { | ||
| commitHistory(); | ||
| pushToHistory(); | ||
| suppressDebounceRef.current = true; |
Member
There was a problem hiding this comment.
A fly by observation, so take with salt.
Is the debounce to control continuous input, like where there are no clear gesture boundaries?
It feels strange that we have suppress debounce on every action to fight against batching. I wonder if it's needed though for toolbar actions like this that have a clear start and end.
I don't know, hence the questions 😄
Contributor
Author
There was a problem hiding this comment.
Here I'm mostly trying to keep things consistent for each of the actions. Something I ran into in an earlier iteration was having separate handling for each of the controls, which felt a bit inconsistent and brittle. Concrete examples are the input fields for fine grained rotation and zoom where a user can press and hold the up or down key to make adjustments (so should be debounced) or use the scroll wheel.
But another case for snap rotate or flip is that if someone's clicking through very quickly (either intentionally or accidentally), it's probably neater for it to be a debounced step rather than creating lots of tiny little undo steps 🤔
Member
There was a problem hiding this comment.
Is the debounce to control continuous input, like where there are no clear gesture boundaries?
I guess I'm stuck on the idea that every interaction in the editor has a natural start and end signal: the canvas exposes onGestureStart/onGestureEnd, and controls have their own start/end events etc.
So I'm a bit slow in understanding why the debounce juggling is required at all.
Member
There was a problem hiding this comment.
Sorry, I hit "Reply" before I saw your response!
if someone's clicking through very quickly (either intentionally or accidentally), it's probably neater for it to be a debounced step rather than creating lots of tiny little undo steps
I get you! Thanks for the explainer
Contributor
Author
There was a problem hiding this comment.
I get you! Thanks for the explainer
No worries! This is a slightly weird one because at first I didn't think the debounce juggling would be required at all. The approach I've ended up with was a result of "feel" of using the onGestureStart and onGestureEnd approach rather than something that's easy to read from the code, unfortunately.
While logically tracking the beginning and end of gestures seemed more appropriate, in practice there's lots of different kinds of gestures that folks end up doing, and that require debouncing anyway (i.e. the holding down a key, mousewheel etc). So in terms of feel of undo/redo I've gone the other direction of going mostly debounced.
I think in the longer-term we'll probably need to end up with something that does both in order for the undo/redo stack to feel properly stable. But for this PR, I think honing in on debounce probably gives us the simplest building blocks for now.
Contributor
Author
|
Alrighty, I think this is in decent shape as a proposal for the first round of undo/redo for the cropper. It gets rabbit-holey very quickly, so I'm leaning toward trying to build this in stages, with this as the first stage (obviously). In follow-ups, it'd be good to:
All that said, let me know if you think this isn't the right way to go for now! Happy to rework things if it feels off. |
Member
|
This is feeling/working really well. Thanks for the updates! 🚀 2026-05-01.10.57.57.mp4I noticed one slight regression with the way the dirty state works after a redo action. When performing a redo, then undoing back to the default state, the Reset button no longer detects that it's back at the starting state, and the Save button is active. The dirty state also affects the close confirmation. Kapture.2026-05-01.at.11.14.49.mp4The main concern I'd have is that we rely on the dirty state to save a new attachment via |
…, and some tests to cover the regression
Contributor
Author
|
Great catch @ramonjd! Yes, that's an important regression to fix up. I've pushed a fix: I think the issue was due to attempting to compare equality with floating point values. I've added a 2026-05-01.11.27.53.mp4Let me know if you notice any other weirdness! |
ramonjd
approved these changes
May 1, 2026
Member
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
Awesome work!
Functionality wise I think we've covered all the cases. I tested
- keyboard controls like crop/pan (plus
rfor rotate and+/-for zoom) - aspect ratio changes
- crop + settle
- reset/save/cancel dirty state detected LGTM
As you say, this this the first stage! But I think it's working pretty well from a user perspective.
I appreciate the back and forth. 🚢
Contributor
Author
Likewise, thanks for all the help here! It's often tricky with these ones where we're adding a new feature from scratch and it's hard to know exactly how it should work, but this is much better thanks to all the discussion 🙇 |
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:
Add undo/redo to the image cropper as part of the experimental media editor modal.
Why?
The particular way of doing undo/redo here is just tied to the image cropper, and doesn't include metadata. The reasoning here is that the metadata fields are already covered by browser undo behaviour for editing text fields, and shouldn't really be mixed in with the flow of editing crops in the canvas.
Also, the local approach to undo/redo means we don't muddy any entity state and everything can be tossed out when we close the modal.
If we need a more integrated approach further down the track, we can pursue it, but for now this seemed expedient.
How?
useCropperState.useCropGestureHandlers)Testing Instructions
Note: the buttons are currently in the footer, but I think they'd feel a bit more "canonical" somewhere in the header? Might look a bit awkward next to the "Edit media" title, so happy for any ideas there.
Screenshots or screencast
2026-04-29.16.49.49.mp4
Use of AI Tools
Claude Code