From a1007a4660697c7e83fec117ce3ab8e6953bc1de Mon Sep 17 00:00:00 2001 From: Dave Allie Date: Tue, 5 May 2026 01:34:09 +1000 Subject: [PATCH] fix: Track block style stack for nested styles (#1582) ## Summary * Add new block style stack to track accumulated styles * When dropping into nested block, apply new styles on last BlockStyle in the stack and push to the back * When leaving block, pop the stack * Fix issue where the image % width always went off screen width, use current container width based on accumulated styles * Bump section cache version to regenerate pages ## Additional Context Fixes https://github.com/crosspoint-reader/crosspoint-reader/pull/1581 more correctly, and includes fix from https://github.com/crosspoint-reader/crosspoint-reader/pull/1580 Consider: ```html

text

text2

``` | Element | Expected | Before #1581 (leaked) | After #1581 (reset) | This PR (style stack) | | --- | --- | --- | --- | --- | | `c3` | 10+20+5 = 35px | 35px | 25px | 35px | | `c4` | 10+5 = 15px | 35px (wrong, leaked c2) | 5px (wrong, lost c1) | 15px | This PR replaces the reset-on-close approach with a proper block style stack. When a block element opens, its resolved style is pushed onto the stack. When it closes, the stack pops back to the parent's style. This correctly accumulates nested margins/padding while preventing style leakage to siblings. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? Yes --------- Co-authored-by: Zach Nelson --- lib/Epub/Epub/Section.cpp | 2 +- lib/Epub/Epub/blocks/BlockStyle.h | 75 ++++++---- .../Epub/parsers/ChapterHtmlSlimParser.cpp | 137 +++++++++++++----- lib/Epub/Epub/parsers/ChapterHtmlSlimParser.h | 1 + 4 files changed, 146 insertions(+), 69 deletions(-) diff --git a/lib/Epub/Epub/Section.cpp b/lib/Epub/Epub/Section.cpp index a9ecd954..d4ddbc66 100644 --- a/lib/Epub/Epub/Section.cpp +++ b/lib/Epub/Epub/Section.cpp @@ -10,7 +10,7 @@ #include "parsers/ChapterHtmlSlimParser.h" namespace { -constexpr uint8_t SECTION_FILE_VERSION = 21; +constexpr uint8_t SECTION_FILE_VERSION = 22; constexpr uint32_t HEADER_SIZE = sizeof(uint8_t) + sizeof(int) + sizeof(float) + sizeof(bool) + sizeof(uint8_t) + sizeof(uint16_t) + sizeof(uint16_t) + sizeof(uint16_t) + sizeof(bool) + sizeof(bool) + sizeof(uint8_t) + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t); diff --git a/lib/Epub/Epub/blocks/BlockStyle.h b/lib/Epub/Epub/blocks/BlockStyle.h index 4166fceb..b63b57b6 100644 --- a/lib/Epub/Epub/blocks/BlockStyle.h +++ b/lib/Epub/Epub/blocks/BlockStyle.h @@ -30,42 +30,61 @@ struct BlockStyle { bool textIndentDefined = false; // true if text-indent was explicitly set in CSS bool textAlignDefined = false; // true if text-align was explicitly set in CSS - // Combined horizontal insets (margin + padding) + // Combined insets (margin + padding) [[nodiscard]] int16_t leftInset() const { return marginLeft + paddingLeft; } [[nodiscard]] int16_t rightInset() const { return marginRight + paddingRight; } [[nodiscard]] int16_t totalHorizontalInset() const { return leftInset() + rightInset(); } + [[nodiscard]] int16_t topInset() const { return marginTop + paddingTop; } + [[nodiscard]] int16_t bottomInset() const { return marginBottom + paddingBottom; } - // Combine with another block style. Useful for parent -> child styles, where the child style should be - // applied on top of the parent's style to get the combined style. - BlockStyle getCombinedBlockStyle(const BlockStyle& child) const { - BlockStyle combinedBlockStyle; + // Return a copy with bottom margins/padding zeroed out. + [[nodiscard]] BlockStyle withoutBottom() const { + BlockStyle result = *this; + result.marginBottom = 0; + result.paddingBottom = 0; + return result; + } - combinedBlockStyle.marginTop = static_cast(child.marginTop + marginTop); - combinedBlockStyle.marginBottom = static_cast(child.marginBottom + marginBottom); - combinedBlockStyle.marginLeft = static_cast(child.marginLeft + marginLeft); - combinedBlockStyle.marginRight = static_cast(child.marginRight + marginRight); + // Return a copy with bottom margins/padding collapsed (max) with the source's. + // Uses CSS margin collapsing: adjacent parent-child margins resolve to the larger value. + [[nodiscard]] BlockStyle addBottom(const BlockStyle& source) const { + BlockStyle result = *this; + result.marginBottom = std::max(marginBottom, source.marginBottom); + result.paddingBottom = static_cast(paddingBottom + source.paddingBottom); + return result; + } - combinedBlockStyle.paddingTop = static_cast(child.paddingTop + paddingTop); - combinedBlockStyle.paddingBottom = static_cast(child.paddingBottom + paddingBottom); - combinedBlockStyle.paddingLeft = static_cast(child.paddingLeft + paddingLeft); - combinedBlockStyle.paddingRight = static_cast(child.paddingRight + paddingRight); - // Text indent: use child's if defined - if (child.textIndentDefined) { - combinedBlockStyle.textIndent = child.textIndent; - combinedBlockStyle.textIndentDefined = true; + enum class CombineAxis : uint8_t { + Horizontal = 1, // margins left/right, padding left/right, text-align, text-indent + Vertical = 2, // margins top/bottom, padding top/bottom + }; + + // Combine this style's properties with a child style along the specified axis. + // Properties on the other axis are kept from the child unchanged. + [[nodiscard]] BlockStyle getCombinedBlockStyle(const BlockStyle& child, CombineAxis axis) const { + BlockStyle result = child; + + if (axis == CombineAxis::Horizontal) { + result.marginLeft = static_cast(child.marginLeft + marginLeft); + result.marginRight = static_cast(child.marginRight + marginRight); + result.paddingLeft = static_cast(child.paddingLeft + paddingLeft); + result.paddingRight = static_cast(child.paddingRight + paddingRight); + if (!child.textIndentDefined && textIndentDefined) { + result.textIndent = textIndent; + result.textIndentDefined = true; + } + if (!child.textAlignDefined && textAlignDefined) { + result.alignment = alignment; + result.textAlignDefined = true; + } } else { - combinedBlockStyle.textIndent = textIndent; - combinedBlockStyle.textIndentDefined = textIndentDefined; + result.marginTop = std::max(child.marginTop, marginTop); + result.marginBottom = std::max(child.marginBottom, marginBottom); + result.paddingTop = static_cast(child.paddingTop + paddingTop); + result.paddingBottom = static_cast(child.paddingBottom + paddingBottom); } - // Text align: use child's if defined - if (child.textAlignDefined) { - combinedBlockStyle.alignment = child.alignment; - combinedBlockStyle.textAlignDefined = true; - } else { - combinedBlockStyle.alignment = alignment; - combinedBlockStyle.textAlignDefined = textAlignDefined; - } - return combinedBlockStyle; + + return result; } // Create a BlockStyle from CSS style properties, resolving CssLength values to pixels diff --git a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp index eee24c49..3c8694af 100644 --- a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp +++ b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp @@ -131,10 +131,13 @@ void ChapterHtmlSlimParser::startNewTextBlock(const BlockStyle& blockStyle) { if (currentTextBlock) { // already have a text block running and it is empty - just reuse it if (currentTextBlock->isEmpty()) { - // Merge with existing block style to accumulate CSS styling from parent block elements. - // This handles cases like

text

where the - // div's margin should be preserved, even though it has no direct text content. - currentTextBlock->setBlockStyle(currentTextBlock->getBlockStyle().getCombinedBlockStyle(blockStyle)); + // The stack accumulates horizontal margins and text properties from ancestors. + // Vertical margins are per-element and not inherited through the stack, but + // container elements deposit their vertical margins on the empty block when they + // open. Merge those into the new style so the first child in a container inherits + // the container's vertical spacing. + const auto style = currentTextBlock->getBlockStyle(); + currentTextBlock->setBlockStyle(style.getCombinedBlockStyle(blockStyle, BlockStyle::CombineAxis::Vertical)); if (!pendingAnchorId.empty()) { anchorData.push_back({std::move(pendingAnchorId), static_cast(completedPageCount)}); @@ -344,18 +347,29 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* const bool hasCssHeight = imgStyle.hasImageHeight(); const bool hasCssWidth = imgStyle.hasImageWidth(); + // Compute effective container width for percentage-based image sizes. + // If the image is inside a block with horizontal margins/padding (e.g. + //
), percentage widths like width:100% + // should resolve against the container width, not the full viewport. + int containerWidth = self->viewportWidth; + if (self->currentTextBlock) { + const int inset = self->currentTextBlock->getBlockStyle().totalHorizontalInset(); + if (inset > 0 && inset < self->viewportWidth) { + containerWidth = self->viewportWidth - inset; + } + } + if (hasCssHeight && hasCssWidth && dims.width > 0 && dims.height > 0) { // Both CSS height and width set: resolve both, then clamp to viewport preserving requested ratio displayHeight = static_cast( imgStyle.imageHeight.toPixels(emSize, static_cast(self->viewportHeight)) + 0.5f); - displayWidth = static_cast( - imgStyle.imageWidth.toPixels(emSize, static_cast(self->viewportWidth)) + 0.5f); + displayWidth = + static_cast(imgStyle.imageWidth.toPixels(emSize, static_cast(containerWidth)) + 0.5f); if (displayHeight < 1) displayHeight = 1; if (displayWidth < 1) displayWidth = 1; - if (displayWidth > self->viewportWidth || displayHeight > self->viewportHeight) { - float scaleX = (displayWidth > self->viewportWidth) - ? static_cast(self->viewportWidth) / displayWidth - : 1.0f; + if (displayWidth > containerWidth || displayHeight > self->viewportHeight) { + float scaleX = + (displayWidth > containerWidth) ? static_cast(containerWidth) / displayWidth : 1.0f; float scaleY = (displayHeight > self->viewportHeight) ? static_cast(self->viewportHeight) / displayHeight : 1.0f; @@ -380,8 +394,8 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* static_cast(displayHeight * (static_cast(dims.width) / dims.height) + 0.5f); if (displayWidth < 1) displayWidth = 1; } - if (displayWidth > self->viewportWidth) { - displayWidth = self->viewportWidth; + if (displayWidth > containerWidth) { + displayWidth = containerWidth; // Rescale height to preserve aspect ratio when width is clamped displayHeight = static_cast(displayWidth * (static_cast(dims.height) / dims.width) + 0.5f); @@ -390,10 +404,10 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* if (displayWidth < 1) displayWidth = 1; LOG_DBG("EHP", "Display size from CSS height: %dx%d", displayWidth, displayHeight); } else if (hasCssWidth && !hasCssHeight && dims.width > 0 && dims.height > 0) { - // Use CSS width (resolve % against viewport width) and derive height from aspect ratio - displayWidth = static_cast( - imgStyle.imageWidth.toPixels(emSize, static_cast(self->viewportWidth)) + 0.5f); - if (displayWidth > self->viewportWidth) displayWidth = self->viewportWidth; + // Use CSS width (resolve % against container width) and derive height from aspect ratio + displayWidth = + static_cast(imgStyle.imageWidth.toPixels(emSize, static_cast(containerWidth)) + 0.5f); + if (displayWidth > containerWidth) displayWidth = containerWidth; if (displayWidth < 1) displayWidth = 1; displayHeight = static_cast(displayWidth * (static_cast(dims.height) / dims.width) + 0.5f); @@ -407,8 +421,8 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* if (displayHeight < 1) displayHeight = 1; LOG_DBG("EHP", "Display size from CSS width: %dx%d", displayWidth, displayHeight); } else { - // Scale to fit viewport while maintaining aspect ratio - int maxWidth = self->viewportWidth; + // Scale to fit container while maintaining aspect ratio + int maxWidth = containerWidth; int maxHeight = self->viewportHeight; float scaleX = (dims.width > maxWidth) ? (float)maxWidth / dims.width : 1.0f; float scaleY = (dims.height > maxHeight) ? (float)maxHeight / dims.height : 1.0f; @@ -429,9 +443,24 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* self->startNewTextBlock(parentBlockStyle); } + // Apply vertical margins from the container to the image. + // Top margin lives on the empty text block (deposited via vertical merge + // in startNewTextBlock). Bottom margin was stripped by withoutBottom() for + // deferred application at element close, so read it from the stack. + int16_t imageMarginTop = 0; + int16_t imageMarginBottom = 0; + if (self->currentTextBlock && self->currentTextBlock->isEmpty()) { + const auto& bs = self->currentTextBlock->getBlockStyle(); + imageMarginTop = bs.topInset(); + if (self->blockStyleStack.size() > 1) { + imageMarginBottom = self->blockStyleStack.back().bottomInset(); + } + } + // Create page for image - only break if image won't fit remaining space if (self->currentPage && !self->currentPage->elements.empty() && - (self->currentPageNextY + displayHeight > self->viewportHeight)) { + (self->currentPageNextY + imageMarginTop + displayHeight + imageMarginBottom > + self->viewportHeight)) { self->completePageFn(std::move(self->currentPage), self->xpathParagraphIndex); self->completedPageCount++; self->currentPage.reset(new Page()); @@ -449,6 +478,9 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* self->currentPageNextY = 0; } + // Apply top margin from container block + self->currentPageNextY += imageMarginTop; + // Create ImageBlock and add to page auto imageBlock = std::make_shared(cachedImagePath, displayWidth, displayHeight); if (!imageBlock) { @@ -462,7 +494,18 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* return; } self->currentPage->elements.push_back(pageImage); - self->currentPageNextY += displayHeight; + self->currentPageNextY += displayHeight + imageMarginBottom; + + // The image consumed the empty block's accumulated vertical spacing. + // Reset the block so the Vertical merge in startNewTextBlock doesn't + // re-apply the same margins to the next text paragraph. + if (self->currentTextBlock && self->currentTextBlock->isEmpty()) { + BlockStyle resetStyle; + resetStyle.alignment = (self->paragraphAlignment == static_cast(CssTextAlign::None)) + ? CssTextAlign::Justify + : static_cast(self->paragraphAlignment); + self->currentTextBlock->setBlockStyle(resetStyle); + } self->depth += 1; return; @@ -480,7 +523,9 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* // Fallback to alt text if image processing fails if (!alt.empty()) { alt = "[Image: " + alt + "]"; - self->startNewTextBlock(centeredBlockStyle); + self->startNewTextBlock(self->blockStyleStack.back() + .getCombinedBlockStyle(centeredBlockStyle, BlockStyle::CombineAxis::Horizontal) + .withoutBottom()); self->italicUntilDepth = std::min(self->italicUntilDepth, self->depth); self->depth += 1; self->characterData(userData, alt.c_str(), alt.length()); @@ -570,7 +615,10 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* if (self->embeddedStyle && cssStyle.hasTextAlign()) { headerBlockStyle.alignment = cssStyle.textAlign; } - self->startNewTextBlock(headerBlockStyle); + const auto accumulated = + self->blockStyleStack.back().getCombinedBlockStyle(headerBlockStyle, BlockStyle::CombineAxis::Horizontal); + self->blockStyleStack.push_back(accumulated); + self->startNewTextBlock(accumulated.withoutBottom()); self->boldUntilDepth = std::min(self->boldUntilDepth, self->depth); self->updateEffectiveInlineStyle(); } else if (matches(name, BLOCK_TAGS, NUM_BLOCK_TAGS)) { @@ -579,10 +627,13 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* // flush word preceding
to currentTextBlock before calling startNewTextBlock self->flushPartWordBuffer(); } - self->startNewTextBlock(self->currentTextBlock->getBlockStyle()); + self->startNewTextBlock(self->blockStyleStack.back().withoutBottom()); } else { self->currentCssStyle = cssStyle; - self->startNewTextBlock(userAlignmentBlockStyle); + const auto accumulated = self->blockStyleStack.back().getCombinedBlockStyle(userAlignmentBlockStyle, + BlockStyle::CombineAxis::Horizontal); + self->blockStyleStack.push_back(accumulated); + self->startNewTextBlock(accumulated.withoutBottom()); self->updateEffectiveInlineStyle(); if (strcmp(name, "li") == 0) { @@ -975,29 +1026,35 @@ void XMLCALL ChapterHtmlSlimParser::endElement(void* userData, const XML_Char* n self->currentCssStyle.reset(); self->updateEffectiveInlineStyle(); - // Reset alignment on empty text blocks to prevent stale alignment from bleeding - // into the next sibling element. This fixes issue #1026 where an empty

(default - // Center) followed by an image-only

causes Center to persist through the chain - // of empty block reuse into subsequent text paragraphs. - // Margins/padding are preserved so parent element spacing still accumulates correctly. - if (self->currentTextBlock && self->currentTextBlock->isEmpty()) { - auto style = self->currentTextBlock->getBlockStyle(); - style.textAlignDefined = false; - style.alignment = (self->paragraphAlignment == static_cast(CssTextAlign::None)) - ? CssTextAlign::Justify - : static_cast(self->paragraphAlignment); - self->currentTextBlock->setBlockStyle(style); + // br is self-closing and not a container — it doesn't push/pop the stack. + if (strcmp(name, "br") != 0 && self->blockStyleStack.size() > 1) { + // Apply closing element's bottom margin to the current text block so + // container spacing appears after the element's content (on the last child), + // not on the first child via the empty-block merge in startNewTextBlock. + if (self->currentTextBlock) { + const auto style = self->currentTextBlock->getBlockStyle(); + self->currentTextBlock->setBlockStyle(style.addBottom(self->blockStyleStack.back())); + } + self->blockStyleStack.pop_back(); } } } bool ChapterHtmlSlimParser::parseAndBuildPages() { + // Initialize block style stack with a root entry representing "no ancestor block elements". + // The user's paragraph alignment is set as the default so child elements without explicit + // text-align inherit it correctly through getCombinedBlockStyle. + BlockStyle rootBlockStyle; + rootBlockStyle.alignment = (this->paragraphAlignment == static_cast(CssTextAlign::None)) + ? CssTextAlign::Justify + : static_cast(this->paragraphAlignment); + blockStyleStack.clear(); + blockStyleStack.reserve(8); + blockStyleStack.push_back(rootBlockStyle); + auto paragraphAlignmentBlockStyle = BlockStyle(); paragraphAlignmentBlockStyle.textAlignDefined = true; - // Resolve None sentinel to Justify for initial block (no CSS context yet) - const auto align = (this->paragraphAlignment == static_cast(CssTextAlign::None)) - ? CssTextAlign::Justify - : static_cast(this->paragraphAlignment); + const auto align = rootBlockStyle.alignment; paragraphAlignmentBlockStyle.alignment = align; startNewTextBlock(paragraphAlignmentBlockStyle); diff --git a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.h b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.h index 8e61b5ac..9f4cf2b5 100644 --- a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.h +++ b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.h @@ -62,6 +62,7 @@ class ChapterHtmlSlimParser { bool hasUnderline = false, underline = false; }; std::vector inlineStyleStack; + std::vector blockStyleStack; // accumulated block styles from open ancestor elements CssStyle currentCssStyle; bool effectiveBold = false; bool effectiveItalic = false;