fix: Fixed Image Sizing When No Width is Set#1002
fix: Fixed Image Sizing When No Width is Set#1002daveallie merged 12 commits intocrosspoint-reader:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughParses CSS Changes
Sequence DiagramsequenceDiagram
participant HTML as HTML Parser
participant CSS as CSS Parser
participant Cache as Style Cache
participant Renderer as Image Renderer
HTML->>CSS: request resolved style for <img> (include inline style)
CSS->>Cache: lookup style (cache includes imageWidth/imageHeight)
Cache-->>CSS: cached style (with imageWidth/imageHeight bits)
CSS->>CSS: parse declarations (height, width) via tryInterpretLength -> set imageWidth/imageHeight & flags
CSS-->>HTML: return resolved Style
HTML->>Renderer: provide image resource + resolved Style
Renderer->>Renderer: toPixels(imageWidth/imageHeight) using emSize / viewport %
Renderer->>Renderer: compute displayWidth/displayHeight\npreserve aspect ratio\nclamp to viewport
alt CSS dims present
Renderer->>Renderer: apply CSS-driven sizing
else
Renderer->>Renderer: fallback viewport-fit sizing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
DestinySpeaker
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/Epub/Epub/css/CssParser.cpplib/Epub/Epub/css/CssStyle.hlib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-19T12:17:00.962Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:00.962Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
Applied to files:
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
lib/Epub/Epub/css/CssStyle.h (1)
72-219: LGTM: image dimension fields are consistently wired into flags, accessors, and reset logic.lib/Epub/Epub/css/CssParser.cpp (3)
298-303: LGTM:height/widthdeclarations now populate image sizing fields.
622-642: LGTM: cache serialization includes image dimensions and defined flags.
741-771: LGTM: cache deserialization mirrors the new image dimension fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp`:
- Around line 264-268: The parser builds imgStyle from
cssParser->resolveStyle("img", classAttr) but never merges parsed inline
styleAttr into that CssStyle, so inline style="height/width" is ignored; update
ChapterHtmlSlimParser (around imgStyle, hasCssHeight, hasCssWidth) to parse
styleAttr into a CssStyle and merge/overlay it onto imgStyle before calling
hasImageHeight()/hasImageWidth() and before using dimensions — ensure the inline
style wins (overrides CSS) or is applied appropriately by using CssStyle
merge/assign methods or constructors available in your codebase so
hasCssHeight/hasCssWidth reflect inline styleAttr.
- Around line 269-277: The CSS percentage height is being resolved against
self->viewportWidth and when displayWidth is clamped the height is not rescaled,
breaking aspect ratio; change percent-height resolution to use viewport height
(call imgStyle.imageHeight.toPixels(emSize,
static_cast<float>(self->viewportHeight)) when imageHeight is percent) and,
after computing displayWidth from displayHeight (using dims.width/dims.height),
if displayWidth > self->viewportWidth scale displayWidth down to
self->viewportWidth and recompute displayHeight = static_cast<int>(displayWidth
* (static_cast<float>(dims.height) / dims.width) + 0.5f) so both dimensions are
scaled together (use the same symbols: hasCssHeight, dims,
imgStyle.imageHeight.toPixels, self->viewportWidth, self->viewportHeight,
displayWidth, displayHeight).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-19T12:17:00.962Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:00.962Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
Applied to files:
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
🧬 Code graph analysis (1)
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp (1)
lib/Epub/Epub/css/CssParser.cpp (2)
parseInlineStyle(566-566)parseInlineStyle(566-566)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp`:
- Around line 273-305: The CSS-size branches in ChapterHtmlSlimParser.cpp risk
divide-by-zero because the height branch only checks dims.height>0 and later
divides by dims.width, and the width branch only checks dims.width>0 and later
divides by dims.height; update the conditionals that use hasCssHeight and
hasCssWidth to require both dims.width>0 && dims.height>0 before performing
aspect-ratio math (the blocks that compute displayWidth/displayHeight using
imgStyle.imageHeight.toPixels and imgStyle.imageWidth.toPixels and rescale using
dims.width/dims.height), and if either dimension is zero fall back to the
viewport-fit branch (use self->viewportWidth/viewportHeight logic) to avoid
division by zero.
There was a problem hiding this comment.
Pull request overview
This PR updates the EPUB CSS + HTML parsing pipeline so <img> elements can be sized more appropriately when no explicit width is set, using CSS height/width (including inline styles) and the image’s intrinsic aspect ratio.
Changes:
- Extend
CssStyleto track imageheightandwidthdeclarations and exposehasImageHeight/hasImageWidth. - Teach
CssParserto parseheight/widthand persist them through the CSS rules cache. - Update
ChapterHtmlSlimParserimage layout to derive missing dimensions from CSS + aspect ratio instead of defaulting to full-page width.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp | Uses resolved CSS (plus inline style) to compute image display size via height/width + aspect ratio. |
| lib/Epub/Epub/css/CssStyle.h | Adds imageHeight/imageWidth fields and defined-flags so image sizing can be expressed in resolved styles. |
| lib/Epub/Epub/css/CssParser.cpp | Parses height/width into CssStyle and serializes/deserializes them in the cache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
daveallie
left a comment
daveallie
left a comment
There was a problem hiding this comment.
I would like to get this into 1.1, but doing so will also make #1015 necessary to complete as well. As atm merging this will require everyone to manually rebuild their caches as CSS will just stop working. I'm keen to get in both to 1.1
@daveallie Would you like me to add the auto rebuild to this PR? |
|
yeah, that would be great! |
Done :) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/Epub/Epub/css/CssParser.cpp (1)
229-268: Consider reducing duplication withinterpretLength.
tryInterpretLengthandinterpretLengthshare nearly identical parsing logic. You could implementinterpretLengthin terms oftryInterpretLengthto eliminate the duplication:♻️ Proposed refactor to reduce duplication
CssLength CssParser::interpretLength(const std::string& val) { - const std::string v = normalized(val); - if (v.empty()) return CssLength{}; - - // Find where the number ends - size_t unitStart = v.size(); - for (size_t i = 0; i < v.size(); ++i) { - const char c = v[i]; - if (!std::isdigit(c) && c != '.' && c != '-' && c != '+') { - unitStart = i; - break; - } - } - - 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 - - // Determine unit type (preserve for deferred resolution) - auto unit = CssUnit::Pixels; - if (unitPart == "em") { - unit = CssUnit::Em; - } else if (unitPart == "rem") { - unit = CssUnit::Rem; - } else if (unitPart == "pt") { - unit = CssUnit::Points; - } else if (unitPart == "%") { - unit = CssUnit::Percent; - } - // px and unitless default to Pixels - - return CssLength{numericValue, unit}; + CssLength result; + tryInterpretLength(val, result); + return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Epub/Epub/css/CssParser.cpp` around lines 229 - 268, interpretLength duplicates the parsing logic from tryInterpretLength; refactor by making interpretLength call tryInterpretLength (or extract the shared parsing into a new helper used by both) so parsing is implemented in one place. Update interpretLength to call tryInterpretLength(const std::string&, CssLength&) and return the same CssLength result (or return default/throw on false) and ensure you reuse CssLength and CssUnit enums used in tryInterpretLength to preserve behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/Epub/Epub.cpplib/Epub/Epub/Section.cpplib/Epub/Epub/css/CssParser.cpplib/Epub/Epub/css/CssParser.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-19T12:17:00.962Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:00.962Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
Applied to files:
lib/Epub/Epub/Section.cpp
🧬 Code graph analysis (3)
lib/Epub/Epub/Section.cpp (1)
lib/Serialization/Serialization.h (1)
writePod(8-10)
lib/Epub/Epub/css/CssParser.cpp (1)
src/CrossPointSettings.cpp (2)
file(94-100)file(94-94)
lib/Epub/Epub/css/CssParser.h (1)
lib/Epub/Epub/css/CssParser.cpp (3)
tryInterpretLength(229-268)tryInterpretLength(229-229)string(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🔇 Additional comments (10)
lib/Epub/Epub/css/CssParser.h (2)
33-35: LGTM!The
CSS_CACHE_VERSIONconstant is well-placed as a public static member, enabling consistent version checks acrossSection.cppand the CSS cache logic. The comment clearly documents the invalidation behavior.
119-120: LGTM!The
tryInterpretLengthAPI with out-parameter pattern correctly distinguishes between "parsed a numeric value" vs "encountered auto/inherit/initial", which is essential for the image sizing feature where unspecified dimensions need different handling than explicit zero.lib/Epub/Epub/Section.cpp (2)
7-7: LGTM!The
SECTION_FILE_VERSIONbump to 14 and inclusion ofCSS_CACHE_VERSIONin the header ensures that section caches are invalidated when CSS parsing rules change. The header size calculation correctly accounts for the additionaluint8_tfield.Also applies to: 13-16
97-109: LGTM!The CSS cache version is correctly read and validated against
CssParser::CSS_CACHE_VERSION. This ensures section caches are rebuilt when CSS format changes, even if other parameters match.lib/Epub/Epub.cpp (2)
342-357: LGTM!The CSS cache rebuild logic correctly handles three scenarios:
- No cache exists → rebuild
- Cache exists but version mismatched (stale) →
loadFromCache()returns false and removes stale file → rebuild- Cache exists and loads successfully → no rebuild needed
The section cache invalidation via
Storage.removeDirensures sections are rebuilt with the updated CSS rules.
455-459: LGTM!Section cache invalidation after the initial build ensures freshly parsed CSS is applied to newly created sections.
lib/Epub/Epub/css/CssParser.cpp (4)
340-352: LGTM!Using
tryInterpretLengthforheightandwidthcorrectly ensures that only explicit numeric values setimageHeight/imageWidth, whileauto,inherit, andinitialare properly ignored. This aligns with the PR objective of using the image's intrinsic dimensions when no explicit size is specified.
669-670: LGTM!The new
imageHeightandimageWidthfields are correctly serialized with their lengths (bits 13-14 ofdefinedBits), maintaining consistency with the existing cache format pattern.Also applies to: 687-688
712-717: LGTM!Proactively removing the stale cache file on version mismatch ensures a clean rebuild path without requiring manual cache deletion. The debug log with both actual and expected versions aids troubleshooting.
792-793: LGTM!The deserialization correctly reads
imageHeightandimageWidthin the same order as serialization, and the defined flags are properly restored from the bitmask.Also applies to: 819-820
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/Epub/Epub/css/CssParser.cpp`:
- Around line 229-268: interpretLength duplicates the parsing logic from
tryInterpretLength; refactor by making interpretLength call tryInterpretLength
(or extract the shared parsing into a new helper used by both) so parsing is
implemented in one place. Update interpretLength to call
tryInterpretLength(const std::string&, CssLength&) and return the same CssLength
result (or return default/throw on false) and ensure you reuse CssLength and
CssUnit enums used in tryInterpretLength to preserve behavior.
daveallie
left a comment
daveallie
left a comment
There was a problem hiding this comment.
Looking good, one comment about the section cache which I think we should address. Otherwise happy to merge
lib/Epub/Epub/Section.cpp
Outdated
| @@ -42,8 +43,8 @@ void Section::writeSectionFileHeader(const int fontId, const float lineCompressi | |||
| } | |||
| static_assert(HEADER_SIZE == sizeof(SECTION_FILE_VERSION) + sizeof(fontId) + sizeof(lineCompression) + | |||
| sizeof(extraParagraphSpacing) + sizeof(paragraphAlignment) + sizeof(viewportWidth) + | |||
| sizeof(viewportHeight) + sizeof(pageCount) + sizeof(hyphenationEnabled) + | |||
| sizeof(embeddedStyle) + sizeof(uint32_t), | |||
| sizeof(viewportHeight) + sizeof(hyphenationEnabled) + sizeof(embeddedStyle) + | |||
| sizeof(pageCount) + sizeof(uint32_t), // LUT offset | |||
There was a problem hiding this comment.
Let's just revert all the changes to this file
|
Apologies, my comment wasn't quite clear, I meant that we should just reset that file back to what it is on master. Your changes prior to your reverts there were failing the build because you had some header content changing failing the static assertion. We still don't want that new CSS Version value in the header, and so everything in that file can be discarded I believe. |
No worries! I will fix it in 2 hours and commit :) |
## 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:  After:  * **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
## 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:  After:  * **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
Summary
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:
After:
Changes to the CSS parser
Additional Context
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