From a2dde801634125cc5f33b4dfd6306ee45c8cdec4 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 11 Sep 2026 09:00:58 -0500 Subject: [PATCH] Fix BL-16811 Front cover and title page overwrite each other's title padding The book title appears on both the front cover and the title page, and each copy needs a different measured padding-bottom so its descenders are not clipped. BookData synchronised the style attribute across every copy of a data-book field through one data-div entry, so whichever page was visited last overwrote the other, and the published cover title could lose its descender room. The two pages also re-dirtied the book on every visit. BookData now treats (bookTitle, style) as a page-dependent attribute: the value is saved in the data-div once per kind of xmatter page, as data-style-frontcover and data-style-titlepage, alongside the plain style attribute that older Bloom versions and books without variants still use. Gathering collects each xmatter page's own variant (and seeds the other pages' variants from the data-div when a single page is saved), and restoring gives each page its own variant, falling back to the plain value. The cover image, custom-layout pages, and ordinary pages are unaffected. Five new BookDataTests cover both save directions, whole-book sync, the legacy upgrade path, and an ordinary page. Co-Authored-By: Claude Sonnet 5 Co-Authored-By: Claude Fable 5.1 --- src/BloomExe/Book/BookData.cs | 283 ++++++++++++++++++++++++- src/BloomTests/Book/BookDataTests.cs | 305 +++++++++++++++++++++++++++ 2 files changed, 584 insertions(+), 4 deletions(-) diff --git a/src/BloomExe/Book/BookData.cs b/src/BloomExe/Book/BookData.cs index 02e4f15ed6c1..65e8c0f5db33 100644 --- a/src/BloomExe/Book/BookData.cs +++ b/src/BloomExe/Book/BookData.cs @@ -1655,7 +1655,28 @@ public void GatherDataItemsFromXElement( KeysOfVariablesThatAreUrlEncoded.Add(key); } - dsv.SetAttributeList(lang, GetAttributesToSave(nodeToUse)); + var attrsToSave = GetAttributesToSave(nodeToUse, key); + // The other xmatter pages' variants of any page-dependent attribute + // live only in the data-div, which we may not be reading; keep them. + var attrNamesAlreadyFound = new HashSet( + attrsToSave.Select(t => t.Item1) + ); + attrsToSave.AddRange( + GetPageDependentVariantsFromDataDiv( + key, + lang, + attrNamesAlreadyFound + ) + ); + dsv.SetAttributeList(lang, attrsToSave); + } + else + { + // We already have a value for this key/lang, and the first one found + // wins. But the page-dependent attributes are the exception: each + // xmatter page holds its own value, so we must collect this page's + // too instead of letting the first element found decide for them all. + MergePageDependentVariants(dsv, lang, nodeToUse, key); } } } @@ -1681,6 +1702,23 @@ public void GatherDataItemsFromXElement( } } + // (data-book key, attribute) pairs whose value legitimately differs from one xmatter page to + // another. The book title, for example, appears on both the front cover and the title page, + // and each is padded to fit its own font size by OverflowChecker, so a single saved + // padding-bottom would clip the descenders on one of them. + // The value is saved in the data-div once per page type, as data-- (page type + // is the page's data-xmatter-page value, lower-cased, e.g. data-style-frontcover, + // data-style-titlepage), and each page gets back its own value. The plain attribute is still + // written too, holding the most recently saved value, so older Bloom versions and books with + // no variant yet behave as before. See BL-16811. + static readonly HashSet<(string key, string attr)> _pageDependentAttributes = new HashSet<( + string, + string + )> + { + ("bookTitle", "style"), + }; + // Attributes not to copy when saving element attribute data in a DataSetElementValue. static HashSet _attributesNotToCopy = new HashSet( new[] @@ -1749,9 +1787,226 @@ public void GatherDataItemsFromXElement( } ); - private List> GetAttributesToSave(SafeXmlElement node) + /// + /// The name under which a page-dependent attribute value is saved for one kind of xmatter + /// page, e.g. "data-style-frontcover" for the style attribute on a front cover page. + /// + private static string PageDependentVariantName(string attr, string pageType) + { + return "data-" + attr + "-" + pageType; + } + + /// + /// The kind of xmatter page the given node belongs to: the lower-cased data-xmatter-page + /// value of its bloom-page ancestor. Null if it is not on an xmatter page, which includes + /// elements in the data-div (they have no bloom-page ancestor at all) and elements on + /// ordinary content pages. + /// + private static string GetXmatterPageType(SafeXmlElement node) + { + var page = node.ParentOrSelfWithClass("bloom-page"); + if (page == null) + return null; + var pageType = page.GetAttribute(kDataXmatterPage).Trim(); + if (pageType == string.Empty) + return null; + return pageType.ToLowerInvariant(); + } + + /// + /// True if the node is the data-div itself or somewhere inside it. The data-div is where the + /// page-dependent attribute variants are stored, so they must reach it verbatim. + /// + private static bool IsInDataDiv(SafeXmlElement node) + { + for ( + var current = node; + current != null; + current = current.ParentNode as SafeXmlElement + ) + { + if (current.GetAttribute("id") == "bloomDataDiv") + return true; + } + return false; + } + + /// + /// True if the given data-book key has any page-dependent attributes at all. Used to leave + /// every other key untouched by this mechanism. + /// + private static bool HasPageDependentAttributes(string key) + { + return _pageDependentAttributes.Any(pair => pair.key == key); + } + + /// + /// True if attrName is the name under which some kind of xmatter page's variant of one of + /// this key's page-dependent attributes is saved, e.g. "data-style-titlepage" when the key + /// is "bookTitle". baseAttr is then the attribute it is a variant of, e.g. "style". + /// + private static bool IsPageDependentVariantName( + string key, + string attrName, + out string baseAttr + ) + { + baseAttr = null; + foreach (var pair in _pageDependentAttributes) + { + if (pair.key != key) + continue; + var prefix = PageDependentVariantName(pair.attr, ""); + if (attrName.Length > prefix.Length && attrName.StartsWith(prefix)) + { + baseAttr = pair.attr; + return true; + } + } + return false; + } + + /// + /// Fill gaps in the attribute list already saved for key/lang from this node: a page-dependent + /// attribute variant (e.g. data-style-frontcover) the list has no entry for, and the plain + /// attribute it is a variant of if that too is missing. Without this, reading the whole book + /// (where the data-div is found first and wins) would throw away the variants the individual + /// pages carry, and could leave the data-div with variants but no plain value for anything that + /// is not on an xmatter page to fall back on. See BL-16811. + /// + /// It only fills gaps: a name the list already has keeps its value, because the first element + /// found for a key/lang wins and, when we are reading the whole book, that is deliberately the + /// data-div (see the comment in GatherDataItemsFromXElement, and BL-10739). A page's own edited + /// value reaches the data-div through the single-page read instead. + /// + private void MergePageDependentVariants( + DataSetElementValue dsv, + string lang, + SafeXmlElement node, + string key + ) + { + if (!HasPageDependentAttributes(key)) + return; + var existing = dsv.GetAttributeList(lang); + if (existing == null) + return; + var existingNames = new HashSet(existing.Select(t => t.Item1)); + foreach (var tuple in GetAttributesToSave(node, key)) + { + if (existingNames.Contains(tuple.Item1)) + continue; + if ( + IsPageDependentVariantName(key, tuple.Item1, out _) + || _pageDependentAttributes.Contains((key, tuple.Item1)) + ) + { + existing.Add(tuple); + existingNames.Add(tuple.Item1); + } + } + } + + /// + /// When we gather from a single edited page, the data-div is not part of what we read, so the + /// page-dependent attribute variants belonging to the OTHER xmatter pages are missing from the + /// data we gathered. Pushing that data back into the book would then give every other xmatter + /// page the value from the page we just read, which is the very thing this mechanism exists to + /// prevent. So this returns the variants the data-div element for key/lang already has, for + /// the caller to add to the attributes it is saving. See BL-16811. + /// + /// The names of the attributes we have already decided to + /// save from the winning element for this key and lang (including, if it is on an xmatter + /// page, its own variant such as data-style-frontcover). Those values are newer than the + /// data-div's, so no attribute with one of these names is returned. + private List> GetPageDependentVariantsFromDataDiv( + string key, + string lang, + HashSet attrNamesAlreadyFound + ) { var result = new List>(); + if (!HasPageDependentAttributes(key)) + return result; + var dataDivElement = + _dataDiv?.SelectSingleNode($"div[@data-book='{key}' and @lang='{lang}']") + as SafeXmlElement; + if (dataDivElement == null) + return result; + foreach (var attr in dataDivElement.AttributePairs) + { + if (attrNamesAlreadyFound.Contains(attr.Name)) + continue; + if (IsPageDependentVariantName(key, attr.Name, out _)) + result.Add(Tuple.Create(attr.Name, XmlString.FromUnencoded(attr.Value))); + } + return result; + } + + /// + /// Turn the attribute list saved for a data-book key into the list to apply to one particular + /// element of the book. Page-dependent attributes (see _pageDependentAttributes) are saved + /// under names like data-style-frontcover; the element on the front cover must get that value + /// as its plain style attribute, and must not get the title page's. An element that is not on + /// an xmatter page (including the data-div copy) gets none of the variants, just the plain + /// attribute, which is also what a book saved by an older Bloom has. Everything not part of + /// this mechanism passes through unchanged. See BL-16811. + /// + private List> ResolvePageDependentAttributes( + string key, + List> attrs, + SafeXmlElement targetNode + ) + { + if (attrs == null || !HasPageDependentAttributes(key)) + return attrs; + // The data-div is where the variants are kept, so it gets them exactly as saved. (That is + // also how a whole-book synchronize teaches the data-div the variants of a book that was + // last saved by a Bloom which knew nothing about them.) + if (IsInDataDiv(targetNode)) + return attrs; + var pageType = GetXmatterPageType(targetNode); + var result = new List>(); + // First, this page's own variants (e.g. data-style-frontcover for a front cover element) + // become the plain attributes they are variants of (style). Such a variant wins over the + // plain attribute in attrs, which holds whatever was saved most recently from any page. + var attrsAddedFromVariant = new HashSet(); + if (pageType != null) + { + foreach (var tuple in attrs) + { + if ( + IsPageDependentVariantName(key, tuple.Item1, out var baseAttr) + && tuple.Item1 == PageDependentVariantName(baseAttr, pageType) + ) + { + result.Add(Tuple.Create(baseAttr, tuple.Item2)); + attrsAddedFromVariant.Add(baseAttr); + } + } + } + // Then everything else, skipping the variants (this page's were handled above; the rest + // belong to other kinds of page) and any plain attribute already added from a variant. + foreach (var tuple in attrs) + { + if (IsPageDependentVariantName(key, tuple.Item1, out _)) + continue; + if (attrsAddedFromVariant.Contains(tuple.Item1)) + continue; + result.Add(tuple); + } + return result; + } + + /// + /// Collect the attributes of the given element that we want to save in the data-div (and hence + /// copy to the other elements with the same data-book key). The key is needed because a few + /// attributes are saved once per kind of xmatter page rather than once per book. + /// + private List> GetAttributesToSave(SafeXmlElement node, string key) + { + var result = new List>(); + var pageType = GetXmatterPageType(node); var isInCustomLayoutPage = HtmlDom.IsInCustomLayoutPage(node); if (node.Name == "img" && isInCustomLayoutPage) { @@ -1798,6 +2053,19 @@ private List> GetAttributesToSave(SafeXmlElement node) continue; } result.Add(Tuple.Create(attr.Name, XmlString.FromUnencoded(attr.Value))); + if (pageType != null && _pageDependentAttributes.Contains((key, attr.Name))) + { + // Save the value a second time under a name specific to this kind of xmatter page, + // so that each page can get its own value back rather than the last one saved from + // anywhere in the book. (Only attributes that reach this general case can have + // variants; class, and style on a custom layout page, are handled above.) + result.Add( + Tuple.Create( + PageDependentVariantName(attr.Name, pageType), + XmlString.FromUnencoded(attr.Value) + ) + ); + } } // if the node is an img, save some extra data if (node.Name == "img") @@ -2094,7 +2362,10 @@ SafeXmlElement node // data-div; but that is prevented by renaming the key attribute in that copy. if (attrs != null && !HtmlDom.IsInCustomLayoutPage(node)) { - MergeAttrsIntoElement(attrs, node); + MergeAttrsIntoElement( + ResolvePageDependentAttributes(key, attrs, node), + node + ); } } } @@ -2430,7 +2701,11 @@ internal bool UpdateImageFromDataSet(DataSet data, SafeXmlElement node, string k return false; var variable = data.TextVariables[key]; var getFirstAlt = variable.TextAlternatives.GetFirstAlternative(); - var otherAttributes = variable.GetAttributeList("*"); + var otherAttributes = ResolvePageDependentAttributes( + key, + variable.GetAttributeList("*"), + node + ); // Make sure we don't re-encode the new image url. var newImageUrl = KeysOfVariablesThatAreUrlEncoded.Contains(key) ? UrlPathString.CreateFromUrlEncodedString(getFirstAlt) diff --git a/src/BloomTests/Book/BookDataTests.cs b/src/BloomTests/Book/BookDataTests.cs index 9c2f98bc22ad..0d86d091933f 100644 --- a/src/BloomTests/Book/BookDataTests.cs +++ b/src/BloomTests/Book/BookDataTests.cs @@ -3654,6 +3654,311 @@ public void SynchronizeDataItemsThroughoutDOM_StripsDisplayFromStyleAttribute() Assert.That(style, Does.Contain("font-weight:bold")); } + // The title appears on both the front cover and the title page, in different styles, so + // OverflowChecker pads each of them by a different amount. BL-16811: the two padding values + // must not overwrite each other through the data-div. + private const string kTitleOnTwoXmatterPagesTemplate = + @" +
+

My Title

+
+
+
+

My Title

+
+
+
+
+

My Title

+
+
+ "; + + /// + /// The title on the given kind of xmatter page (frontCover, titlePage). + /// + private static SafeXmlElement GetTitleOnXmatterPage(HtmlDom dom, string xmatterPage) + { + var result = + dom.SelectSingleNodeHonoringDefaultNS( + $"//div[@data-xmatter-page='{xmatterPage}']//div[@data-book='bookTitle']" + ) as SafeXmlElement; + if (result == null) + Assert.Fail($"test setup problem: no bookTitle on the {xmatterPage} page"); + return result; + } + + /// + /// The page div for the given kind of xmatter page (frontCover, titlePage), as the editing + /// code would pass it to SuckInDataFromEditedDom after the user edited that page. + /// + private static SafeXmlElement GetXmatterPageElement(HtmlDom dom, string xmatterPage) + { + var result = + dom.SelectSingleNodeHonoringDefaultNS($"//div[@data-xmatter-page='{xmatterPage}']") + as SafeXmlElement; + if (result == null) + Assert.Fail($"test setup problem: no {xmatterPage} page"); + return result; + } + + /// + /// The data-div copy of the title. + /// + private static SafeXmlElement GetTitleInDataDiv(HtmlDom dom) + { + var result = + dom.SelectSingleNodeHonoringDefaultNS( + "//div[@id='bloomDataDiv']/div[@data-book='bookTitle']" + ) as SafeXmlElement; + if (result == null) + Assert.Fail("test setup problem: no bookTitle in the data-div"); + return result; + } + + /// + /// BL-16811: saving the front cover must not push the cover's padding-bottom onto the title + /// page, which has its own (recorded in the data-div as data-style-titlepage). + /// + [Test] + public void SuckInDataFromEditedDom_CoverTitleSaved_TitlePageKeepsItsOwnStyle() + { + var dom = new HtmlDom( + string.Format( + kTitleOnTwoXmatterPagesTemplate, + "data-style-titlepage='padding-bottom: 0px' style='padding-bottom: 0px'", + "style='padding-bottom: 3px'", + "style='padding-bottom: 0px'" + ) + ); + var data = new BookData(dom, _collectionSettings, null); + // Sanity checks on the starting state: the data-div knows only the title page's value, + // and each page has its own. + Assert.That( + GetTitleInDataDiv(dom).GetAttribute("data-style-frontcover"), + Is.Empty, + "sanity check: the data-div should not yet have a front cover variant" + ); + Assert.That( + GetTitleOnXmatterPage(dom, "frontCover").GetAttribute("style"), + Is.EqualTo("padding-bottom: 3px") + ); + Assert.That( + GetTitleOnXmatterPage(dom, "titlePage").GetAttribute("style"), + Is.EqualTo("padding-bottom: 0px") + ); + + data.SuckInDataFromEditedDom(GetXmatterPageElement(dom, "frontCover")); + + Assert.That( + GetTitleOnXmatterPage(dom, "frontCover").GetAttribute("style"), + Is.EqualTo("padding-bottom: 3px"), + "the cover keeps the value we just saved from it" + ); + Assert.That( + GetTitleOnXmatterPage(dom, "titlePage").GetAttribute("style"), + Is.EqualTo("padding-bottom: 0px"), + "the title page must keep its own value, not get the cover's" + ); + var dataDivTitle = GetTitleInDataDiv(dom); + Assert.That( + dataDivTitle.GetAttribute("data-style-frontcover"), + Is.EqualTo("padding-bottom: 3px") + ); + Assert.That( + dataDivTitle.GetAttribute("data-style-titlepage"), + Is.EqualTo("padding-bottom: 0px") + ); + Assert.That( + dataDivTitle.GetAttribute("style"), + Is.EqualTo("padding-bottom: 3px"), + "the plain attribute holds the most recently saved value" + ); + } + + /// + /// BL-16811: the same in the other direction. Saving the title page must not push its + /// padding-bottom onto the front cover, whose descenders would then be clipped. + /// + [Test] + public void SuckInDataFromEditedDom_TitlePageTitleSaved_CoverKeepsItsOwnStyle() + { + var dom = new HtmlDom( + string.Format( + kTitleOnTwoXmatterPagesTemplate, + "data-style-frontcover='padding-bottom: 3px' style='padding-bottom: 3px'", + "style='padding-bottom: 3px'", + "style='padding-bottom: 0px'" + ) + ); + var data = new BookData(dom, _collectionSettings, null); + Assert.That( + GetTitleInDataDiv(dom).GetAttribute("data-style-titlepage"), + Is.Empty, + "sanity check: the data-div should not yet have a title page variant" + ); + + data.SuckInDataFromEditedDom(GetXmatterPageElement(dom, "titlePage")); + + Assert.That( + GetTitleOnXmatterPage(dom, "frontCover").GetAttribute("style"), + Is.EqualTo("padding-bottom: 3px"), + "the cover must keep its own value, not get the title page's" + ); + Assert.That( + GetTitleOnXmatterPage(dom, "titlePage").GetAttribute("style"), + Is.EqualTo("padding-bottom: 0px") + ); + var dataDivTitle = GetTitleInDataDiv(dom); + Assert.That( + dataDivTitle.GetAttribute("data-style-frontcover"), + Is.EqualTo("padding-bottom: 3px") + ); + Assert.That( + dataDivTitle.GetAttribute("data-style-titlepage"), + Is.EqualTo("padding-bottom: 0px") + ); + Assert.That( + dataDivTitle.GetAttribute("style"), + Is.EqualTo("padding-bottom: 0px"), + "the plain attribute holds the most recently saved value" + ); + } + + /// + /// BL-16811: a book whose data-div has no variants yet (saved by an older Bloom) but whose + /// two xmatter pages carry different styles. A whole-book synchronize must record a variant + /// for each page rather than making them agree. + /// + [Test] + public void SynchronizeDataItemsThroughoutDOM_XmatterPageStylesDiffer_EachPageKeepsItsOwn() + { + var dom = new HtmlDom( + string.Format( + kTitleOnTwoXmatterPagesTemplate, + "", + "style='padding-bottom: 3px'", + "style='padding-bottom: 0px'" + ) + ); + var data = new BookData(dom, _collectionSettings, null); + Assert.That( + GetTitleInDataDiv(dom).GetAttribute("style"), + Is.Empty, + "sanity check: the data-div entry should start with no style at all" + ); + + data.SynchronizeDataItemsThroughoutDOM(); + + Assert.That( + GetTitleOnXmatterPage(dom, "frontCover").GetAttribute("style"), + Is.EqualTo("padding-bottom: 3px") + ); + Assert.That( + GetTitleOnXmatterPage(dom, "titlePage").GetAttribute("style"), + Is.EqualTo("padding-bottom: 0px") + ); + var dataDivTitle = GetTitleInDataDiv(dom); + Assert.That( + dataDivTitle.GetAttribute("data-style-frontcover"), + Is.EqualTo("padding-bottom: 3px") + ); + Assert.That( + dataDivTitle.GetAttribute("data-style-titlepage"), + Is.EqualTo("padding-bottom: 0px") + ); + // The plain attribute must be filled in too, so that anything without a variant of its + // own - an element on an ordinary page, or an older Bloom opening the book - still has a + // value to fall back on. + Assert.That( + dataDivTitle.GetAttribute("style"), + Is.EqualTo("padding-bottom: 3px"), + "the data-div should also gain the plain style attribute it was missing" + ); + } + + /// + /// BL-16811: a book saved by a Bloom that knew nothing about the variants (the data-div has + /// only the plain style, and the xmatter pages, freshly regenerated, have none). Both pages + /// must get the plain value, exactly as before. + /// + [Test] + public void SynchronizeDataItemsThroughoutDOM_NoVariantsSaved_BothXmatterPagesGetPlainStyle() + { + var dom = new HtmlDom( + string.Format( + kTitleOnTwoXmatterPagesTemplate, + "style='padding-bottom: 4px'", + "", + "" + ) + ); + var data = new BookData(dom, _collectionSettings, null); + Assert.That( + GetTitleOnXmatterPage(dom, "frontCover").GetAttribute("style"), + Is.Empty, + "sanity check: the pages should start with no style at all" + ); + + data.SynchronizeDataItemsThroughoutDOM(); + + Assert.That( + GetTitleOnXmatterPage(dom, "frontCover").GetAttribute("style"), + Is.EqualTo("padding-bottom: 4px") + ); + Assert.That( + GetTitleOnXmatterPage(dom, "titlePage").GetAttribute("style"), + Is.EqualTo("padding-bottom: 4px") + ); + } + + /// + /// BL-16811: the per-page mechanism only applies to xmatter pages. A title on an ordinary + /// page round-trips its plain style attribute and gets no variant. + /// + [Test] + public void SuckInDataFromEditedDom_TitleOnOrdinaryPage_PlainStyleWithNoVariant() + { + var dom = new HtmlDom( + @" +
+

My Title

+
+
+
+

My Title

+
+
+ " + ); + var data = new BookData(dom, _collectionSettings, null); + Assert.That( + GetTitleInDataDiv(dom).GetAttribute("style"), + Is.Empty, + "sanity check: the data-div entry should start with no style at all" + ); + + var page = + dom.SelectSingleNodeHonoringDefaultNS("//div[@id='page1']") as SafeXmlElement; + data.SuckInDataFromEditedDom(page); + + var dataDivTitle = GetTitleInDataDiv(dom); + Assert.That(dataDivTitle.GetAttribute("style"), Is.EqualTo("padding-bottom: 3px")); + foreach (var attr in dataDivTitle.AttributePairs) + { + Assert.That( + attr.Name, + Does.Not.StartWith("data-style-"), + "a page with no data-xmatter-page should produce no page-dependent variant" + ); + } + var pageTitle = + dom.SelectSingleNodeHonoringDefaultNS( + "//div[@id='page1']//div[@data-book='bookTitle']" + ) as SafeXmlElement; + Assert.That(pageTitle.GetAttribute("style"), Is.EqualTo("padding-bottom: 3px")); + } + [Test] public void GatherDataItemsFromXElement_BloomEditableTrailingEmptyDiv_NormalizesStoredValue() {