BL-15441 add canvas origami element, use css for placeHolders

[nabalone]

This change is 

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch BL-15441_canvas_element

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nabalone]

Reviewable status: 0 of 40 files reviewed, 9 unresolved discussions

---

src/BloomBrowserUI/placeHolderImages.less line 32 at r1 (raw file):

}

// In a bloom-canvas that has canvas elements other than the background image, we want to

I'm not sure what to put in this file vs. in editMode.less and previewMode.less. Should this file have 1) everything related to placeHolderImages, so it's easy to see and modify behavior all at once or 2) only put behavior that is actually being used by both previewMode.less and editMode.less, so they can have class name collisions if they want, etc?

---

artwork/canvas-placeholder.svg line 0 at r1 (raw file):
Seems like this artwork folder is the place we put things for the record?

---

src/BloomBrowserUI/bookEdit/css/editMode.less line 1676 at r1 (raw file):

    .bloom-ui-canvas-element-side-handle,
    .bloom-ui-canvas-element-resize-handle,
    .bloom-ui-canvas-element-move-crop-handle {

What is a move crop handle? I'm finding one of them in the inspector, not sure it's visible or doing anything. Sometimes it is included in selectors and sometimes not. I don't understand what the desired behavior is

---

src/BloomBrowserUI/bookEdit/js/origami.ts line 21 at r1 (raw file):

export function setupOrigami() {
    getFeatureStatusAsync("widget").then((widgetFeatureStatus) => {
        getFeatureStatusAsync("canvas").then((canvasFeatureStatus) => {

Another level of nesting :( maybe I should switch to async/await syntax

---

src/BloomExe/Book/BookStorage.cs line 337 at r1 (raw file):

        /// Add any copies of the placeHolder.png image file left from older versions of Bloom to accumulator. We don't use them anymore.
        /// </summary>
        private static void AddUnusedPlaceholderImages(

Is it worth even doing this still? Should we just get rid of this method and let books originally from older versions carry around their unused placeholder images?

---

src/BloomBrowserUI/images/placeHolder.png line 0 at r1 (raw file):
For making thumbnails

---

src/BloomExe/BookThumbNailer.cs line 239 at r1 (raw file):

            var imageSrc = book.GetCoverImagePathAndElt(out SafeXmlElement coverImgElt);
            // We still use src="placeHolder.png" to indicate no image has been chosen, but we use css rather than a file to display it.
            // Detect this case and use a placeHolder file to make the thumbnail.

I know we wanted to decrease complexity, but we need to do some sort of special case handling in order to have the thumbnail in the collection tab be the same color as the preview pane. We could use just a blank transparent image instead, would that be better?

Also, we are unnecessarily running some image checks/processing below even in the placeholder case, but we were doing that before anyway and I need to wrap this card up so I didn't bother trying to make it more efficient

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 1320 at r1 (raw file):

                    }
                    const target = event.currentTarget as HTMLElement;
                    if (target.closest(`.bloom-image-control-frame-no-image`)) {

I've made it so you can't crop or resize any placeholder image, whether background or not

---

src/BloomExe/Book/Book.cs line 5191 at r1 (raw file):

            );
            // We no longer put placeHolder.png files in books (BL-15441) but we still need to detect when the placeholder
            // is called for, so here we return placeHolder.png instead of null. Callers of this method should handle this special case.

See BookThumbNailer.cs

[nabalone]

Wait I take that back. I think we decided to put the background-image on the bloom-canvasElement but I still have it on the bloom-backgroundImage. Still working

[Copilot]

Pull Request Overview

This PR implements a transition from using physical placeHolder.png files in books to using CSS-based placeholder images. It also adds support for a new "Canvas" origami element type alongside the existing "Image" element (renamed from "Picture").

Key Changes:

• Replaced physical placeHolder.png files with CSS-based placeholders using inline SVG data URLs
• Removed "Cut Image" functionality from the image context menu
• Added "Canvas" as a new origami element type with a distinct placeholder design
• Updated placeholder image cleanup logic to remove legacy placeHolder.png files from older Bloom versions

Reviewed Changes

Copilot reviewed 22 out of 40 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/BloomExe/web/controllers/EditingViewApi.cs	Removed HandleCutImage endpoint registration
src/BloomExe/web/controllers/BookMetadataApi.cs	Added logic to return empty string for cover image when it's placeHolder.png
src/BloomExe/Publish/Epub/EpubMaker.cs	Updated thumbnail handling to gracefully handle missing cover images
src/BloomExe/ImageProcessing/ImageUtils.cs	Updated comment explaining placeholder handling in BL-15441
src/BloomExe/Edit/EditingView.cs	Removed OnCutImage method implementation
src/BloomExe/Edit/EditingModel.cs	Updated comment about video placeholder (removed reference to placeHolder.png)
src/BloomExe/BookThumbNailer.cs	Added logic to use centralized placeHolder.png for thumbnails when src is placeHolder.png
src/BloomExe/Book/RuntimeInformationInjector.cs	Removed CutImage translation string
src/BloomExe/Book/BookStorage.cs	Simplified cleanup to remove all placeHolder*.png files as they're now legacy
src/BloomExe/Book/Book.cs	Added check to skip alt text for placeHolder.png images
src/content/templates/bloom-foundation-mixins.pug	Removed alt attribute from placeholder images
src/BloomBrowserUI/placeHolderImages.less	New stylesheet defining CSS-based placeholder images with SVG backgrounds
src/BloomBrowserUI/collectionsTab/collectionsTabBookPane/previewMode.less	Updated hidePlaceHolders to hide background-image instead of img element
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts	Exported kTalkingBookToolId constant
src/BloomBrowserUI/bookEdit/js/origami.ts	Added Canvas element creation alongside Image (renamed from Picture)
src/BloomBrowserUI/bookEdit/js/bloomImages.ts	Added early return for placeHolder.png in SetAlternateTextOnImages
src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts	Added click handler to show canvas tool for canvas elements
src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx	Disabled Copy and Edit Metadata menu items for placeholder images
src/BloomBrowserUI/bookEdit/css/editMode.less	Updated placeholder hiding rules to use background-image instead of img display
src/BloomBrowserUI/images/placeHolder.png	Added new binary PNG file for central placeholder image
artwork/placeholder.svg	Simplified SVG to single-line minified version
artwork/canvas-placeholder.svg	New SVG for canvas-specific placeholder (easel design)
DistFiles/localization/en/Bloom.xlf	Added localization entries for "Image" and "Canvas", marked "Picture" as obsolete

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The argument to MakeManifest should be a relative path (filename with folder), not a full path. Line 554 now passes epubThumbnailImagePath which is a full path (e.g., from Path.Combine(...)), while the original code passed kImagesFolder + "/" + Path.GetFileName(epubThumbnailImagePath).

Inside MakeManifest:

• Line 749 uses Path.GetFileNameWithoutExtension(coverPageImageFile)
• Line 823 compares item == coverPageImageFile against items in _manifestItems (which are relative paths)

This will break the cover image matching. The fix should be:

MakeManifest(thumbnailFileExists ? kImagesFolder + "/" + Path.GetFileName(epubThumbnailImagePath) : null);

Suggested change

	MakeManifest(thumbnailFileExists ? epubThumbnailImagePath : null);
	MakeManifest(thumbnailFileExists ? kImagesFolder + "/" + Path.GetFileName(epubThumbnailImagePath) : null);

[nabalone]

Done

[Copilot]

The path separator is hardcoded as \\ which is Windows-specific. This could cause issues on non-Windows platforms. Use Path.Combine or Path.DirectorySeparatorChar for cross-platform compatibility:

imageSrc = Path.Combine(bloomRoot, "images", "placeHolder.png");

Suggested change

	imageSrc = Path.Combine(bloomRoot, "images\\placeHolder.png");
	imageSrc = Path.Combine(bloomRoot, "images", "placeHolder.png");

[nabalone]

Reviewable status: 0 of 58 files reviewed, 17 unresolved discussions (waiting on @copilot)

---

src/BloomBrowserUI/placeHolderImages.less line 0 at r4 (raw file):
TODO still going to refactor this file to condense with mixins

---

src/BloomExe/web/BloomServer.cs line 751 at r4 (raw file):

                    else if (ImageUtils.IsPlaceholderImageFilename(imageFile))
                    {
                        return true;

Not sure this is right

---

src/BloomExe/web/BloomServer.cs line 813 at r4 (raw file):

                // thumbnail requests have the thumbnail parameter set in the query string
                var thumb = info.GetQueryParameters()["thumbnail"] != null;
                //TODO erroring here

I still need to look at how and why we get here sometimes with placeholders

---

src/BloomBrowserUI/bookEdit/js/bloomImages.ts line 44 at r4 (raw file):

        return false;
    }
    return filename.toLowerCase().endsWith("placeholder.png");

When checking for these placeHolder, looks like we have tended to use startsWith in typescript and endsWith in c#. I think we should use endsWith everywhere

[nabalone]

Done

[Copilot]

@nabalone I've opened a new pull request, #7510, to work on those changes. Once the pull request is ready, I'll request review from you.

[andrew-polk]

@andrew-polk reviewed 25 of 40 files at r1, 2 of 5 files at r2, 28 of 29 files at r4, all commit messages.
@andrew-polk dismissed @copilot-pull-request-reviewer[bot] from 2 discussions.
Reviewable status: 55 of 58 files reviewed, 23 unresolved discussions (waiting on @copilot and @nabalone)

---

artwork/canvas-placeholder.svg line at r1 (raw file):

Previously, nabalone (Noel) wrote…

Seems like this artwork folder is the place we put things for the record?

If it is just a copy of the one we are putting elsewhere in code, we don't need a second copy.
I think artwork would be for something like an original svg from which we create a png or similar.
Something which is not checked in elsewhere but which we may want to have again in the future as a source.

(So I think you can remove both from artwork if I'm right that they are just copies)

---

src/BloomBrowserUI/bookEdit/css/editMode.less line 1676 at r1 (raw file):

Previously, nabalone (Noel) wrote…

What is a move crop handle? I'm finding one of them in the inspector, not sure it's visible or doing anything. Sometimes it is included in selectors and sometimes not. I don't understand what the desired behavior is

Is it the middle circle which is only useable (visible?) once an image is cropped?

---

src/BloomBrowserUI/bookEdit/js/bloomImages.ts line 44 at r4 (raw file):

Previously, nabalone (Noel) wrote…

When checking for these placeHolder, looks like we have tended to use startsWith in typescript and endsWith in c#. I think we should use endsWith everywhere

Previous logic in this file was contains, not starts or ends with. I could imagine a src something like placeholder.png?cachebust=12345 though I don't know if we ever do that... but I don't know why else we might have startsWith.

Also, the param in this function is filename, but you're passing a url. Or at least that's what the local callers call it.

---

src/BloomExe/BookThumbNailer.cs line 239 at r1 (raw file):

I know we wanted to decrease complexity, but we need to do some sort of special case handling in order to have the thumbnail in the collection tab be the same color as the preview pane. We could use just a blank transparent image instead, would that be better?

I think we decided we want the flower for the book thumbnail so it matches the preview. So this seems correct.

Also, we are unnecessarily running some image checks/processing below even in the placeholder case, but we were doing that before anyway and I need to wrap this card up so I didn't bother trying to make it more efficient

Reducing scope of this card is ok. While we are aware of it, let's add an // Enhance: comment.

---

src/BloomExe/Book/Book.cs line 5191 at r1 (raw file):

Previously, nabalone (Noel) wrote…

See BookThumbNailer.cs

I think this would be clearer as

            // We no longer put placeHolder.png files in books (BL-15441) but we still need to detect when the placeholder
            // is called for, so here we return placeHolder.png instead of null. Callers of this method should handle this special case.
            if (ImageUtils.IsPlaceholderImageFilename(coverImagePath))
                return coverImagePath;

            if (!RobustFile.Exists(coverImagePath))
            {
...

---

src/BloomExe/web/BloomServer.cs line 751 at r4 (raw file):

Previously, nabalone (Noel) wrote…

Not sure this is right

I would have thought this would cause an error on the client side saying it never got a response.
I think we always have to respond with something.

---

src/BloomBrowserUI/bookEdit/css/editMode.less line 675 at r4 (raw file):

        }
    }
}

Did this move somewhere?

(I think we still want to show the placeholder iff background is selected. Or am I missing something?)

---

src/BloomExe/Book/RuntimeInformationInjector.cs line 329 at r4 (raw file):

            );
            AddTranslationToDictionaryUsingKey(d, "EditTab.Image.CopyImage", "Copy image");
            AddTranslationToDictionaryUsingKey(d, "EditTab.Image.CutImage", "Cut image");

Can/should we mark this string as obsolete in the xlf?

---

src/BloomTests/Publish/EpubValidAndAccessible.cs line 539 at r4 (raw file):

                    "package/manifest/item[@properties='cover-image']",
                    1
                );

Is there still a test somewhere which validates this for the real thumbnail case?

Should we have a test which validates that it isn't there for the no-thumbnail case?

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 731 at r4 (raw file):

            {
                // As of BL-15441, we don't use real files for place holders but we still mark them with src="placeHolder.png".
                // Don't strip these - we want them to persist in template books, and for other books we are already removing them elsewhere.

This comment could be clarified a little.
Something like

                // As of BL-15441, we don't use real files for place holders (except in templates) but we still mark them with src="placeHolder.png".
                // Don't strip them here -- we want them to persist in template books. For other books we are already removing them elsewhere.

(and maybe state where elsewhere is?)

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 2697 at r4 (raw file):

            ) {
                // Image is in an error state; we probably won't ever get useful dimensions. Just leave
                // the canvas element the shape it is.

Comment is out of date (add placeholder case besides error state)

---

src/BloomTests/Book/BookStorageTests.cs line 778 at r4 (raw file):

            Assert.IsTrue(File.Exists(p2.Path));
            Assert.IsTrue(File.Exists(p3.Path));
            Assert.IsFalse(File.Exists(dropmeTemp.Path));

Did we add a test somewhere to show they are removed as desired?

---

DistFiles/localization/en/Bloom.xlf line 1360 at r4 (raw file):

        <note>Obsolete as of 6.3</note>
      </trans-unit>
      <trans-unit id="EditTab.CustomPage.Image" sil:dynamic="true">

Let's add translate="no" to these new ones.

---

src/BloomBrowserUI/bookEdit/css/editMode.less line 1382 at r4 (raw file):

    &.bloom-backgroundImage-control-frame {
        .bloom-ui-canvas-element-resize-handle,
        .bloom-ui-canvas-element-side-handle {

Unless I'm missing something, adding this means we can no longer crop background images.
We do want to be able to crop non-placeholder background images.

[Copilot]

@andrew-polk I've opened a new pull request, #7511, to work on those changes. Once the pull request is ready, I'll request review from you.

[andrew-polk]

@andrew-polk reviewed 2 of 40 files at r1, 1 of 29 files at r4.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @nabalone)

---

src/BloomBrowserUI/placeHolderImages.less line 32 at r1 (raw file):
Yeah, I understand your dilemma.
Tough call.
I guess I would lean toward consolidating the logic in one place unless it is a selector AND rule which are very specific to only one mode and really don't apply in any way to the other mode.
That's pretty vague, I know. But what you've done so far looks pretty reasonable.

For example, I don't think I would split out the origami selector above into editMode since there are two selectors with one rule and the other selector is not specific to editmode.

However, I don't understand what this means:

so they can have class name collisions if they want

so I'm not sure I'm fully addressing your question.

---

src/BloomBrowserUI/bookEdit/js/origami.ts line 21 at r1 (raw file):

Previously, nabalone (Noel) wrote…

Another level of nesting :( maybe I should switch to async/await syntax

It does seem like await would be cleaner.
But let's just leave it.

---

src/BloomExe/Book/BookStorage.cs line 337 at r1 (raw file):

Previously, nabalone (Noel) wrote…

Is it worth even doing this still? Should we just get rid of this method and let books originally from older versions carry around their unused placeholder images?

I think it is worth removing them.
I forget... what did we say about templates? Are they going to retain their placeholder.png files? If yes, what does that mean for this?

---

src/BloomBrowserUI/placeHolderImages.less line 22 at r4 (raw file):

// when previewing templates and in Change Layout mode, img elements may not be wrapped in canvas-element divs (see BL-15514). Also below.
.preview:not([data-l1]),

This not([data-l1] warrants a comment

[nabalone]

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @andrew-polk)

---

artwork/canvas-placeholder.svg line at r1 (raw file):

Previously, andrew-polk wrote…

If it is just a copy of the one we are putting elsewhere in code, we don't need a second copy.
I think artwork would be for something like an original svg from which we create a png or similar.
Something which is not checked in elsewhere but which we may want to have again in the future as a source.

(So I think you can remove both from artwork if I'm right that they are just copies)

These are the svgs that are embedded inline in placeHolderImages.less, they aren't check in as standalone files anywhere else

---

DistFiles/localization/en/Bloom.xlf line 1360 at r4 (raw file):

Previously, andrew-polk wrote…

Let's add translate="no" to these new ones.

Done.

---

src/BloomBrowserUI/placeHolderImages.less line 22 at r4 (raw file):

Previously, andrew-polk wrote…

This not([data-l1] warrants a comment

Done.

---

src/BloomBrowserUI/bookEdit/css/editMode.less line 1676 at r1 (raw file):

Previously, andrew-polk wrote…

Is it the middle circle which is only useable (visible?) once an image is cropped?

Ah yes thanks

---

src/BloomBrowserUI/bookEdit/css/editMode.less line 675 at r4 (raw file):

Previously, andrew-polk wrote…

Did this move somewhere?

(I think we still want to show the placeholder iff background is selected. Or am I missing something?)

moved to placeHolderImages.less

---

src/BloomBrowserUI/bookEdit/css/editMode.less line 1382 at r4 (raw file):

Previously, andrew-polk wrote…

Unless I'm missing something, adding this means we can no longer crop background images.
We do want to be able to crop non-placeholder background images.

Done. Oops. Was getting overriden anyways.

---

src/BloomBrowserUI/bookEdit/js/bloomImages.ts line 44 at r4 (raw file):

Previously, andrew-polk wrote…

Previous logic in this file was contains, not starts or ends with. I could imagine a src something like placeholder.png?cachebust=12345 though I don't know if we ever do that... but I don't know why else we might have startsWith.

Also, the param in this function is filename, but you're passing a url. Or at least that's what the local callers call it.

Done.

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 1320 at r1 (raw file):

Previously, nabalone (Noel) wrote…

I've made it so you can't crop or resize any placeholder image, whether background or not

Actually now I am only preventing cropping of placeholder images, resizing works as expected

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 2697 at r4 (raw file):

Previously, andrew-polk wrote…

Comment is out of date (add placeholder case besides error state)

Done.

---

src/BloomExe/BookThumbNailer.cs line 239 at r1 (raw file):

Previously, andrew-polk wrote…

I know we wanted to decrease complexity, but we need to do some sort of special case handling in order to have the thumbnail in the collection tab be the same color as the preview pane. We could use just a blank transparent image instead, would that be better?

I think we decided we want the flower for the book thumbnail so it matches the preview. So this seems correct.

Also, we are unnecessarily running some image checks/processing below even in the placeholder case, but we were doing that before anyway and I need to wrap this card up so I didn't bother trying to make it more efficient

Reducing scope of this card is ok. While we are aware of it, let's add an // Enhance: comment.

Done

---

src/BloomExe/Book/Book.cs line 5191 at r1 (raw file):

Previously, andrew-polk wrote…

I think this would be clearer as

            // We no longer put placeHolder.png files in books (BL-15441) but we still need to detect when the placeholder
            // is called for, so here we return placeHolder.png instead of null. Callers of this method should handle this special case.
            if (ImageUtils.IsPlaceholderImageFilename(coverImagePath))
                return coverImagePath;

            if (!RobustFile.Exists(coverImagePath))
            {
...

Done

---

src/BloomExe/Book/BookStorage.cs line 337 at r1 (raw file):

Previously, andrew-polk wrote…

I think it is worth removing them.
I forget... what did we say about templates? Are they going to retain their placeholder.png files? If yes, what does that mean for this?

We now have a css rule to put background-image on template book bloompubs, so we don't need the placeHolder.png file in template books either.

---

src/BloomExe/Book/RuntimeInformationInjector.cs line 329 at r4 (raw file):

Previously, andrew-polk wrote…

Can/should we mark this string as obsolete in the xlf?

Done.

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 731 at r4 (raw file):

Previously, andrew-polk wrote…

This comment could be clarified a little.
Something like

                // As of BL-15441, we don't use real files for place holders (except in templates) but we still mark them with src="placeHolder.png".
                // Don't strip them here -- we want them to persist in template books. For other books we are already removing them elsewhere.

(and maybe state where elsewhere is?)

How's this? Now that we aren't using real files in templates either anymore

---

src/BloomExe/web/BloomServer.cs line 751 at r4 (raw file):

Previously, andrew-polk wrote…

I would have thought this would cause an error on the client side saying it never got a response.
I think we always have to respond with something.

Is empty string ok?

---

src/BloomExe/web/BloomServer.cs line 813 at r4 (raw file):

Previously, nabalone (Noel) wrote…

I still need to look at how and why we get here sometimes with placeholders

No longer a problem

---

src/BloomTests/Book/BookStorageTests.cs line 778 at r4 (raw file):

Previously, andrew-polk wrote…

Did we add a test somewhere to show they are removed as desired?

Done.

---

src/BloomTests/Publish/EpubValidAndAccessible.cs line 539 at r4 (raw file):

Previously, andrew-polk wrote…

Is there still a test somewhere which validates this for the real thumbnail case?

Should we have a test which validates that it isn't there for the no-thumbnail case?

🙈

[nabalone]

Reviewable status: 45 of 61 files reviewed, 18 unresolved discussions (waiting on @andrew-polk)

---

src/BloomBrowserUI/bookEdit/js/bloomEditing.ts line 435 at r5 (raw file):

    const ancestor = imgOrImageContainer.parentElement?.parentElement;
    if (ancestor) {
        SetupMetadataButton(ancestor);

I propose moving the call to SetupMetadataButton into theOneCanvasElementManager.updateCanvasElementForChangedImage below. See comment below

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 2836 at r5 (raw file):

                true,
            );
            SetupMetadataButton(canvasElement);

Is it appropriate to ok the call to SetupMetadataButton into updateCanvasElementForChangedImage here? This updateCanvasElementForChangedImage method is only called twice, once in changeImage and once in changeImage and once in deleteCanvasElement. We had previously been neglecting to call SetupMetadataButton (or similar) in deleteCanvasElement, and so in Alpha if you add a background image and then delete it, you still get the top left Metadata button and potentially it's red question mark

[andrew-polk]

@andrew-polk reviewed 16 of 16 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions

---

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 2836 at r5 (raw file):

Previously, nabalone (Noel) wrote…

Is it appropriate to ok the call to SetupMetadataButton into updateCanvasElementForChangedImage here? This updateCanvasElementForChangedImage method is only called twice, once in changeImage and once in changeImage and once in deleteCanvasElement. We had previously been neglecting to call SetupMetadataButton (or similar) in deleteCanvasElement, and so in Alpha if you add a background image and then delete it, you still get the top left Metadata button and potentially it's red question mark

Seems good.

---

src/BloomExe/web/BloomServer.cs line 751 at r4 (raw file):

Previously, nabalone (Noel) wrote…

Is empty string ok?

If it works, sure. : )

---

artwork/canvas-placeholder.svg line at r1 (raw file):

Previously, nabalone (Noel) wrote…

These are the svgs that are embedded inline in placeHolderImages.less, they aren't check in as standalone files anywhere else

Seems ok to just have them in the css. They will live on in the youtrack cards, too.

[andrew-polk]

Reviewable status: all files reviewed, 3 unresolved discussions