diff --git a/CHANGELOG.md b/CHANGELOG.md index e5459388c..37b743b9e 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 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 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..26d5f3e66 100644 --- a/src/odr/internal/html/frontend/pdf-annotation.js +++ b/src/odr/internal/html/frontend/pdf-annotation.js @@ -148,17 +148,19 @@ if (type === "highlight") { return rect(b[0], b[1], b[2] - b[0], h); } + // `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] - 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/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/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); 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();