Fix BL-16819 "Opaque" setting is forgotten on cover

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<string> _classesToRemoveIfAbsent = new HashSet<string>(
-            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<Tuple<string, XmlString>> 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",
+        };
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        private static void RestoreImgClassesFromDataDiv(
+            SafeXmlElement img,
+            List<Tuple<string, XmlString>> 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);
+            }
+        }
+
         /// <summary>
         /// In some cases, we're better off copying from another national language than leaving the field empty.
         /// </summary>

src/BloomTests/Book/BookDataTests.cs

@@ -3316,6 +3316,137 @@ bool expectStyle
             }
         }
 
+        /// <summary>
+        /// 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).
+        /// </summary>
+        [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(
+                @"<html ><head></head><body>
+				<div id='bloomDataDiv'>
+					<div data-book='coverImage' lang='*' src='old.png'>old.png</div>
+				</div>
+				<div class='bloom-page'>
+					 <div class='bloom-canvas'>
+						<img data-book='coverImage' src='old.png'></img>
+					</div>
+				</div>
+				</body></html>"
+            );
+            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(
+                    $@"<html ><head></head><body>
+					<div class='bloom-page'>
+						 <div class='bloom-canvas'>
+							<img data-book='coverImage' src='new.png' class='{transparencyClass}'></img>
+						</div>
+					</div>
+				 </body></html>"
+                );
+                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"
+                );
+            }
+        }
+
+        /// <summary>
+        /// 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).
+        /// </summary>
+        [Test]
+        public void SynchronizeDataItemsThroughoutDOM_DataDivCoverImageIsAuto_RemovesStaleTransparencyClass_CopiesNoOtherClasses()
+        {
+            var dom = new HtmlDom(
+                @"<html ><head></head><body>
+				<div id='bloomDataDiv'>
+					<div data-book='coverImage' src='new.png' class='bloom-imageLoadError'>new.png</div>
+				</div>
+				<div class='bloom-page'>
+					 <div class='bloom-canvas'>
+						<img data-book='coverImage' src='placeholder.png' class='bloom-opaque bloom-transparent someOtherClass'></img>
+					</div>
+				</div>
+				</body></html>"
+            );
+            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()
         {