From aff93f1dc08a5f18bb4215271c2ccc7622267efb Mon Sep 17 00:00:00 2001 From: jpirnay Date: Mon, 2 Mar 2026 12:02:09 +0100 Subject: [PATCH] fix: Hanging indent (negative text-indent) and em-unit sizing (#1229) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary * **What is the goal of this PR?** Fixing two independent CSS rendering bugs combined to make hanging-indent list styles (e.g. margin-left:3em; text-indent:-1em) render incorrectly: * **What changes are included?** 1. Negative text-indent was silently ignored Three guards in ParsedText.cpp (computeLineBreaks, computeHyphenatedLineBreaks, extractLine) conditioned firstLineIndent on blockStyle.textIndent > 0, so any negative value collapsed to zero. Additionally, wordXpos was uint16_t, which cannot represent negative offsets — a cast of e.g. −18 would wrap to 65518 and render the word far off-screen. 2. extraParagraphSpacing suppressed hanging indents Even after removing the > 0 guard, the existing !extraParagraphSpacing condition would still suppress all text-indent when that setting is on (its default). Positive text-indent is a decorative paragraph indent that the user can reasonably replace with vertical spacing — negative text-indent is structural (it positions the list marker) and must always apply. 3. em unit was calibrated against line height, not font size emSize was computed as getLineHeight() * lineCompression (the full line advance). CSS em units are defined relative to the font-size, which corresponds to the ascender height — not the line height. Using line height makes every em-based margin/indent ~20–30% wider than a browser would render it, and is especially noticeable for CSS that uses font-size: small (which we do not implement). ## Additional Context Test case ``` .lsl1 { margin-left: 3em; text-indent: -1em; }
• First list item that wraps across lines
• Short item
``` Before: all lines of all items started at 3 em from the left edge (indent ignored). After: the bullet marker hangs at 2 em; continuation lines align at 3 em. before after --- ### 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**_ --- lib/Epub/Epub/ParsedText.cpp | 28 +++++++++++++------ lib/Epub/Epub/Section.cpp | 2 +- lib/Epub/Epub/blocks/TextBlock.cpp | 2 +- lib/Epub/Epub/blocks/TextBlock.h | 4 +-- .../Epub/parsers/ChapterHtmlSlimParser.cpp | 5 ++-- 5 files changed, 25 insertions(+), 16 deletions(-) diff --git a/lib/Epub/Epub/ParsedText.cpp b/lib/Epub/Epub/ParsedText.cpp index 584d080b..3c26fb29 100644 --- a/lib/Epub/Epub/ParsedText.cpp +++ b/lib/Epub/Epub/ParsedText.cpp @@ -144,9 +144,12 @@ std::vector ParsedText::computeLineBreaks(const GfxRenderer& renderer, c return {}; } - // Calculate first line indent (only for left/justified text without extra paragraph spacing) + // Calculate first line indent (only for left/justified text). + // Positive text-indent (paragraph indent) is suppressed when extraParagraphSpacing is on. + // Negative text-indent (hanging indent, e.g. margin-left:3em; text-indent:-1em) always applies — + // it is structural (positions the bullet/marker), not decorative. const int firstLineIndent = - blockStyle.textIndent > 0 && !extraParagraphSpacing && + blockStyle.textIndentDefined && (blockStyle.textIndent < 0 || !extraParagraphSpacing) && (blockStyle.alignment == CssTextAlign::Justify || blockStyle.alignment == CssTextAlign::Left) ? blockStyle.textIndent : 0; @@ -275,9 +278,12 @@ std::vector ParsedText::computeHyphenatedLineBreaks(const GfxRenderer& r const int pageWidth, const int spaceWidth, std::vector& wordWidths, std::vector& continuesVec) { - // Calculate first line indent (only for left/justified text without extra paragraph spacing) + // Calculate first line indent (only for left/justified text). + // Positive text-indent (paragraph indent) is suppressed when extraParagraphSpacing is on. + // Negative text-indent (hanging indent, e.g. margin-left:3em; text-indent:-1em) always applies — + // it is structural (positions the bullet/marker), not decorative. const int firstLineIndent = - blockStyle.textIndent > 0 && !extraParagraphSpacing && + blockStyle.textIndentDefined && (blockStyle.textIndent < 0 || !extraParagraphSpacing) && (blockStyle.alignment == CssTextAlign::Justify || blockStyle.alignment == CssTextAlign::Left) ? blockStyle.textIndent : 0; @@ -443,10 +449,13 @@ void ParsedText::extractLine(const size_t breakIndex, const int pageWidth, const const size_t lastBreakAt = breakIndex > 0 ? lineBreakIndices[breakIndex - 1] : 0; const size_t lineWordCount = lineBreak - lastBreakAt; - // Calculate first line indent (only for left/justified text without extra paragraph spacing) + // Calculate first line indent (only for left/justified text). + // Positive text-indent (paragraph indent) is suppressed when extraParagraphSpacing is on. + // Negative text-indent (hanging indent, e.g. margin-left:3em; text-indent:-1em) always applies — + // it is structural (positions the bullet/marker), not decorative. const bool isFirstLine = breakIndex == 0; const int firstLineIndent = - isFirstLine && blockStyle.textIndent > 0 && !extraParagraphSpacing && + isFirstLine && blockStyle.textIndentDefined && (blockStyle.textIndent < 0 || !extraParagraphSpacing) && (blockStyle.alignment == CssTextAlign::Justify || blockStyle.alignment == CssTextAlign::Left) ? blockStyle.textIndent : 0; @@ -485,8 +494,9 @@ void ParsedText::extractLine(const size_t breakIndex, const int pageWidth, const ? spareSpace / static_cast(actualGapCount) : 0; - // Calculate initial x position (first line starts at indent for left/justified text) - auto xpos = static_cast(firstLineIndent); + // Calculate initial x position (first line starts at indent for left/justified text; + // may be negative for hanging indents, e.g. margin-left:3em; text-indent:-1em). + auto xpos = static_cast(firstLineIndent); if (blockStyle.alignment == CssTextAlign::Right) { xpos = effectivePageWidth - lineWordWidthSum - totalNaturalGaps; } else if (blockStyle.alignment == CssTextAlign::Center) { @@ -495,7 +505,7 @@ void ParsedText::extractLine(const size_t breakIndex, const int pageWidth, const // Pre-calculate X positions for words // Continuation words attach to the previous word with no space before them - std::vector lineXPos; + std::vector lineXPos; lineXPos.reserve(lineWordCount); for (size_t wordIdx = 0; wordIdx < lineWordCount; wordIdx++) { diff --git a/lib/Epub/Epub/Section.cpp b/lib/Epub/Epub/Section.cpp index 814f5029..6e5aadf9 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 = 14; +constexpr uint8_t SECTION_FILE_VERSION = 16; 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(uint32_t); diff --git a/lib/Epub/Epub/blocks/TextBlock.cpp b/lib/Epub/Epub/blocks/TextBlock.cpp index 1671e59a..e3e35d42 100644 --- a/lib/Epub/Epub/blocks/TextBlock.cpp +++ b/lib/Epub/Epub/blocks/TextBlock.cpp @@ -74,7 +74,7 @@ bool TextBlock::serialize(FsFile& file) const { std::unique_ptr TextBlock::deserialize(FsFile& file) { uint16_t wc; std::vector words; - std::vector wordXpos; + std::vector wordXpos; std::vector wordStyles; BlockStyle blockStyle; diff --git a/lib/Epub/Epub/blocks/TextBlock.h b/lib/Epub/Epub/blocks/TextBlock.h index b654dcf1..85fdd55a 100644 --- a/lib/Epub/Epub/blocks/TextBlock.h +++ b/lib/Epub/Epub/blocks/TextBlock.h @@ -13,12 +13,12 @@ class TextBlock final : public Block { private: std::vector words; - std::vector wordXpos; + std::vector wordXpos; std::vector wordStyles; BlockStyle blockStyle; public: - explicit TextBlock(std::vector words, std::vector word_xpos, + explicit TextBlock(std::vector words, std::vector word_xpos, std::vector word_styles, const BlockStyle& blockStyle = BlockStyle()) : words(std::move(words)), wordXpos(std::move(word_xpos)), diff --git a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp index e732b60a..2753f928 100644 --- a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp +++ b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp @@ -278,8 +278,7 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* int displayWidth = 0; int displayHeight = 0; - const float emSize = - static_cast(self->renderer.getLineHeight(self->fontId)) * self->lineCompression; + const float emSize = static_cast(self->renderer.getFontAscenderSize(self->fontId)); CssStyle imgStyle = self->cssParser ? self->cssParser->resolveStyle("img", classAttr) : CssStyle{}; // Merge inline style (e.g. style="height: 2em") so it overrides stylesheet rules if (!styleAttr.empty()) { @@ -505,7 +504,7 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* } } - const float emSize = static_cast(self->renderer.getLineHeight(self->fontId)) * self->lineCompression; + const float emSize = static_cast(self->renderer.getFontAscenderSize(self->fontId)); const auto userAlignmentBlockStyle = BlockStyle::fromCssStyle( cssStyle, emSize, static_cast(self->paragraphAlignment), self->viewportWidth);