From b9863a3df29cbddbfd30bb5d21f9bf5b51cdf7c7 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Tue, 8 Sep 2026 11:02:38 -0500 Subject: [PATCH] Fix BL-16819 "Opaque" setting is forgotten on cover https://issues.bloomlibrary.org/youtrack/issue/BL-16819 The user's Transparency choice for an image (Auto / Transparent / Opaque in the image menu) is stored as a bloom-opaque or bloom-transparent class on the img. Every time a book is opened, Bloom regenerates the xmatter from the template and refills the cover image from the data-div in BookData.UpdateImageFromDataSet. That method copied the src back (and style, in special cases) but never the class attribute, so the cover reverted to Auto on every reopen. Content pages were unaffected because their images do not round-trip through the data-div. There was a second half to the bug: saving a page merges the img's classes into the data-div entry as a union, only ever adding (apart from a short remove-if-absent list). So even with the classes restored, changing Opaque back to Auto (or to Transparent) would have brought the old choice back on reopen. The fix: - UpdateImageFromDataSet now restores a short list of img classes that are user data (_imgClassesToRestoreFromDataDiv, currently the two transparency classes) from the saved class list, removing a listed class the data-div lacks so that Auto is restored too. No other classes are copied; bloom-imageLoadError in particular is still re-derived each time the book is opened (BL-14241). - The two transparency classes are added to _classesToRemoveIfAbsent so the data-div copy follows the img when the user changes the choice. Tests: new BookDataTests cover the save-then-reopen round trip for both override classes, changing the choice afterwards (to the other override and back to Auto) and reopening again, the removal of a stale override when the data-div says Auto, and that no other classes are copied. They failed before the fix. The full C# suite passes (3333 passed, 13 skipped). Co-Authored-By: Claude Fable 5.1 --- src/BloomExe/Book/BookData.cs | 55 ++++++++++- src/BloomTests/Book/BookDataTests.cs | 131 +++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 1 deletion(-) diff --git a/src/BloomExe/Book/BookData.cs b/src/BloomExe/Book/BookData.cs index f556c83c5d91..2eecbb4c7061 100644 --- a/src/BloomExe/Book/BookData.cs +++ b/src/BloomExe/Book/BookData.cs @@ -1736,7 +1736,17 @@ public void GatherDataItemsFromXElement( // doesn't have them. (ui-suppressHighlight should never get into the DOM at all, but if it somehow sneaks by, // at least the next Save should be able to remove it.) static HashSet _classesToRemoveIfAbsent = new HashSet( - new[] { "bloom-postAudioSplit", "ui-suppressHighlight" } + new[] + { + "bloom-postAudioSplit", + "ui-suppressHighlight", + // The user's Transparency choice for an image (Opaque/Transparent; Auto is the absence + // of both). Changing the choice removes the old class from the img, and the data-div + // copy must follow, or the old choice comes back when the cover image is restored from + // the data-div on the next open. See _imgClassesToRestoreFromDataDiv and BL-16819. + "bloom-opaque", + "bloom-transparent", + } ); private List> GetAttributesToSave(SafeXmlElement node) @@ -2508,9 +2518,52 @@ internal bool UpdateImageFromDataSet(DataSet data, SafeXmlElement node, string k { HtmlDom.ReconstructBackgroundImgWrapper(node, backgroundImgValues); } + + RestoreImgClassesFromDataDiv(imgOrDivWithBackgroundImage, otherAttributes); return true; } + // The img classes that are user data and so must be restored from the data-div copy when + // an image (currently only the cover image) is refilled from it. Classes in general are + // deliberately NOT copied back (e.g. bloom-imageLoadError is meant to be re-derived each + // time the book is opened), so a class only gets restored by being listed here. + // A class listed here should normally also be in _classesToRemoveIfAbsent, so that the + // data-div copy follows the img when the class is removed from it. + // Currently these are the classes that record the user's Transparency choice for an image + // (Auto is the absence of both). See getImageTransparencyMode in bloomImages.ts and + // HtmlDom.GetImageTransparencyMode. + private static readonly string[] _imgClassesToRestoreFromDataDiv = + { + "bloom-opaque", + "bloom-transparent", + }; + + /// + /// Make the img's _imgClassesToRestoreFromDataDiv classes match the class attribute saved + /// in the data-div. The data-div copy is authoritative: a listed class it lacks is removed + /// from the image (so, for the transparency classes, Auto is restored too). Without this, + /// the user's choice would be lost every time the xmatter is regenerated from the template. + /// See BL-16819. + /// + private static void RestoreImgClassesFromDataDiv( + SafeXmlElement img, + List> savedAttributes + ) + { + var savedClasses = + savedAttributes + ?.Find(a => a.Item1 == "class") + ?.Item2.Unencoded.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries) + ?? new string[0]; + foreach (var className in _imgClassesToRestoreFromDataDiv) + { + if (savedClasses.Contains(className)) + img.AddClass(className); + else + img.RemoveClass(className); + } + } + /// /// In some cases, we're better off copying from another national language than leaving the field empty. /// diff --git a/src/BloomTests/Book/BookDataTests.cs b/src/BloomTests/Book/BookDataTests.cs index 3995f0e4a626..75b237d6c0c3 100644 --- a/src/BloomTests/Book/BookDataTests.cs +++ b/src/BloomTests/Book/BookDataTests.cs @@ -3316,6 +3316,137 @@ bool expectStyle } } + /// + /// BL-16819: the user's Transparency choice for the cover image (Opaque or Transparent, as + /// opposed to Auto) is stored as a class on the img. It must survive the round trip through + /// the data-div that happens every time the book is opened and the xmatter is regenerated, + /// and so must a later change of that choice, including back to Auto (no class at all). + /// + [TestCase("bloom-opaque", "bloom-transparent")] + [TestCase("bloom-transparent", "bloom-opaque")] + [TestCase("bloom-opaque", "")] + [TestCase("bloom-transparent", "")] + public void SuckInDataFromEditedDom_ThenSynchronize_CoverImageTransparencyChoiceSurvives( + string firstChoice, + string secondChoice + ) + { + // Like a real book, the data-div entry has lang='*', so that saving the page updates + // that entry in place (merging attributes) rather than creating a new one. + var bookDom = new HtmlDom( + @" +
+
old.png
+
+
+
+ +
+
+ " + ); + var data = new BookData(bookDom, _collectionSettings, null); + var dataDivImageXpath = "//div[@id='bloomDataDiv']/div[@data-book='coverImage']"; + var pageImageXpath = "//div[@class='bloom-page']//img[@data-book='coverImage']"; + + // The user picks a Transparency option from the image menu and Bloom saves the page. + void SaveCoverWithTransparencyChoice(string transparencyClass) + { + var editedPageDom = new HtmlDom( + $@" +
+
+ +
+
+ " + ); + data.SuckInDataFromEditedDom(editedPageDom); + } + + // Simulate reopening the book: the xmatter is regenerated from the template (whose + // img has no transparency class), then filled in from the data-div. + SafeXmlElement ReopenBookAndGetCoverImage() + { + var templateImage = (SafeXmlElement) + bookDom.SelectSingleNodeHonoringDefaultNS(pageImageXpath); + templateImage.RemoveAttribute("class"); + data.SynchronizeDataItemsThroughoutDOM(); + return (SafeXmlElement)bookDom.SelectSingleNodeHonoringDefaultNS(pageImageXpath); + } + + SaveCoverWithTransparencyChoice(firstChoice); + var dataDivImages = bookDom.SafeSelectNodes(dataDivImageXpath); + Assert.That(dataDivImages.Length, Is.EqualTo(1), "sanity check"); + Assert.That(dataDivImages[0].GetAttribute("src"), Is.EqualTo("new.png")); + Assert.That( + dataDivImages[0].GetAttribute("class"), + Contains.Substring(firstChoice), + "the transparency choice should be saved in the data-div" + ); + + var pageImage = ReopenBookAndGetCoverImage(); + Assert.That(pageImage.GetAttribute("src"), Is.EqualTo("new.png")); + Assert.That( + pageImage.HasClass(firstChoice), + Is.True, + $"{firstChoice} should be restored to the cover image from the data-div" + ); + + // Now the user changes their mind. + SaveCoverWithTransparencyChoice(secondChoice); + pageImage = ReopenBookAndGetCoverImage(); + foreach (var transparencyClass in new[] { "bloom-opaque", "bloom-transparent" }) + { + Assert.That( + pageImage.HasClass(transparencyClass), + Is.EqualTo(transparencyClass == secondChoice), + $"after changing from {firstChoice} to '{secondChoice}' and reopening, " + + $"{transparencyClass} should {(transparencyClass == secondChoice ? "" : "not ")}be on the cover image" + ); + } + } + + /// + /// BL-16819: the data-div is authoritative, so if it says the cover image is on Auto (no + /// transparency class), a stale override on the page image must be removed. And copying the + /// transparency classes must not start copying other classes; in particular + /// bloom-imageLoadError is deliberately re-derived each time the book is opened (BL-14241). + /// + [Test] + public void SynchronizeDataItemsThroughoutDOM_DataDivCoverImageIsAuto_RemovesStaleTransparencyClass_CopiesNoOtherClasses() + { + var dom = new HtmlDom( + @" +
+
new.png
+
+
+
+ +
+
+ " + ); + var data = new BookData(dom, _collectionSettings, null); + data.SynchronizeDataItemsThroughoutDOM(); + var pageImage = (SafeXmlElement) + dom.SelectSingleNodeHonoringDefaultNS("//img[@data-book='coverImage']"); + Assert.That(pageImage.GetAttribute("src"), Is.EqualTo("new.png")); + Assert.That(pageImage.HasClass("bloom-opaque"), Is.False); + Assert.That(pageImage.HasClass("bloom-transparent"), Is.False); + Assert.That( + pageImage.HasClass("someOtherClass"), + Is.True, + "unrelated classes on the page image should be left alone" + ); + Assert.That( + pageImage.HasClass("bloom-imageLoadError"), + Is.False, + "classes other than the transparency ones should not be copied from the data-div" + ); + } + [Test] public void SynchronizeDataItemsThroughoutDOM_CopiesTextBoxAudioData_ButNotJunkData_RemovesUnwantedItems() {