From 5f8b5209768be117a0ccbbeab25fe7730ea4a387 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 20 Sep 2026 12:15:15 +0200 Subject: [PATCH 1/2] fix(html): the underline and the squiggle sit under the text `barPath` drew both inside the box: the underline in its bottom sixteenth and the squiggle between an eighth and a quarter above the bottom. Both therefore ran through the descenders they belong under. They now start at `b[3]`, where the glyphs end, and run below it. A strike-out keeps the middle of the box, which is the one that is meant to cross. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0149gFxhkvKTBQidz6brU8Vt --- CHANGELOG.md | 4 ++ .../internal/html/frontend/pdf-annotation.js | 10 ++-- test/browser/annotation/tests.html | 54 +++++++++++++++++++ 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5459388c..d63b35232 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- **Fix**: a pdf underline and squiggle were drawn inside the text rather than + under it, so they crossed the descenders they belong below. Both now start + where the glyphs end. A strike-out still crosses the text. + - **Fix**: a pdf mark cut the tails off the text it marked and broke apart at every word gap, because it took the selection layer's em box. It now covers the glyphs under the selection, as one box per line. diff --git a/src/odr/internal/html/frontend/pdf-annotation.js b/src/odr/internal/html/frontend/pdf-annotation.js index 92b523858..3735f892d 100644 --- a/src/odr/internal/html/frontend/pdf-annotation.js +++ b/src/odr/internal/html/frontend/pdf-annotation.js @@ -148,17 +148,21 @@ if (type === "highlight") { return rect(b[0], b[1], b[2] - b[0], h); } + // `b[3]` is where the glyphs end, so an underline and a squiggle start + // there and run below it. Drawing them inside the box puts them through + // the descenders they belong under. A strike-out is the one that is + // meant to cross the text. if (type === "underline") { - return rect(b[0], b[3] - h / 16, b[2] - b[0], Math.max(h / 16, 0.5)); + return rect(b[0], b[3], b[2] - b[0], Math.max(h / 16, 0.5)); } if (type === "strikeOut") { return rect(b[0], b[1] + h / 2, b[2] - b[0], Math.max(h / 16, 0.5)); } var step = Math.max(h / 8, 1); - var d = "M " + b[0] + " " + (b[3] - step); + var d = "M " + b[0] + " " + b[3]; var up = true; for (var x = b[0] + step; x < b[2]; x += step, up = !up) { - d += " L " + x + " " + (up ? b[3] - step * 2 : b[3] - step); + d += " L " + x + " " + (up ? b[3] + step : b[3]); } return d; }; diff --git a/test/browser/annotation/tests.html b/test/browser/annotation/tests.html index 7259bb11f..06ca64e03 100644 --- a/test/browser/annotation/tests.html +++ b/test/browser/annotation/tests.html @@ -513,6 +513,60 @@ line.boxes[0] ); + // --- what is drawn under the text stays under it ---------------------- + + // Every y the path names, in the page-box points the box is stated in. + function pathRows(id) { + const d = document + .querySelector('[data-odr-annotation="' + id + '"]') + .getAttribute("d"); + const rows = []; + let y = 0; + d.replace(/([MLhv]) *(-?[\d.]+)(?: +(-?[\d.]+))?/g, (all, op, a, b) => { + if (op === "M" || op === "L") { + y = +b; + rows.push(y); + } else if (op === "v") { + y += +a; + rows.push(y); + } + return all; + }); + return rows; + } + + function markDual(type) { + api.clear(); + api.setTool(type); + window.getSelection().removeAllRanges(); + window.getSelection().addRange(both.cloneRange()); + api.mark(); + return api.list()[0]; + } + + const under = markDual("underline"); + check( + "an underline starts where the glyphs end", + Math.min.apply(null, pathRows(under.id)) >= under.boxes[0][3] - 0.01, + [Math.min.apply(null, pathRows(under.id)), under.boxes[0][3]] + ); + + const wave = markDual("squiggly"); + check( + "and so does a squiggle", + Math.min.apply(null, pathRows(wave.id)) >= wave.boxes[0][3] - 0.01, + [Math.min.apply(null, pathRows(wave.id)), wave.boxes[0][3]] + ); + + const struck = markDual("strikeOut"); + const strikeRows = pathRows(struck.id); + check( + "a strike-out is the one that crosses the text", + Math.min.apply(null, strikeRows) > struck.boxes[0][1] && + Math.max.apply(null, strikeRows) < struck.boxes[0][3], + [strikeRows, struck.boxes[0]] + ); + window.odr.onAnnotationChange = () => {}; api.clear(); api.setTool(null); From 31a3a117b702d76c4b2700714c2d784a2363422e Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 20 Sep 2026 12:41:07 +0200 Subject: [PATCH 2/2] fix(html): the saved file draws the underline under the text too `text_markup_appearance` carries its own copy of the geometry, so the view and the appearance stream a save writes have to move together. Both now hang the underline and the squiggle off the bottom edge of the quad. A gtest reads every y the stream names and holds the two under the quad, with the strike-out inside it, which is the rule the browser checks state as well. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0149gFxhkvKTBQidz6brU8Vt --- CHANGELOG.md | 6 +-- .../internal/html/frontend/pdf-annotation.js | 6 +-- src/odr/internal/pdf/pdf_annotation.cpp | 14 ++++-- test/src/internal/pdf/pdf_annotation.cpp | 50 +++++++++++++++++++ 4 files changed, 64 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d63b35232..37b743b9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,9 +16,9 @@ The release run heads these entries with the version and opens a fresh ## Unreleased -- **Fix**: a pdf underline and squiggle were drawn inside the text rather than - under it, so they crossed the descenders they belong below. Both now start - where the glyphs end. A strike-out still crosses the text. +- **Fix**: a pdf underline and squiggle were drawn through the text rather than + under it, both in the view and in the saved file. They now hang below the + glyphs. A strike-out still crosses the text. - **Fix**: a pdf mark cut the tails off the text it marked and broke apart at every word gap, because it took the selection layer's em box. It now covers diff --git a/src/odr/internal/html/frontend/pdf-annotation.js b/src/odr/internal/html/frontend/pdf-annotation.js index 3735f892d..26d5f3e66 100644 --- a/src/odr/internal/html/frontend/pdf-annotation.js +++ b/src/odr/internal/html/frontend/pdf-annotation.js @@ -148,10 +148,8 @@ if (type === "highlight") { return rect(b[0], b[1], b[2] - b[0], h); } - // `b[3]` is where the glyphs end, so an underline and a squiggle start - // there and run below it. Drawing them inside the box puts them through - // the descenders they belong under. A strike-out is the one that is - // meant to cross the text. + // `b[3]` is the bottom of the glyph boxes, so an underline and a + // squiggle hang below it. A strike-out is the one meant to cross. if (type === "underline") { return rect(b[0], b[3], b[2] - b[0], Math.max(h / 16, 0.5)); } diff --git a/src/odr/internal/pdf/pdf_annotation.cpp b/src/odr/internal/pdf/pdf_annotation.cpp index a3b6ab304..215288f82 100644 --- a/src/odr/internal/pdf/pdf_annotation.cpp +++ b/src/odr/internal/pdf/pdf_annotation.cpp @@ -145,16 +145,18 @@ void bar(std::ostringstream &out, const QuadCorners &quad, const double bottom, << number(quad.right - quad.left) << ' ' << number(height) << " re\n"; } -/// A wave along the bottom of `quad`, as a stroked zigzag of `amplitude`. +/// A wave under `quad`, as a stroked zigzag of `amplitude` hanging off its +/// bottom edge: `quad.bottom` is where the glyphs end, so the wave belongs +/// below it rather than through the descenders. void wave(std::ostringstream &out, const QuadCorners &quad, const double amplitude) { - const double base = quad.bottom + amplitude; - out << number(quad.left) << ' ' << number(base) << " m\n"; + const double base = quad.bottom - amplitude; + out << number(quad.left) << ' ' << number(quad.bottom) << " m\n"; const auto steps = static_cast( std::max(1.0, std::floor((quad.right - quad.left) / amplitude))); for (std::size_t i = 1; i <= steps; ++i) { out << number(quad.left + static_cast(i) * amplitude) << ' ' - << number(i % 2 == 1 ? base + amplitude : base) << " l\n"; + << number(i % 2 == 1 ? base : quad.bottom) << " l\n"; } out << "S\n"; } @@ -176,7 +178,9 @@ std::string text_markup_appearance(const TextMarkup &markup) { bar(out, c, c.bottom, height); break; case TextMarkupKind::underline: - bar(out, c, c.bottom + height / 16, std::max(height / 16, 0.5)); + // Under the glyphs, not through them: `c.bottom` is where they end. + bar(out, c, c.bottom - std::max(height / 16, 0.5), + std::max(height / 16, 0.5)); break; case TextMarkupKind::strike_out: bar(out, c, c.bottom + height / 2, std::max(height / 16, 0.5)); diff --git a/test/src/internal/pdf/pdf_annotation.cpp b/test/src/internal/pdf/pdf_annotation.cpp index b5448c864..8c3ae13da 100644 --- a/test/src/internal/pdf/pdf_annotation.cpp +++ b/test/src/internal/pdf/pdf_annotation.cpp @@ -10,6 +10,8 @@ #include +#include +#include #include #include #include @@ -146,6 +148,54 @@ TEST(PdfAnnotation, only_highlight_multiplies) { } } +// The quad is the text, so an underline and a squiggle hang below it and only +// a strike-out crosses it. `pdf-annotation.js` draws the same three shapes for +// the view, and the two have to agree. +TEST(PdfAnnotation, an_underline_and_a_squiggle_stay_under_the_quad) { + // Every y the appearance stream names, in pdf user space. + const auto rows = [](const TextMarkupKind kind) { + TextMarkup markup = one_line_highlight(); + markup.kind = kind; + const std::string pdf = with_markup(markup); + DocumentParser parser(std::make_unique(pdf)); + const std::unique_ptr document = parser.parse_document(); + const Dictionary &dictionary = + first_page(*document)->annotations.front()->object.as_dictionary(); + const std::string stream = parser.read_object_stream( + dictionary.get("AP").as_dictionary().get("N").as_reference()); + std::vector out; + std::istringstream in(stream); + std::vector words{std::istream_iterator(in), + std::istream_iterator()}; + for (std::size_t i = 0; i < words.size(); ++i) { + if (words[i] == "l" || words[i] == "m") { + out.push_back(std::stod(words[i - 1])); + } else if (words[i] == "re") { + out.push_back(std::stod(words[i - 3])); + out.push_back(std::stod(words[i - 3]) + std::stod(words[i - 1])); + } + } + return out; + }; + + // `one_line_highlight` states a quad from y=688 to y=700. + constexpr double bottom = 688; + constexpr double top = 700; + + for (const TextMarkupKind kind : + {TextMarkupKind::underline, TextMarkupKind::squiggly}) { + const std::vector y = rows(kind); + ASSERT_FALSE(y.empty()); + EXPECT_LE(*std::max_element(y.begin(), y.end()), bottom) + << "kind " << static_cast(kind); + } + + const std::vector struck = rows(TextMarkupKind::strike_out); + ASSERT_FALSE(struck.empty()); + EXPECT_GT(*std::min_element(struck.begin(), struck.end()), bottom); + EXPECT_LT(*std::max_element(struck.begin(), struck.end()), top); +} + TEST(PdfAnnotation, text_markup_subtypes) { const auto subtype = [](const TextMarkupKind kind) { TextMarkup markup = one_line_highlight();