Allow undo for image pasting and deleting (BL-16330)

[StephenMcConnel]

This change is 

[cubic-dev-ai]

2 issues found across 7 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[JohnThomson]

@JohnThomson reviewed 7 files and all commit messages, and made 10 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on StephenMcConnel).

---

a discussion (no related file):
I think I picked up a few things worth thinking about. I realize this is not meant to be a perfect way to handle this sort of Undo, but at least some of them may be worth considering.

---

src/BloomBrowserUI/bookEdit/workspaceRoot.ts line 94 at r2 (raw file):

const showAdjustTimingsDialogFromWorkspaceRoot = showAdjustTimingsDialog;

//Called by c# using workspaceBundle.handleUndo()

Is this comment no longer useful or no longer true?

---

src/BloomBrowserUI/bookEdit/workspaceRoot.ts line 110 at r2 (raw file):

    if (contentWindow && contentWindow.imageOperationCanUndo()) {
        contentWindow.imageOperationUndo();
        return;

The ordering here implies that an origami change will be undone before an image change, which will be undone before a typing or other ckeditor change, even if the actual order of events was different, which might be surprising.
I think an origami change will clear image changes (we restart canvas element editing). But then, later I question whether that should happen.
I didn't notice anything that would obviously clear the image stack if you type, but it may be implicit in the clearing when the current element changes. (But remember you can type in the caption of a navigation button. Every feature makes it harder to add others.)
Anyway, anything you can add here by way of comments explaining the sequence and anything that needs to be preserved to make the sequencing work would probably be useful.

---

src/BloomBrowserUI/bookEdit/js/bloomEditing.ts line 425 at r2 (raw file):

export function changeImage(imageInfo: IImageInfo) {
    if (imageInfo.undoable !== "true") {
        // Explicit image selection starts a new image-edit session; old image undo history should not carry over.

Is there some particular difficulty that would make us not want to support undoing choosing an image?

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 122 at r2 (raw file):

}

type ImageOperationUndoItem =

Could we put all the new stuff here somewhere else, maybe a new file? JohnH is very worried about this file being too big, and undoing image changes is not obviously a canvas element manager responsibility; also, there is a huge pending PR to refactor this class, so the less we have to change it, the better.

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 217 at r2 (raw file):

    }

    /** Commit the pending image operation undo after the replacement has actually happened. */

Could you say a bit more about why we don't just do this as part of a (possibly renamed) prepareUndoForImageOperation? Are there situations where we'd do the former but end up not wanting to do this? Just guarding against getting into a weird state if we throw while making the change?...

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 266 at r2 (raw file):

                        : (undoItem.element.getElementsByTagName("img")[0] ??
                          undefined);
                notifyToolOfChangedImage(img);

This might need tweaking if Undo can restore a cropped image. It probably resets cropping.

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 306 at r2 (raw file):

            creator: imageOrContainer.getAttribute("data-creator") ?? "",
            license: imageOrContainer.getAttribute("data-license") ?? "",
            undoable: "false", // we're not trying to undo something we've undone.

If the image we pasted over was cropped, ideally we should save the cropping data. The style of the image for sure, and it may be important to save the size and position of the containing canvas element, too.
Of course, that's more of a luxury...the user can re-crop...but it probably doesn't add much work?

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 1310 at r2 (raw file):

            this.activeElement !== element &&
            element &&
            (element.classList.contains(kBackgroundImageClass) ||

Do we need the check above? A background image will always have an image container, so I think it just confuses things to imply that background images are special.

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 6771 at r2 (raw file):

    };

    public initializeCanvasElementEditing(): void {

There's a few things that call this that we might not want to clear the undo stack. Switching to origami and back...IIRC, running a motion book preview...would it be better to just capture the page (ID?) as part of the Undo record, and then when we query the list or do anything with it, filter out any that aren't for the current page?
Or maybe, we just don't need to worry about page changes at all? Page changes currently result in a complete reload of the page iframe, so any pending Undo info may get automatically wiped.
Then again, maybe you thought all this through and all the things that call this really do want to clear the image undo stack...