Fix undo handling in the players and the metadata cells - #2170
Merged
Conversation
The player claimed Ctrl+Z without preventing its default action, so Chromium ran its own undo alongside ours. That undo is scoped to the frame, not to the focused element: it replayed the last edit made in the list search field, whose input handler clears the task selection and unmounts the side panel hosting the player. Reviewers saw their preview close instead of their stroke being undone. Consume the event whenever Ctrl/Cmd is held. Ctrl+Shift+Z is swallowed too, since the browser maps it to redo and it would reopen the same hole through that combo; it no longer undoes either, redo staying on Alt+R.
Browsers uppercase event.key when Shift is held, so the redo combo never reached the bare `case 'z'` a5229fca5 hardened: Ctrl+Shift+Z still ran the browser's redo, replayed the page's search field and closed the preview, the very bug that commit fixed. Windows binds redo to Ctrl+Y too, which was never handled at all. The test meant to pin the combo built an event no browser produces, so it passed against a case that never matched. Match the uppercase variants and Ctrl+Y, swallow them all, and undo only on z so redo stays on Alt+R. Ctrl+Z typed with caps lock on now undoes instead of doing nothing.
Chromium's undo stack belongs to the frame, not to the focused element: Ctrl+Z anywhere on the page rewrites the last edited metadata cell and fires a plain input event, indistinguishable from a keystroke. The handler then propagated that value to every selected entry, so a reflex Ctrl+Z after a multi-selection edit overwrote the other entries server side with the edited cell's former value. Bail out on historyUndo and historyRedo, restoring the stored value in the DOM so the field does not drift from the model.
b36e2ec rightly stopped replaying a history entry made on another frame, but it dropped both stacks to do it. One Ctrl+Z pressed a frame too far therefore threw away everything drawn before it: coming back to the right frame no longer undid anything, which reads as a dead Ctrl+Z. Skip the foreign entry instead of dropping it. It stays at the top of the stack and becomes replayable again once the user is back on its frame, so nothing is ever grafted onto the wrong frame and nothing is lost either. The annotation test helper now takes currentTime as a ref so a test can move the playhead between two calls.
…e one Entering fullscreen resizes the canvas, which reloads the annotation and replaces every live object with one rebuilt at the new scale. The undo entry still pointed at the instance created while drawing, whose left and top belong to the previous canvas box. Undo could look the object up by id and hit the live one, but redo cannot: the object is off the canvas by then, so it re-injected the stale instance and the stroke came back at its reduced-player position. Re-pin the entry on the instance actually replayed, on both sides. It is a no-op when the resolution already returned the stored object, so groups and unreloaded canvases are untouched. Only videos were affected in practice: a paused frame change reloads the annotation on every step, which pictures never do.
…k to An object waiting for its redo sits off the canvas, where no reload can reach it: it keeps the coordinates of the box it was last projected onto. Toggling fullscreen between the undo and the redo therefore brought the stroke back at the other box's scale, usually far outside the visible area, which reads as a redo that does nothing and as lost annotations. Stamp every history entry with the canvas box its object's coordinates belong to, and map the object onto the live box when the resolution had to fall back to the stored instance. Objects still on the canvas are already correct and take the untouched path.
undoLastAction cancels the opposite pending entry after replaying an action, redoLastAction did not. Drawing a stroke, undoing it then redoing it therefore left its id queued as an addition AND as a deletion for the same time. Zou applies additions, then updates, then deletions, and filters by id, so the batch created the annotation and erased it in the same request: the stroke stayed on screen until the next repaint from the store, then vanished for good. The bug predates today's work but was unreachable: Ctrl+Z only started working in the player with a5229fca5.
The share page has its own keydown handler and never claimed Ctrl+Z, so the browser's undo ran instead. Being scoped to the frame rather than to the focused element, it rewound the guest's comment draft long after the textarea lost focus, and never undid the stroke they had just drawn. Swallow every variant whatever the state, as the studio player does, and undo when the annotation mode is on. The overlay already had the undo wired to its toolbar button but did not expose it, and the player held no ref on it: a script setup component exposes nothing without defineExpose. Undo stays tied to the annotation mode rather than to hasChanges(): with the pencil off the toolbar is gone, so the key going quiet matches what the guest sees.
The studio player and the shared one had grown two copies of the same rule, already diverging, and neither could be tested without mounting a player. Extract undoRedoCommand next to isAltLetter, where the exported matching primitives live, and have both keydown handlers call it. It also closes two gaps both copies had. They matched on event.key alone, so on a non-Latin layout the press matched nothing and escaped to the browser, whose undo then rewrote a page text field; falling back to event.code there mirrors what isAltLetter already does, while the typed letter still wins on Latin layouts so the shortcut follows the printed cap on AZERTY and QWERTZ. And they swallowed Cmd+Y, which is the browser history on macOS and never a redo, so it is left alone now. Nine cases added, covering the combos on both platforms, caps lock, both layout families and the non-matching keys.
Delete went through usePreviewShortcuts (onDelete) and through the player's own keydown handler. Both listen on window in the bubble phase, on the same target, so both ran on every press. The second call deleted nothing: fabric discards the active object when it removes it, so getActiveObject() came back null. But deleteObject ends on an unconditional saveAnnotationsCb(), so each press re-serialized the whole canvas and armed a second save timer. Keep the composable wiring, which is the shared one, and correct the comment that still listed Delete among the player-specific keys.
deleteObject ran its trailing saveAnnotationsCb() even when called without an object, so every Delete press with an empty selection re-serialized the whole canvas and armed the save debounce. That save was not harmless: it created a phantom empty annotation entry when the frame had none, flipped notSaved for the 3s window (arming the beforeunload warning and dropping incoming socket annotation updates) and ended in an empty-batch PUT whose response rewrote the local annotations from server state. Bail out on a falsy object instead, collapsing the branch guards it made redundant. 866765c removed the duplicate handler that doubled this save; this removes the save itself. The undo and replay paths are unaffected: they guarantee a resolved object before calling in.
No component ever calls it, it only sits in the composable return. It was also broken: it iterated fabricCanvas._objects with forEach while deleteObject splices the same array through canvas.remove, so any future caller would have deleted every other object only.
Every mutation path ends on saveAnnotationsCb, whose synchronous part re-serializes the live canvas into the annotation entry. The erase branches of undo and redo were the only ones that did not, so the entry kept the stale mask, and every reload (fullscreen exit, paused frame step) rebuilds the canvas from that entry: the undone eraser path came back, and the next save re-serialized the reloaded canvas, making the re-erase permanent. Gesture that surfaced it: draw a stroke, enter fullscreen, two eraser strokes, Ctrl+Z, exit fullscreen. The save is skipped when no eraser path was actually popped or restored, matching the empty-selection delete guard. It also closes a smaller hole: an erase undo never armed the save window, so its diff could sit unflushed on unload without the not-saved warning.
The eraser counterpart of 94e2370: a fully erased object waits off the canvas for its undo, where no reload can re-scale it, so undoing it after a fullscreen toggle re-added it with the coordinates of the box it was erased on. Erased in the small player then undone in fullscreen, it came back too small near the top-left corner; the other way around it landed outside the visible area, reading as a dead Ctrl+Z. Stamp erase actions with the canvas box at creation, map the stored instance onto the live box when the undo falls back to it, and re-stamp after the replay. Redo deliberately does not re-stamp: it removes the live instance and never moves the stashed refs, whose coordinates still belong to the box the last undo left them on.
The spec was an empty test.skip and the mutation it names is a deliberate no-op in the store: there is nothing to assert, and the placeholder only put a permanent '1 skipped' in every test run.
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
1 participant
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.
Problem
Solution