From 7c4f69680ccca1ce6c88180a31047e6d8275c3f3 Mon Sep 17 00:00:00 2001 From: DestinySpeaker Date: Thu, 19 Feb 2026 21:34:28 -0800 Subject: [PATCH] fix: Fixed Image Sizing When No Width is Set (#1002) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary * **What is the goal of this PR?** (e.g., Implements the new feature for file uploading.) When no width is set for an image, the image currently automatically sets to the width of the page. However, with this fix, the parser will use the height and aspect ratio of the image to properly set a height for it. See below example: Before: ![IMG_8862](https://github.com/user-attachments/assets/64b3b92f-1165-45ca-8bdb-8e69613d9725) After: ![IMG_8863](https://github.com/user-attachments/assets/5cb99b12-d150-4b37-ae4c-c8a20eb9f3a0) * **What changes are included?✱ Changes to the CSS parser ## Additional Context * Add any other information that might be helpful for the reviewer (e.g., performance implications, potential risks, specific areas to focus on). --- ### 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, Cursor --- lib/Epub/Epub.cpp | 23 +++-- lib/Epub/Epub/Section.cpp | 1 + lib/Epub/Epub/css/CssParser.cpp | 58 ++++++++--- lib/Epub/Epub/css/CssParser.h | 5 + lib/Epub/Epub/css/CssStyle.h | 23 ++++- .../Epub/parsers/ChapterHtmlSlimParser.cpp | 95 +++++++++++++++++-- 6 files changed, 172 insertions(+), 33 deletions(-) diff --git a/lib/Epub/Epub.cpp b/lib/Epub/Epub.cpp index dd949e7b..da23dfe6 100644 --- a/lib/Epub/Epub.cpp +++ b/lib/Epub/Epub.cpp @@ -339,14 +339,22 @@ bool Epub::load(const bool buildIfMissing, const bool skipLoadingCss) { // Try to load existing cache first if (bookMetadataCache->load()) { - if (!skipLoadingCss && !cssParser->hasCache()) { - LOG_DBG("EBP", "Warning: CSS rules cache not found, attempting to parse CSS files"); - // to get CSS file list - if (!parseContentOpf(bookMetadataCache->coreMetadata)) { - LOG_ERR("EBP", "Could not parse content.opf from cached bookMetadata for CSS files"); - // continue anyway - book will work without CSS and we'll still load any inline style CSS + if (!skipLoadingCss) { + // Rebuild CSS cache when missing or when cache version changed (loadFromCache removes stale file) + bool needCssRebuild = !cssParser->hasCache(); + if (cssParser->hasCache() && !cssParser->loadFromCache()) { + needCssRebuild = true; + } + if (needCssRebuild) { + LOG_DBG("EBP", "CSS rules cache missing or stale, attempting to parse CSS files"); + if (!parseContentOpf(bookMetadataCache->coreMetadata)) { + LOG_ERR("EBP", "Could not parse content.opf from cached bookMetadata for CSS files"); + // continue anyway - book will work without CSS and we'll still load any inline style CSS + } + parseCssFiles(); + // Invalidate section caches so they are rebuilt with the new CSS + Storage.removeDir((cachePath + "/sections").c_str()); } - parseCssFiles(); } LOG_DBG("EBP", "Loaded ePub: %s", filepath.c_str()); return true; @@ -447,6 +455,7 @@ bool Epub::load(const bool buildIfMissing, const bool skipLoadingCss) { if (!skipLoadingCss) { // Parse CSS files after cache reload parseCssFiles(); + Storage.removeDir((cachePath + "/sections").c_str()); } LOG_DBG("EBP", "Loaded ePub: %s", filepath.c_str()); diff --git a/lib/Epub/Epub/Section.cpp b/lib/Epub/Epub/Section.cpp index cc93de72..d2ef2779 100644 --- a/lib/Epub/Epub/Section.cpp +++ b/lib/Epub/Epub/Section.cpp @@ -4,6 +4,7 @@ #include #include +#include "Epub/css/CssParser.h" #include "Page.h" #include "hyphenation/Hyphenator.h" #include "parsers/ChapterHtmlSlimParser.h" diff --git a/lib/Epub/Epub/css/CssParser.cpp b/lib/Epub/Epub/css/CssParser.cpp index 590b2215..8bdd0f1a 100644 --- a/lib/Epub/Epub/css/CssParser.cpp +++ b/lib/Epub/Epub/css/CssParser.cpp @@ -189,10 +189,18 @@ CssTextDecoration CssParser::interpretDecoration(const std::string& val) { } CssLength CssParser::interpretLength(const std::string& val) { - const std::string v = normalized(val); - if (v.empty()) return CssLength{}; + CssLength result; + tryInterpretLength(val, result); + return result; +} + +bool CssParser::tryInterpretLength(const std::string& val, CssLength& out) { + const std::string v = normalized(val); + if (v.empty()) { + out = CssLength{}; + return false; + } - // Find where the number ends size_t unitStart = v.size(); for (size_t i = 0; i < v.size(); ++i) { const char c = v[i]; @@ -205,12 +213,13 @@ CssLength CssParser::interpretLength(const std::string& val) { const std::string numPart = v.substr(0, unitStart); const std::string unitPart = v.substr(unitStart); - // Parse numeric value char* endPtr = nullptr; const float numericValue = std::strtof(numPart.c_str(), &endPtr); - if (endPtr == numPart.c_str()) return CssLength{}; // No number parsed + if (endPtr == numPart.c_str()) { + out = CssLength{}; + return false; // No number parsed (e.g. auto, inherit, initial) + } - // Determine unit type (preserve for deferred resolution) auto unit = CssUnit::Pixels; if (unitPart == "em") { unit = CssUnit::Em; @@ -221,10 +230,11 @@ CssLength CssParser::interpretLength(const std::string& val) { } else if (unitPart == "%") { unit = CssUnit::Percent; } - // px and unitless default to Pixels - return CssLength{numericValue, unit}; + out = CssLength{numericValue, unit}; + return true; } + // Declaration parsing void CssParser::parseDeclarationIntoStyle(const std::string& decl, CssStyle& style, std::string& propNameBuf, @@ -295,6 +305,18 @@ void CssParser::parseDeclarationIntoStyle(const std::string& decl, CssStyle& sty style.defined.paddingTop = style.defined.paddingRight = style.defined.paddingBottom = style.defined.paddingLeft = 1; } + } else if (propNameBuf == "height") { + CssLength len; + if (tryInterpretLength(propValueBuf, len)) { + style.imageHeight = len; + style.defined.imageHeight = 1; + } + } else if (propNameBuf == "width") { + CssLength len; + if (tryInterpretLength(propValueBuf, len)) { + style.imageWidth = len; + style.defined.imageWidth = 1; + } } } @@ -561,8 +583,7 @@ CssStyle CssParser::parseInlineStyle(const std::string& styleValue) { return par // Cache serialization -// Cache format version - increment when format changes -constexpr uint8_t CSS_CACHE_VERSION = 2; +// Cache file name (version is CssParser::CSS_CACHE_VERSION) constexpr char rulesCache[] = "/css_rules.cache"; bool CssParser::hasCache() const { return Storage.exists((cachePath + rulesCache).c_str()); } @@ -578,7 +599,7 @@ bool CssParser::saveToCache() const { } // Write version - file.write(CSS_CACHE_VERSION); + file.write(CssParser::CSS_CACHE_VERSION); // Write rule count const auto ruleCount = static_cast(rulesBySelector_.size()); @@ -613,6 +634,8 @@ bool CssParser::saveToCache() const { writeLength(style.paddingBottom); writeLength(style.paddingLeft); writeLength(style.paddingRight); + writeLength(style.imageHeight); + writeLength(style.imageWidth); // Write defined flags as uint16_t uint16_t definedBits = 0; @@ -629,6 +652,8 @@ bool CssParser::saveToCache() const { if (style.defined.paddingBottom) definedBits |= 1 << 10; if (style.defined.paddingLeft) definedBits |= 1 << 11; if (style.defined.paddingRight) definedBits |= 1 << 12; + if (style.defined.imageHeight) definedBits |= 1 << 13; + if (style.defined.imageWidth) definedBits |= 1 << 14; file.write(reinterpret_cast(&definedBits), sizeof(definedBits)); } @@ -652,9 +677,11 @@ bool CssParser::loadFromCache() { // Read and verify version uint8_t version = 0; - if (file.read(&version, 1) != 1 || version != CSS_CACHE_VERSION) { - LOG_DBG("CSS", "Cache version mismatch (got %u, expected %u)", version, CSS_CACHE_VERSION); + if (file.read(&version, 1) != 1 || version != CssParser::CSS_CACHE_VERSION) { + LOG_DBG("CSS", "Cache version mismatch (got %u, expected %u), removing stale cache for rebuild", version, + CssParser::CSS_CACHE_VERSION); file.close(); + Storage.remove((cachePath + rulesCache).c_str()); return false; } @@ -730,7 +757,8 @@ bool CssParser::loadFromCache() { if (!readLength(style.textIndent) || !readLength(style.marginTop) || !readLength(style.marginBottom) || !readLength(style.marginLeft) || !readLength(style.marginRight) || !readLength(style.paddingTop) || - !readLength(style.paddingBottom) || !readLength(style.paddingLeft) || !readLength(style.paddingRight)) { + !readLength(style.paddingBottom) || !readLength(style.paddingLeft) || !readLength(style.paddingRight) || + !readLength(style.imageHeight) || !readLength(style.imageWidth)) { rulesBySelector_.clear(); file.close(); return false; @@ -756,6 +784,8 @@ bool CssParser::loadFromCache() { style.defined.paddingBottom = (definedBits & 1 << 10) != 0; style.defined.paddingLeft = (definedBits & 1 << 11) != 0; style.defined.paddingRight = (definedBits & 1 << 12) != 0; + style.defined.imageHeight = (definedBits & 1 << 13) != 0; + style.defined.imageWidth = (definedBits & 1 << 14) != 0; rulesBySelector_[selector] = style; } diff --git a/lib/Epub/Epub/css/CssParser.h b/lib/Epub/Epub/css/CssParser.h index 60f70d23..0c6ce61d 100644 --- a/lib/Epub/Epub/css/CssParser.h +++ b/lib/Epub/Epub/css/CssParser.h @@ -30,6 +30,9 @@ */ class CssParser { public: + // Bump when CSS cache format or rules change; section caches are invalidated when this changes + static constexpr uint8_t CSS_CACHE_VERSION = 3; + explicit CssParser(std::string cachePath) : cachePath(std::move(cachePath)) {} ~CssParser() = default; @@ -113,6 +116,8 @@ class CssParser { static CssFontWeight interpretFontWeight(const std::string& val); static CssTextDecoration interpretDecoration(const std::string& val); static CssLength interpretLength(const std::string& val); + /** Returns true only when a numeric length was parsed (e.g. 2em, 50%). False for auto/inherit/initial. */ + static bool tryInterpretLength(const std::string& val, CssLength& out); // String utilities static std::string normalized(const std::string& s); diff --git a/lib/Epub/Epub/css/CssStyle.h b/lib/Epub/Epub/css/CssStyle.h index b90fa7ab..bac858e0 100644 --- a/lib/Epub/Epub/css/CssStyle.h +++ b/lib/Epub/Epub/css/CssStyle.h @@ -69,6 +69,8 @@ struct CssPropertyFlags { uint16_t paddingBottom : 1; uint16_t paddingLeft : 1; uint16_t paddingRight : 1; + uint16_t imageHeight : 1; + uint16_t imageWidth : 1; CssPropertyFlags() : textAlign(0), @@ -83,17 +85,21 @@ struct CssPropertyFlags { paddingTop(0), paddingBottom(0), paddingLeft(0), - paddingRight(0) {} + paddingRight(0), + imageHeight(0), + imageWidth(0) {} [[nodiscard]] bool anySet() const { return textAlign || fontStyle || fontWeight || textDecoration || textIndent || marginTop || marginBottom || - marginLeft || marginRight || paddingTop || paddingBottom || paddingLeft || paddingRight; + marginLeft || marginRight || paddingTop || paddingBottom || paddingLeft || paddingRight || imageHeight || + imageWidth; } void clearAll() { textAlign = fontStyle = fontWeight = textDecoration = textIndent = 0; marginTop = marginBottom = marginLeft = marginRight = 0; paddingTop = paddingBottom = paddingLeft = paddingRight = 0; + imageHeight = imageWidth = 0; } }; @@ -115,6 +121,8 @@ struct CssStyle { CssLength paddingBottom; // Padding after CssLength paddingLeft; // Padding left CssLength paddingRight; // Padding right + CssLength imageHeight; // Height for img (e.g. 2em) – width derived from aspect ratio when only height set + CssLength imageWidth; // Width for img when both or only width set CssPropertyFlags defined; // Tracks which properties were explicitly set @@ -173,6 +181,14 @@ struct CssStyle { paddingRight = base.paddingRight; defined.paddingRight = 1; } + if (base.hasImageHeight()) { + imageHeight = base.imageHeight; + defined.imageHeight = 1; + } + if (base.hasImageWidth()) { + imageWidth = base.imageWidth; + defined.imageWidth = 1; + } } [[nodiscard]] bool hasTextAlign() const { return defined.textAlign; } @@ -188,6 +204,8 @@ struct CssStyle { [[nodiscard]] bool hasPaddingBottom() const { return defined.paddingBottom; } [[nodiscard]] bool hasPaddingLeft() const { return defined.paddingLeft; } [[nodiscard]] bool hasPaddingRight() const { return defined.paddingRight; } + [[nodiscard]] bool hasImageHeight() const { return defined.imageHeight; } + [[nodiscard]] bool hasImageWidth() const { return defined.imageWidth; } void reset() { textAlign = CssTextAlign::Left; @@ -197,6 +215,7 @@ struct CssStyle { textIndent = CssLength{}; marginTop = marginBottom = marginLeft = marginRight = CssLength{}; paddingTop = paddingBottom = paddingLeft = paddingRight = CssLength{}; + imageHeight = imageWidth = CssLength{}; defined.clearAll(); } }; diff --git a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp index 7bada8f2..4fbba8af 100644 --- a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp +++ b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp @@ -257,18 +257,93 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char* if (decoder && decoder->getDimensions(cachedImagePath, dims)) { LOG_DBG("EHP", "Image dimensions: %dx%d", dims.width, dims.height); - // Scale to fit viewport while maintaining aspect ratio - int maxWidth = self->viewportWidth; - 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; - float scale = (scaleX < scaleY) ? scaleX : scaleY; - if (scale > 1.0f) scale = 1.0f; + int displayWidth = 0; + int displayHeight = 0; + const float emSize = + static_cast(self->renderer.getLineHeight(self->fontId)) * self->lineCompression; + 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()) { + imgStyle.applyOver(CssParser::parseInlineStyle(styleAttr)); + } + const bool hasCssHeight = imgStyle.hasImageHeight(); + const bool hasCssWidth = imgStyle.hasImageWidth(); - int displayWidth = (int)(dims.width * scale); - int displayHeight = (int)(dims.height * scale); + 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); + 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; + float scaleY = (displayHeight > self->viewportHeight) + ? static_cast(self->viewportHeight) / displayHeight + : 1.0f; + float scale = (scaleX < scaleY) ? scaleX : scaleY; + displayWidth = static_cast(displayWidth * scale + 0.5f); + displayHeight = static_cast(displayHeight * scale + 0.5f); + if (displayWidth < 1) displayWidth = 1; + if (displayHeight < 1) displayHeight = 1; + } + LOG_DBG("EHP", "Display size from CSS height+width: %dx%d", displayWidth, displayHeight); + } else if (hasCssHeight && !hasCssWidth && dims.width > 0 && dims.height > 0) { + // Use CSS height (resolve % against viewport height) and derive width from aspect ratio + displayHeight = static_cast( + imgStyle.imageHeight.toPixels(emSize, static_cast(self->viewportHeight)) + 0.5f); + if (displayHeight < 1) displayHeight = 1; + displayWidth = + static_cast(displayHeight * (static_cast(dims.width) / dims.height) + 0.5f); + if (displayHeight > self->viewportHeight) { + displayHeight = self->viewportHeight; + // Rescale width to preserve aspect ratio when height is clamped + displayWidth = + static_cast(displayHeight * (static_cast(dims.width) / dims.height) + 0.5f); + if (displayWidth < 1) displayWidth = 1; + } + if (displayWidth > self->viewportWidth) { + displayWidth = self->viewportWidth; + // Rescale height to preserve aspect ratio when width is clamped + displayHeight = + static_cast(displayWidth * (static_cast(dims.height) / dims.width) + 0.5f); + if (displayHeight < 1) displayHeight = 1; + } + 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; + if (displayWidth < 1) displayWidth = 1; + displayHeight = + static_cast(displayWidth * (static_cast(dims.height) / dims.width) + 0.5f); + if (displayHeight > self->viewportHeight) { + displayHeight = self->viewportHeight; + // Rescale width to preserve aspect ratio when height is clamped + displayWidth = + static_cast(displayHeight * (static_cast(dims.width) / dims.height) + 0.5f); + if (displayWidth < 1) displayWidth = 1; + } + 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; + 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; + float scale = (scaleX < scaleY) ? scaleX : scaleY; + if (scale > 1.0f) scale = 1.0f; - LOG_DBG("EHP", "Display size: %dx%d (scale %.2f)", displayWidth, displayHeight, scale); + displayWidth = (int)(dims.width * scale); + displayHeight = (int)(dims.height * scale); + LOG_DBG("EHP", "Display size: %dx%d (scale %.2f)", displayWidth, displayHeight, scale); + } // Create page for image - only break if image won't fit remaining space if (self->currentPage && !self->currentPage->elements.empty() &&