fix(html): a pdf mark covers the text it marks - #913
Merged
Merged
Conversation
The box came from the selection layer's run alone. That run is one em of a substituted font, so a descender falls out of it, and the gap spacers were left out, so a mark over a sentence became a bar per word with a hole at every break. The box now takes its vertical edges from the glyphs it stands over, counts the spacers, and joins what touches on a line into one box, which is also what a pdf quad is. Measured over the reference pdfs: the share of glyph area a mark covers goes from 0.897 to 0.996 on `Auf nach Rom-Herbert.pdf/page1`, and from 0.958 to 1.000 on `Core_v5.1.pdf/page1`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149gFxhkvKTBQidz6brU8Vt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported from the app: a highlight does not cover the text it marks. It is not a recent regression —
runBoxhas taken the selection layer's own rect since #852, andbarPathsince #849.Two faults, one box.
.srrun's client rect: one em of the substitutedsfface, placed fromascent_em, which is clamped to[0.5, 1.0]. The glyph layer draws with the embedded font and its ink runs past that em. Measured over the reference pdfs, the worst 5% of runs stick out 2.3px of a 14.7px line.selectedRunstook.sronly. The word break lives in the.sg/.swspacer beside it, so a mark over a sentence came out as a bar per word with a hole between them. OnAntragsformular_Sozialtopf_wise19.pdf/page1, 85 glyph runs had no box over them at all — every one a space or an unmapped glyph.The box now takes its horizontal edges from the runs as before, the spacers included; its vertical edges from the glyphs it stands over; and boxes that touch on a line are joined into one, which is also what a pdf quad is.
glyphRects()reads every.grect once per mark rather than per run, so this is one layout pass, not one per selected run.Measured
Share of glyph area a mark covers, over the reference pdfs:
Auf nach Rom-Herbert.pdf/page1Core_v5.1.pdf/page1style-various-1.pdf/page0Antragsformular_Sozialtopf_wise19.pdf/page1The last page keeps 84 uncovered runs. Those are glyphs with no Unicode at all — the list bullets — which carry no selection-layer run, so they are not in a text selection to begin with.
Checked
test/browser/annotation: 67 checks, none failing, four of them new. The fixture gained a third page with both layers as a pdf view writes them: glyphs whose ink runs below the selection layer's box, and a gap spacer at the word break. Againstmain's script three of the four fail, and the output is the bug itself — two boxes with a hole between them, and a box 19.2pt tall over 26pt of glyphs.