Address issue #504: clickable internal anchor links in exported PDF#516
Open
schuyler wants to merge 7 commits into
Open
Address issue #504: clickable internal anchor links in exported PDF#516schuyler wants to merge 7 commits into
schuyler wants to merge 7 commits into
Conversation
Exported PDFs previously lost all intra-document anchor links: printing the WebView through NSPrintOperation does not carry <a href="#slug"> link annotations or id destinations into the PDF, so TOC entries were inert even when their fragment exactly matched a heading id. Fix via PDFKit post-processing of the written export: - New MPPDFAnchorInjector: a pure, headlessly-testable engine that locates headings and TOC links in the rendered PDF by native text search (findString:/PDFSelection) and injects .link GoTo annotations. Geometry comes from the PDF itself, not on-screen DOM rects, so @media print reflow can't misplace links. Slug collisions resolve to first match; a taller-than-TOC-entry occurrence is promoted over same-size prose, with a document-order fallback so same-size (h5/h6) headings stay clickable. - MPDocument reads a layout-independent link/heading model from the WebView DOM (evaluateScript:) and post-processes the exported file in the existing document:didPrint:context: callback, gated on a stashed export URL so ordinary Cmd-P printing is unaffected. Write-back is atomic and best-effort: any failure leaves the valid un-annotated PDF. - Link PDFKit.framework into the app and test targets. Tests: 8 headless XCTest cases against in-test-generated fixture PDFs cover the happy path, first-match collision, no-match/inert safety, empty no-op, same-size fallback, height-promotes-heading, multiple links to one slug, and the bodyGroup-empty skip. Related to #504
Contributor
Code Coverage ReportCurrent Coverage: 63.53% Coverage Details (Summary) |
Test fixture (CI fix): the in-test fixture PDFs drew text via a raw CGPDFContext, which produced no ToUnicode mapping and so was not findString:-searchable on the CI runners — the injector found nothing and every behavioral assertion saw zero annotations. Regenerate each fixture page through an offscreen NSView's -dataWithPDFInsideRect: (the AppKit text-layout path that yields genuinely searchable text, matching the real export), merge pages via PDFKit, and force a synchronous text parse before searching. Expected rects are now measured from the finished document so geometry assertions stay self-consistent. Export post-processing hardening (self-review): - Guard against overlapping exports: exportPdf: is a no-op while a prior export is still in flight, so the single-slot URL stash can't be clobbered (which would have left both exports un-annotated). - Log and clean up on the post-processing failure paths (reopen/write/ replace), passing a real NSError and removing the temp file so no stray <uuid>.pdf is left beside the user's export. Success and Cmd-P paths unchanged. - Log (instead of silently swallowing) the injector's per-link and search exceptions; collapse a redundant source-height lookup. Tests: add coverage for a blank link text being skipped and for a second same-text link with no second PDF occurrence being skipped. Remove the stale red-state/no-op-stub comments now that the engine is implemented. Related to #504
The fixture drew text with the system font (San Francisco). When AppKit embeds that private UI font via -dataWithPDFInsideRect: in headless CI it does not emit a ToUnicode CMap, so the glyphs render but are not text-extractable and -[PDFDocument findString:] finds nothing — the injector saw an empty document and every annotation assertion read zero. Draw with Helvetica (a standard PDF base-14 font, always present on macOS), which embeds a correct ToUnicode mapping, so the fixture text is searchable and the tests exercise the real engine. Related to #504
…archable The fixture built each page as a separate one-page PDF via -[NSView dataWithPDFInsideRect:] and merged them with -insertPage:. That cross-document copy dropped the searchable text on every merged page (page 0 was searchable, pages 1+ were not), so the injector could not locate headings on later pages and added no annotations. Generate the whole multi-page fixture in a single CGPDFContext instead (one document, no insertPage: merge), still drawing with Helvetica. Every page's glyphs live in the one document PDFKit loads, so findString: locates text on all pages and the tests exercise the real engine end to end. Related to #504
…-page text Raw CGPDFContext text was findString:-searchable only on the first page in CI (a Core Graphics multi-page font/ToUnicode embedding pitfall), so the injector could not locate headings on later pages and added no annotations. Two CGPDFContext variants (per-page NSView merge, then single context) reproduced the identical page-0-only-searchable signature. Generate the multi-page fixture by printing a tall, auto-paginated NSView through NSPrintOperation (NSPrintSaveJob) instead — the same AppKit printing path MacDown's real PDF export uses, which embeds correct text encoding on every page. Each page is one paper-height slice of the view, so a pageIndex-k item lands on printed page k; the produced PDF is loaded and its text located exactly as the engine does. Related to #504
Switching the fixture to NSPrintOperation made multi-page text searchable, but every fixture came out with one extra page: the tall view was sized as 792pt * pageCount, while AppKit auto-paginates against the printer's imageable height (nominal paper minus the "Save as PDF" printer's fixed unprintable margins, which zeroing the user margins does not remove). The view was therefore an exact multiple of 792 but sliced against a slightly shorter height, yielding pageCount+1 pages and shifting items off their intended pages. Size the view and place items by printInfo.imageablePageBounds.size.height (the height AppKit actually slices against) so the view is exactly pageCount imageable-slices tall and produces exactly pageCount pages, with each pageIndex-k item on printed page k. Related to #504
The injector added link annotations to, and built PDFDestinations from, the PDFPage returned by PDFSelection.pages.firstObject. That is a transient page wrapper, not the object -[PDFDocument pageAtIndex:] vends, so the annotations were invisible when the page was re-queried and — critically — were not serialized by -[PDFDocument writeToURL:] in the real export path (MPDocument postProcessExportedPDFAtURL:). The exported PDF would have had no clickable links even though injection reported success. Re-home both the source page (for addAnnotation:) and the destination page (for PDFDestination) onto [document pageAtIndex:[document indexForPage:...]] before mutating, so annotations land on — and persist through writeToURL: on — the canonical page. Geometry, counts, ordering, and disambiguation are unchanged (bounds/heights are computed before re-homing). Also nudge the lone body heading in one fixture off the page-top edge so it renders searchably within its page. Related to #504
Owner
Author
Status: blocked on CI — needs a macOS/Xcode runtime to finishHonest state of this PR: the new What got solvedGenerating a What remains (2 independent, real issues + 1 flake)
Why I stoppedSix CI-fix cycles, and the last one made no measurable progress. The remaining problems require observing actual PDFKit behavior (what To finish (on macOS)Run Maintainer notes (from self-review; deliberately not addressed here)
Branch Generated by Claude Code |
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
2 participants
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.
Summary
Exported PDFs previously lost every intra-document anchor link: printing the WebView through
NSPrintOperationdoes not carry<a href="#slug">link annotations oriddestinations into the PDF, so TOC entries were inert even when their fragment exactly matched a headingid(they worked fine in the preview and in exported HTML). This is the upstream-MacDown limitation described in the issue.This PR fixes it with PDFKit post-processing of the written export:
MPPDFAnchorInjector(MacDown/Code/Document/MPPDFAnchorInjector.{h,m}) — a pure, headlessly-testable engine. It locates headings and TOC links in the rendered PDF by native text search (findString:/PDFSelection) and injects.linkPDFActionGoToannotations. Geometry comes from the PDF itself, not from on-screen DOM rects, so the repo's@media printreflow (code blocks →pre-wrap, tables →table-layout: fixed,page-break-inside: avoid, a different body width than screen) can't misplace links.MPDocumentreads a layout-independent link/heading model (text + slug only) from the WebView DOM via the existingevaluateScript:bridge, then post-processes the exported file in the existingdocument:didPrint:context:completion callback — gated on a stashed export URL so ordinary ⌘P printing is completely unaffected. Write-back is atomic (temp sibling +replaceItemAtURL:) and best-effort: any failure leaves the valid, un-annotated PDF and never surfaces an error.PDFKit.frameworklinked into the app and test targets.Related Issue
Related to #504
Judgment Calls
http(s)://links are also dropped by the print pipeline, but they use a different destination model (a URL action, no destination correlation) and are deliberately out of scope. Notably this is not because they're harder — they're actually simpler — but to keep the review surface focused on the issue's subject. See open question below.## Cand## C++both produceid="c"; a PDF named destination is a single point, so a link resolves to the first matching heading, mirroring browsergetElementById/ fragment navigation.@media printreflow above; this also removes any coordinate-transform/pagination arithmetic.Testing
8 headless XCTest cases (
MacDownTests/MPPDFAnchorInjectorTests.m) run the pure engine against in-test-generated fixture PDFs (real selectable text drawn viaCGPDFContext, sofindString:can locate it): happy path (correct page + destination point), first-match on slug collision, no-match/inert safety (no crash, valid links unperturbed), empty/no-op, same-size-heading fallback, height-promotes-heading over preceding same-text prose, multiple links to one slug sharing a destination, and thebodyGroup-empty skip.Limitations & notes for the maintainer
#fragmentwith no target is inert and doesn't crash; a document with colliding slugs jumps to the first; a heading near a page break lands on the correct page.GitHub2rendersh5/h6at body size, so height can't distinguish those headings from body text — they resolve via the document-order fallback, which is correct unless the exact heading text is repeated by body prose appearing before the heading (then the link may land on that prose; on-topic, never a crash).testExportPdfUsesDeferralMechanism(MacDownTests/MPRenderDeferralTests.m) was left as-is rather than extended — the live export path can't be driven headlessly beyond the crash-free assertion it already makes.Open questions
Generated by Claude Code