From 2303cbcc2428df019dcd4118c41c2c09379195d0 Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 24 Aug 2026 13:48:05 -0600 Subject: [PATCH 01/15] Fix BL-16744 Allow image placeholders to get "edit with AI" https://issues.bloomlibrary.org/youtrack/issue/BL-16744 "Edit with AI..." was disabled on an empty image slot, so the only way to fill a placeholder with an AI image was to put some other image there first. The host side already supported empty slots: EnumerateBookImages lists them with isPlaceholder, the commit path can write into them, and the AI image editor has "create" tools that make an image with no source image. Only the menu rule stood in the way. Changes: - canvasControlAvailabilityRules.editWithAi is now enabled when the user can modify the slot AND the slot is either an empty placeholder or a real image of a format the editor can open. A broken image stays disabled, as before. - IControlContext gains isPlaceholderImage, set from isPlaceHolderImage(img.src). It is needed because !hasRealImage is also true for an image that failed to load, and those two cases now have to be told apart. - aiEditorOverlay sends a placeholder slot's id as selectedBookImageId instead of withholding it. Enabling the menu item exposed a trap: with no selectedBookImageId the AI image editor targets the FIRST image of the book, which is normally the front cover, so a user who launched on an empty slot and ran an edit tool would have changed the cover. Sending the id targets the slot the user clicked. A "create" tool ignores the target, so the ordinary flow is unchanged. No change was needed in C# or in the separate bloom-ai-image-tools editor. Tests: added four cases to canvasControlAvailabilityRules.test.ts (placeholder enabled; placeholder enabled whatever the format check says; placeholder disabled when unmodifiable; broken image still disabled) and rewrote the overlay's placeholder target test. Ran the whole front-end suite with vitest: 736 passed, 5 skipped, 0 failed. Ran pnpm typecheck (passed) and eslint on the changed files (clean). --- .../aiImageEditor/aiEditorOverlay.test.ts | 15 ++++- .../bookEdit/aiImageEditor/aiEditorOverlay.ts | 13 +++-- ...uildCanvasElementControlRegistryContext.ts | 2 + .../canvasControlAvailabilityRules.test.ts | 57 +++++++++++++++++-- .../canvas/canvasControlAvailabilityRules.ts | 16 ++++-- .../toolbox/canvas/canvasControlTypes.ts | 4 ++ 6 files changed, 90 insertions(+), 17 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts index 3dc63acd6461..6aca1766ea04 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts @@ -198,11 +198,18 @@ describe("aiEditorOverlay: the edit target", () => { expect(payload.selectedBookImageId).toBeUndefined(); }); - test("an empty placeholder slot is not preloaded as the target", () => { - // There is nothing to edit, and the placeholder graphic isn't a real raster image. + test("an empty placeholder slot becomes the target too (BL-16744)", () => { + // The user launched on an empty slot to create an image for it, so that slot is + // the target. Withholding it made the editor fall back to the first image of the + // book (usually the front cover), which is not what the user clicked. + const kCoverId = "cover:0"; const { iframe, postFromEditor } = openAgainstABookWithOneImage( { pageId: kPageId, imageFileName: "placeHolder.png" }, [ + { + id: kCoverId, + src: "http://localhost:8089/bloom/book/cover.png", + }, { id: `${kPageId}:0`, src: "http://localhost:8089/bloom/book/placeHolder.png", @@ -213,7 +220,9 @@ describe("aiEditorOverlay: the edit target", () => { const payload = getInitPayloadSentToEditor(iframe, postFromEditor); - expect(payload.selectedBookImageId).toBeUndefined(); + // Sanity: the cover comes first in the list, so a fallback would have picked it. + expect(payload.selectedBookImageId).not.toBe(kCoverId); + expect(payload.selectedBookImageId).toBe(`${kPageId}:0`); }); }); diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts index 7c87fb8cb89b..5abf28f596d7 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts @@ -121,11 +121,14 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { fileNameOf(bi.src) === target.imageFileName, ) : undefined; - // Don't preload an empty placeholder slot into the edit target — there's - // nothing to edit, and its placeholder graphic isn't a real raster image. - const selectedBookImageId = clickedMatch?.isPlaceholder - ? undefined - : clickedMatch?.id; + // An empty placeholder slot is sent like any other (BL-16744). It used to be + // withheld, on the grounds that an empty slot has nothing to edit — but the AI + // image editor answers a missing selectedBookImageId by targeting the FIRST + // image of the book, which is normally the front cover. So withholding it aimed + // the user at the cover when they had asked for an empty slot on some other + // page. Sending it targets the slot they clicked, which is where the image they + // are about to create belongs; a "create" tool ignores the target anyway. + const selectedBookImageId = clickedMatch?.id; const initPayload = { ...launchData, diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/buildCanvasElementControlRegistryContext.ts b/src/BloomBrowserUI/bookEdit/toolbox/canvas/buildCanvasElementControlRegistryContext.ts index 80ac5c47152c..7e0b85c3c540 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/buildCanvasElementControlRegistryContext.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/buildCanvasElementControlRegistryContext.ts @@ -174,6 +174,8 @@ export const buildCanvasElementControlRegistryContext = ( elementType, hasImage, hasRealImage: hasRealImage(img ?? undefined), + isPlaceholderImage: + hasImage && isPlaceHolderImage(img?.getAttribute("src")), hasVideo, hasPreviousVideoContainer: videoContainer ? !!findPreviousVideoContainer(videoContainer) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlAvailabilityRules.test.ts b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlAvailabilityRules.test.ts index 7b1e8f4215ef..dc0a3a81ad2f 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlAvailabilityRules.test.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlAvailabilityRules.test.ts @@ -5,8 +5,9 @@ import { IControlAvailability, IControlContext } from "./canvasControlTypes"; // Unit tests for the `editWithAi` availability rule (see canvasControlAvailabilityRules.ts). // This is the gating logic behind the "Edit with AI..." image menu item: it must stay hidden -// until the experimental feature is on, and be disabled unless there is a real, modifiable -// image whose format the editor can actually edit. The rule is a pure function of +// until the experimental feature is on, and be disabled unless the user can modify the slot +// and the slot is either an empty placeholder (the editor can create an image for it) or a +// real image whose format the editor can actually edit. The rule is a pure function of // IControlContext, so we exercise it directly rather than driving the whole menu. // A context with every flag off/neutral. Each test flips only the flags the rule reads, so a @@ -17,6 +18,7 @@ function makeCtx(overrides: Partial): IControlContext { aiImageEditingAvailable: false, hasImage: false, hasRealImage: false, + isPlaceholderImage: false, canModifyImage: false, imageIsAiEditableFormat: false, }; @@ -90,13 +92,60 @@ describe("imageAvailabilityRules.editWithAi", () => { }); }); - describe("enabled = hasRealImage && canModifyImage && imageIsAiEditableFormat", () => { - test("disabled when there is only a placeholder (no real image)", () => { + describe("enabled = canModifyImage && (placeholder || real && editable format)", () => { + test("enabled on an empty placeholder slot, so a new image can be created for it", () => { + // BL-16744. The editor's "create" tools need no source image, so an empty + // slot is a valid thing to launch on. expect( evaluate( rule.enabled, makeCtx({ hasRealImage: false, + isPlaceholderImage: true, + canModifyImage: true, + imageIsAiEditableFormat: true, + }), + ), + ).toBe(true); + }); + + test("enabled on a placeholder even though nothing examines its format", () => { + // The user is going to make a new image, not edit placeHolder.png, so the + // format check must not gate the placeholder case. + expect( + evaluate( + rule.enabled, + makeCtx({ + hasRealImage: false, + isPlaceholderImage: true, + canModifyImage: true, + imageIsAiEditableFormat: false, + }), + ), + ).toBe(true); + }); + + test("disabled on a placeholder the user may not modify", () => { + expect( + evaluate( + rule.enabled, + makeCtx({ + hasRealImage: false, + isPlaceholderImage: true, + canModifyImage: false, + imageIsAiEditableFormat: true, + }), + ), + ).toBe(false); + }); + + test("disabled when the image is neither real nor a placeholder (a broken image)", () => { + expect( + evaluate( + rule.enabled, + makeCtx({ + hasRealImage: false, + isPlaceholderImage: false, canModifyImage: true, imageIsAiEditableFormat: true, }), diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlAvailabilityRules.ts b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlAvailabilityRules.ts index 44dd69e8c0c2..d22a6c162481 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlAvailabilityRules.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlAvailabilityRules.ts @@ -33,14 +33,20 @@ export const imageAvailabilityRules: AvailabilityRulesMap = { }, editWithAi: { // Only offered when the AI Image Editing experimental feature is turned on. - // The AI Image Editor needs a real raster image to work on, the user must be - // allowed to modify it, and its format must be one the editor can actually edit - // (so it stays disabled for e.g. an svg the editor can't open). + // Two cases are allowed, and the user must be able to modify the image in both: + // - An empty placeholder slot. The editor's "create" tools make an image with + // no source, so an empty slot is a perfectly good thing to launch on + // (BL-16744). Its format is not examined: the user is going to make a new + // image, not edit placeHolder.png. + // - A real raster image whose format the editor can actually open, so the item + // stays disabled for e.g. an svg. + // A broken image (hasImage, but neither real nor a placeholder) stays disabled: + // there is nothing to edit and nothing the user asked to fill. visible: (ctx) => ctx.aiImageEditingAvailable && ctx.hasImage, enabled: (ctx) => - ctx.hasRealImage && ctx.canModifyImage && - ctx.imageIsAiEditableFormat, + (ctx.isPlaceholderImage || + (ctx.hasRealImage && ctx.imageIsAiEditableFormat)), }, missingMetadata: { surfacePolicy: { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlTypes.ts b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlTypes.ts index fdd0766f9514..5f008ca07d18 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlTypes.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlTypes.ts @@ -116,6 +116,10 @@ export interface IControlContext { elementType: CanvasElementType; hasImage: boolean; hasRealImage: boolean; + // True when the image slot is an empty placeholder (it shows placeHolder.png). + // Distinct from !hasRealImage, which is also true for an image that failed to + // load: an empty slot is a normal state a user can fill, a broken image is not. + isPlaceholderImage: boolean; hasVideo: boolean; hasPreviousVideoContainer: boolean; hasNextVideoContainer: boolean; From baf9e854ac7f4fec0af38491174ca4e51d6c2dd3 Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 24 Aug 2026 14:00:26 -0600 Subject: [PATCH 02/15] Point the comment at the editor's new empty-slot behavior (BL-16744) The AI image editor now recognizes an empty slot from isPlaceholder: it puts nothing in "Image to Edit" and opens its "Create an Image" tool, keeping the slot so the created image can be committed into it. The comment here described the older behavior. Needs a bloom-ai-image-tools build newer than dist-v0.1.5. --- .../bookEdit/aiImageEditor/aiEditorOverlay.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts index 5abf28f596d7..f050f8ac4b51 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts @@ -125,9 +125,12 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { // withheld, on the grounds that an empty slot has nothing to edit — but the AI // image editor answers a missing selectedBookImageId by targeting the FIRST // image of the book, which is normally the front cover. So withholding it aimed - // the user at the cover when they had asked for an empty slot on some other - // page. Sending it targets the slot they clicked, which is where the image they - // are about to create belongs; a "create" tool ignores the target anyway. + // the user at the cover when they had asked for an empty slot on some other page. + // The editor reads isPlaceholder on the named slot and, for an empty one, puts + // nothing in its "Image to Edit" panel and opens its "Create an Image" tool + // instead; it keeps the slot so the created image can be committed straight into + // it. That behavior needs a bloom-ai-image-tools build newer than dist-v0.1.5, + // so the dep in package.json has to be bumped for this to work in a real Bloom. const selectedBookImageId = clickedMatch?.id; const initPayload = { From 728b419152efc7648f6c11b51335f1a3a46b081c Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 10:08:51 -0600 Subject: [PATCH 03/15] Name each image slot for the AI image editor (BL-16744) Every empty image slot looks the same in the AI image editor, so a book with two of them on one page gave the user no way to know which slot they were filling. Bloom now sends a label with each slot: "Page 3" for a numbered page, the page's own English name (e.g. "Front Cover") for front or back matter, and "Page 3 - image 2" when a page offers more than one image. The label was previously the raw data-page-number attribute, which said nothing at all for xmatter pages and never distinguished two slots on one page. The name is English because the AI image editor's user interface is English only; a localized page name inside it would look out of place, and it would need a new localizable string for "Page". GetPageNameForImageSlotLabel deliberately does not call HtmlDom.GetNumberOrLabelOfPageWhereElementLives: that helper returns the number bare, with no "Page", and answers "unknown" for a page whose data-page-number attribute is missing rather than empty, which HtmlDom itself can produce (see BL-12903). No label is better than "unknown". Co-Authored-By: Claude Opus 5 (1M context) --- .../web/controllers/AiImageEditorApi.cs | 77 +++++++++++++++-- .../web/controllers/AiImageEditorApiTests.cs | 83 +++++++++++++++++++ 2 files changed, 151 insertions(+), 9 deletions(-) diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index 41c71e2ba2d5..bae7bd6db09b 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -914,6 +914,46 @@ internal class ImageCreditAttributes public string license { get; set; } } + /// + /// What to call a page when naming one of its image slots to the user: "Page 3" for a + /// numbered page, or the page's own name (e.g. "Front Cover") for front or back matter. + /// Null when the page says neither, in which case the slot goes unlabelled. + /// The name is English, because the AI image editor's user interface is English only. + /// + /// + /// Deliberately not HtmlDom.GetNumberOrLabelOfPageWhereElementLives: that returns the + /// number bare, with no "Page", and answers "unknown" for a page whose + /// data-page-number attribute is missing rather than empty, which HtmlDom itself can + /// produce (see BL-12903). "unknown" would be worse than no label at all. + /// + internal static string GetPageNameForImageSlotLabel(SafeXmlElement page) + { + // Back matter pages do carry page numbers, but they are clearer by name. + var number = page.GetAttribute("data-page-number"); + if (!string.IsNullOrWhiteSpace(number) && !HtmlDom.IsBackMatterPage(page)) + return "Page " + number; + + var label = page.SelectSingleNode("./div[@class='pageLabel']")?.InnerText.Trim(); + return string.IsNullOrEmpty(label) ? null : label; + } + + /// + /// Names one image slot for the user: just the page name when the page offers a single + /// image, or the page name plus " - image N" when it offers more than one. Every empty + /// slot looks the same in the AI image editor, so this label is the only thing that tells + /// two of them apart (BL-16744). + /// + /// from , may be null + /// how many image slots this page offers + /// the zero-based position of this slot among them + internal static string BuildImageSlotLabel(string pageName, int slotCount, int index) + { + var number = "Image " + (index + 1); + if (string.IsNullOrEmpty(pageName)) + return slotCount > 1 ? number : null; + return slotCount > 1 ? pageName + " - image " + (index + 1) : pageName; + } + /// /// Enumerates every image the user is allowed to change across the whole book — all /// pages including front cover and xmatter, including empty placeholder slots — @@ -934,9 +974,14 @@ private List EnumerateBookImages(Bloom.Book.Book book) var pageId = page.GetAttribute("id"); if (string.IsNullOrEmpty(pageId) || !SafeId.IsMatch(pageId)) continue; - var pageLabel = page.GetAttribute("data-page-number"); + var pageName = GetPageNameForImageSlotLabel(page); var holders = HtmlDom.SelectChildImgAndBackgroundImageElements(page); + // The slots this page offers, gathered before any is added to the result, + // because a slot's label depends on how many the page has: a page with one + // image says just "Page 2", a page with more says "Page 2 - image 1" and so on. + var slotsOnThisPage = + new List<(string id, string src, bool isPlaceholder, ImageCredits credits)>(); // Ordinal is the index within the full holder list so that commit can // re-find the element deterministically; the branding/license skip below // only affects which slots we offer, not the indexing. @@ -958,22 +1003,36 @@ private List EnumerateBookImages(Bloom.Book.Book book) if (!IsImageFileName(relativePath)) continue; - images.Add( - new - { - id = pageId + ":" + ordinal, - src = (folderAsUrlPrefix + "/" + relativePath).ToLocalhost(), - pageLabel = string.IsNullOrEmpty(pageLabel) ? null : pageLabel, + slotsOnThisPage.Add( + ( + id: pageId + ":" + ordinal, + src: (folderAsUrlPrefix + "/" + relativePath).ToLocalhost(), // The AI image editor shows its own placeholder graphic for empty // slots rather than trying to load the (book-less) // placeHolder.png. - isPlaceholder = ImageUtils.IsPlaceholderImageFilename(relativePath), + isPlaceholder: ImageUtils.IsPlaceholderImageFilename(relativePath), // The image's current credits, so a result derived from it can // carry (or amend) them. The AI image editor owns the credit // *decision* and hands back whatever it chose on commit; Bloom // only embeds that into the file. Null when the image has no // usable metadata. - credits = GetCreditsForImageFile(book.FolderPath, relativePath), + credits: GetCreditsForImageFile(book.FolderPath, relativePath) + ) + ); + } + + // Now that the page's slot count is known, label each slot and add it. + for (var i = 0; i < slotsOnThisPage.Count; i++) + { + var slot = slotsOnThisPage[i]; + images.Add( + new + { + id = slot.id, + src = slot.src, + pageLabel = BuildImageSlotLabel(pageName, slotsOnThisPage.Count, i), + isPlaceholder = slot.isPlaceholder, + credits = slot.credits, } ); } diff --git a/src/BloomTests/web/controllers/AiImageEditorApiTests.cs b/src/BloomTests/web/controllers/AiImageEditorApiTests.cs index 6cae42278f9f..0cb84369e39c 100644 --- a/src/BloomTests/web/controllers/AiImageEditorApiTests.cs +++ b/src/BloomTests/web/controllers/AiImageEditorApiTests.cs @@ -8,6 +8,7 @@ using System.Threading; using Bloom.Book; using Bloom.ImageProcessing; +using Bloom.SafeXml; using Bloom.web.controllers; using NUnit.Framework; using SIL.Code; @@ -1687,5 +1688,87 @@ public void ReadCreditAttributes_MatchesWhatBloomsOwnUpdaterWouldWrite() Assert.That(attributes.creator, Is.EqualTo(img.GetAttribute("data-creator"))); Assert.That(attributes.license, Is.EqualTo(img.GetAttribute("data-license"))); } + + // The single page of a one-page DOM, as the label helpers take it. + private static SafeXmlElement FirstPageOf(string pageMarkup) + { + var dom = new HtmlDom("" + pageMarkup + ""); + var page = dom + .RawDom.SafeSelectNodes("//div[contains(@class,'bloom-page')]") + .OfType() + .FirstOrDefault(); + if (page == null) + Assert.Fail("the test markup has no bloom-page, so there is nothing to name"); + return page; + } + + [Test] + public void GetPageNameForImageSlotLabel_NumberedPage_SaysPageAndTheNumber() + { + var page = FirstPageOf( + "
" + + "
Basic Text & Picture
" + ); + + // The user thinks in page numbers, not template names, so the number wins over the + // page's own label when the page has one. + Assert.That(AiImageEditorApi.GetPageNameForImageSlotLabel(page), Is.EqualTo("Page 3")); + } + + [Test] + public void GetPageNameForImageSlotLabel_FrontMatter_UsesThePageName() + { + var page = FirstPageOf( + // Bloom writes data-page-number='' on an unnumbered page (BL-7303). + "
" + + "
Front Cover
" + ); + + // Front matter has no page number, so the page's own name is all we can say. It is + // the English name: the AI image editor's user interface is English only. + Assert.That( + AiImageEditorApi.GetPageNameForImageSlotLabel(page), + Is.EqualTo("Front Cover") + ); + } + + [Test] + public void BuildImageSlotLabel_OnePictureOnThePage_JustNamesThePage() + { + Assert.That(AiImageEditorApi.BuildImageSlotLabel("Page 3", 1, 0), Is.EqualTo("Page 3")); + } + + [Test] + public void BuildImageSlotLabel_TwoPicturesOnThePage_NumbersThem() + { + // Two empty slots on one page show the same graphic in the AI image editor, so + // without these numbers the user cannot tell which slot is which (BL-16744). + Assert.That( + AiImageEditorApi.BuildImageSlotLabel("Page 3", 2, 0), + Is.EqualTo("Page 3 - image 1") + ); + Assert.That( + AiImageEditorApi.BuildImageSlotLabel("Page 3", 2, 1), + Is.EqualTo("Page 3 - image 2") + ); + } + + [Test] + public void GetPageNameForImageSlotLabel_NoNumberAndNoLabel_SaysNothing() + { + // HtmlDom can leave a page with no data-page-number attribute at all (BL-12903). + // No label is better than a made-up one such as "unknown". + var page = FirstPageOf("
"); + + Assert.That(AiImageEditorApi.GetPageNameForImageSlotLabel(page), Is.Null); + } + + [Test] + public void BuildImageSlotLabel_NoPageName_NumbersTheImagesOnly() + { + // Nothing to say about the page, but two slots still have to be told apart. + Assert.That(AiImageEditorApi.BuildImageSlotLabel(null, 1, 0), Is.Null); + Assert.That(AiImageEditorApi.BuildImageSlotLabel(null, 2, 1), Is.EqualTo("Image 2")); + } } } From c8f6026c9a9b46078d285aa75ae702214751bb89 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 10:36:51 -0600 Subject: [PATCH 04/15] Name the canvas background image as such (BL-16744) A bloom-canvas can hold one background image with pictures on top of it. A page with a background and one picture reported "Page 1 - image 1" and "Page 1 - image 2", which tells the user nothing about which is which. It now reports "Page 1 - Canvas Background" and "Page 1 - Image 1". The background is named, not numbered, so the pictures on top of it start at 1 whether or not the page has a background. A page with a single image slot still gets the page name alone, which is the common case: a full-page picture is one canvas background image and nothing else. HtmlDom.IsBackgroundImage answers which slot is the background. A test pins the canvas markup that question is asked about, so a change to that DOM breaks here rather than quietly mislabelling every page. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/controllers/AiImageEditorApi.cs | 63 +++++++++++---- .../web/controllers/AiImageEditorApiTests.cs | 78 ++++++++++++++++--- 2 files changed, 115 insertions(+), 26 deletions(-) diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index bae7bd6db09b..8f889d0c4ffe 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -938,20 +938,32 @@ internal static string GetPageNameForImageSlotLabel(SafeXmlElement page) } /// - /// Names one image slot for the user: just the page name when the page offers a single - /// image, or the page name plus " - image N" when it offers more than one. Every empty - /// slot looks the same in the AI image editor, so this label is the only thing that tells - /// two of them apart (BL-16744). + /// Names one image slot for the user. A page with a single image slot needs no more than + /// the page name. A page with several says which slot this is: "Page 3 - Canvas + /// Background" for the canvas background image, "Page 3 - Image 1" for the pictures on + /// top of it. Every empty slot looks the same in the AI image editor, so this label is + /// the only thing that tells two of them apart (BL-16744). /// /// from , may be null - /// how many image slots this page offers - /// the zero-based position of this slot among them - internal static string BuildImageSlotLabel(string pageName, int slotCount, int index) + /// true for the canvas background image + /// + /// the 1-based position of this slot among the page's images, counting only the ones that + /// are not the canvas background. Ignored when isCanvasBackground is true. + /// + /// how many image slots this page offers in all + internal static string BuildImageSlotLabel( + string pageName, + bool isCanvasBackground, + int imageNumber, + int slotCount + ) { - var number = "Image " + (index + 1); - if (string.IsNullOrEmpty(pageName)) - return slotCount > 1 ? number : null; - return slotCount > 1 ? pageName + " - image " + (index + 1) : pageName; + // The only picture on the page. There is nothing to tell it apart from. + if (slotCount < 2) + return pageName; + + var whichSlot = isCanvasBackground ? "Canvas Background" : "Image " + imageNumber; + return string.IsNullOrEmpty(pageName) ? whichSlot : pageName + " - " + whichSlot; } /// @@ -979,9 +991,15 @@ private List EnumerateBookImages(Bloom.Book.Book book) var holders = HtmlDom.SelectChildImgAndBackgroundImageElements(page); // The slots this page offers, gathered before any is added to the result, // because a slot's label depends on how many the page has: a page with one - // image says just "Page 2", a page with more says "Page 2 - image 1" and so on. + // image says just "Page 2", a page with more says "Page 2 - Image 1" and so on. var slotsOnThisPage = - new List<(string id, string src, bool isPlaceholder, ImageCredits credits)>(); + new List<( + string id, + string src, + bool isPlaceholder, + bool isCanvasBackground, + ImageCredits credits + )>(); // Ordinal is the index within the full holder list so that commit can // re-find the element deterministically; the branding/license skip below // only affects which slots we offer, not the indexing. @@ -1011,6 +1029,10 @@ private List EnumerateBookImages(Bloom.Book.Book book) // slots rather than trying to load the (book-less) // placeHolder.png. isPlaceholder: ImageUtils.IsPlaceholderImageFilename(relativePath), + // A bloom-canvas can hold one background image with pictures on + // top of it. The background is worth naming as such, because the + // user thinks of it as the page's picture, not as "image 1". + isCanvasBackground: HtmlDom.IsBackgroundImage(element), // The image's current credits, so a result derived from it can // carry (or amend) them. The AI image editor owns the credit // *decision* and hands back whatever it chose on commit; Bloom @@ -1022,15 +1044,24 @@ private List EnumerateBookImages(Bloom.Book.Book book) } // Now that the page's slot count is known, label each slot and add it. - for (var i = 0; i < slotsOnThisPage.Count; i++) + var imageNumber = 0; + foreach (var slot in slotsOnThisPage) { - var slot = slotsOnThisPage[i]; + // The canvas background is named, not numbered, so the numbering of the + // pictures on top of it starts at 1 whether or not there is a background. + if (!slot.isCanvasBackground) + imageNumber++; images.Add( new { id = slot.id, src = slot.src, - pageLabel = BuildImageSlotLabel(pageName, slotsOnThisPage.Count, i), + pageLabel = BuildImageSlotLabel( + pageName, + slot.isCanvasBackground, + imageNumber, + slotsOnThisPage.Count + ), isPlaceholder = slot.isPlaceholder, credits = slot.credits, } diff --git a/src/BloomTests/web/controllers/AiImageEditorApiTests.cs b/src/BloomTests/web/controllers/AiImageEditorApiTests.cs index 0cb84369e39c..3400722bfc1a 100644 --- a/src/BloomTests/web/controllers/AiImageEditorApiTests.cs +++ b/src/BloomTests/web/controllers/AiImageEditorApiTests.cs @@ -1735,21 +1735,44 @@ public void GetPageNameForImageSlotLabel_FrontMatter_UsesThePageName() [Test] public void BuildImageSlotLabel_OnePictureOnThePage_JustNamesThePage() { - Assert.That(AiImageEditorApi.BuildImageSlotLabel("Page 3", 1, 0), Is.EqualTo("Page 3")); + // Nothing to tell it apart from, so the page name is enough. This is the common + // case: a full-page picture is one canvas background image and nothing else. + Assert.That( + AiImageEditorApi.BuildImageSlotLabel("Page 3", true, 1, 1), + Is.EqualTo("Page 3") + ); + Assert.That( + AiImageEditorApi.BuildImageSlotLabel("Page 3", false, 1, 1), + Is.EqualTo("Page 3") + ); } [Test] - public void BuildImageSlotLabel_TwoPicturesOnThePage_NumbersThem() + public void BuildImageSlotLabel_BackgroundAndAPictureOnIt_NamesTheBackgroundAndNumbersTheRest() { // Two empty slots on one page show the same graphic in the AI image editor, so - // without these numbers the user cannot tell which slot is which (BL-16744). + // without these names the user cannot tell which slot is which (BL-16744). Assert.That( - AiImageEditorApi.BuildImageSlotLabel("Page 3", 2, 0), - Is.EqualTo("Page 3 - image 1") + AiImageEditorApi.BuildImageSlotLabel("Page 1", true, 0, 2), + Is.EqualTo("Page 1 - Canvas Background") ); + // The pictures on top of the background start at 1, not at 2. Assert.That( - AiImageEditorApi.BuildImageSlotLabel("Page 3", 2, 1), - Is.EqualTo("Page 3 - image 2") + AiImageEditorApi.BuildImageSlotLabel("Page 1", false, 1, 2), + Is.EqualTo("Page 1 - Image 1") + ); + } + + [Test] + public void BuildImageSlotLabel_TwoPicturesAndNoBackground_NumbersThem() + { + Assert.That( + AiImageEditorApi.BuildImageSlotLabel("Page 3", false, 1, 2), + Is.EqualTo("Page 3 - Image 1") + ); + Assert.That( + AiImageEditorApi.BuildImageSlotLabel("Page 3", false, 2, 2), + Is.EqualTo("Page 3 - Image 2") ); } @@ -1764,11 +1787,46 @@ public void GetPageNameForImageSlotLabel_NoNumberAndNoLabel_SaysNothing() } [Test] - public void BuildImageSlotLabel_NoPageName_NumbersTheImagesOnly() + public void BuildImageSlotLabel_NoPageName_NamesTheSlotOnly() { // Nothing to say about the page, but two slots still have to be told apart. - Assert.That(AiImageEditorApi.BuildImageSlotLabel(null, 1, 0), Is.Null); - Assert.That(AiImageEditorApi.BuildImageSlotLabel(null, 2, 1), Is.EqualTo("Image 2")); + Assert.That(AiImageEditorApi.BuildImageSlotLabel(null, false, 1, 1), Is.Null); + Assert.That( + AiImageEditorApi.BuildImageSlotLabel(null, false, 2, 2), + Is.EqualTo("Image 2") + ); + Assert.That( + AiImageEditorApi.BuildImageSlotLabel(null, true, 0, 2), + Is.EqualTo("Canvas Background") + ); + } + + [Test] + public void IsBackgroundImage_TellsTheCanvasBackgroundFromAPictureOnTopOfIt() + { + // EnumerateBookImages asks HtmlDom which slot is the canvas background, and labels + // that one "Canvas Background" rather than "Image 1". This test pins the markup + // that question is asked about, so a change to the canvas DOM breaks here. + var page = FirstPageOf( + "
" + + "
" + + "
" + + "
" + + "
" + + "
" + + "
" + ); + var images = HtmlDom + .SelectChildImgAndBackgroundImageElements(page) + .OfType() + .ToList(); + + // Sanity: both pictures are there, in the order the labels will number them. + Assert.That(images.Count, Is.EqualTo(2), "setup"); + Assert.That(images[0].GetAttribute("src"), Is.EqualTo("back.png"), "setup"); + + Assert.That(HtmlDom.IsBackgroundImage(images[0]), Is.True); + Assert.That(HtmlDom.IsBackgroundImage(images[1]), Is.False); } } } From bee9561dfc6f88d6eeacfa69bde563fe6e3f0720 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 10:48:10 -0600 Subject: [PATCH 05/15] Localize the image slot labels (BL-16744) The labels Bloom sends to the AI image editor for its strip of the book's images were English, on the grounds that the editor's own user interface is English. But these labels name the user's own book, so they now come from BloomMediumPriority.xlf in the user interface language. Four new entries: "Page {0}", "Canvas Background", "Image {0}", and "{0} - {1}" to join the two halves. The separate join lets a translator reorder the halves or change the separator. A front or back matter page is named by its page label, which is localized the same way the Edit tab's page list does it, under the dynamic id "TemplateBooks.PageLabel." plus the English label. Co-Authored-By: Claude Opus 5 (1M context) --- .../localization/en/BloomMediumPriority.xlf | 20 ++++++++++++ .../web/controllers/AiImageEditorApi.cs | 32 ++++++++++++++++--- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/DistFiles/localization/en/BloomMediumPriority.xlf b/DistFiles/localization/en/BloomMediumPriority.xlf index 758dbf172590..2ece6085828a 100644 --- a/DistFiles/localization/en/BloomMediumPriority.xlf +++ b/DistFiles/localization/en/BloomMediumPriority.xlf @@ -50,6 +50,26 @@ ID: EditTab.Image.EditWithAI Context-menu item on an image in the Edit tab; opens the AI image editor in an overlay. The "..." indicates further UI will open, as in the sibling item "Choose image from your computer...". + + Page {0} + ID: AiImageEditor.SlotLabel.Page + Names the page an image sits on, shown over the picture in the AI image editor's strip of the book's images. {0} is replaced with the page number. + + + Canvas Background + ID: AiImageEditor.SlotLabel.CanvasBackground + Names one picture of a page that has several, shown over the picture in the AI image editor's strip of the book's images. This is the picture behind the page, which other pictures sit on top of. Combined with the page name by AiImageEditor.SlotLabel.PageAndSlot, giving e.g. "Page 3 - Canvas Background". + + + Image {0} + ID: AiImageEditor.SlotLabel.Image + Names one picture of a page that has several, shown over the picture in the AI image editor's strip of the book's images. {0} is replaced with the number of the picture on its page, counting from 1 and not counting the background picture. Combined with the page name by AiImageEditor.SlotLabel.PageAndSlot, giving e.g. "Page 3 - Image 2". + + + {0} - {1} + ID: AiImageEditor.SlotLabel.PageAndSlot + Joins the two halves of the name shown over a picture in the AI image editor's strip of the book's images. {0} is the page, e.g. "Page 3" or "Front Cover". {1} is which picture of that page, e.g. "Canvas Background" or "Image 2". Change the order or the separator if that reads better in your language. + No Indent ID: EditTab.TextContextMenu.NoIndent diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index 8f889d0c4ffe..c98da43029f0 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -10,6 +10,7 @@ using Bloom.Edit; using Bloom.ImageProcessing; using Bloom.SafeXml; +using L10NSharp; using Newtonsoft.Json; using SIL.Core.ClearShare; using SIL.IO; @@ -918,7 +919,7 @@ internal class ImageCreditAttributes /// What to call a page when naming one of its image slots to the user: "Page 3" for a /// numbered page, or the page's own name (e.g. "Front Cover") for front or back matter. /// Null when the page says neither, in which case the slot goes unlabelled. - /// The name is English, because the AI image editor's user interface is English only. + /// In the user interface language. /// /// /// Deliberately not HtmlDom.GetNumberOrLabelOfPageWhereElementLives: that returns the @@ -931,10 +932,17 @@ internal static string GetPageNameForImageSlotLabel(SafeXmlElement page) // Back matter pages do carry page numbers, but they are clearer by name. var number = page.GetAttribute("data-page-number"); if (!string.IsNullOrWhiteSpace(number) && !HtmlDom.IsBackMatterPage(page)) - return "Page " + number; + return string.Format( + LocalizationManager.GetString("AiImageEditor.SlotLabel.Page", "Page {0}"), + number + ); var label = page.SelectSingleNode("./div[@class='pageLabel']")?.InnerText.Trim(); - return string.IsNullOrEmpty(label) ? null : label; + if (string.IsNullOrEmpty(label)) + return null; + // Page labels are localized under a dynamic id built from the English label, the + // same way the page list in the Edit tab does it. + return LocalizationManager.GetString("TemplateBooks.PageLabel." + label, label); } /// @@ -962,8 +970,22 @@ int slotCount if (slotCount < 2) return pageName; - var whichSlot = isCanvasBackground ? "Canvas Background" : "Image " + imageNumber; - return string.IsNullOrEmpty(pageName) ? whichSlot : pageName + " - " + whichSlot; + var whichSlot = isCanvasBackground + ? LocalizationManager.GetString( + "AiImageEditor.SlotLabel.CanvasBackground", + "Canvas Background" + ) + : string.Format( + LocalizationManager.GetString("AiImageEditor.SlotLabel.Image", "Image {0}"), + imageNumber + ); + if (string.IsNullOrEmpty(pageName)) + return whichSlot; + return string.Format( + LocalizationManager.GetString("AiImageEditor.SlotLabel.PageAndSlot", "{0} - {1}"), + pageName, + whichSlot + ); } /// From 433d3de492b7562e407bd7c7668f9e482e37366d Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 11:09:20 -0600 Subject: [PATCH 06/15] Do not call six slots "Canvas Background" (BL-16744) A Picture Dictionary page holds six bloom-canvases, so six canvas background images. Every one of them was labelled "Page 4 - Canvas Background", which is the confusion these labels exist to remove. "Canvas Background" now names a slot only when the page has exactly one canvas. A page with several numbers them all instead, which keeps every label distinct without a new localizable string. The naming of a page's slots moved into BuildImageSlotLabelsForPage, because a slot's name depends on what else the page holds. That also makes it testable without a Book. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/controllers/AiImageEditorApi.cs | 61 +++++++++++++++---- .../web/controllers/AiImageEditorApiTests.cs | 45 ++++++++++++++ 2 files changed, 93 insertions(+), 13 deletions(-) diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index c98da43029f0..86feddb72257 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -988,6 +988,46 @@ int slotCount ); } + /// + /// Names every image slot of one page, in the order the page offers them. Kept in one + /// place because a slot's name depends on what else the page holds, not just on itself. + /// + /// from , may be null + /// + /// for each slot of the page, in order, whether it is the background image of a canvas + /// + internal static List BuildImageSlotLabelsForPage( + string pageName, + IReadOnlyList isCanvasBackground + ) + { + // "Canvas Background" names a slot only when the page has exactly one canvas. A + // Picture Dictionary page has six, so six background images; calling them all + // "Canvas Background" would be six identical labels, which is the very confusion + // the label exists to remove. There, plain numbering tells them apart. + var nameTheBackground = isCanvasBackground.Count(background => background) == 1; + + var labels = new List(); + var imageNumber = 0; + foreach (var background in isCanvasBackground) + { + // The background is named, not numbered, so the numbering of the pictures on + // top of it starts at 1 whether or not the page has a background. + var nameAsBackground = background && nameTheBackground; + if (!nameAsBackground) + imageNumber++; + labels.Add( + BuildImageSlotLabel( + pageName, + nameAsBackground, + imageNumber, + isCanvasBackground.Count + ) + ); + } + return labels; + } + /// /// Enumerates every image the user is allowed to change across the whole book — all /// pages including front cover and xmatter, including empty placeholder slots — @@ -1065,25 +1105,20 @@ ImageCredits credits ); } - // Now that the page's slot count is known, label each slot and add it. - var imageNumber = 0; - foreach (var slot in slotsOnThisPage) + // Now that the whole page is known, name its slots and add them. + var labels = BuildImageSlotLabelsForPage( + pageName, + slotsOnThisPage.Select(slot => slot.isCanvasBackground).ToList() + ); + for (var i = 0; i < slotsOnThisPage.Count; i++) { - // The canvas background is named, not numbered, so the numbering of the - // pictures on top of it starts at 1 whether or not there is a background. - if (!slot.isCanvasBackground) - imageNumber++; + var slot = slotsOnThisPage[i]; images.Add( new { id = slot.id, src = slot.src, - pageLabel = BuildImageSlotLabel( - pageName, - slot.isCanvasBackground, - imageNumber, - slotsOnThisPage.Count - ), + pageLabel = labels[i], isPlaceholder = slot.isPlaceholder, credits = slot.credits, } diff --git a/src/BloomTests/web/controllers/AiImageEditorApiTests.cs b/src/BloomTests/web/controllers/AiImageEditorApiTests.cs index 3400722bfc1a..b72e808bbc2c 100644 --- a/src/BloomTests/web/controllers/AiImageEditorApiTests.cs +++ b/src/BloomTests/web/controllers/AiImageEditorApiTests.cs @@ -1828,5 +1828,50 @@ public void IsBackgroundImage_TellsTheCanvasBackgroundFromAPictureOnTopOfIt() Assert.That(HtmlDom.IsBackgroundImage(images[0]), Is.True); Assert.That(HtmlDom.IsBackgroundImage(images[1]), Is.False); } + + [Test] + public void BuildImageSlotLabelsForPage_BackgroundAndTwoPicturesOnIt_NamesThenNumbers() + { + var labels = AiImageEditorApi.BuildImageSlotLabelsForPage( + "Page 4", + new[] { true, false, false } + ); + + Assert.That( + labels, + Is.EqualTo( + new[] { "Page 4 - Canvas Background", "Page 4 - Image 1", "Page 4 - Image 2" } + ) + ); + } + + [Test] + public void BuildImageSlotLabelsForPage_SeveralCanvasesOnThePage_NumbersThemAll() + { + // A Picture Dictionary page has six canvases, so six background images. Naming + // them all "Canvas Background" would give six identical labels, which is the + // confusion these labels exist to remove (BL-16744). + var labels = AiImageEditorApi.BuildImageSlotLabelsForPage( + "Page 4", + new[] { true, true, true } + ); + + Assert.That( + labels, + Is.EqualTo(new[] { "Page 4 - Image 1", "Page 4 - Image 2", "Page 4 - Image 3" }), + "identical labels would tell the user nothing" + ); + Assert.That(labels.Distinct().Count(), Is.EqualTo(3), "every label must be distinct"); + } + + [Test] + public void BuildImageSlotLabelsForPage_OneCanvasOnly_JustNamesThePage() + { + // The common case: a full-page picture is one canvas background and nothing else. + Assert.That( + AiImageEditorApi.BuildImageSlotLabelsForPage("Page 4", new[] { true }), + Is.EqualTo(new[] { "Page 4" }) + ); + } } } From 0014df11d446cb0591d5d12f9a837c0ea2b240cc Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 11:16:52 -0600 Subject: [PATCH 07/15] Say what imageNumber counts when a page has several canvases (BL-16744) The doc said imageNumber counts only the slots that are not the canvas background. A page with several canvases names none of them as the background, so there every slot is counted. The labels were right; the comment was not. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomExe/web/controllers/AiImageEditorApi.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index 86feddb72257..f2192178cee7 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -955,8 +955,9 @@ internal static string GetPageNameForImageSlotLabel(SafeXmlElement page) /// from , may be null /// true for the canvas background image /// - /// the 1-based position of this slot among the page's images, counting only the ones that - /// are not the canvas background. Ignored when isCanvasBackground is true. + /// the 1-based position of this slot among the page's images, counting every slot that is + /// not being named as the canvas background. A page with several canvases names none of + /// them, so there every slot is counted. Ignored when isCanvasBackground is true. /// /// how many image slots this page offers in all internal static string BuildImageSlotLabel( From dc89512c589940094c5d702d9d217bcf42027ac1 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 12:10:21 -0600 Subject: [PATCH 08/15] Pin the AI image editor at dist-v0.1.6 (BL-16744) The editor half of this work is now released: bloom-ai-image-tools 0.1.6 opens its "Create an Image" tool when Bloom launches it on an empty picture box, puts the created image back into that box, and shows the per-slot labels this branch sends. Bloom pins the editor as an immutable dist- tag, so it keeps using the old build until the pin moves. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/package.json | 2 +- src/BloomBrowserUI/pnpm-lock.yaml | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/BloomBrowserUI/package.json b/src/BloomBrowserUI/package.json index 403fec8ccd10..b4161e343e9d 100644 --- a/src/BloomBrowserUI/package.json +++ b/src/BloomBrowserUI/package.json @@ -145,7 +145,7 @@ "@types/react-transition-group": "4.4.1", "@use-it/event-listener": "0.1.7", "axios": "0.21.1", - "bloom-ai-image-tools": "github:BloomBooks/bloom-ai-image-tools#dist-v0.1.5", + "bloom-ai-image-tools": "github:BloomBooks/bloom-ai-image-tools#dist-v0.1.6", "bloom-image-gallery": "github:BloomBooks/bloom-image-gallery", "bloom-player": "2.20.1", "calculate-aspect-ratio": "0.1.3", diff --git a/src/BloomBrowserUI/pnpm-lock.yaml b/src/BloomBrowserUI/pnpm-lock.yaml index e921eb241bb1..d7aef2a44c5a 100644 --- a/src/BloomBrowserUI/pnpm-lock.yaml +++ b/src/BloomBrowserUI/pnpm-lock.yaml @@ -85,8 +85,8 @@ importers: specifier: 0.21.1 version: 0.21.1 bloom-ai-image-tools: - specifier: github:BloomBooks/bloom-ai-image-tools#dist-v0.1.5 - version: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/bcf08003b52c204cb42535040b39118c4725e01e + specifier: github:BloomBooks/bloom-ai-image-tools#dist-v0.1.6 + version: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/498f41fae27c270f6572e88d16d17fcd32829a08 bloom-image-gallery: specifier: github:BloomBooks/bloom-image-gallery version: https://codeload.github.com/BloomBooks/bloom-image-gallery/tar.gz/7be5548b663124d5e7544afb66167035b154c5f7(@types/react@18.3.31)(supports-color@5.5.0) @@ -4049,14 +4049,14 @@ packages: integrity: sha512-p2q/t/mhvuOj/UeLlV6566GD/guowlr0hHxClI0W9m7MWYkL1F0hLo+0Aexs9HSPCtR1SXQ0TD3MMKrXZajbiQ==, } - bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/bcf08003b52c204cb42535040b39118c4725e01e: + bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/498f41fae27c270f6572e88d16d17fcd32829a08: resolution: { gitHosted: true, - integrity: sha512-HDrS02dOijc4H48OvrXUC5wFkhB7ZhTswIx9ywfZTq9hxFHd03t2R0j8ZyxbPmG8785lnpfoEOuuQkxswW5+5w==, - tarball: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/bcf08003b52c204cb42535040b39118c4725e01e, + integrity: sha512-1Q8eIvPn9ipw0r6JhgK5Xa3ty8o4259g9dI0834+vhh0jbJ7BYCsdqaoWOt8KuMPol2Wm9k7jqvjXbcYpWxLoQ==, + tarball: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/498f41fae27c270f6572e88d16d17fcd32829a08, } - version: 0.1.5 + version: 0.1.6 bloom-image-gallery@https://codeload.github.com/BloomBooks/bloom-image-gallery/tar.gz/7be5548b663124d5e7544afb66167035b154c5f7: resolution: @@ -13532,7 +13532,7 @@ snapshots: file-uri-to-path: 1.0.0 optional: true - bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/bcf08003b52c204cb42535040b39118c4725e01e: + bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/498f41fae27c270f6572e88d16d17fcd32829a08: {} bloom-image-gallery@https://codeload.github.com/BloomBooks/bloom-image-gallery/tar.gz/7be5548b663124d5e7544afb66167035b154c5f7(@types/react@18.3.31)(supports-color@5.5.0): From bdbb4ba8849b9df96eb19ae9607ad5b9852a79b7 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 12:28:21 -0600 Subject: [PATCH 09/15] Send the AI editor the empty slot the user actually clicked (BL-16744) Every empty picture box shows placeHolder.png, and the overlay found the clicked box by page and file name alone. On a page with two empty boxes that always found the first one, so a user who chose "Edit with AI..." on the second box got the first as the target, and the image they made landed there. Devin caught this. The page frame now counts how many same-named boxes come before the one the user clicked and sends that count along with the file name; C# carries it back to the overlay, which takes that one out of the same-named boxes on the page. Counting only the same-named boxes is what makes the count safe: the extra images Bloom injects into the live page, and the pictures C# leaves out of the book image list, carry other file names, so neither can shift it. A count that overshoots falls back to the first same-named box. Co-Authored-By: Claude Opus 5 (1M context) --- .../aiImageEditor/aiEditorOverlay.test.ts | 64 ++++++++++++++++++- .../bookEdit/aiImageEditor/aiEditorOverlay.ts | 19 ++++-- .../aiEditorPageCommands.test.ts | 47 +++++++++++++- .../aiImageEditor/aiEditorPageCommands.ts | 35 +++++++++- .../bookEdit/aiImageEditor/aiEditorShared.ts | 7 ++ .../web/controllers/AiImageEditorApi.cs | 13 +++- 6 files changed, 171 insertions(+), 14 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts index 6aca1766ea04..02fb7c0fed46 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts @@ -41,7 +41,7 @@ const kImageFile = "old.png"; // Opens the overlay as C# does, and answers the launch request as C# would. Returns the // handles a test needs, with the overlay up and the editor about to be sent its `init`. const openAgainstABookWithOneImage = ( - target = { pageId: kPageId, imageFileName: kImageFile }, + target = { pageId: kPageId, imageFileName: kImageFile, sameNameOrdinal: 0 }, bookImages: Array<{ id: string; src: string; isPlaceholder?: boolean }> = [ { id: `${kPageId}:0`, @@ -175,6 +175,7 @@ describe("aiEditorOverlay: the edit target", () => { const { iframe, postFromEditor } = openAgainstABookWithOneImage({ pageId: kPageId, imageFileName: "somethingElse.png", + sameNameOrdinal: 0, }); const payload = getInitPayloadSentToEditor(iframe, postFromEditor); @@ -184,7 +185,7 @@ describe("aiEditorOverlay: the edit target", () => { test("a matching slot on a different page is not selected", () => { const { iframe, postFromEditor } = openAgainstABookWithOneImage( - { pageId: "page2", imageFileName: kImageFile }, + { pageId: "page2", imageFileName: kImageFile, sameNameOrdinal: 0 }, [ { id: `${kPageId}:0`, @@ -204,7 +205,11 @@ describe("aiEditorOverlay: the edit target", () => { // book (usually the front cover), which is not what the user clicked. const kCoverId = "cover:0"; const { iframe, postFromEditor } = openAgainstABookWithOneImage( - { pageId: kPageId, imageFileName: "placeHolder.png" }, + { + pageId: kPageId, + imageFileName: "placeHolder.png", + sameNameOrdinal: 0, + }, [ { id: kCoverId, @@ -224,6 +229,59 @@ describe("aiEditorOverlay: the edit target", () => { expect(payload.selectedBookImageId).not.toBe(kCoverId); expect(payload.selectedBookImageId).toBe(`${kPageId}:0`); }); + + test("the SECOND of two empty slots is the target when that is the one clicked (BL-16744)", () => { + // Both empty slots show placeHolder.png, so the file name alone cannot tell them + // apart. The page frame says how many same-named slots come first; without that the + // editor opened on slot 0 and the created image landed in the wrong box. + const { iframe, postFromEditor } = openAgainstABookWithOneImage( + { + pageId: kPageId, + imageFileName: "placeHolder.png", + sameNameOrdinal: 1, + }, + [ + { + id: `${kPageId}:0`, + src: "http://localhost:8089/bloom/book/placeHolder.png", + isPlaceholder: true, + }, + { + id: `${kPageId}:1`, + src: "http://localhost:8089/bloom/book/placeHolder.png", + isPlaceholder: true, + }, + ], + ); + + const payload = getInitPayloadSentToEditor(iframe, postFromEditor); + + expect(payload.selectedBookImageId).toBe(`${kPageId}:1`); + }); + + test("a count past the end of the list falls back to the first same-named slot", () => { + // C# leaves some pictures out of the book image list, so a page can hold more + // same-named slots than it sent. Aiming at the first one beats aiming at nothing, + // which the editor answers by targeting the first image of the whole book. + const { iframe, postFromEditor } = openAgainstABookWithOneImage( + { + pageId: kPageId, + imageFileName: "placeHolder.png", + sameNameOrdinal: 3, + }, + [ + { + id: `${kPageId}:0`, + src: "http://localhost:8089/bloom/book/placeHolder.png", + isPlaceholder: true, + }, + ], + ); + + const payload = getInitPayloadSentToEditor(iframe, postFromEditor); + + expect(payload.selectedBookImageId).toBe(`${kPageId}:0`); + }); }); describe("aiEditorOverlay: saving the live page after a commit", () => { diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts index f050f8ac4b51..acf3f4714f21 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts @@ -112,15 +112,22 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { // Identify the image the user right-clicked so the AI image editor can open with it // already in the "Image to Edit" slot. We match by page + filename rather than DOM // ordinal, because the live page has extra injected UI images that would throw - // positional indices off. - const clickedMatch = + // positional indices off. A page can hold two slots of the same name, though — every + // empty slot shows placeHolder.png — so the page frame counts the same-named slots + // ahead of the clicked one and we take that one here. Without it the user who clicked + // the second empty slot got the first, and the image they made landed there. + const sameNameOnPage = target.pageId && target.imageFileName - ? (launchData.bookImages ?? []).find( + ? (launchData.bookImages ?? []).filter( (bi) => bi.id.startsWith(target.pageId + ":") && fileNameOf(bi.src) === target.imageFileName, ) - : undefined; + : []; + // The fallback covers the one way the count can overshoot: C# leaves some pictures + // out of the book image list, so a page could hold more same-named slots than it sent. + const clickedMatch = + sameNameOnPage[target.sameNameOrdinal] ?? sameNameOnPage[0]; // An empty placeholder slot is sent like any other (BL-16744). It used to be // withheld, on the grounds that an empty slot has nothing to edit — but the AI // image editor answers a missing selectedBookImageId by targeting the FIRST @@ -129,8 +136,8 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { // The editor reads isPlaceholder on the named slot and, for an empty one, puts // nothing in its "Image to Edit" panel and opens its "Create an Image" tool // instead; it keeps the slot so the created image can be committed straight into - // it. That behavior needs a bloom-ai-image-tools build newer than dist-v0.1.5, - // so the dep in package.json has to be bumped for this to work in a real Bloom. + // it. That behavior arrived in bloom-ai-image-tools 0.1.6, which package.json pins + // as dist-v0.1.6; an older pin gets the placeholder graphic as the image to edit. const selectedBookImageId = clickedMatch?.id; const initPayload = { diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts index b2c25646883f..d915f92d2ddf 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts @@ -77,9 +77,10 @@ describe("aiEditorPageCommands: the menu command", () => { expect(postJson).toHaveBeenCalledTimes(1); expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { - // Only the file name travels: the save reloads this frame, so a live element + // Only plain data travels: the save reloads this frame, so a live element // reference could not survive the round trip. C# adds the page id. imageFileName: "old.png", + sameNameOrdinal: 0, }); }); @@ -97,6 +98,50 @@ describe("aiEditorPageCommands: the menu command", () => { expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { imageFileName: "fromContainer.png", + sameNameOrdinal: 0, + }); + }); + + test("counts the same-named slots ahead of the clicked one (BL-16744)", () => { + // Every empty slot shows placeHolder.png, so the file name alone cannot say which + // one the user clicked. Without the count the overlay picked the first, and the + // image the user made for the second slot landed in the first. + const [first, second] = makePageWithImages( + "placeHolder.png", + "placeHolder.png", + ); + + launchAiImageEditor(second, undefined); + + expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { + imageFileName: "placeHolder.png", + sameNameOrdinal: 1, + }); + + // Sanity: the first slot of the same pair still counts as none ahead of it. + postJson.mockClear(); + launchAiImageEditor(first, undefined); + expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { + imageFileName: "placeHolder.png", + sameNameOrdinal: 0, + }); + }); + + test("a differently-named picture in between does not shift the count", () => { + // The count runs over the same-named slots only, which is what keeps it immune to + // the extra images Bloom injects into the live page and to the pictures C# leaves + // out of the book image list. + const images = makePageWithImages( + "placeHolder.png", + "photo.png", + "placeHolder.png", + ); + + launchAiImageEditor(images[2], undefined); + + expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { + imageFileName: "placeHolder.png", + sameNameOrdinal: 1, }); }); }); diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts index ab799466202a..daddb45e8010 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts @@ -43,11 +43,44 @@ export function launchAiImageEditor( const clickedUrl = imgContainer ? getImageUrlFromImageContainer(imgContainer) : img?.getAttribute("src"); + const imageFileName = fileNameOf(clickedUrl); postJson("aiImageEditor/saveThenLaunch", { - imageFileName: fileNameOf(clickedUrl), + imageFileName, + sameNameOrdinal: sameNameOrdinalOnPage( + imgContainer ?? img, + imageFileName, + ), }); } +// Counts the slots BEFORE `clicked` on its page that show the same file name, so the overlay +// can tell two same-named slots apart (BL-16744). Every empty slot shows placeHolder.png, so +// on a page with two of them the file name alone made the overlay pick the first one, and the +// image the user made for the second slot landed in the first. Counting only the same-named +// slots is what makes this safe: the extra images Bloom injects into the live page, and the +// slots C# leaves out of the book image list, carry other file names and so cannot shift it. +function sameNameOrdinalOnPage( + clicked: HTMLElement | undefined, + imageFileName: string, +): number { + if (!clicked || !imageFileName) return 0; + const pageRoot = clicked.closest(".bloom-page") ?? document; + const sameName = Array.from( + pageRoot.querySelectorAll('img, [style*="background-image"]'), + ) + // A container that carries the background image AND holds an matches twice; + // keep the inner one only, so each slot counts once. + .filter((el) => el.tagName === "IMG" || !el.querySelector("img")) + .filter( + (el) => + fileNameOf(GetRawImageUrl(el as HTMLElement)) === imageFileName, + ); + const index = sameName.findIndex( + (el) => el === clicked || el.contains(clicked) || clicked.contains(el), + ); + return index < 0 ? 0 : index; +} + function asMessage(e: unknown): string { return e instanceof Error ? e.message : String(e); } diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorShared.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorShared.ts index 3f3663db9dee..38ae7c04b0bf 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorShared.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorShared.ts @@ -11,6 +11,13 @@ export interface IAiImageEditorTarget { pageId: string; imageFileName: string; + // How many slots BEFORE the clicked one on that page show the same file name. The file + // name alone cannot say which slot the user clicked when a page holds two of the same: + // every empty slot shows placeHolder.png, and one photo can be used twice. We count only + // the same-named slots, which keeps the count immune both to the extra images Bloom + // injects into the live page and to the slots C# leaves out of the book image list, + // since neither of those shares the clicked file name. + sameNameOrdinal: number; } // One entry of aiImageEditor/commit's reply. The ones flagged isCurrentPage are the slots diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index f2192178cee7..18d23ef988cf 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -234,14 +234,21 @@ private string GetAiImageEditorUrl() /// The image the user right-clicked, as the page frame sends it to /// and as we hand it back to the browser once the page - /// has been saved. See IAiImageEditorTarget in aiEditorShared.ts. The page frame sends only - /// the file name; we fill in the page id, since we are the ones who know which page we - /// saved, and the overlay (in the top window) has no page DOM of its own to read it from. + /// has been saved. See IAiImageEditorTarget in aiEditorShared.ts. The page frame sends the + /// file name and how many same-named slots come before the clicked one; we fill in the page + /// id, since we are the ones who know which page we saved, and the overlay (in the top + /// window) has no page DOM of its own to read it from. /// private class SaveThenLaunchRequest { public string imageFileName { get; set; } public string pageId { get; set; } + + /// How many slots before the clicked one on that page show the same file + /// name. The page frame counts them, because only it can see which of two + /// same-named slots the user clicked; we just carry it back to the overlay. + /// + public int sameNameOrdinal { get; set; } } /// From 5b116fe12501a89668b471f7aa4d6aae76414f37 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 12:38:43 -0600 Subject: [PATCH 10/15] Do not count the slots C# never offers (BL-16744) The count of same-named boxes ran over every picture on the live page, but the list it indexes into leaves out the branding, license, and QR-code slots. An empty one of those shows placeHolder.png like any other empty box, so a page that had one before the box the user clicked put the count one too high, the overlay fell back to the first empty box, and the image landed there. Devin caught this. The count now skips those three classes, mirroring IsUserChangeableImageElement, and skips the controls Bloom injects into the live page, which are in no saved book. Each side names the other in a comment, so the two lists stay together. Co-Authored-By: Claude Opus 5 (1M context) --- .../aiEditorPageCommands.test.ts | 44 +++++++++++++++++++ .../aiImageEditor/aiEditorPageCommands.ts | 24 ++++++++-- .../web/controllers/AiImageEditorApi.cs | 4 ++ 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts index d915f92d2ddf..a2f1a6b09d9b 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts @@ -127,6 +127,50 @@ describe("aiEditorPageCommands: the menu command", () => { }); }); + test("a branding slot showing the same placeholder does not shift the count", () => { + // C# never offers a branding, license, or QR slot to the editor, but an empty one of + // those shows placeHolder.png too. Counting it would put the count one ahead of the + // list C# sent, and the overlay would fall back to the first empty slot. + document.body.innerHTML = ` +
+
+
+
+
`; + const images = Array.from( + document.querySelectorAll("img"), + ) as HTMLImageElement[]; + + launchAiImageEditor(images[2], undefined); + + // The branding slot is not in C#'s list, so the clicked slot is its SECOND entry. + expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { + imageFileName: "placeHolder.png", + sameNameOrdinal: 1, + }); + }); + + test("a control Bloom injects into the live page does not shift the count", () => { + // Injected controls are in the live page only; C# read the saved book, which has + // none of them. + document.body.innerHTML = ` +
+
+
+
+
`; + const images = Array.from( + document.querySelectorAll("img"), + ) as HTMLImageElement[]; + + launchAiImageEditor(images[2], undefined); + + expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { + imageFileName: "placeHolder.png", + sameNameOrdinal: 1, + }); + }); + test("a differently-named picture in between does not shift the count", () => { // The count runs over the same-named slots only, which is what keeps it immune to // the extra images Bloom injects into the live page and to the pictures C# leaves diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts index daddb45e8010..a88e713be714 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts @@ -53,12 +53,21 @@ export function launchAiImageEditor( }); } +// The classes C# refuses to offer the AI image editor (IsUserChangeableImageElement in +// AiImageEditorApi.cs). An empty one of those shows placeHolder.png like any other empty slot, +// so the count below has to skip them or it would run ahead of the list C# sent. +const kNotUserChangeableClasses = ["branding", "licenseImage", "bloom-qrcode"]; + // Counts the slots BEFORE `clicked` on its page that show the same file name, so the overlay // can tell two same-named slots apart (BL-16744). Every empty slot shows placeHolder.png, so // on a page with two of them the file name alone made the overlay pick the first one, and the -// image the user made for the second slot landed in the first. Counting only the same-named -// slots is what makes this safe: the extra images Bloom injects into the live page, and the -// slots C# leaves out of the book image list, carry other file names and so cannot shift it. +// image the user made for the second slot landed in the first. +// +// Counting only the same-named slots is what keeps this in step with the list C# sent: a +// picture C# left out for having a format the editor cannot open carries its own file name, so +// it cannot shift the count. The two exclusions below cover the cases that would: a slot C# +// refuses on class alone, and the controls Bloom injects into the live page, neither of which +// is in that list. function sameNameOrdinalOnPage( clicked: HTMLElement | undefined, imageFileName: string, @@ -71,6 +80,15 @@ function sameNameOrdinalOnPage( // A container that carries the background image AND holds an matches twice; // keep the inner one only, so each slot counts once. .filter((el) => el.tagName === "IMG" || !el.querySelector("img")) + .filter( + (el) => + !kNotUserChangeableClasses.some((name) => + el.classList.contains(name), + ), + ) + // Bloom's own injected controls live in the live page only, never in the saved book + // C# read, so they must not count either. + .filter((el) => !el.closest(".bloom-ui")) .filter( (el) => fileNameOf(GetRawImageUrl(el as HTMLElement)) === imageFileName, diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index 18d23ef988cf..eadcf4f47ccf 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -809,6 +809,10 @@ out int ordinal /// license, and QR-code images are never user-changeable, so they are excluded both /// from the list offered to the AI image editor and from being overwritten at commit. /// Internal for testing. + /// + /// kNotUserChangeableClasses in aiEditorPageCommands.ts holds the same three names, + /// because the page frame has to skip the same slots when it counts the same-named + /// ones ahead of the clicked one. Change one list and change the other. /// internal static bool IsUserChangeableImageElement(SafeXmlElement element) => !element.HasClass("branding") From d2540747251431103210ab16b5f46e557d36c2b7 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 14:31:14 -0600 Subject: [PATCH 11/15] Fix Edit w/ AI when there are many images on the page. --- .../aiEditorPageCommands.test.ts | 39 ++++++++++++++ .../aiImageEditor/aiEditorPageCommands.ts | 21 ++++---- .../aiEditorSlotMatching.test.ts | 51 ++++++++++++++++++ .../aiImageEditor/aiEditorSlotMatching.ts | 54 ++++++++++++------- 4 files changed, 138 insertions(+), 27 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts index a2f1a6b09d9b..a105464a2a1b 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts @@ -212,6 +212,45 @@ describe("aiEditorPageCommands: applying current-page replacements", () => { }); }); + test("a lone swap lands on the slot its ordinal names, not the first same-named slot (BL-16744)", () => { + // Every empty slot shows placeHolder.png. A commit for the third of them must not + // land on the first, which is where a filename-only match always put it. + const images = makePageWithImages( + "placeHolder.png", + "placeHolder.png", + "placeHolder.png", + ); + + const outcome = applyAiImageEditorReplacements([ + currentPageResult(2, "placeHolder.png", "ai-image1.png"), + ]); + + expect(outcome).toEqual({ applied: 1, expected: 1 }); + expect(changeImageByElement).toHaveBeenCalledTimes(1); + expect(changeImageByElement.mock.calls[0][0]).toBe(images[2]); + }); + + test("a control Bloom injects into the live page does not shift the ordinal", () => { + // Injected controls are in the live page only; the ordinal counts the saved page's + // holders, which have none of them. + document.body.innerHTML = ` +
+
+
+
+
`; + const images = Array.from( + document.querySelectorAll("img"), + ) as HTMLImageElement[]; + + const outcome = applyAiImageEditorReplacements([ + currentPageResult(1, "placeHolder.png", "ai-image1.png"), + ]); + + expect(outcome).toEqual({ applied: 1, expected: 1 }); + expect(changeImageByElement.mock.calls[0][0]).toBe(images[2]); + }); + test("ignores results for other pages and results that failed", () => { makePageWithImages("old.png"); diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts index a88e713be714..506c1d64f09a 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts @@ -118,17 +118,20 @@ export function applyAiImageEditorReplacements( if (toApply.length === 0) return { applied: 0, expected: 0 }; const pageRoot = (document.querySelector(".bloom-page") as HTMLElement) || document; - // Look up the page's image-bearing elements once, not per replacement. + // Look up the page's image-bearing elements once, not per replacement. The selector + // mirrors C#'s SelectChildImgAndBackgroundImageElements, and the .bloom-ui filter + // removes the controls Bloom injects into the live page only, so each candidate's + // index is its ordinal among the saved page's holders — the "{pageId}:{ordinal}" the + // replacement carries. const candidates = Array.from( pageRoot.querySelectorAll('img, [style*="background-image"]'), - ); - // A page can have several slots sharing the same source (e.g. multiple empty - // placeholders). matchReplacementsToElements consumes each matched element once so - // distinct replacements land on distinct elements instead of collapsing onto the first - // match, and applies in slot (ordinal) order. We match by filename, not full src, so a - // cache-busting query string or path prefix on the live element doesn't cause a silent - // miss. oldSrc arrives from C# already decoded; the live srcs are encoded, so - // fileNameOf normalizes both sides. + ).filter((el) => !el.closest(".bloom-ui")); + // A page can have several slots sharing the same source (every empty slot shows + // placeHolder.png), so matchReplacementsToElements takes each replacement's slot by + // its ordinal, checks it by filename, and consumes each matched element once. We + // match by filename, not full src, so a cache-busting query string or path prefix on + // the live element doesn't cause a silent miss. oldSrc arrives from C# already + // decoded; the live srcs are encoded, so fileNameOf normalizes both sides. const pairs = matchReplacementsToElements( toApply, (r) => parseInt((r.incomingId ?? "").split(":").pop() ?? "", 10) || 0, diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.test.ts index a913e027e934..6d3e7d9efee6 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.test.ts @@ -118,6 +118,57 @@ describe("matchReplacementsToElements", () => { expect(result[0].replacement.newSrc).toBe("gen-0.png"); }); + test("a lone replacement lands on the slot its ordinal names, not the first same-filename slot", () => { + // The BL-16744 case: a page full of empty slots, all showing placeHolder.png, and + // the user made an image for the seventh. Filename alone cannot tell them apart; + // the ordinal must pick the element. + const els: El[] = Array.from({ length: 9 }, (_, i) => ({ + filename: "placeHolder.png", + tag: `slot-${i}`, + })); + const result = match( + [ + { + incomingId: "p:7", + oldSrc: "placeHolder.png", + newSrc: "gen.png", + }, + ], + els, + ); + + expect(result).toHaveLength(1); + expect(result[0].element.tag).toBe("slot-7"); + }); + + test("an ordinal pointing at a different filename falls back to the filename match", () => { + // The live page grew an element the saved page lacks, so the indexes shifted. The + // filename check refuses the shifted candidate and the filename match still finds + // the right one. + const els: El[] = [ + { filename: "photo.jpg", tag: "photo" }, + { filename: "a.png", tag: "A" }, + ]; + const result = match( + [{ incomingId: "p:0", oldSrc: "a.png", newSrc: "gen-a.png" }], + els, + ); + + expect(result).toHaveLength(1); + expect(result[0].element.tag).toBe("A"); + }); + + test("an ordinal past the end of the candidates falls back to the filename match", () => { + const els: El[] = [{ filename: "a.png", tag: "A" }]; + const result = match( + [{ incomingId: "p:5", oldSrc: "a.png", newSrc: "gen-a.png" }], + els, + ); + + expect(result).toHaveLength(1); + expect(result[0].element.tag).toBe("A"); + }); + test("does not mutate the caller's replacements array order", () => { const replacements: Repl[] = [ { incomingId: "p:2", oldSrc: "a.png", newSrc: "n2.png" }, diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts index 4cddcc55b5e7..d54c0c375ad9 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts @@ -1,17 +1,24 @@ // Pure slot-matching helper for applying AI-editor replacements to the currently-open page. // // When the AI commit returns replacements for the page the user is looking at, the front-end -// has to pair each replacement with the right live image element. Matching is by filename -// (a cache-busting query string or path prefix on the live element would defeat a full-src -// compare), but a single page can have several image slots that share the same filename — -// e.g. two empty placeholders, or the same photo used twice. Filename alone can't tell those -// apart, so we (a) apply in slot order — the ordinal in each replacement's "{pageId}:{n}" id -// counts the saved page's image holders in document order, which is the live candidates' -// order too — and (b) consume each element at most once, so distinct replacements land on -// distinct elements instead of all collapsing onto the first same-filename match. +// has to pair each replacement with the right live image element. The ordinal in each +// replacement's "{pageId}:{n}" id counts the saved page's image holders in document order, +// which is the live candidates' order too (the caller strips the live-only elements), so the +// candidate AT that index is the slot the user chose — a page can hold several slots showing +// the same filename (every empty slot shows placeHolder.png), and a lone replacement for the +// seventh of them must not land on the first (BL-16744). // -// This is factored out of canvasControlRegistry.ts's editWithAi command so the pairing logic -// can be unit-tested without a DOM or the changeImage side effects; the caller supplies the +// The filename (from the replacement's oldSrc) is the safety check on that index: the live +// page can grow an image-bearing element the saved page lacks, which would shift every index +// after it. A candidate whose filename is not the one the replacement expects is refused, and +// the replacement falls back to the first unused same-filename candidate — the pre-BL-16744 +// behavior, wrong only among same-named slots and safe everywhere else. Filename, not full +// src, because a cache-busting query string or path prefix on the live element would defeat a +// full-src compare. Each candidate is consumed at most once, so distinct replacements land on +// distinct elements. +// +// This is factored out of aiEditorPageCommands.ts's apply step so the pairing logic can be +// unit-tested without a DOM or the changeImage side effects; the caller supplies the // ordinal/filename accessors and performs the actual image swap on the returned pairs. export interface IReplacementMatch { @@ -20,14 +27,19 @@ export interface IReplacementMatch { } /** - * Pairs replacements to candidate elements by filename, in ascending ordinal order, using each + * Pairs each replacement with the candidate at its slot ordinal, falling back to the first + * unused same-filename candidate when that index is out of range, already used, or shows a + * different filename (see the header). Applies in ascending ordinal order and uses each * candidate at most once. A replacement with no filename match (given the still-unused * candidates) is skipped and simply omitted from the result. * * @param replacements the current-page replacements to place - * @param ordinalOf extracts a replacement's slot ordinal (used only to order placement) + * @param ordinalOf extracts a replacement's slot ordinal — the index of its slot among the + * saved page's image holders, which the candidates must mirror * @param wantedFilenameOf the filename a replacement wants to land on (from its oldSrc) - * @param candidates the live page's image-bearing elements, in document order + * @param candidates the live page's image-bearing elements, in document order, with the + * live-only elements (e.g. Bloom's injected controls) already removed so indexes + * line up with the saved page's holders * @param candidateFilenameOf the filename currently shown by a candidate element * @returns one {replacement, element} pair per successfully matched replacement, in the order * they were applied (ascending ordinal) @@ -45,11 +57,17 @@ export function matchReplacementsToElements( .sort((a, b) => ordinalOf(a) - ordinalOf(b)) .forEach((replacement) => { const wanted = wantedFilenameOf(replacement); - const element = candidates.find( - (candidate) => - !used.has(candidate) && - candidateFilenameOf(candidate) === wanted, - ); + const atOrdinal = candidates[ordinalOf(replacement)]; + const element = + atOrdinal !== undefined && + !used.has(atOrdinal) && + candidateFilenameOf(atOrdinal) === wanted + ? atOrdinal + : candidates.find( + (candidate) => + !used.has(candidate) && + candidateFilenameOf(candidate) === wanted, + ); if (element === undefined) { return; } From 5498a9e6da2a5f5bcd3b00e53976474e8dc73d4d Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 15:56:24 -0600 Subject: [PATCH 12/15] Pin the AI image editor at dist-v0.1.7 (BL-16744) This editor release makes the local dummy model draw the page label of the launched slot in a random high-contrast color, gives the launched book image a wider and brighter thumbnail border, and scrolls that thumbnail into view when the editor opens. Co-Authored-By: Claude Fable 5 --- src/BloomBrowserUI/package.json | 2 +- src/BloomBrowserUI/pnpm-lock.yaml | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/BloomBrowserUI/package.json b/src/BloomBrowserUI/package.json index b4161e343e9d..4892ac909ad2 100644 --- a/src/BloomBrowserUI/package.json +++ b/src/BloomBrowserUI/package.json @@ -145,7 +145,7 @@ "@types/react-transition-group": "4.4.1", "@use-it/event-listener": "0.1.7", "axios": "0.21.1", - "bloom-ai-image-tools": "github:BloomBooks/bloom-ai-image-tools#dist-v0.1.6", + "bloom-ai-image-tools": "github:BloomBooks/bloom-ai-image-tools#dist-v0.1.7", "bloom-image-gallery": "github:BloomBooks/bloom-image-gallery", "bloom-player": "2.20.1", "calculate-aspect-ratio": "0.1.3", diff --git a/src/BloomBrowserUI/pnpm-lock.yaml b/src/BloomBrowserUI/pnpm-lock.yaml index d7aef2a44c5a..cc4d5550e729 100644 --- a/src/BloomBrowserUI/pnpm-lock.yaml +++ b/src/BloomBrowserUI/pnpm-lock.yaml @@ -85,8 +85,8 @@ importers: specifier: 0.21.1 version: 0.21.1 bloom-ai-image-tools: - specifier: github:BloomBooks/bloom-ai-image-tools#dist-v0.1.6 - version: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/498f41fae27c270f6572e88d16d17fcd32829a08 + specifier: github:BloomBooks/bloom-ai-image-tools#dist-v0.1.7 + version: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/a24e636bd05a943b1b909811210c9803be091494 bloom-image-gallery: specifier: github:BloomBooks/bloom-image-gallery version: https://codeload.github.com/BloomBooks/bloom-image-gallery/tar.gz/7be5548b663124d5e7544afb66167035b154c5f7(@types/react@18.3.31)(supports-color@5.5.0) @@ -4049,14 +4049,14 @@ packages: integrity: sha512-p2q/t/mhvuOj/UeLlV6566GD/guowlr0hHxClI0W9m7MWYkL1F0hLo+0Aexs9HSPCtR1SXQ0TD3MMKrXZajbiQ==, } - bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/498f41fae27c270f6572e88d16d17fcd32829a08: + bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/a24e636bd05a943b1b909811210c9803be091494: resolution: { gitHosted: true, - integrity: sha512-1Q8eIvPn9ipw0r6JhgK5Xa3ty8o4259g9dI0834+vhh0jbJ7BYCsdqaoWOt8KuMPol2Wm9k7jqvjXbcYpWxLoQ==, - tarball: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/498f41fae27c270f6572e88d16d17fcd32829a08, + integrity: sha512-F67/eh54qujL/f1XtFVfJSwiMeiVr2OsRZHpnlyfhh49g7drMaXGs0cjTBhhDYVMiA+O4UFaACN58wrFMTGyCA==, + tarball: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/a24e636bd05a943b1b909811210c9803be091494, } - version: 0.1.6 + version: 0.1.7 bloom-image-gallery@https://codeload.github.com/BloomBooks/bloom-image-gallery/tar.gz/7be5548b663124d5e7544afb66167035b154c5f7: resolution: @@ -13532,7 +13532,7 @@ snapshots: file-uri-to-path: 1.0.0 optional: true - bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/498f41fae27c270f6572e88d16d17fcd32829a08: + bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/a24e636bd05a943b1b909811210c9803be091494: {} bloom-image-gallery@https://codeload.github.com/BloomBooks/bloom-image-gallery/tar.gz/7be5548b663124d5e7544afb66167035b154c5f7(@types/react@18.3.31)(supports-color@5.5.0): From bf3a4df3a8582adf02fb81abcdc4d0961efb7d44 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 16:06:16 -0600 Subject: [PATCH 13/15] Make an accepted AI image undoable (BL-16744) Accepting an image from the AI editor now registers with the image undo system, the same way Paste Image does. To keep that undo alive, the overlay no longer saves the page after a commit: a save reloads the page frame and discards the undo stack. Each "Edit with AI..." launch already saves first, so a later editor session still reads a fresh DOM. Dropping the save exposed a retry problem: when a commit partially fails and the user retries, the slot's src no longer matches the oldSrc the editor remembers. A WeakSet now marks each element the AI already swapped, and the matcher accepts the ordinal match on such an element despite the stale filename. The swap also makes its element the active canvas element, because the undo button stays disabled without one. Co-Authored-By: Claude Fable 5 --- .../aiImageEditor/aiEditorOverlay.test.ts | 51 ++++++------------- .../bookEdit/aiImageEditor/aiEditorOverlay.ts | 37 +++++--------- .../aiEditorPageCommands.test.ts | 44 +++++++++++++++- .../aiImageEditor/aiEditorPageCommands.ts | 30 +++++++++-- .../aiImageEditor/aiEditorSlotMatching.ts | 9 +++- 5 files changed, 105 insertions(+), 66 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts index 02fb7c0fed46..2e702ba36874 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts @@ -33,7 +33,6 @@ vi.mock("../js/workspaceFrames", () => ({ import { openAiImageEditor } from "./aiEditorOverlay"; -const kSaveEvent = "common/saveChangesAndRethinkPageEvent"; const kEditorUrl = "http://localhost:8089/bloom/aiImageEditor/index.html"; const kPageId = "page1"; const kImageFile = "old.png"; @@ -284,8 +283,12 @@ describe("aiEditorOverlay: the edit target", () => { }); }); -describe("aiEditorOverlay: saving the live page after a commit", () => { - test("a successful commit closes the overlay and saves at once", () => { +describe("aiEditorOverlay: the live page is NOT saved after a commit", () => { + // A current-page swap registers an image undo in the page frame, and a save would + // reload that frame and discard the undo (BL-16330's reasoning for ordinary image + // changes). So the overlay must never post the save event: the page saves by the + // normal mechanisms when the user moves on, and every launch saves first. + test("a successful commit closes the overlay without saving", () => { const { postFromEditor } = openAgainstABookWithOneImage(); commitAndReplyFromHost(postFromEditor, true); @@ -294,43 +297,20 @@ describe("aiEditorOverlay: saving the live page after a commit", () => { // assertions below aren't just watching a no-op. expect(applyAiImageEditorReplacements).toHaveBeenCalledTimes(1); expect(document.getElementById("ai-editor-overlay")).toBeNull(); - expect(postThatMightNavigate).toHaveBeenCalledTimes(1); - expect(postThatMightNavigate).toHaveBeenCalledWith(kSaveEvent); + expect(postThatMightNavigate).not.toHaveBeenCalled(); }); - test("a partial failure keeps the overlay up AND still saves what landed", () => { + test("a partial failure keeps the overlay up, still without saving", () => { const { closeButton, postFromEditor } = openAgainstABookWithOneImage(); commitAndReplyFromHost(postFromEditor, false); - // The overlay stays up so the user can read the error about the slot that failed — - // and, unlike when this code lived in the page frame, saving now does not endanger - // it, so the swap that did land is persisted immediately rather than held hostage - // until the user closes the overlay. + // The overlay stays up so the user can read the error about the slot that failed. expect(document.getElementById("ai-editor-overlay")).not.toBeNull(); - expect(postThatMightNavigate).toHaveBeenCalledTimes(1); - expect(postThatMightNavigate).toHaveBeenCalledWith(kSaveEvent); + expect(postThatMightNavigate).not.toHaveBeenCalled(); - // The ✕ still works after that save, because these controls belong to the top - // window, not to the page frame the save reloaded. closeButton.click(); expect(document.getElementById("ai-editor-overlay")).toBeNull(); - expect(postThatMightNavigate).toHaveBeenCalledTimes(1); - }); - - test("a commit that changed nothing on this page never saves", () => { - applyAiImageEditorReplacements.mockReturnValue({ - applied: 0, - expected: 0, - }); - const { closeButton, postFromEditor } = openAgainstABookWithOneImage(); - - // C# applied everything itself (all the slots were off-page), so there is no - // live-DOM change here to persist. - commitAndReplyFromHost(postFromEditor, true); - - expect(postThatMightNavigate).not.toHaveBeenCalled(); - closeButton.click(); expect(postThatMightNavigate).not.toHaveBeenCalled(); }); @@ -357,16 +337,15 @@ describe("aiEditorOverlay: saving the live page after a commit", () => { expect(ack.ok).toBe(false); expect(ack.error).toContain("Only 1 of 2"); expect(ack.error).toContain("kaboom"); - // What did land still gets saved. - expect(postThatMightNavigate).toHaveBeenCalledWith(kSaveEvent); + expect(postThatMightNavigate).not.toHaveBeenCalled(); postMessageToEditor.mockRestore(); }); test("an all-off-page commit succeeds even if the page frame is unreachable", () => { - // The page frame is briefly null while it reloads — which this feature's own - // post-commit save causes. Asking for it when the commit has nothing to do on the - // open page reported an error for images C# had in fact replaced and saved, and - // invited a retry that would redo them and orphan the files. + // The page frame is briefly null while it reloads (e.g. from the save at launch). + // Asking for it when the commit has nothing to do on the open page reported an + // error for images C# had in fact replaced and saved, and invited a retry that + // would redo them and orphan the files. getEditablePageBundleExports.mockReturnValue(null); const { iframe, postFromEditor } = openAgainstABookWithOneImage(); const postMessageToEditor = vi.spyOn( diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts index acf3f4714f21..503fbb34a00b 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts @@ -29,7 +29,7 @@ // overlay down. (There is intentionally no C#->iframe message channel; init flows from // here, because only the browser can postMessage to the iframe.) -import { post, postJson, postThatMightNavigate } from "../../utils/bloomApi"; +import { post, postJson } from "../../utils/bloomApi"; import { getEditablePageBundleExports } from "../js/workspaceFrames"; import { fileNameOf, @@ -302,7 +302,6 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { // apply fails) so its overlay can't hang. let finalOk = false; let message: string | undefined; - let currentPageApplied = 0; try { // Only involve the page frame when this commit actually has a // swap for the page being edited. Asking for it unconditionally @@ -315,7 +314,6 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { ) ? applyOnThePageBeingEdited(result?.results) : { applied: 0, expected: 0 }; - currentPageApplied = cp.applied; const serverOk = result?.ok !== false; finalOk = serverOk && @@ -342,26 +340,19 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { : String(e)); } finally { ackEditor(finalOk, message); - // changeImageByElement only mutated the LIVE page DOM; - // unlike the off-page slots (which C# saved), a - // current-page swap is not otherwise persisted. Save + - // rethink the page so the saved DOM matches the live one: - // otherwise a second commit in this same session would - // read its oldSrc from a saved page still showing the - // pre-edit image and match nothing ("0 of N could be - // updated"). Mirrors doVideoCommand's save after - // updateVideoInContainer. - // - // We can save right now, even with the overlay still up, - // precisely because this overlay lives in the top window: - // the page reload underneath it leaves its controls alone. - // (currentPageApplied is what the page frame says landed, - // so a failure part way through still saves the rest.) - if (currentPageApplied > 0) { - postThatMightNavigate( - "common/saveChangesAndRethinkPageEvent", - ); - } + // Deliberately NO save here. A current-page swap lives in + // the live page DOM only, like an image pasted or chosen + // from the gallery, and is saved the same way: by the + // normal page save when the user moves on. Saving now + // would reload the page frame, and the reload would + // discard the image undo the swap just registered — the + // whole reason ordinary image changes don't save either + // (BL-16330). Later sessions still read a fresh book DOM, + // because every launch saves first (HandleSaveThenLaunch); + // a retry from THIS still-open overlay reads stale oldSrc + // for the slots that landed, which the page frame handles + // by remembering the elements it already swapped (see + // applyAiImageEditorReplacements). if (finalOk) { cleanup(); } diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts index a105464a2a1b..f33eee9b2c34 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts @@ -11,6 +11,7 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; const postJson = vi.fn(); const changeImageByElement = vi.fn(); +const setActiveElementToClosest = vi.fn(); vi.mock("../../utils/bloomApi", () => ({ postJson: (...args: unknown[]) => postJson(...args), @@ -20,6 +21,13 @@ vi.mock("../js/bloomEditing", () => ({ changeImageByElement: (...args: unknown[]) => changeImageByElement(...args), })); +vi.mock("../js/canvasElementManager/CanvasElementManager", () => ({ + theOneCanvasElementManager: { + setActiveElementToClosest: (...args: unknown[]) => + setActiveElementToClosest(...args), + }, +})); + vi.mock("../js/bloomImages", () => ({ getImageUrlFromImageContainer: (container: HTMLElement) => container.getAttribute("data-url") ?? "", @@ -193,10 +201,11 @@ describe("aiEditorPageCommands: the menu command", () => { describe("aiEditorPageCommands: applying current-page replacements", () => { beforeEach(() => { changeImageByElement.mockClear(); + setActiveElementToClosest.mockClear(); document.body.innerHTML = ""; }); - test("swaps the matching image and reports it applied", () => { + test("swaps the matching image, undoably, and reports it applied", () => { const [img] = makePageWithImages("old.png"); const outcome = applyAiImageEditorReplacements([ @@ -208,8 +217,39 @@ describe("aiEditorPageCommands: applying current-page replacements", () => { expect(changeImageByElement.mock.calls[0][0]).toBe(img); expect(changeImageByElement.mock.calls[0][1]).toMatchObject({ src: "ai-image1.png", - undoable: "false", + // Registers an image undo, like a pasted image; that is why the overlay must + // not save afterwards (the reload would discard the undo stack). + undoable: "true", }); + // The swapped slot becomes the active element, or canUndoImageOperation would + // refuse to offer the undo until the user clicked the image. + expect(setActiveElementToClosest).toHaveBeenCalledWith(img); + }); + + test("a retry re-targets a slot this session already swapped, despite the stale oldSrc", () => { + // After a partial failure the overlay stays up and nothing was saved, so a retry's + // oldSrc (read from the saved page) still names the pre-swap file. The slot the + // earlier apply swapped must accept the retry at its ordinal anyway — falling back + // to a filename match would land it on a different same-named slot. + const images = makePageWithImages( + "placeHolder.png", + "placeHolder.png", + "placeHolder.png", + ); + // First apply: slot 2 gets the image, and the live element now shows it. + applyAiImageEditorReplacements([ + currentPageResult(2, "placeHolder.png", "ai-image1.png"), + ]); + images[2].setAttribute("src", "ai-image1.png"); + changeImageByElement.mockClear(); + + // Retry: same slot, stale oldSrc, a new result file. + const outcome = applyAiImageEditorReplacements([ + currentPageResult(2, "placeHolder.png", "ai-image2.png"), + ]); + + expect(outcome).toEqual({ applied: 1, expected: 1 }); + expect(changeImageByElement.mock.calls[0][0]).toBe(images[2]); }); test("a lone swap lands on the slot its ordinal names, not the first same-named slot (BL-16744)", () => { diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts index 506c1d64f09a..81dd0d8d5e63 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts @@ -17,6 +17,7 @@ import { GetRawImageUrl, } from "../js/bloomImages"; import { changeImageByElement } from "../js/bloomEditing"; +import { theOneCanvasElementManager } from "../js/canvasElementManager/CanvasElementManager"; import { matchReplacementsToElements } from "./aiEditorSlotMatching"; import { fileNameOf, @@ -103,11 +104,26 @@ function asMessage(e: unknown): string { return e instanceof Error ? e.message : String(e); } +// The elements this page's live DOM has already had an AI replacement applied to. A retry +// after a partial failure sends the same slots again, but C# reads each slot's oldSrc from +// the SAVED page, which does not have our unsaved swap yet — so for exactly these elements +// the filename check below is expected to fail, and the ordinal must be trusted anyway. +// A WeakSet keyed on the elements themselves: a page reload discards both the elements and, +// with them, the entries. +const elementsAlreadySwappedByAi = new WeakSet(); + // Applies the replacements C# flagged as being on the currently-edited page. It cannot // change that page itself (this live browser owns it), so it returns oldSrc/newSrc and we // use Bloom's changeImageByElement() here. Returns how many swaps landed and how many were // asked for, so the caller can report a partial failure honestly. // +// Each swap registers an image undo, like a pasted image or a picture chosen from the +// gallery, so Ctrl+Z puts the old image back. That works because nothing here saves the +// page: the save's reload would throw the live undo stack away (the same reasoning as +// BL-16330 for ordinary image changes). The page is saved by the normal mechanisms when the +// user moves on, and every editor launch saves first (see HandleSaveThenLaunch), so later +// sessions still read a fresh book DOM. +// // Called from the top window, so it must not assume anything about who is calling: `results` // has crossed a frame boundary but is plain data, and everything it touches is this // document. @@ -138,6 +154,7 @@ export function applyAiImageEditorReplacements( (r) => fileNameOf(r.oldSrc, false), candidates as HTMLElement[], (el) => fileNameOf(GetRawImageUrl(el)), + (el) => elementsAlreadySwappedByAi.has(el), ); // Count as we go rather than from pairs.length at the end: if a swap throws, the ones // already made are in the live DOM and the caller still has to know to save them. @@ -155,11 +172,16 @@ export function applyAiImageEditorReplacements( creator: r.creator ?? "", copyright: r.copyright ?? "", license: r.license ?? "", - // The AI commit applies replacements book-wide in C# (saved directly, not - // undoable), so don't register a separate per-image undo for the - // current-page piece. - undoable: "false", + // Register an image undo for each swap, like any other image change + // (see the header comment). + undoable: "true", }); + elementsAlreadySwappedByAi.add(target); + // Make the swapped slot the active element. canUndoImageOperation only + // offers the undo while an image container is active, and after the launch + // saved and reloaded this page nothing is — so without this, Ctrl+Z right + // after the editor closes would do nothing until the user clicked the image. + theOneCanvasElementManager.setActiveElementToClosest(target); applied++; }); } catch (e) { diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts index d54c0c375ad9..ac172aad399e 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts @@ -41,6 +41,11 @@ export interface IReplacementMatch { * live-only elements (e.g. Bloom's injected controls) already removed so indexes * line up with the saved page's holders * @param candidateFilenameOf the filename currently shown by a candidate element + * @param isKnownStaleFilename optional: true for a candidate whose live filename is KNOWN to + * disagree with the saved page the replacement's oldSrc was read from — e.g. an + * element an earlier, not-yet-saved apply already swapped. For such a candidate the + * filename check is expected to fail and the ordinal is trusted on its own; a retry + * after a partial failure re-sends those slots (BL-16744). * @returns one {replacement, element} pair per successfully matched replacement, in the order * they were applied (ascending ordinal) */ @@ -50,6 +55,7 @@ export function matchReplacementsToElements( wantedFilenameOf: (replacement: TReplacement) => string, candidates: TElement[], candidateFilenameOf: (element: TElement) => string, + isKnownStaleFilename?: (element: TElement) => boolean, ): Array> { const used = new Set(); const matches: Array> = []; @@ -61,7 +67,8 @@ export function matchReplacementsToElements( const element = atOrdinal !== undefined && !used.has(atOrdinal) && - candidateFilenameOf(atOrdinal) === wanted + (candidateFilenameOf(atOrdinal) === wanted || + isKnownStaleFilename?.(atOrdinal) === true) ? atOrdinal : candidates.find( (candidate) => From ad7a92e589916b8482d8dd7ccb06dced6fbd6ba6 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 25 Aug 2026 16:43:28 -0600 Subject: [PATCH 14/15] Do not name the pinned editor tag in a comment (BL-16744) The comment said package.json pins dist-v0.1.6, and the pin had already moved to dist-v0.1.7. The comment now names only the version where the behavior arrived, so a later pin bump cannot make it wrong again. Co-Authored-By: Claude Fable 5 --- src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts index 503fbb34a00b..217dde01cb5f 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts @@ -136,8 +136,8 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { // The editor reads isPlaceholder on the named slot and, for an empty one, puts // nothing in its "Image to Edit" panel and opens its "Create an Image" tool // instead; it keeps the slot so the created image can be committed straight into - // it. That behavior arrived in bloom-ai-image-tools 0.1.6, which package.json pins - // as dist-v0.1.6; an older pin gets the placeholder graphic as the image to edit. + // it. That behavior arrived in bloom-ai-image-tools 0.1.6 (package.json pins a + // later dist tag); an older pin gets the placeholder graphic as the image to edit. const selectedBookImageId = clickedMatch?.id; const initPayload = { From 4ac17b60f671c51a26bb30785dc226f434849b62 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 26 Aug 2026 17:43:50 -0600 Subject: [PATCH 15/15] Identify an image slot by its position, not its file name (BL-16744) Review asked why a slot needed a file name and an ordinal among same-named images, plus a list of classes to skip. It does not. A slot is now simply the Nth bloom-imageContainer on the page, in document order, and that index is the whole of its identity on both sides of the wire. An image container is exactly what a user may replace, so the branding, license and QR-code images, which live outside any container, are excluded by construction rather than by name. C# numbers the containers in SelectImageSlotsOnPage and the page frame numbers the same containers in slotIndexOnPage, so the two lists agree without sharing anything but the index. The ordinal stays positional: a slot C# declines to offer still holds its place. Gone with it: aiEditorSlotMatching.ts and its test, sameNameOrdinalOnPage, fileNameOf, the elementsAlreadySwappedByAi WeakSet, and IsUserChangeableImageElement. IAiImageEditorTarget is now { pageId, slotIndex }. The only exclusion left on the live-page side is .bloom-ui, for the controls Bloom injects that no saved book has. Also, per review: the slot label no longer uses a localizable "{0} - {1}" joiner, which was confusing for translators. It reuses the existing "Page" and "Image" strings and appends the number, so the three new trans-units are removed. Co-Authored-By: Claude Opus 5 --- .../localization/en/BloomMediumPriority.xlf | 17 +- .../bookEdit/aiImageEditor/AGENTS.md | 2 +- .../aiImageEditor/aiEditorOverlay.test.ts | 56 ++---- .../bookEdit/aiImageEditor/aiEditorOverlay.ts | 47 ++--- .../aiEditorPageCommands.test.ts | 164 ++++++++------- .../aiImageEditor/aiEditorPageCommands.ts | 134 +++++-------- .../bookEdit/aiImageEditor/aiEditorShared.ts | 34 +--- .../aiEditorSlotMatching.test.ts | 187 ------------------ .../aiImageEditor/aiEditorSlotMatching.ts | 85 -------- .../web/controllers/AiImageEditorApi.cs | 105 +++++----- .../web/controllers/AiImageEditorApiTests.cs | 105 ++++++++-- 11 files changed, 319 insertions(+), 617 deletions(-) delete mode 100644 src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.test.ts delete mode 100644 src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts diff --git a/DistFiles/localization/en/BloomMediumPriority.xlf b/DistFiles/localization/en/BloomMediumPriority.xlf index 1e8fc5997b57..92acec8a47a4 100644 --- a/DistFiles/localization/en/BloomMediumPriority.xlf +++ b/DistFiles/localization/en/BloomMediumPriority.xlf @@ -50,25 +50,10 @@ ID: EditTab.Image.EditWithAI Context-menu item on an image in the Edit tab; opens the AI image editor in an overlay. The "..." indicates further UI will open, as in the sibling item "Choose image from your computer...". - - Page {0} - ID: AiImageEditor.SlotLabel.Page - Names the page an image sits on, shown over the picture in the AI image editor's strip of the book's images. {0} is replaced with the page number. - Canvas Background ID: AiImageEditor.SlotLabel.CanvasBackground - Names one picture of a page that has several, shown over the picture in the AI image editor's strip of the book's images. This is the picture behind the page, which other pictures sit on top of. Combined with the page name by AiImageEditor.SlotLabel.PageAndSlot, giving e.g. "Page 3 - Canvas Background". - - - Image {0} - ID: AiImageEditor.SlotLabel.Image - Names one picture of a page that has several, shown over the picture in the AI image editor's strip of the book's images. {0} is replaced with the number of the picture on its page, counting from 1 and not counting the background picture. Combined with the page name by AiImageEditor.SlotLabel.PageAndSlot, giving e.g. "Page 3 - Image 2". - - - {0} - {1} - ID: AiImageEditor.SlotLabel.PageAndSlot - Joins the two halves of the name shown over a picture in the AI image editor's strip of the book's images. {0} is the page, e.g. "Page 3" or "Front Cover". {1} is which picture of that page, e.g. "Canvas Background" or "Image 2". Change the order or the separator if that reads better in your language. + Names one picture of a page that has several, shown over the picture in the AI image editor's strip of the book's images. This is the picture behind the page, which other pictures sit on top of. The code puts the page name in front of it, giving e.g. "Page 3 - Canvas Background". No Indent diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md b/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md index 41374ecb90d8..4a956b1b6e18 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/AGENTS.md @@ -19,7 +19,7 @@ stops the wrong half ending up in the wrong bundle: | `aiEditorOverlay.ts` | **top window** (workspace root) | `workspaceBundle.openAiImageEditor` | | `aiEditorPageCommands.ts` | **page iframe** | `editablePageBundle` (`launchAiImageEditor`, `applyAiImageEditorReplacements`) | | `aiEditorShared.ts` | either — pure, no DOM, no api calls | — | -| `aiEditorSlotMatching.ts`, `aiEditorImageFormats.ts` | either — pure | — | +| `aiEditorImageFormats.ts` | either — pure | — | So: diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts index 00ea559583d0..262d015bb8ef 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts @@ -44,7 +44,7 @@ const kImageFile = "old.png"; // Opens the overlay as C# does, and answers the launch request as C# would. Returns the // handles a test needs, with the overlay up and the AI Image Editor about to be sent its `init`. const openAgainstABookWithOneImage = ( - target = { pageId: kPageId, imageFileName: kImageFile, sameNameOrdinal: 0 }, + target = { pageId: kPageId, slotIndex: 0 }, bookImages: Array<{ id: string; src: string; isPlaceholder?: boolean }> = [ { id: `${kPageId}:0`, @@ -174,13 +174,13 @@ describe("aiEditorOverlay: the edit target", () => { expect(payload.selectedBookImageId).toBe(`${kPageId}:0`); }); - test("an image the saved book doesn't have leaves the target unset", () => { - // Sanity check on the matching: the book image list names old.png, so a click on - // some other file must not silently select old.png. + test("a slot the saved book doesn't offer leaves the target unset", () => { + // C# leaves a slot out when it holds a picture the editor cannot open, so a page + // can hold slots the list does not name. Naming one anyway would send the editor an + // id it knows nothing about; leaving it unset is what the editor understands. const { iframe, postFromEditor } = openAgainstABookWithOneImage({ pageId: kPageId, - imageFileName: "somethingElse.png", - sameNameOrdinal: 0, + slotIndex: 3, }); const payload = getInitPayloadSentToEditor(iframe, postFromEditor); @@ -190,7 +190,7 @@ describe("aiEditorOverlay: the edit target", () => { test("a matching slot on a different page is not selected", () => { const { iframe, postFromEditor } = openAgainstABookWithOneImage( - { pageId: "page2", imageFileName: kImageFile, sameNameOrdinal: 0 }, + { pageId: "page2", slotIndex: 0 }, [ { id: `${kPageId}:0`, @@ -210,11 +210,7 @@ describe("aiEditorOverlay: the edit target", () => { // book (usually the front cover), which is not what the user clicked. const kCoverId = "cover:0"; const { iframe, postFromEditor } = openAgainstABookWithOneImage( - { - pageId: kPageId, - imageFileName: "placeHolder.png", - sameNameOrdinal: 0, - }, + { pageId: kPageId, slotIndex: 0 }, [ { id: kCoverId, @@ -236,15 +232,11 @@ describe("aiEditorOverlay: the edit target", () => { }); test("the SECOND of two empty slots is the target when that is the one clicked (BL-16744)", () => { - // Both empty slots show placeHolder.png, so the file name alone cannot tell them - // apart. The page frame says how many same-named slots come first; without that the - // editor opened on slot 0 and the created image landed in the wrong box. + // Both empty slots show placeHolder.png, so nothing about the picture could tell + // them apart. The page frame numbered the slot; without that the editor opened on + // slot 0 and the created image landed in the wrong box. const { iframe, postFromEditor } = openAgainstABookWithOneImage( - { - pageId: kPageId, - imageFileName: "placeHolder.png", - sameNameOrdinal: 1, - }, + { pageId: kPageId, slotIndex: 1 }, [ { id: `${kPageId}:0`, @@ -263,30 +255,6 @@ describe("aiEditorOverlay: the edit target", () => { expect(payload.selectedBookImageId).toBe(`${kPageId}:1`); }); - - test("a count past the end of the list falls back to the first same-named slot", () => { - // C# leaves some pictures out of the book image list, so a page can hold more - // same-named slots than it sent. Aiming at the first one beats aiming at nothing, - // which the editor answers by targeting the first image of the whole book. - const { iframe, postFromEditor } = openAgainstABookWithOneImage( - { - pageId: kPageId, - imageFileName: "placeHolder.png", - sameNameOrdinal: 3, - }, - [ - { - id: `${kPageId}:0`, - src: "http://localhost:8089/bloom/book/placeHolder.png", - isPlaceholder: true, - }, - ], - ); - - const payload = getInitPayloadSentToEditor(iframe, postFromEditor); - - expect(payload.selectedBookImageId).toBe(`${kPageId}:0`); - }); }); describe("aiEditorOverlay: the live page is NOT saved after a commit", () => { diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts index 7088a7d171df..556de0d22c97 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts @@ -37,7 +37,6 @@ import { } from "../../utils/bloomApi"; import { getEditablePageBundleExports } from "../js/workspaceFrames"; import { - fileNameOf, IAiImageEditorApplyOutcome, IAiImageEditorCommitResult, IAiImageEditorTarget, @@ -138,35 +137,25 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { // id wrangling here anymore. // Identify the image the user right-clicked so the AI Image Editor can open with it - // already in the "Image to Edit" slot. We match by page + filename rather than DOM - // ordinal, because the live page has extra injected UI images that would throw - // positional indices off. A page can hold two slots of the same name, though — every - // empty slot shows placeHolder.png — so the page frame counts the same-named slots - // ahead of the clicked one and we take that one here. Without it the user who clicked - // the second empty slot got the first, and the image they made landed there. - const sameNameOnPage = - target.pageId && target.imageFileName - ? (launchData.bookImages ?? []).filter( - (bi) => - bi.id.startsWith(target.pageId + ":") && - fileNameOf(bi.src) === target.imageFileName, - ) - : []; - // The fallback covers the one way the count can overshoot: C# leaves some pictures - // out of the book image list, so a page could hold more same-named slots than it sent. - const clickedMatch = - sameNameOnPage[target.sameNameOrdinal] ?? sameNameOnPage[0]; - // An empty placeholder slot is sent like any other (BL-16744). It used to be + // already in the "Image to Edit" slot. The page frame numbered the slot it was + // clicked on, and C# builds each book image's id from the same numbering, so naming + // the clicked one is just building that id. + // + // An empty placeholder slot is named like any other (BL-16744). It used to be // withheld, on the grounds that an empty slot has nothing to edit — but the AI - // image editor answers a missing selectedBookImageId by targeting the FIRST - // image of the book, which is normally the front cover. So withholding it aimed - // the user at the cover when they had asked for an empty slot on some other page. - // The editor reads isPlaceholder on the named slot and, for an empty one, puts - // nothing in its "Image to Edit" panel and opens its "Create an Image" tool - // instead; it keeps the slot so the created image can be committed straight into - // it. That behavior arrived in bloom-ai-image-tools 0.1.6 (package.json pins a - // later dist tag); an older pin gets the placeholder graphic as the image to edit. - const selectedBookImageId = clickedMatch?.id; + // image editor answers a missing selectedBookImageId by targeting the FIRST image + // of the book, which is normally the front cover. So withholding it aimed the user + // at the cover when they had asked for an empty slot on some other page. The editor + // reads isPlaceholder on the named slot and, for an empty one, puts nothing in its + // "Image to Edit" panel and opens its "Create an Image" tool instead; it keeps the + // slot so the created image can be committed straight into it. That behavior + // arrived in bloom-ai-image-tools 0.1.6. + const clickedId = target.pageId + ":" + target.slotIndex; + const selectedBookImageId = (launchData.bookImages ?? []).some( + (bi) => bi.id === clickedId, + ) + ? clickedId + : undefined; const initPayload = { ...launchData, diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts index f33eee9b2c34..8d1b683a430e 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.test.ts @@ -6,8 +6,12 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; // The menu command deliberately does NOT open the editor. Everything C# tells the editor // about the book is read from the SAVED book DOM, so an image the user just added — which // lives only in the live page — wouldn't be there (BL-16682). The command therefore reports -// the clicked image and asks C# to save the page first; C# opens the overlay itself, in the -// top window. +// which slot was clicked and asks C# to save the page first; C# opens the overlay itself, in +// the top window. +// +// A slot is an image container, and its index among the page's image containers is its whole +// identity. C# numbers the same containers on the saved page (SelectImageSlotsOnPage), so an +// index means the same thing on both sides. These tests are mostly about that agreement. const postJson = vi.fn(); const changeImageByElement = vi.fn(); @@ -29,11 +33,7 @@ vi.mock("../js/canvasElementManager/CanvasElementManager", () => ({ })); vi.mock("../js/bloomImages", () => ({ - getImageUrlFromImageContainer: (container: HTMLElement) => - container.getAttribute("data-url") ?? "", - // The matcher compares filenames off the live elements; in the real thing this reads - // an or a background-image url, which for our fixture is just the src. - GetRawImageUrl: (element: HTMLElement) => element.getAttribute("src") ?? "", + kImageContainerClass: "bloom-imageContainer", })); import { @@ -43,19 +43,26 @@ import { const kPageId = "page1"; +// A page whose slots hold the given files, in order. Each slot is an image container inside +// a canvas element, which is how a real page holds a picture. const makePageWithImages = (...fileNames: string[]) => { document.body.innerHTML = `
${fileNames .map( (name) => - `
`, + `
`, ) .join("")}
`; return Array.from(document.querySelectorAll("img")) as HTMLImageElement[]; }; +const containers = () => + Array.from( + document.querySelectorAll(".bloom-imageContainer"), + ) as HTMLElement[]; + // A current-page commit result for slot `ordinal`, replacing `oldSrc` with `newSrc`. const currentPageResult = ( ordinal: number, @@ -87,63 +94,57 @@ describe("aiEditorPageCommands: the menu command", () => { expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { // Only plain data travels: the save reloads this frame, so a live element // reference could not survive the round trip. C# adds the page id. - imageFileName: "old.png", - sameNameOrdinal: 0, - }); - }); - - test("prefers the image container's url over the img src", () => { - // An image container's url is the authoritative one (it may be a background-image - // rather than an ), which is why the command asks for it when there is one. - makePageWithImages("ignored.png"); - const container = document.querySelector( - ".bloom-canvas-element", - ) as HTMLElement; - container.setAttribute("data-url", "fromContainer.png"); - const img = document.querySelector("img") as HTMLImageElement; - - launchAiImageEditor(img, container); - - expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { - imageFileName: "fromContainer.png", - sameNameOrdinal: 0, + slotIndex: 0, }); }); - test("counts the same-named slots ahead of the clicked one (BL-16744)", () => { - // Every empty slot shows placeHolder.png, so the file name alone cannot say which - // one the user clicked. Without the count the overlay picked the first, and the - // image the user made for the second slot landed in the first. + test("numbers the slot that was clicked, not the picture it shows (BL-16744)", () => { + // Every empty slot shows placeHolder.png, so nothing about the picture could say + // which slot the user clicked. The index can, and it says so whatever the pictures + // are: here the same file twice. const [first, second] = makePageWithImages( "placeHolder.png", "placeHolder.png", ); launchAiImageEditor(second, undefined); - expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { - imageFileName: "placeHolder.png", - sameNameOrdinal: 1, + slotIndex: 1, }); - // Sanity: the first slot of the same pair still counts as none ahead of it. + // Sanity: the other slot of the same pair is a different index. postJson.mockClear(); launchAiImageEditor(first, undefined); expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { - imageFileName: "placeHolder.png", - sameNameOrdinal: 0, + slotIndex: 0, }); }); - test("a branding slot showing the same placeholder does not shift the count", () => { - // C# never offers a branding, license, or QR slot to the editor, but an empty one of - // those shows placeHolder.png too. Counting it would put the count one ahead of the - // list C# sent, and the overlay would fall back to the first empty slot. + test("the clicked container may be given instead of the img", () => { + // canvasControlRegistry passes both when it has both. Either must number the same + // slot, because they are the same slot. + makePageWithImages("a.png", "b.png"); + const [, secondContainer] = containers(); + + launchAiImageEditor( + secondContainer.querySelector("img") as HTMLImageElement, + secondContainer, + ); + + expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { + slotIndex: 1, + }); + }); + + test("a branding image does not shift the index", () => { + // Branding, license and QR-code images are not in image containers, so they are not + // slots at all — which is why neither side needs a list of them. C# will not offer + // one and cannot overwrite one. document.body.innerHTML = `
-
-
-
+ +
+
`; const images = Array.from( document.querySelectorAll("img"), @@ -151,21 +152,19 @@ describe("aiEditorPageCommands: the menu command", () => { launchAiImageEditor(images[2], undefined); - // The branding slot is not in C#'s list, so the clicked slot is its SECOND entry. expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { - imageFileName: "placeHolder.png", - sameNameOrdinal: 1, + slotIndex: 1, }); }); - test("a control Bloom injects into the live page does not shift the count", () => { - // Injected controls are in the live page only; C# read the saved book, which has - // none of them. + test("a control Bloom injects into the live page does not shift the index", () => { + // Injected controls live in the live page only — the save strips them — so the + // saved page C# numbers has none of them. document.body.innerHTML = `
-
-
-
+
+
+
`; const images = Array.from( document.querySelectorAll("img"), @@ -174,26 +173,24 @@ describe("aiEditorPageCommands: the menu command", () => { launchAiImageEditor(images[2], undefined); expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { - imageFileName: "placeHolder.png", - sameNameOrdinal: 1, + slotIndex: 1, }); }); - test("a differently-named picture in between does not shift the count", () => { - // The count runs over the same-named slots only, which is what keeps it immune to - // the extra images Bloom injects into the live page and to the pictures C# leaves - // out of the book image list. + test("every slot counts, whatever picture it shows", () => { + // The index counts slots, not pictures of one name. A slot C# declines to offer — + // an svg it cannot open, say — still holds its place in the numbering on both + // sides, which is what lets this side number slots without knowing C#'s rules. const images = makePageWithImages( "placeHolder.png", - "photo.png", + "photo.svg", "placeHolder.png", ); launchAiImageEditor(images[2], undefined); expect(postJson).toHaveBeenCalledWith("aiImageEditor/saveThenLaunch", { - imageFileName: "placeHolder.png", - sameNameOrdinal: 1, + slotIndex: 2, }); }); }); @@ -205,7 +202,7 @@ describe("aiEditorPageCommands: applying current-page replacements", () => { document.body.innerHTML = ""; }); - test("swaps the matching image, undoably, and reports it applied", () => { + test("swaps the image of the named slot, undoably, and reports it applied", () => { const [img] = makePageWithImages("old.png"); const outcome = applyAiImageEditorReplacements([ @@ -214,6 +211,8 @@ describe("aiEditorPageCommands: applying current-page replacements", () => { expect(outcome).toEqual({ applied: 1, expected: 1 }); expect(changeImageByElement).toHaveBeenCalledTimes(1); + // The img, not the container: changeImageInfo paints a background image on anything + // that is not an , which would leave the img showing the old picture. expect(changeImageByElement.mock.calls[0][0]).toBe(img); expect(changeImageByElement.mock.calls[0][1]).toMatchObject({ src: "ai-image1.png", @@ -226,24 +225,37 @@ describe("aiEditorPageCommands: applying current-page replacements", () => { expect(setActiveElementToClosest).toHaveBeenCalledWith(img); }); - test("a retry re-targets a slot this session already swapped, despite the stale oldSrc", () => { + test("a slot with no img of its own is swapped on the container", () => { + // A slot can wear its picture as a background image instead of holding an img. + document.body.innerHTML = ` +
+
+
`; + const [container] = containers(); + + const outcome = applyAiImageEditorReplacements([ + currentPageResult(0, "old.png", "ai-image1.png"), + ]); + + expect(outcome).toEqual({ applied: 1, expected: 1 }); + expect(changeImageByElement.mock.calls[0][0]).toBe(container); + }); + + test("a retry re-targets the same slot, though it no longer shows what C# read", () => { // After a partial failure the overlay stays up and nothing was saved, so a retry's - // oldSrc (read from the saved page) still names the pre-swap file. The slot the - // earlier apply swapped must accept the retry at its ordinal anyway — falling back - // to a filename match would land it on a different same-named slot. + // oldSrc (read from the saved page) still names the pre-swap file. The index does + // not care, which is the point: nothing here compares pictures. const images = makePageWithImages( "placeHolder.png", "placeHolder.png", "placeHolder.png", ); - // First apply: slot 2 gets the image, and the live element now shows it. applyAiImageEditorReplacements([ currentPageResult(2, "placeHolder.png", "ai-image1.png"), ]); images[2].setAttribute("src", "ai-image1.png"); changeImageByElement.mockClear(); - // Retry: same slot, stale oldSrc, a new result file. const outcome = applyAiImageEditorReplacements([ currentPageResult(2, "placeHolder.png", "ai-image2.png"), ]); @@ -252,7 +264,7 @@ describe("aiEditorPageCommands: applying current-page replacements", () => { expect(changeImageByElement.mock.calls[0][0]).toBe(images[2]); }); - test("a lone swap lands on the slot its ordinal names, not the first same-named slot (BL-16744)", () => { + test("a lone swap lands on the slot its ordinal names (BL-16744)", () => { // Every empty slot shows placeHolder.png. A commit for the third of them must not // land on the first, which is where a filename-only match always put it. const images = makePageWithImages( @@ -272,12 +284,12 @@ describe("aiEditorPageCommands: applying current-page replacements", () => { test("a control Bloom injects into the live page does not shift the ordinal", () => { // Injected controls are in the live page only; the ordinal counts the saved page's - // holders, which have none of them. + // slots, which have none of them. document.body.innerHTML = `
-
-
-
+
+
+
`; const images = Array.from( document.querySelectorAll("img"), @@ -306,7 +318,7 @@ describe("aiEditorPageCommands: applying current-page replacements", () => { expect(changeImageByElement).not.toHaveBeenCalled(); }); - test("reports a shortfall when a slot cannot be matched", () => { + test("reports a shortfall when the page has no such slot", () => { // The caller turns applied < expected into "Only N of M ... could be updated", so // this has to be counted honestly rather than reported as success. makePageWithImages("old.png"); diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts index 81dd0d8d5e63..385503d61fb7 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorPageCommands.ts @@ -12,15 +12,10 @@ // getEditablePageBundleExports().applyAiImageEditorReplacements(). import { postJson } from "../../utils/bloomApi"; -import { - getImageUrlFromImageContainer, - GetRawImageUrl, -} from "../js/bloomImages"; +import { kImageContainerClass } from "../js/bloomImages"; import { changeImageByElement } from "../js/bloomEditing"; import { theOneCanvasElementManager } from "../js/canvasElementManager/CanvasElementManager"; -import { matchReplacementsToElements } from "./aiEditorSlotMatching"; import { - fileNameOf, IAiImageEditorApplyOutcome, IAiImageEditorCommitResult, isCurrentPageSwap, @@ -41,77 +36,46 @@ export function launchAiImageEditor( img: HTMLImageElement, imgContainer: HTMLElement | undefined, ): void { - const clickedUrl = imgContainer - ? getImageUrlFromImageContainer(imgContainer) - : img?.getAttribute("src"); - const imageFileName = fileNameOf(clickedUrl); postJson("aiImageEditor/saveThenLaunch", { - imageFileName, - sameNameOrdinal: sameNameOrdinalOnPage( - imgContainer ?? img, - imageFileName, - ), + slotIndex: slotIndexOnPage(imgContainer ?? img), }); } -// The classes C# refuses to offer the AI image editor (IsUserChangeableImageElement in -// AiImageEditorApi.cs). An empty one of those shows placeHolder.png like any other empty slot, -// so the count below has to skip them or it would run ahead of the list C# sent. -const kNotUserChangeableClasses = ["branding", "licenseImage", "bloom-qrcode"]; - -// Counts the slots BEFORE `clicked` on its page that show the same file name, so the overlay -// can tell two same-named slots apart (BL-16744). Every empty slot shows placeHolder.png, so -// on a page with two of them the file name alone made the overlay pick the first one, and the -// image the user made for the second slot landed in the first. +// Numbers this page's image slots the way C# does (SelectImageSlotsOnPage in +// AiImageEditorApi.cs): its image containers, in document order. An image container is +// exactly what a user may replace, so the branding, license and QR-code images, which live +// outside any container, are not slots at all. // -// Counting only the same-named slots is what keeps this in step with the list C# sent: a -// picture C# left out for having a format the editor cannot open carries its own file name, so -// it cannot shift the count. The two exclusions below cover the cases that would: a slot C# -// refuses on class alone, and the controls Bloom injects into the live page, neither of which -// is in that list. -function sameNameOrdinalOnPage( - clicked: HTMLElement | undefined, - imageFileName: string, -): number { - if (!clicked || !imageFileName) return 0; +// The index IS the slot's identity — it is the "{pageId}:{ordinal}" ordinal C# builds — so the +// two lists have to hold the same containers. Bloom injects controls into the live page that +// no saved book has, and the save strips them (Cleanup in bloomEditing.ts), so those are the +// one thing to leave out here. +function slotIndexOnPage(clicked: HTMLElement | undefined): number { + if (!clicked) return 0; const pageRoot = clicked.closest(".bloom-page") ?? document; - const sameName = Array.from( - pageRoot.querySelectorAll('img, [style*="background-image"]'), - ) - // A container that carries the background image AND holds an matches twice; - // keep the inner one only, so each slot counts once. - .filter((el) => el.tagName === "IMG" || !el.querySelector("img")) - .filter( - (el) => - !kNotUserChangeableClasses.some((name) => - el.classList.contains(name), - ), - ) - // Bloom's own injected controls live in the live page only, never in the saved book - // C# read, so they must not count either. - .filter((el) => !el.closest(".bloom-ui")) - .filter( - (el) => - fileNameOf(GetRawImageUrl(el as HTMLElement)) === imageFileName, - ); - const index = sameName.findIndex( + const slots = Array.from( + pageRoot.querySelectorAll("." + kImageContainerClass), + ).filter((el) => !el.closest(".bloom-ui")); + const index = slots.findIndex( (el) => el === clicked || el.contains(clicked) || clicked.contains(el), ); return index < 0 ? 0 : index; } +// The element of a slot that carries the picture: the container's own img, or the container +// itself when it wears the picture as a background image. Mirrors GetImageElementOfSlot in +// AiImageEditorApi.cs. It matters here because changeImageInfo sets a background image on +// anything that is not an , so handing it a container that holds an img would leave the +// img untouched and paint the new picture behind it. +function imageElementOfSlot(slot: HTMLElement): HTMLElement { + const img = Array.from(slot.children).find((c) => c.tagName === "IMG"); + return (img as HTMLElement) ?? slot; +} + function asMessage(e: unknown): string { return e instanceof Error ? e.message : String(e); } -// The elements this page's live DOM has already had an AI replacement applied to. A retry -// after a partial failure sends the same slots again, but C# reads each slot's oldSrc from -// the SAVED page, which does not have our unsaved swap yet — so for exactly these elements -// the filename check below is expected to fail, and the ordinal must be trusted anyway. -// A WeakSet keyed on the elements themselves: a page reload discards both the elements and, -// with them, the entries. -const elementsAlreadySwappedByAi = new WeakSet(); - // Applies the replacements C# flagged as being on the currently-edited page. It cannot // change that page itself (this live browser owns it), so it returns oldSrc/newSrc and we // use Bloom's changeImageByElement() here. Returns how many swaps landed and how many were @@ -134,33 +98,28 @@ export function applyAiImageEditorReplacements( if (toApply.length === 0) return { applied: 0, expected: 0 }; const pageRoot = (document.querySelector(".bloom-page") as HTMLElement) || document; - // Look up the page's image-bearing elements once, not per replacement. The selector - // mirrors C#'s SelectChildImgAndBackgroundImageElements, and the .bloom-ui filter - // removes the controls Bloom injects into the live page only, so each candidate's - // index is its ordinal among the saved page's holders — the "{pageId}:{ordinal}" the - // replacement carries. - const candidates = Array.from( - pageRoot.querySelectorAll('img, [style*="background-image"]'), - ).filter((el) => !el.closest(".bloom-ui")); - // A page can have several slots sharing the same source (every empty slot shows - // placeHolder.png), so matchReplacementsToElements takes each replacement's slot by - // its ordinal, checks it by filename, and consumes each matched element once. We - // match by filename, not full src, so a cache-busting query string or path prefix on - // the live element doesn't cause a silent miss. oldSrc arrives from C# already - // decoded; the live srcs are encoded, so fileNameOf normalizes both sides. - const pairs = matchReplacementsToElements( - toApply, - (r) => parseInt((r.incomingId ?? "").split(":").pop() ?? "", 10) || 0, - (r) => fileNameOf(r.oldSrc, false), - candidates as HTMLElement[], - (el) => fileNameOf(GetRawImageUrl(el)), - (el) => elementsAlreadySwappedByAi.has(el), - ); - // Count as we go rather than from pairs.length at the end: if a swap throws, the ones - // already made are in the live DOM and the caller still has to know to save them. + // The page's slots, numbered as slotIndexOnPage numbers them and as C# numbers them + // (SelectImageSlotsOnPage): the image containers, less the controls Bloom injects into + // the live page. The ordinal in a replacement's "{pageId}:{ordinal}" is an index into + // this list, and that index is the whole of a slot's identity — nothing here compares + // file names, because two slots can honestly show the same file (every empty slot shows + // placeHolder.png) and a slot we already swapped no longer shows what C# read. + const slots = Array.from( + pageRoot.querySelectorAll("." + kImageContainerClass), + ).filter((el) => !el.closest(".bloom-ui")) as HTMLElement[]; + // Count as we go rather than at the end: if a swap throws, the ones already made are in + // the live DOM and the caller still has to know to save them. A replacement whose slot + // this page does not have is left out, which the caller sees as applied < expected. let applied = 0; try { - pairs.forEach(({ replacement: r, element: target }) => { + toApply.forEach((r) => { + const slot = + slots[ + parseInt((r.incomingId ?? "").split(":").pop() ?? "", 10) || + 0 + ]; + if (!slot) return; + const target = imageElementOfSlot(slot); changeImageByElement(target, { src: r.newSrc as string, // Take the credits from C#, which read them off the new image file. @@ -176,7 +135,6 @@ export function applyAiImageEditorReplacements( // (see the header comment). undoable: "true", }); - elementsAlreadySwappedByAi.add(target); // Make the swapped slot the active element. canUndoImageOperation only // offers the undo while an image container is active, and after the launch // saved and reloaded this page nothing is — so without this, Ctrl+Z right diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorShared.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorShared.ts index 38ae7c04b0bf..bff526f0dc7f 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorShared.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorShared.ts @@ -10,14 +10,13 @@ // match the click against the book image list. export interface IAiImageEditorTarget { pageId: string; - imageFileName: string; - // How many slots BEFORE the clicked one on that page show the same file name. The file - // name alone cannot say which slot the user clicked when a page holds two of the same: - // every empty slot shows placeHolder.png, and one photo can be used twice. We count only - // the same-named slots, which keeps the count immune both to the extra images Bloom - // injects into the live page and to the slots C# leaves out of the book image list, - // since neither of those shares the clicked file name. - sameNameOrdinal: number; + // Which image slot of that page the user clicked, as its index among the page's image + // containers in document order. That index is the slot's whole identity: it is the + // ordinal in the book image's "{pageId}:{ordinal}" id, so the overlay names the clicked + // image by building that id rather than by looking for a file name. A file name could + // never have said which slot was clicked anyway, since every empty slot shows + // placeHolder.png and one photo can be used twice on a page. + slotIndex: number; } // One entry of aiImageEditor/commit's reply. The ones flagged isCurrentPage are the slots @@ -60,22 +59,3 @@ export interface IAiImageEditorApplyOutcome { expected: number; error?: string; } - -// Pull the file name off an image url. `encoded` says whether the url is percent-encoded: -// live DOM srcs and host-served URLs are, but oldSrc in commit results arrives from C# -// already decoded (PathOnly.NotEncoded) — decoding it again corrupts (or throws on) -// filenames containing a literal '%'. On a failed decode fall back to the raw name rather -// than "", so an oddly-encoded src degrades to a possible mismatch instead of matching -// nothing ever. -export function fileNameOf( - url?: string | null, - encoded: boolean = true, -): string { - const raw = (url ?? "").split("?")[0].split("/").pop() ?? ""; - if (!encoded) return raw; - try { - return decodeURIComponent(raw); - } catch { - return raw; - } -} diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.test.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.test.ts deleted file mode 100644 index 6d3e7d9efee6..000000000000 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.test.ts +++ /dev/null @@ -1,187 +0,0 @@ -import { describe, expect, test } from "vitest"; - -import { matchReplacementsToElements } from "./aiEditorSlotMatching"; - -// Unit tests for the current-page slot matcher used by the AI editor's commit (see -// aiEditorSlotMatching.ts and canvasControlRegistry.ts editWithAi). The tricky case is a page -// with several image slots that share a filename: distinct replacements must land on distinct -// elements, in slot (ordinal) order, not all collapse onto the first same-filename element. - -// A minimal stand-in for a replacement and a live page element, so the test needs no DOM. -interface Repl { - incomingId: string; // "{pageId}:{ordinal}" - oldSrc: string; // filename the replacement wants to land on - newSrc: string; // the replacement image (only used to identify the pair here) -} -interface El { - filename: string; // filename the live element currently shows - tag: string; // just a label so assertions can name the element -} - -const ordinalOf = (r: Repl) => - parseInt(r.incomingId.split(":").pop() ?? "", 10) || 0; - -// Run the matcher with the Repl/El accessors wired up the way the real caller does. -function match(replacements: Repl[], candidates: El[]) { - return matchReplacementsToElements( - replacements, - ordinalOf, - (r) => r.oldSrc, - candidates, - (e) => e.filename, - ); -} - -describe("matchReplacementsToElements", () => { - test("matches a single replacement to the element with the same filename", () => { - const els: El[] = [ - { filename: "a.png", tag: "A" }, - { filename: "b.png", tag: "B" }, - ]; - const result = match( - [{ incomingId: "p:1", oldSrc: "b.png", newSrc: "new-b.png" }], - els, - ); - - expect(result).toHaveLength(1); - expect(result[0].element.tag).toBe("B"); - expect(result[0].replacement.newSrc).toBe("new-b.png"); - }); - - test("two same-filename slots get distinct elements, paired by ordinal order", () => { - // Two placeholder slots that both currently show placeholder.png. The ordinal-0 - // replacement must take the first placeholder element and ordinal-1 the second — - // neither collapsing onto the same element. - const els: El[] = [ - { filename: "placeholder.png", tag: "first" }, - { filename: "placeholder.png", tag: "second" }, - ]; - // Deliberately pass them out of ordinal order to prove the matcher sorts. - const result = match( - [ - { - incomingId: "p:1", - oldSrc: "placeholder.png", - newSrc: "gen-1.png", - }, - { - incomingId: "p:0", - oldSrc: "placeholder.png", - newSrc: "gen-0.png", - }, - ], - els, - ); - - expect(result).toHaveLength(2); - // Applied in ascending ordinal order: ordinal 0 first, ordinal 1 second. - expect(result[0].replacement.newSrc).toBe("gen-0.png"); - expect(result[0].element.tag).toBe("first"); - expect(result[1].replacement.newSrc).toBe("gen-1.png"); - expect(result[1].element.tag).toBe("second"); - // Sanity: the two replacements did NOT land on the same element. - expect(result[0].element).not.toBe(result[1].element); - }); - - test("a replacement with no filename match is skipped, others still match", () => { - const els: El[] = [{ filename: "a.png", tag: "A" }]; - const result = match( - [ - { - incomingId: "p:0", - oldSrc: "missing.png", - newSrc: "gen-miss.png", - }, - { incomingId: "p:1", oldSrc: "a.png", newSrc: "gen-a.png" }, - ], - els, - ); - - expect(result).toHaveLength(1); - expect(result[0].replacement.newSrc).toBe("gen-a.png"); - expect(result[0].element.tag).toBe("A"); - }); - - test("more same-filename replacements than elements: extras drop out, no reuse", () => { - const els: El[] = [{ filename: "dup.png", tag: "only" }]; - const result = match( - [ - { incomingId: "p:0", oldSrc: "dup.png", newSrc: "gen-0.png" }, - { incomingId: "p:1", oldSrc: "dup.png", newSrc: "gen-1.png" }, - ], - els, - ); - - // Only one element exists, so only the first (ordinal 0) replacement lands; the - // second finds no unused same-filename element and is omitted rather than reusing. - expect(result).toHaveLength(1); - expect(result[0].replacement.newSrc).toBe("gen-0.png"); - }); - - test("a lone replacement lands on the slot its ordinal names, not the first same-filename slot", () => { - // The BL-16744 case: a page full of empty slots, all showing placeHolder.png, and - // the user made an image for the seventh. Filename alone cannot tell them apart; - // the ordinal must pick the element. - const els: El[] = Array.from({ length: 9 }, (_, i) => ({ - filename: "placeHolder.png", - tag: `slot-${i}`, - })); - const result = match( - [ - { - incomingId: "p:7", - oldSrc: "placeHolder.png", - newSrc: "gen.png", - }, - ], - els, - ); - - expect(result).toHaveLength(1); - expect(result[0].element.tag).toBe("slot-7"); - }); - - test("an ordinal pointing at a different filename falls back to the filename match", () => { - // The live page grew an element the saved page lacks, so the indexes shifted. The - // filename check refuses the shifted candidate and the filename match still finds - // the right one. - const els: El[] = [ - { filename: "photo.jpg", tag: "photo" }, - { filename: "a.png", tag: "A" }, - ]; - const result = match( - [{ incomingId: "p:0", oldSrc: "a.png", newSrc: "gen-a.png" }], - els, - ); - - expect(result).toHaveLength(1); - expect(result[0].element.tag).toBe("A"); - }); - - test("an ordinal past the end of the candidates falls back to the filename match", () => { - const els: El[] = [{ filename: "a.png", tag: "A" }]; - const result = match( - [{ incomingId: "p:5", oldSrc: "a.png", newSrc: "gen-a.png" }], - els, - ); - - expect(result).toHaveLength(1); - expect(result[0].element.tag).toBe("A"); - }); - - test("does not mutate the caller's replacements array order", () => { - const replacements: Repl[] = [ - { incomingId: "p:2", oldSrc: "a.png", newSrc: "n2.png" }, - { incomingId: "p:0", oldSrc: "a.png", newSrc: "n0.png" }, - ]; - const els: El[] = [ - { filename: "a.png", tag: "A0" }, - { filename: "a.png", tag: "A2" }, - ]; - match(replacements, els); - - // The matcher sorts a copy; the original array keeps its input order. - expect(replacements[0].incomingId).toBe("p:2"); - expect(replacements[1].incomingId).toBe("p:0"); - }); -}); diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts deleted file mode 100644 index ac172aad399e..000000000000 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorSlotMatching.ts +++ /dev/null @@ -1,85 +0,0 @@ -// Pure slot-matching helper for applying AI-editor replacements to the currently-open page. -// -// When the AI commit returns replacements for the page the user is looking at, the front-end -// has to pair each replacement with the right live image element. The ordinal in each -// replacement's "{pageId}:{n}" id counts the saved page's image holders in document order, -// which is the live candidates' order too (the caller strips the live-only elements), so the -// candidate AT that index is the slot the user chose — a page can hold several slots showing -// the same filename (every empty slot shows placeHolder.png), and a lone replacement for the -// seventh of them must not land on the first (BL-16744). -// -// The filename (from the replacement's oldSrc) is the safety check on that index: the live -// page can grow an image-bearing element the saved page lacks, which would shift every index -// after it. A candidate whose filename is not the one the replacement expects is refused, and -// the replacement falls back to the first unused same-filename candidate — the pre-BL-16744 -// behavior, wrong only among same-named slots and safe everywhere else. Filename, not full -// src, because a cache-busting query string or path prefix on the live element would defeat a -// full-src compare. Each candidate is consumed at most once, so distinct replacements land on -// distinct elements. -// -// This is factored out of aiEditorPageCommands.ts's apply step so the pairing logic can be -// unit-tested without a DOM or the changeImage side effects; the caller supplies the -// ordinal/filename accessors and performs the actual image swap on the returned pairs. - -export interface IReplacementMatch { - replacement: TReplacement; - element: TElement; -} - -/** - * Pairs each replacement with the candidate at its slot ordinal, falling back to the first - * unused same-filename candidate when that index is out of range, already used, or shows a - * different filename (see the header). Applies in ascending ordinal order and uses each - * candidate at most once. A replacement with no filename match (given the still-unused - * candidates) is skipped and simply omitted from the result. - * - * @param replacements the current-page replacements to place - * @param ordinalOf extracts a replacement's slot ordinal — the index of its slot among the - * saved page's image holders, which the candidates must mirror - * @param wantedFilenameOf the filename a replacement wants to land on (from its oldSrc) - * @param candidates the live page's image-bearing elements, in document order, with the - * live-only elements (e.g. Bloom's injected controls) already removed so indexes - * line up with the saved page's holders - * @param candidateFilenameOf the filename currently shown by a candidate element - * @param isKnownStaleFilename optional: true for a candidate whose live filename is KNOWN to - * disagree with the saved page the replacement's oldSrc was read from — e.g. an - * element an earlier, not-yet-saved apply already swapped. For such a candidate the - * filename check is expected to fail and the ordinal is trusted on its own; a retry - * after a partial failure re-sends those slots (BL-16744). - * @returns one {replacement, element} pair per successfully matched replacement, in the order - * they were applied (ascending ordinal) - */ -export function matchReplacementsToElements( - replacements: TReplacement[], - ordinalOf: (replacement: TReplacement) => number, - wantedFilenameOf: (replacement: TReplacement) => string, - candidates: TElement[], - candidateFilenameOf: (element: TElement) => string, - isKnownStaleFilename?: (element: TElement) => boolean, -): Array> { - const used = new Set(); - const matches: Array> = []; - [...replacements] - .sort((a, b) => ordinalOf(a) - ordinalOf(b)) - .forEach((replacement) => { - const wanted = wantedFilenameOf(replacement); - const atOrdinal = candidates[ordinalOf(replacement)]; - const element = - atOrdinal !== undefined && - !used.has(atOrdinal) && - (candidateFilenameOf(atOrdinal) === wanted || - isKnownStaleFilename?.(atOrdinal) === true) - ? atOrdinal - : candidates.find( - (candidate) => - !used.has(candidate) && - candidateFilenameOf(candidate) === wanted, - ); - if (element === undefined) { - return; - } - used.add(element); - matches.push({ replacement, element }); - }); - return matches; -} diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index 503c29056852..3976cf6525fa 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -241,14 +241,14 @@ private string GetAiImageEditorUrl() /// private class SaveThenLaunchRequest { - public string imageFileName { get; set; } public string pageId { get; set; } - /// How many slots before the clicked one on that page show the same file - /// name. The page frame counts them, because only it can see which of two - /// same-named slots the user clicked; we just carry it back to the overlay. + /// Which image slot of that page the user clicked, as its index among the + /// page's image containers in document order. The page frame counts them, because + /// only it can see which slot the user clicked; we just carry it back to the + /// overlay, where it names the book image "{pageId}:{slotIndex}". /// - public int sameNameOrdinal { get; set; } + public int slotIndex { get; set; } } /// @@ -291,7 +291,7 @@ private void HandleSaveThenLaunch(ApiRequest request) { // Must be read before SaveThen: by the time our callbacks run the request is complete. // Deliberately unguarded: the only caller is launchAiImageEditor in - // aiEditorPageCommands.ts, which always sends {imageFileName}, so a parse failure means + // aiEditorPageCommands.ts, which always sends {slotIndex}, so a parse failure means // we broke our own contract and we want to hear about it with the real exception rather // than a generic "invalid payload" that says nothing (see AGENTS.md, "Don't be overly // defensive about error handling"). @@ -805,19 +805,40 @@ out int ordinal } /// - /// True if an image-bearing element is one the user is allowed to replace. Branding, - /// license, and QR-code images are never user-changeable, so they are excluded both - /// from the list offered to the AI image editor and from being overwritten at commit. + /// The image slots of one page, in document order: its image containers. A slot's index + /// in this list is its whole identity, and it is what "{pageId}:{ordinal}" holds. /// Internal for testing. /// - /// kNotUserChangeableClasses in aiEditorPageCommands.ts holds the same three names, - /// because the page frame has to skip the same slots when it counts the same-named - /// ones ahead of the clicked one. Change one list and change the other. + /// An image container is exactly what a user may replace, which is why nothing here + /// filters. The branding, license and QR-code images live outside any container, so + /// they are not slots and cannot be edited or overwritten. + /// + /// slotIndexOnPage in aiEditorPageCommands.ts numbers the same containers on the live + /// page, so the index the page frame sends at launch means the same thing here. It has + /// one exclusion this does not need: Bloom injects controls into the live page, and a + /// save strips them, so they are never in the DOM we read. /// - internal static bool IsUserChangeableImageElement(SafeXmlElement element) => - !element.HasClass("branding") - && !element.HasClass("licenseImage") - && !element.HasClass("bloom-qrcode"); + internal static SafeXmlElement[] SelectImageSlotsOnPage(SafeXmlElement page) => + page.SafeSelectNodes( + ".//*[contains(concat(' ', normalize-space(@class), ' '), ' " + + HtmlDom.kImageContainerClass + + " ')]" + ) + .OfType() + .ToArray(); + + /// + /// The element of a slot that carries the picture: the container's own img, or the + /// container itself when it wears the picture as a background image. Null when the slot + /// holds neither, which a slot with no picture at all can do. + /// + internal static SafeXmlElement GetImageElementOfSlot(SafeXmlElement slot) + { + var img = slot.SelectSingleNode("./img") as SafeXmlElement; + if (img != null) + return img; + return (slot.GetAttribute("style") ?? "").Contains("background-image") ? slot : null; + } /// /// Locates the bytes for a history result by id. The AI image editor may store a @@ -943,10 +964,12 @@ internal static string GetPageNameForImageSlotLabel(SafeXmlElement page) // Back matter pages do carry page numbers, but they are clearer by name. var number = page.GetAttribute("data-page-number"); if (!string.IsNullOrWhiteSpace(number) && !HtmlDom.IsBackMatterPage(page)) - return string.Format( - LocalizationManager.GetString("AiImageEditor.SlotLabel.Page", "Page {0}"), - number - ); + // "Page" plus the number, rather than a "Page {0}" string of our own: the + // word already exists in our strings, and a translator meeting a bare format + // string has less to go on than the word itself. + return LocalizationManager.GetString("ReaderSetup.PageHeader", "Page") + + " " + + number; var label = page.SelectSingleNode("./div[@class='pageLabel']")?.InnerText.Trim(); if (string.IsNullOrEmpty(label)) @@ -987,17 +1010,14 @@ int slotCount "AiImageEditor.SlotLabel.CanvasBackground", "Canvas Background" ) - : string.Format( - LocalizationManager.GetString("AiImageEditor.SlotLabel.Image", "Image {0}"), - imageNumber - ); + : LocalizationManager.GetString("EditTab.CustomPage.Image", "Image") + + " " + + imageNumber; if (string.IsNullOrEmpty(pageName)) return whichSlot; - return string.Format( - LocalizationManager.GetString("AiImageEditor.SlotLabel.PageAndSlot", "{0} - {1}"), - pageName, - whichSlot - ); + // The separator is not localizable. It carries no meaning to translate, and a + // string of nothing but two placeholders and a dash gives a translator no context. + return pageName + " - " + whichSlot; } /// @@ -1062,7 +1082,7 @@ private List EnumerateBookImages(Bloom.Book.Book book) continue; var pageName = GetPageNameForImageSlotLabel(page); - var holders = HtmlDom.SelectChildImgAndBackgroundImageElements(page); + var slots = SelectImageSlotsOnPage(page); // The slots this page offers, gathered before any is added to the result, // because a slot's label depends on how many the page has: a page with one // image says just "Page 2", a page with more says "Page 2 - Image 1" and so on. @@ -1074,14 +1094,13 @@ private List EnumerateBookImages(Bloom.Book.Book book) bool isCanvasBackground, ImageCredits credits )>(); - // Ordinal is the index within the full holder list so that commit can - // re-find the element deterministically; the branding/license skip below - // only affects which slots we offer, not the indexing. - for (var ordinal = 0; ordinal < holders.Length; ordinal++) + // Ordinal is the index within the full slot list, so a slot we decline to offer + // below still holds its place. That is what lets the page frame send an index it + // worked out for itself, without knowing which slots we kept. + for (var ordinal = 0; ordinal < slots.Length; ordinal++) { - if (!(holders[ordinal] is SafeXmlElement element)) - continue; - if (!IsUserChangeableImageElement(element)) + var element = GetImageElementOfSlot(slots[ordinal]); + if (element == null) continue; var relativePath = HtmlDom.GetImageElementUrl(element).PathOnly.NotEncoded; @@ -1412,22 +1431,18 @@ out SafeXmlElement pageForDataDivSync return false; } - var holders = HtmlDom.SelectChildImgAndBackgroundImageElements(page); - if (ordinal < 0 || ordinal >= holders.Length) + var slots = SelectImageSlotsOnPage(page); + if (ordinal < 0 || ordinal >= slots.Length) { error = "Image index out of range"; return false; } - if (!(holders[ordinal] is SafeXmlElement element)) + var element = GetImageElementOfSlot(slots[ordinal]); + if (element == null) { error = "Image element not found"; return false; } - if (!IsUserChangeableImageElement(element)) - { - error = "Image is not user-changeable"; - return false; - } isCurrentPage = pageId == currentPageId; oldSrc = HtmlDom.GetImageElementUrl(element).PathOnly.NotEncoded; diff --git a/src/BloomTests/web/controllers/AiImageEditorApiTests.cs b/src/BloomTests/web/controllers/AiImageEditorApiTests.cs index b72e808bbc2c..3d0218fdb890 100644 --- a/src/BloomTests/web/controllers/AiImageEditorApiTests.cs +++ b/src/BloomTests/web/controllers/AiImageEditorApiTests.cs @@ -1231,53 +1231,107 @@ out var ordinal } // ------------------------------------------------------------------ - // IsUserChangeableImageElement: branding/license/QR images are off-limits. + // SelectImageSlotsOnPage: a page's slots are its image containers, in document + // order. The index of a slot in that list is its whole identity, and the page frame + // works out the same index for itself (slotIndexOnPage in aiEditorPageCommands.ts), + // so these two numberings have to agree. // ------------------------------------------------------------------ - private static Bloom.SafeXml.SafeXmlElement MakeImgWithClass(string className) + private static SafeXmlElement MakePageWithBody(string bodyOfPage) { - var classAttr = className == null ? "" : $" class='{className}'"; var dom = new HtmlDom( $@"
- + {bodyOfPage}
" ); - return (Bloom.SafeXml.SafeXmlElement)dom.RawDom.SelectSingleNode("//img"); + return (SafeXmlElement)dom.RawDom.SelectSingleNode("//div[@id='page1']"); } [Test] - public void IsUserChangeableImageElement_PlainImage_IsChangeable() + public void SelectImageSlotsOnPage_ReturnsContainersInDocumentOrder() { + var page = MakePageWithBody( + @"
+
" + ); + + var slots = AiImageEditorApi.SelectImageSlotsOnPage(page); + + Assert.That(slots.Length, Is.EqualTo(2)); + Assert.That( + AiImageEditorApi.GetImageElementOfSlot(slots[0]).GetAttribute("src"), + Is.EqualTo("first.png") + ); Assert.That( - AiImageEditorApi.IsUserChangeableImageElement(MakeImgWithClass(null)), - Is.True + AiImageEditorApi.GetImageElementOfSlot(slots[1]).GetAttribute("src"), + Is.EqualTo("second.png") ); } [TestCase("branding")] [TestCase("licenseImage")] [TestCase("bloom-qrcode")] - public void IsUserChangeableImageElement_ProtectedImage_IsNotChangeable(string className) + public void SelectImageSlotsOnPage_ImageOutsideAContainer_IsNotASlot(string className) { + // Branding, license and QR-code images are never in an image container, which is + // why neither side of the bridge needs a list of them: they are not slots, so + // they cannot be offered to the AI image editor or overwritten by a commit. + var page = MakePageWithBody( + $@" +
" + ); + + var slots = AiImageEditorApi.SelectImageSlotsOnPage(page); + + Assert.That(slots.Length, Is.EqualTo(1), $"'{className}' must not be a slot"); Assert.That( - AiImageEditorApi.IsUserChangeableImageElement(MakeImgWithClass(className)), - Is.False, - $"an image with class '{className}' must not be user-changeable" + AiImageEditorApi.GetImageElementOfSlot(slots[0]).GetAttribute("src"), + Is.EqualTo("real.png") ); } [Test] - public void IsUserChangeableImageElement_ProtectedClassAmongOthers_IsNotChangeable() + public void SelectImageSlotsOnPage_ClassAmongOthers_IsStillASlot() { - // The class check must find the protected class even when combined with others. + // The class test must find bloom-imageContainer among other classes, and must + // not match a class that merely contains those characters. + var page = MakePageWithBody( + @"
+
" + ); + + var slots = AiImageEditorApi.SelectImageSlotsOnPage(page); + + Assert.That(slots.Length, Is.EqualTo(1)); Assert.That( - AiImageEditorApi.IsUserChangeableImageElement( - MakeImgWithClass("bloom-imageContainer branding") - ), - Is.False + AiImageEditorApi.GetImageElementOfSlot(slots[0]).GetAttribute("src"), + Is.EqualTo("real.png") + ); + } + + [Test] + public void GetImageElementOfSlot_BackgroundImageSlot_ReturnsTheContainer() + { + // A slot can wear its picture as a background image instead of holding an img. + var page = MakePageWithBody( + @"
" ); + + var slot = AiImageEditorApi.SelectImageSlotsOnPage(page)[0]; + + Assert.That(AiImageEditorApi.GetImageElementOfSlot(slot), Is.SameAs(slot)); + } + + [Test] + public void GetImageElementOfSlot_SlotWithNoPicture_ReturnsNull() + { + var page = MakePageWithBody(@"
"); + + var slot = AiImageEditorApi.SelectImageSlotsOnPage(page)[0]; + + Assert.That(AiImageEditorApi.GetImageElementOfSlot(slot), Is.Null); } // ------------------------------------------------------------------ @@ -1671,7 +1725,7 @@ public void ReadCreditAttributes_MatchesWhatBloomsOwnUpdaterWouldWrite() // (updated by ImageUpdater) and as the next book-up-to-date pass. Pin that // agreement down rather than trusting the two to stay in step by inspection. var name = MakePngWithCredits("pic.png", "Ada Lovelace", "Copyright 1843 Ada"); - var img = MakeImgWithClass(null); // its src is "pic.png", the file we just made + var img = MakePlainImg(); // its src is "pic.png", the file we just made ImageUpdater.UpdateImgMetadataAttributesToMatchImage( _bookFolder.Path, @@ -1689,6 +1743,19 @@ public void ReadCreditAttributes_MatchesWhatBloomsOwnUpdaterWouldWrite() Assert.That(attributes.license, Is.EqualTo(img.GetAttribute("data-license"))); } + // A plain image element, for a test that needs one and nothing around it. + private static SafeXmlElement MakePlainImg() + { + var dom = new HtmlDom( + @" +
+ +
+ " + ); + return (SafeXmlElement)dom.RawDom.SelectSingleNode("//img"); + } + // The single page of a one-page DOM, as the label helpers take it. private static SafeXmlElement FirstPageOf(string pageMarkup) {