eric/Crosspoint
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: Track block style stack for nested styles (#1582)

Browse Source 
## 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
<div class="c1">          <!-- marginLeft: 10px -->
  <div class="c2">        <!-- marginLeft: 20px -->
    <p class="c3">text</p>  <!-- marginLeft: 5px -->
  </div>
  <p class="c4">text2</p>   <!-- marginLeft: 5px -->
</div>
```

| 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 <zach@zdnelson.com>
This commit is contained in:
Dave Allie
2026-05-04 10:34:09 -05:00
committed by GitHub
co-authored by Zach Nelson
parent 6c4ae7c41a
commit a1007a4660
4 changed files with 146 additions and 69 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/Epub/Epub/Section.cpp
+1 -1
View File
@@ -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);
lib/Epub/Epub/blocks/BlockStyle.h
+49 -30
View File
@@ -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;
combinedBlockStyle.marginTop = static_cast<int16_t>(child.marginTop + marginTop);
combinedBlockStyle.marginBottom = static_cast<int16_t>(child.marginBottom + marginBottom);
combinedBlockStyle.marginLeft = static_cast<int16_t>(child.marginLeft + marginLeft);
combinedBlockStyle.marginRight = static_cast<int16_t>(child.marginRight + marginRight);
combinedBlockStyle.paddingTop = static_cast<int16_t>(child.paddingTop + paddingTop);
combinedBlockStyle.paddingBottom = static_cast<int16_t>(child.paddingBottom + paddingBottom);
combinedBlockStyle.paddingLeft = static_cast<int16_t>(child.paddingLeft + paddingLeft);
combinedBlockStyle.paddingRight = static_cast<int16_t>(child.paddingRight + paddingRight);
// Text indent: use child's if defined
if (child.textIndentDefined) {
combinedBlockStyle.textIndent = child.textIndent;
combinedBlockStyle.textIndentDefined = true;
} else {
combinedBlockStyle.textIndent = textIndent;
combinedBlockStyle.textIndentDefined = textIndentDefined;
// Return a copy with bottom margins/padding zeroed out.
[[nodiscard]] BlockStyle withoutBottom() const {
BlockStyle result = *this;
result.marginBottom = 0;
result.paddingBottom = 0;
return result;
}
// 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 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<int16_t>(paddingBottom + source.paddingBottom);
return result;
}
return combinedBlockStyle;
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<int16_t>(child.marginLeft + marginLeft);
result.marginRight = static_cast<int16_t>(child.marginRight + marginRight);
result.paddingLeft = static_cast<int16_t>(child.paddingLeft + paddingLeft);
result.paddingRight = static_cast<int16_t>(child.paddingRight + paddingRight);
if (!child.textIndentDefined && textIndentDefined) {
result.textIndent = textIndent;
result.textIndentDefined = true;
}
if (!child.textAlignDefined && textAlignDefined) {
result.alignment = alignment;
result.textAlignDefined = true;
}
} else {
result.marginTop = std::max(child.marginTop, marginTop);
result.marginBottom = std::max(child.marginBottom, marginBottom);
result.paddingTop = static_cast<int16_t>(child.paddingTop + paddingTop);
result.paddingBottom = static_cast<int16_t>(child.paddingBottom + paddingBottom);
}
return result;
}
// Create a BlockStyle from CSS style properties, resolving CssLength values to pixels
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
+97 -40
View File
@@ -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 <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.
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<uint16_t>(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.
// <div style="margin: 1em 40%">), 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<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;
@@ -380,8 +394,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);
@@ -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<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);
@@ -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<ImageBlock>(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<uint8_t>(CssTextAlign::None))
? CssTextAlign::Justify
: static_cast<CssTextAlign>(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 <br/> 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 <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();
style.textAlignDefined = false;
style.alignment = (self->paragraphAlignment == static_cast<uint8_t>(CssTextAlign::None))
? CssTextAlign::Justify
: static_cast<CssTextAlign>(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() {
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<uint8_t>(CssTextAlign::None))
// 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<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;
const auto align = rootBlockStyle.alignment;
paragraphAlignmentBlockStyle.alignment = align;
startNewTextBlock(paragraphAlignmentBlockStyle);
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.h
+1
View File
@@ -62,6 +62,7 @@ class ChapterHtmlSlimParser {
bool hasUnderline = false, underline = false;
};
std::vector<StyleStackEntry> inlineStyleStack;
std::vector<BlockStyle> blockStyleStack; // accumulated block styles from open ancestor elements
CssStyle currentCssStyle;
bool effectiveBold = false;
bool effectiveItalic = false;