From cd60b068dc55d514fc11dd950735dc576c87cd9f Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 16 Aug 2026 21:42:54 +0200 Subject: [PATCH 1/2] fix(pdf): place a run by its own font, and land a search hit on its word MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ios app reported a search hit covering the wrong word. Four faults in the html pdf renderer, all in how a run is placed relative to the one before it. A recovered word break was written into the run text *and* spanned by the `margin-left` placing that run, so a page whose fonts are not embedded drifted a space to the right per word — far enough by the end of a line that the selection layer's highlight sat over a neighbouring word. Both fallback paths now take the run's core text; single-layer gives the break its own span, as its collapse path already did. A substitute `@font-face` named only the family, so `local(Helvetica)` resolved the regular cut and the browser faked the bold at the regular's advance widths. It is now asked for by the name of the cut it wants, and declares the weight and style so nothing is synthesised on top. A visual line block carries its first run's placement and the rest flow inline off it, so a run in another font or size was anchored against metrics that were not its own — subscripts flattened onto the baseline, and a bullet floated the text beside it a line away from the highlight that selects it. The block now breaks on the same flow key the single-layer path already used. A gap spacer's lone space cannot be justified to its box the way a word is, so a selection or a search hit left a sliver of white at every word break. Its advance is padded and clipped back, for a gap that still reads as a word space — a wider one is a column of white, not a space. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QELPoDuuBiFJ6B3jVFDok5 --- CHANGELOG.md | 12 +++ src/odr/internal/html/pdf_file.cpp | 113 +++++++++++++++++++++++------ 2 files changed, 101 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ff17edc..838afa4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,18 @@ The release run heads these entries with the version and opens a fresh - A tap on a paged document reaches its text: a negative `z-index` painted the page behind its container, which took every tap. No caret, and no keyboard on ios. +- A pdf whose fonts are not embedded sits where the file puts it: a recovered + word break was both written into the run and spanned by its offset, so every + word drifted another space right and a search hit landed on its neighbour. +- A pdf's bold and italic text is bold and italic — a non-embedded font is + asked for by the name of that cut, not faked at the regular cut's widths. +- A pdf line that mixes fonts or sizes puts each run at its own baseline + instead of against the metrics of whichever run opened the line. Subscripts + and superscripts sit where the file puts them, and the text after a bullet + no longer parts company with the highlight that selects it. +- A search hit or a selection running across words in a pdf paints the word + gaps between them rather than leaving a sliver of white in each. A gap wider + than the text is left alone: it is a column of white, not a space. ## v6.6.0 - 2026-08-14 diff --git a/src/odr/internal/html/pdf_file.cpp b/src/odr/internal/html/pdf_file.cpp index 93fd729f..b15e3dd3 100644 --- a/src/odr/internal/html/pdf_file.cpp +++ b/src/odr/internal/html/pdf_file.cpp @@ -374,12 +374,41 @@ std::string font_substitute_declaration(const pdf::FontSubstitute &substitute) { return declaration; } +/// The suffixes naming a family's styled cut, most specific first. `local()` +/// matches a face name, not a family plus a weight, so bold must be asked for. +std::vector style_suffixes(const bool bold, + const bool italic) { + if (bold && italic) { + return {"-BoldItalic", " Bold Italic", "-BoldOblique", " Bold Oblique"}; + } + if (bold) { + return {"-Bold", " Bold"}; + } + if (italic) { + return {"-Italic", " Italic", "-Oblique", " Oblique"}; + } + return {}; +} + /// The `local(...)` sources of a `font-family` stack, dropping the generic -/// keywords an `@font-face src` cannot name. "" when the stack is generic-only. -std::string local_font_sources(const std::string_view css_family) { +/// keywords an `@font-face src` cannot name, each family styled-cut-first so a +/// system without the cut still resolves. "" when the stack is generic-only. +std::string local_font_sources(const std::string_view css_family, + const bool bold, const bool italic) { static constexpr std::array generics = { "serif", "sans-serif", "monospace", "cursive", "fantasy", "system-ui"}; + const std::vector suffixes = style_suffixes(bold, italic); std::string src; + const auto add = [&src](const std::string_view name, + const std::string_view suffix) { + if (!src.empty()) { + src += ','; + } + src += "local(\""; + src += name; + src += suffix; + src += "\")"; + }; std::size_t start = 0; while (start <= css_family.size()) { const std::size_t comma = css_family.find(',', start); @@ -394,12 +423,10 @@ std::string local_font_sources(const std::string_view css_family) { } const bool generic = std::ranges::find(generics, name) != generics.end(); if (!name.empty() && !generic) { - if (!src.empty()) { - src += ','; + for (const std::string_view suffix : suffixes) { + add(name, suffix); } - src += "local("; - src += name; - src += ')'; + add(name, ""); } if (comma == std::string_view::npos) { break; @@ -420,7 +447,8 @@ class SubstituteFontFaces { /// to the plain family stack when the stack names no concrete font. std::string declaration(const pdf::FontSubstitute &substitute, const double ascent_em) { - const std::string src = local_font_sources(substitute.css_family); + const std::string src = local_font_sources( + substitute.css_family, substitute.bold, substitute.italic); if (src.empty()) { return font_substitute_declaration(substitute); } @@ -436,8 +464,15 @@ class SubstituteFontFaces { std::move(key).str(), static_cast(m_faces.size()) + 1); if (inserted) { std::ostringstream face; - face << "@font-face{font-family:'odr-s" << it->second << "';src:" << src - << ";ascent-override:" << round2(ascent * 100.0) + face << "@font-face{font-family:'odr-s" << it->second << "';src:" << src; + // Declared, so the browser does not synthesise them a second time. + if (substitute.bold) { + face << ";font-weight:bold"; + } + if (substitute.italic) { + face << ";font-style:italic"; + } + face << ";ascent-override:" << round2(ascent * 100.0) << "%;descent-override:" << round2(descent * 100.0) << "%;line-gap-override:0%}"; m_faces.push_back(std::move(face).str()); @@ -1350,6 +1385,7 @@ class HtmlServiceImpl final : public HtmlService { double vis_prev_baseline = 0; double vis_prev_font_pt = 0; bool vis_prev_was_matrix = false; + std::string vis_cur_flow_key; const auto vis_close_line = [&] { vis_cur_line = -1; }; // Selection layer state: content-stream (reading) order grouping. @@ -1393,8 +1429,18 @@ class HtmlServiceImpl final : public HtmlService { // Invisible runs paint nothing and Type3 runs are painted by their char // procs, so both contribute to the selection layer only. if (!invisible && !text.render_as_graphics) { - bool new_vis_line = - is_matrix || vis_prev_was_matrix || vis_cur_line < 0; + // A block carries its first run's placement and the rest flow off it, + // so another font, size or ascent needs a block of its own. + std::ostringstream key; + key << font << '|' << font_size_pt << '|' << round2(asc * text.size); + if (font == 0 && text.font != nullptr && text.font->substitute) { + key << '|' << font_substitute_declaration(*text.font->substitute); + } + const std::string vis_flow_key = std::move(key).str(); + + bool new_vis_line = is_matrix || vis_prev_was_matrix || + vis_cur_line < 0 || + vis_flow_key != vis_cur_flow_key; double vis_margin_pt = 0; if (!new_vis_line && vis_prev_font_pt > 0) { if (starts_new_line(baseline, vis_prev_baseline, ox, vis_prev_end, @@ -1413,6 +1459,7 @@ class HtmlServiceImpl final : public HtmlService { line_out.classes = std::move(line_base); page_out.vis_items.push_back(std::move(line_out)); vis_cur_line = static_cast(page_out.vis_items.size()) - 1; + vis_cur_flow_key = vis_flow_key; } std::string run_classes = "g"; // user-select:none @@ -1436,7 +1483,9 @@ class HtmlServiceImpl final : public HtmlService { if (font != 0) { run_text = escape_text(glyph_run_str(*text.font, text.codes)); } else { - run_text = escape_text(text.text); + // `margin-left` already spans the word break; rendering it too + // shifts the glyphs by a space, once per run. + run_text = escape_text(core_text(text)); } if (const double cs_pt = round2(text.char_spacing * scale); @@ -1517,6 +1566,11 @@ class HtmlServiceImpl final : public HtmlService { const double rounded_gap = round2(gap_pt); if (rounded_gap > 0) { add_class(gap_cls, "w", pt_decl("width", rounded_gap)); + // Only a gap that still reads as a word space: a column of + // white painted solid is worse than the sliver. + if (rounded_gap <= font_size_pt) { + gap_cls += " sw"; + } } runs.push_back(SelRunOut{std::move(gap_cls), " "}); } @@ -1621,6 +1675,9 @@ class HtmlServiceImpl final : public HtmlService { // inline-block baseline-aligns to its bottom margin edge only when // overflow isn't visible, so without it the spacer shifts in y. out.out() << ".sg{display:inline-block;overflow:hidden}"; + // A lone space cannot be justified to its box, so pad the advance and let + // the width clip it: else every word break shows a sliver of white. + out.out() << ".sw{letter-spacing:1000pt}"; }); const auto write_vis_line = [&](const VisLineOut &line) { @@ -1750,19 +1807,11 @@ class HtmlServiceImpl final : public HtmlService { }; // A leading inferred space carries no character code or advance, so the 1:1 - // codes-to-text alignment starts after it. These helpers view the run text - // past that space. + // codes-to-text alignment starts after it — as `core_text_begin` views it. const auto core_char_count = [](const pdf::TextElement &t) { return util::string::utf8_length(t.text) - (t.leading_space_inferred ? 1u : 0u); }; - const auto core_text_begin = [](const pdf::TextElement &t) { - auto cp = t.text.begin(); - if (t.leading_space_inferred) { - utf8::unchecked::next(cp); // skip the one-byte U+0020 - } - return cp; - }; std::uint32_t family_count = 0; std::string font_faces; @@ -1920,8 +1969,10 @@ class HtmlServiceImpl final : public HtmlService { color_suffix.empty() ? std::string() : color_suffix.substr(1); if (font == 0 || invisible) { - // Fallback / invisible: render real unicode directly. - run.text = escape_markup(text.text); + // Fallback / invisible: real unicode directly. The word break rides + // its own span as below, else it is margined *and* advanced over. + run.lead_space = text.leading_space_inferred; + run.text = escape_markup(core_text(text)); } else { // Collapse needs 1:1 codes-to-core-text and every (uchar, glyph) to // match the frequency winner. The leading inferred space is metadata, @@ -2564,6 +2615,20 @@ class HtmlServiceImpl final : public HtmlService { return s; } + /// `text.text` past an inferred leading space, which backs no advance. + static std::string::const_iterator + core_text_begin(const pdf::TextElement &text) { + auto cp = text.text.begin(); + if (text.leading_space_inferred) { + utf8::unchecked::next(cp); // skip the one-byte U+0020 + } + return cp; + } + + static std::string core_text(const pdf::TextElement &text) { + return std::string(core_text_begin(text), text.text.end()); + } + /// Escapes only the three markup-significant characters. Deliberately *not* /// `html::escape_text`: its ` ` substitution is a distinct character /// from U+0020 and breaks the find and word selection these layers exist for. From 3bd49b4b1c550af525d1bd117f8b5b90a7b01e83 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 16 Aug 2026 21:54:14 +0200 Subject: [PATCH 2/2] fix(pdf): unquote a substitute family before naming its local face MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `family_stack` quotes a name that needs it ('Times New Roman', 'Courier New', 'Zapf Dingbats'), and `local_font_sources` quotes again — so a serif or monospace substitute asked for a face named with literal apostrophes, matched nothing, and rendered through the plain stack without the ascent/descent overrides its placement assumes. Advances the reference-output pins for the whole branch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QELPoDuuBiFJ6B3jVFDok5 --- src/odr/internal/html/pdf_file.cpp | 7 +++++++ test/data.cmake | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/odr/internal/html/pdf_file.cpp b/src/odr/internal/html/pdf_file.cpp index b15e3dd3..5a97abfb 100644 --- a/src/odr/internal/html/pdf_file.cpp +++ b/src/odr/internal/html/pdf_file.cpp @@ -421,6 +421,13 @@ std::string local_font_sources(const std::string_view css_family, while (!name.empty() && name.back() == ' ') { name.remove_suffix(1); } + // The stack quotes a name that needs it ('Times New Roman'); `add` quotes + // again, and a doubly quoted name matches no installed face. + if (name.size() > 1 && (name.front() == '\'' || name.front() == '"') && + name.back() == name.front()) { + name.remove_prefix(1); + name.remove_suffix(1); + } const bool generic = std::ranges::find(generics, name) != generics.end(); if (!name.empty() && !generic) { for (const std::string_view suffix : suffixes) { diff --git a/test/data.cmake b/test/data.cmake index b44af101..b20de084 100644 --- a/test/data.cmake +++ b/test/data.cmake @@ -17,9 +17,9 @@ odr_test_data( odr_test_data( PATH "reference-output/odr-public" URL "https://github.com/opendocument-app/OpenDocument.test.output.git" - REVISION "b216dbe317768333081995d46382b0f01f5bcad8") + REVISION "ea671153a51dd968496e39a874f50e35c4a18138") odr_test_data( PATH "reference-output/odr-private" URL "https://github.com/opendocument-app/OpenDocument.test-private.output.git" - REVISION "4c5030479e6412c3c8829fef96a7d00826b851cf") + REVISION "97f3bb9574850526a0d38241574af087d3f5e915")