Merge pull request #25 from jpirnay/fix-pr1582

fix: Adapt upstream PR1582 by daveallie
2026-04-06 12:14:47 +02:00
parent ed8c23ee72 22267f038a
commit d4df19d686
9 changed files with 178 additions and 50 deletions
@@ -12,7 +12,7 @@
 #include "parsers/ChapterHtmlSlimParser.h"

 namespace {
-constexpr uint8_t SECTION_FILE_VERSION = 20;
+constexpr uint8_t SECTION_FILE_VERSION = 22;
 constexpr uint32_t HEADER_SIZE = sizeof(uint8_t) +   // SECTION_FILE_VERSION
                                 sizeof(int) +       // fontId
                                 sizeof(float) +     // lineCompression
@@ -122,6 +122,28 @@ bool isZeroHeightSpacerParagraph(const char* name, const std::string& styleAttr)
  return hasZeroHeight && hasZeroMargin && hasZeroBorder;
 }

+BlockStyle getInheritedBlockStyle(const BlockStyle& parent, const BlockStyle& child) {
+  BlockStyle inherited = child;
+
+  inherited.marginLeft = static_cast<int16_t>(parent.marginLeft + child.marginLeft);
+  inherited.marginRight = static_cast<int16_t>(parent.marginRight + child.marginRight);
+  inherited.paddingLeft = static_cast<int16_t>(parent.paddingLeft + child.paddingLeft);
+  inherited.paddingRight = static_cast<int16_t>(parent.paddingRight + child.paddingRight);
+
+  if (!child.textIndentDefined) {
+    inherited.textIndent = parent.textIndent;
+    inherited.textIndentDefined = parent.textIndentDefined;
+  }
+
+  if (!child.textAlignDefined) {
+    inherited.alignment = parent.alignment;
+    inherited.textAlignDefined = parent.textAlignDefined;
+  }
+
+  inherited.fromBrElement = false;
+  return inherited;
+}
+
 // Update effective bold/italic/underline based on block style and inline style stack
 void ChapterHtmlSlimParser::updateEffectiveInlineStyle() {
  // Start with block-level styles
@@ -176,11 +198,9 @@ 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 <div style="margin-bottom:2em"><h1>text</h1></div> where the
-      // div's margin should be preserved, even though it has no direct text content.
      BlockStyle incoming = blockStyle;
-      const bool brGapPending = currentTextBlock->getBlockStyle().fromBrElement;
+      const BlockStyle& currentStyle = currentTextBlock->getBlockStyle();
+      const bool brGapPending = currentStyle.fromBrElement;
      if (brGapPending) {
        // The empty block was created by a <br> section separator. Inject a full line of
        // blank space before the following paragraph so the scene/section break is visible.
@@ -188,12 +208,8 @@ void ChapterHtmlSlimParser::startNewTextBlock(const BlockStyle& blockStyle) {
        const int16_t lineHeight = static_cast<int16_t>(renderer.getLineHeight(fontId) * lineCompression + 0.5f);
        incoming.marginTop = static_cast<int16_t>(incoming.marginTop + lineHeight);
      }
-
-      BlockStyle merged = currentTextBlock->getBlockStyle().getCombinedBlockStyle(incoming);
-      // Preserve only whether the current empty block still represents <br> separators.
-      // This lets consecutive <br> accumulate one line each without leaking the flag to real content blocks.
-      merged.fromBrElement = blockStyle.fromBrElement;
-      currentTextBlock->setBlockStyle(merged);
+      incoming.fromBrElement = blockStyle.fromBrElement;
+      currentTextBlock->setBlockStyle(incoming);

      if (!pendingAnchorId.empty()) {
        if (std::find(tocAnchors.begin(), tocAnchors.end(), pendingAnchorId) != tocAnchors.end()) {
@@ -475,19 +491,25 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
                }
                const bool hasCssHeight = imgStyle.hasImageHeight();
                const bool hasCssWidth = imgStyle.hasImageWidth();
+                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
+                  // Both CSS height and width set: resolve both, then clamp to the current container preserving ratio.
                  displayHeight = static_cast<int>(
                      imgStyle.imageHeight.toPixels(emSize, static_cast<float>(self->viewportHeight)) + 0.5f);
-                  displayWidth = static_cast<int>(
-                      imgStyle.imageWidth.toPixels(emSize, static_cast<float>(self->viewportWidth)) + 0.5f);
+                  displayWidth =
+                      static_cast<int>(imgStyle.imageWidth.toPixels(emSize, static_cast<float>(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<float>(self->viewportWidth) / displayWidth
-                                       : 1.0f;
+                  if (displayWidth > containerWidth || displayHeight > self->viewportHeight) {
+                    float scaleX =
+                        (displayWidth > containerWidth) ? static_cast<float>(containerWidth) / displayWidth : 1.0f;
                    float scaleY = (displayHeight > self->viewportHeight)
                                       ? static_cast<float>(self->viewportHeight) / displayHeight
                                       : 1.0f;
@@ -512,8 +534,8 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
                        static_cast<int>(displayHeight * (static_cast<float>(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<int>(displayWidth * (static_cast<float>(dims.height) / dims.width) + 0.5f);
@@ -522,10 +544,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<int>(
-                      imgStyle.imageWidth.toPixels(emSize, static_cast<float>(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<int>(imgStyle.imageWidth.toPixels(emSize, static_cast<float>(containerWidth)) + 0.5f);
+                  if (displayWidth > containerWidth) displayWidth = containerWidth;
                  if (displayWidth < 1) displayWidth = 1;
                  displayHeight =
                      static_cast<int>(displayWidth * (static_cast<float>(dims.height) / dims.width) + 0.5f);
@@ -539,8 +561,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 the current 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;
@@ -654,7 +676,10 @@ 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);
+        const BlockStyle altBlockStyle = self->blockStyleStack.empty()
+                                             ? centeredBlockStyle
+                                             : getInheritedBlockStyle(self->blockStyleStack.back(), centeredBlockStyle);
+        self->startNewTextBlock(altBlockStyle);
        self->italicUntilDepth = std::min(self->italicUntilDepth, self->depth);
        self->depth += 1;
        self->characterData(userData, alt.c_str(), alt.length());
@@ -768,7 +793,9 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
    if (self->embeddedStyle && cssStyle.hasTextAlign()) {
      headerBlockStyle.alignment = cssStyle.textAlign;
    }
-    self->startNewTextBlock(headerBlockStyle);
+    const BlockStyle inheritedHeaderBlockStyle = getInheritedBlockStyle(self->blockStyleStack.back(), headerBlockStyle);
+    self->blockStyleStack.push_back(inheritedHeaderBlockStyle);
+    self->startNewTextBlock(inheritedHeaderBlockStyle);
    self->boldUntilDepth = std::min(self->boldUntilDepth, self->depth);
    self->updateEffectiveInlineStyle();
  } else if (matches(name, BLOCK_TAGS, NUM_BLOCK_TAGS)) {
@@ -780,7 +807,9 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
        blockStyle.alignment = cssStyle.textAlign;
        blockStyle.textAlignDefined = true;
      }
-      self->startNewTextBlock(blockStyle);
+      const BlockStyle inheritedBlockStyle = getInheritedBlockStyle(self->blockStyleStack.back(), blockStyle);
+      self->blockStyleStack.push_back(inheritedBlockStyle);
+      self->startNewTextBlock(inheritedBlockStyle);
      self->updateEffectiveInlineStyle();

      self->skipTextUntilDepth = self->depth;
@@ -814,7 +843,9 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
        blockStyle.alignment = cssStyle.textAlign;
        blockStyle.textAlignDefined = true;
      }
-      self->startNewTextBlock(blockStyle);
+      const BlockStyle inheritedBlockStyle = getInheritedBlockStyle(self->blockStyleStack.back(), blockStyle);
+      self->blockStyleStack.push_back(inheritedBlockStyle);
+      self->startNewTextBlock(inheritedBlockStyle);
      self->updateEffectiveInlineStyle();

      if (strcmp(name, "li") == 0) {
@@ -1258,29 +1289,24 @@ 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 <h1> (default
-    // Center) followed by an image-only <p> 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();
-      // Keep alignment only when closing the <br> separator itself so subsequent text
-      // within the same block container stays aligned. Reset alignment when closing
-      // other block tags (e.g. div/p) to avoid leaking centered/right alignment globally.
-      const bool preserveForBrClose = style.fromBrElement && strcmp(name, "br") == 0;
-      if (!preserveForBrClose) {
-        style.textAlignDefined = false;
-        style.alignment = (self->paragraphAlignment == static_cast<uint8_t>(CssTextAlign::None))
-                              ? CssTextAlign::Justify
-                              : static_cast<CssTextAlign>(self->paragraphAlignment);
-        self->currentTextBlock->setBlockStyle(style);
+    if (strcmp(name, "br") != 0 && self->blockStyleStack.size() > 1) {
+      self->blockStyleStack.pop_back();
+      if (self->currentTextBlock && self->currentTextBlock->isEmpty()) {
+        self->currentTextBlock->setBlockStyle(self->blockStyleStack.back());
      }
    }
  }
 }

 bool ChapterHtmlSlimParser::parseAndBuildPages() {
+  BlockStyle rootBlockStyle;
+  rootBlockStyle.alignment = (this->paragraphAlignment == static_cast<uint8_t>(CssTextAlign::None))
+                                 ? CssTextAlign::Justify
+                                 : static_cast<CssTextAlign>(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)
@@ -65,6 +65,7 @@ class ChapterHtmlSlimParser {
    bool hasUnderline = false, underline = false;
  };
  std::vector<StyleStackEntry> inlineStyleStack;
+  std::vector<BlockStyle> blockStyleStack;
  CssStyle currentCssStyle;
  bool effectiveBold = false;
  bool effectiveItalic = false;
@@ -5,6 +5,7 @@ Generate test EPUBs for rendering verification.
 Creates EPUBs to verify:
 - Image: Grayscale rendering (4 levels), scaling, centering, cache performance
 - Text: pre element line breaks, blank lines, nested code element
+- Layout: nested block margins, sibling style restoration, image wrapper spacing
 """

 import os
@@ -46,7 +47,7 @@ def get_font(size=20):
    for path in candidates:
        try:
            return ImageFont.truetype(path, size)
-        except:
+        except Exception:
            continue
    return ImageFont.load_default()

@@ -554,6 +555,7 @@ def create_epub(epub_path, title, chapters):
        # Collect all images and chapters
        manifest_items = []
        spine_items = []
+        written_images = set()

        # Add chapters and images
        for i, (chapter_title, html_content, images) in enumerate(chapters):
@@ -562,6 +564,8 @@ def create_epub(epub_path, title, chapters):

            # Add images for this chapter
            for img_filename, img_data in images:
+                if img_filename in written_images:
+                    continue
                media_type = (
                    "image/png" if img_filename.endswith(".png") else "image/jpeg"
                )
@@ -569,6 +573,7 @@ def create_epub(epub_path, title, chapters):
                    f'    <item id="{img_filename.replace(".", "_")}" href="images/{img_filename}" media-type="{media_type}"/>'
                )
                epub.writestr(f"OEBPS/images/{img_filename}", img_data)
+                written_images.add(img_filename)

            # Add chapter
            manifest_items.append(
@@ -618,12 +623,12 @@ def create_epub(epub_path, title, chapters):
        epub.writestr("OEBPS/nav.xhtml", nav_xhtml)


-def make_chapter(title, body_content):
+def make_chapter(title, body_content, head_content=""):
    """Create XHTML chapter content."""
    return f"""<?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE html>
 <html xmlns="http://www.w3.org/1999/xhtml">
-<head><title>{title}</title></head>
+<head><title>{title}</title>{head_content}</head>
 <body>
 <h1>{title}</h1>
 {body_content}
@@ -1001,6 +1006,102 @@ def main():
            OUTPUT_DIR / "test_mixed_images.epub", "Mixed Format Tests", mixed_chapters
        )

+        print("Creating layout regression test EPUB...")
+
+        layout_chapters = [
+            (
+                "Introduction",
+                make_chapter(
+                    "Layout Regression Tests",
+                    """
+<p>This EPUB exercises recent parser edge cases around nested block styles and image wrappers.</p>
+<p><strong>Recommended settings:</strong> Paragraph Alignment set to Book Style or Justify.</p>
+<p>This regression EPUB uses inline style attributes rather than a head &lt;style&gt; block so it works even if chapter-local embedded CSS is not loaded.</p>
+<ul>
+<li>Nested horizontal margin inheritance for sibling blocks</li>
+<li>Vertical paragraph spacing should not explode with nested wrappers</li>
+<li>Image wrapper spacing should apply to the image, not leak into following text</li>
+<li>Hidden images should not leave a large blank gap before the next paragraph</li>
+</ul>
+""",
+                ),
+                [],
+            ),
+            (
+                "1. Nested Horizontal Margins",
+                make_chapter(
+                    "Nested Horizontal Margin Inheritance",
+                    """
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">This chapter mirrors the c1/c2/c3/c4 example behind PR 1582.</p>
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">Expected: C3 is indented the most. C4 is indented less than C3, but still more than the baseline paragraph outside the wrapper.</p>
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">Expected order of indentation: C3 &gt; C4 &gt; baseline paragraph.</p>
+<div style="margin-left: 24px;">
+    <div style="margin-left: 48px;">
+        <p style="margin-top: 0.7em; margin-bottom: 0.7em; margin-left: 12px;">C3 paragraph. This text should have the largest left indent because it inherits the outer wrapper, the inner wrapper, and its own left margin. Repeat text to make the paragraph wrap across multiple lines and make the effective left inset obvious while reading.</p>
+    </div>
+    <p style="margin-top: 0.7em; margin-bottom: 0.7em; margin-left: 12px;">C4 paragraph. This text should still inherit the outer wrapper indent, but not the inner wrapper indent. It should therefore appear less indented than the paragraph above, not flush with the body text.</p>
+    <p style="margin-top: 0.7em; margin-bottom: 0.7em;">Outer-wrapper-only control paragraph. This paragraph should still be indented relative to the page body because it inherits the outer wrapper margin, even though it has no paragraph-level margin-left of its own.</p>
+</div>
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">Baseline paragraph outside the wrappers. This paragraph should align with the normal body text and should be the least indented paragraph on this page.</p>
+""",
+                ),
+                [],
+            ),
+            (
+                "2. Nested Vertical Margins",
+                make_chapter(
+                    "Nested Vertical Margin Sanity",
+                    """
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">Expected: wrapper nesting should not create an oversized blank vertical gulf between these paragraphs.</p>
+<div style="margin-top: 1.6em; margin-bottom: 1.6em;">
+    <div style="margin-top: 1.2em; margin-bottom: 1.2em;">
+        <p style="margin-top: 0.8em; margin-bottom: 0.8em;">Nested vertical spacing paragraph. There should be some breathing room above and below, but not dramatically more than a normal section break.</p>
+    </div>
+    <p style="margin-top: 0.8em; margin-bottom: 0.8em;">Sibling paragraph after the nested block. Spacing before this paragraph should feel normal and should not keep growing with every ancestor wrapper.</p>
+</div>
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">Baseline paragraph after the wrapper section. This should not be pushed far down the page.</p>
+""",
+                ),
+                [],
+            ),
+            (
+                "3. Image Wrapper Spacing",
+                make_chapter(
+                    "Image Wrapper Spacing",
+                    """
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">Expected: the wrapper's margins should create space around the image, and the paragraph after the image should start with normal spacing rather than inheriting a second copy of that gap.</p>
+<div style="margin-top: 1.4em; margin-bottom: 1.4em;">
+    <div style="margin-top: 1.4em; margin-bottom: 1.4em; margin-left: 60px; margin-right: 60px;">
+        <p style="margin-top: 0; margin-bottom: 0;"><img src="images/centering_test.jpg" alt="Wrapped image spacing test" style="width: 100%;"/></p>
+    </div>
+</div>
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">Paragraph after wrapped image. If the wrapper spacing leaks, this paragraph will begin too far down the page. If container width is ignored, the image may also appear too wide for the wrapper.</p>
+""",
+                ),
+                [("centering_test.jpg", images["centering_test.jpg"])],
+            ),
+            (
+                "4. Hidden Image Spacing",
+                make_chapter(
+                    "Hidden Image Spacing Reset",
+                    """
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">Expected: the hidden image wrapper should not leave a large blank gap before the following paragraph.</p>
+<div style="margin-top: 1.4em; margin-bottom: 1.4em;">
+    <p style="margin-top: 0; margin-bottom: 0;"><img src="images/centering_test.jpg" alt="This image is intentionally hidden by CSS" style="display: none;"/></p>
+</div>
+<p style="margin-top: 0.7em; margin-bottom: 0.7em;">Paragraph after hidden image. This should follow with near-normal spacing, not the large gap that would be appropriate for a visible wrapped image.</p>
+""",
+                ),
+                [("centering_test.jpg", images["centering_test.jpg"])],
+            ),
+        ]
+
+        create_epub(
+            OUTPUT_DIR / "test_layout_regressions.epub",
+            "Layout Regression Tests",
+            layout_chapters,
+        )
+
        print("Creating text rendering test EPUB...")
        text_chapters = [
            (