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() {