feat(pdf): add highlight support with anchor extraction and resolution #705
Conversation
Adds PDF highlight capabilities: - PDFAnchorExtractor: captures coordinate quads, character ranges, and text context from selections for precise highlight persistence - PDFAnchorResolver: reconstructs highlights from stored anchors using multiple strategies (quads, character range, text search) - PDFNavigatorViewController: integrates selection handling with delegate callbacks and annotation menu control Includes TestApp integration for highlight creation workflow.
…n on iOS 16+ On iOS 16+, UIMenuController is deprecated. This adds UICommand-based routing for custom editing actions (like Highlight) through buildMenu(with:), ensuring they appear correctly in the modern UIEditMenuInteraction system. Also adds handlesAction(_:) helper for custom responder chain handling.
There was a problem hiding this comment.
Pull request overview
Adds DecorableNavigator support for PDF publications by extracting/storing selection anchors (quads, character ranges, context) and resolving them back into PDFKit annotation bounds, plus iOS 16+ edit-menu routing improvements for custom actions.
Changes:
- Add PDF anchor extraction + resolution utilities (with unit tests) to persist and re-render highlights/underlines reliably.
- Extend
PDFNavigatorViewControllerto applyDecorations as PDFKit annotations and emit activation callbacks on annotation taps. - Improve iOS 16+ edit menu handling: add custom actions as
UICommands and optionally route actions up the responder chain / suppress default PDFKit annotation menus.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/NavigatorTests/PDF/PDFAnchorTests.swift | Adds unit tests covering anchor parsing, quad/bounds resolution, and context/whitespace utilities. |
| TestApp/Sources/Reader/PDF/PDFViewController.swift | Wires a custom “Highlight” editing action into the PDF reader configuration. |
| Sources/Navigator/PDF/PDFNavigatorViewController.swift | Implements DecorableNavigator for PDF: manages decorations, renders PDFKit annotations, handles annotation activation, and stores anchors on selection. |
| Sources/Navigator/PDF/PDFDocumentView.swift | Adds optional custom action routing and optional suppression of default PDFKit annotation menus on iOS 16+. |
| Sources/Navigator/PDF/PDFAnchorResolver.swift | Introduces priority-based anchor resolution (quads → character range → context search). |
| Sources/Navigator/PDF/PDFAnchorExtractor.swift | Introduces anchor extraction from PDFSelection (quads + character offsets + context). |
| Sources/Navigator/EditingAction.swift | Adds selector handling helpers and iOS 16+ menu integration for custom editing actions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- parseQuads: validate all 4 points exist before returning quad - resolveFromQuads: return nil when all bounds are empty to enable fallbacks - target(forAction:): fall back to super when no responder found - AnyCancellable: safely dispatch cleanup to main thread - Annotation storage: use composite (group, id) key to prevent collisions - Log messages: fix 'TTS highlight' -> 'decoration' and include group name
|
Addressed the review feedback in 11ed9c5: parseQuads validation (#7) - now validates all 4 points exist before returning the quad resolveFromQuads empty array (#1) - was returning empty array instead of nil which blocked fallbacks. fixed annotation key collision (#2) - switched to composite key target(forAction:) fallback (#5) - now falls back to super when no responder found, should preserve native PDFKit actions AnyCancellable crash (#9) - replaced dispatchPrecondition with Thread.isMainThread check + async dispatch. safer this way log message (#4) - fixed the "TTS highlight" wording, also added group name to logs for easier debuging Skipped #3 (HREF filtering) and #6 (canPerformAction scope) for now - PDFs are typicaly single-resource so the HREF thing shouldnt be an issue in practice, and #6 should be handled by the #5 fix re #10 - highlightSelection() is already implemented in TestApp/PDFViewController.swift (lines 60-73) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
Sources/Navigator/PDF/PDFDocumentView.swift:152
- The override of
canPerformAction(_:withSender:)bypassessuper.canPerformActionfor any selector handled byEditingActionsControllerwheneverenableCustomActionRoutingis true, which can unintentionally ignore PDFKit’s built‑in restrictions (e.g.,PDFDocument.allowsCopying) and other system gating. Combined with the customcopy(_:)implementation that directly copies text viaeditingActions.copy()instead of delegating toPDFView, this allows copy/share‑style actions to execute on documents that PDFKit itself would block, enabling users or attackers to extract content from copy‑protected PDFs simply by enabling custom actions. To fix this, keepsuper.canPerformActionin the decision path for native actions (likecopy:,share:, etc.) or explicitly consult the underlyingPDFDocumentpermissions before authorizing these selectors when custom routing is enabled.
override public func canPerformAction(_ action: Selector, withSender sender: Any?) -> Bool {
// When custom action routing is enabled and the action is handled by EditingActionsController,
// delegate the decision to the controller. This ensures custom actions are properly authorized.
if enableCustomActionRouting, editingActions.handlesAction(action) {
return editingActions.canPerformAction(action)
}
// Standard behavior: check with EditingActionsController first, then defer to super
return super.canPerformAction(action, withSender: sender) && editingActions.canPerformAction(action)
}
override public func copy(_ sender: Any?) {
Task {
await editingActions.copy()
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Normalize locators and convert to diffable decorations | ||
| let target = newDecorations.map { | ||
| var d = $0 | ||
| d.locator = publication.normalizeLocator(d.locator) | ||
| return DiffableDecoration(decoration: d) | ||
| } | ||
|
|
||
| let source = decorations[group] ?? [] | ||
| decorations[group] = target | ||
|
|
||
| // Calculate changes | ||
| let changes = target.changesByHREF(from: source) | ||
|
|
||
| // Apply changes to PDF annotations | ||
| for (_, changeList) in changes { | ||
| for change in changeList { | ||
| switch change { | ||
| case .add(let decoration): | ||
| addAnnotation(for: decoration, in: document, group: group) | ||
| case .remove(let id): | ||
| removeAnnotation(withId: id, from: document, group: group) | ||
| case .update(let decoration): | ||
| removeAnnotation(withId: decoration.id, from: document, group: group) | ||
| addAnnotation(for: decoration, in: document, group: group) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
apply(decorations:in:) stores decorations by group but only applies incremental changes to the current pdfView.document at call time. When the PDF document changes (eg multi-resource publications where go(to:) swaps documents), previously declared decorations won’t be re-rendered on the new document unless the caller re-calls apply. Consider re-applying the stored decoration state for all groups whenever a new document is loaded, and/or filtering updates by the currently loaded HREF to avoid trying to add annotations for non-visible resources.
| static func parseQuads(_ data: Any?) -> [[CGPoint]]? { | ||
| guard let quadsArray = data as? [[[String: Double]]] else { | ||
| return nil | ||
| } | ||
|
|
||
| return quadsArray.compactMap { quad -> [CGPoint]? in | ||
| guard quad.count == 4 else { return nil } |
There was a problem hiding this comment.
parseQuads(_:) only accepts [[[String: Double]]], but when anchors are provided as a JSON string (parseAnchor(_:) JSON branch), JSONSerialization yields [[[String: Any]]] with NSNumber values. This makes quad parsing always fail for JSON-formatted anchors (even when the JSON contains valid numbers), which contradicts the intended “dictionary and JSON formats” support. Consider loosening the cast to accept [[[String: Any]]] (or [[[Any]]]) and converting x/y via NSNumber/Double so quads work for both dictionary and JSON inputs.
| // Walk through original text, tracking position in normalized space | ||
| var normalizedPosition = 0 | ||
| var originalStart: String.Index? | ||
| var originalEnd: String.Index? | ||
| var inWhitespace = false | ||
| var i = originalText.startIndex | ||
|
|
||
| let targetStart = normalizedPrefix.count | ||
| let targetEnd = targetStart + matchLength | ||
|
|
||
| while i < originalText.endIndex { | ||
| let char = originalText[i] | ||
| let isWhitespace = char.isWhitespace || char.isNewline | ||
|
|
||
| if isWhitespace { | ||
| if !inWhitespace { | ||
| normalizedPosition += 1 // Count whitespace run as single space | ||
| inWhitespace = true | ||
| } | ||
| } else { | ||
| normalizedPosition += 1 | ||
| inWhitespace = false | ||
| } | ||
|
|
||
| if originalStart == nil && normalizedPosition > targetStart { | ||
| originalStart = i | ||
| } |
There was a problem hiding this comment.
findOriginalRange(in:normalizedPrefix:matchLength:) doesn’t mirror normalizeWhitespace(_:) when the original text has leading whitespace. normalizeWhitespace trims leading whitespace entirely, but this mapper increments normalizedPosition for an initial whitespace run, causing ranges to start before the actual match (e.g. returning a range starting at the first whitespace). Consider skipping leading whitespace runs until the first non-whitespace character, or implementing mapping by building an index map while normalizing so the normalized and original positions stay aligned.
| override public func target(forAction action: Selector, withSender sender: Any?) -> Any? { | ||
| // When custom action routing is enabled, route custom actions up the responder chain. | ||
| // This ensures custom actions (like "Highlight") reach the parent view controller | ||
| // instead of being handled by PDFView, which is necessary for them to work properly. | ||
| guard enableCustomActionRouting, editingActions.handlesAction(action) else { | ||
| return super.target(forAction: action, withSender: sender) | ||
| } | ||
|
|
||
| // Traverse the responder chain manually to find the first responder | ||
| // that implements the action. Simply returning `next` is not sufficient | ||
| // because UIKit will still send the action back to this view. | ||
| var responder = next | ||
| while let currentResponder = responder { | ||
| if currentResponder.responds(to: action) { | ||
| return currentResponder | ||
| } | ||
| responder = currentResponder.next | ||
| } | ||
|
|
||
| // If no responder in the chain handles this action, fall back to default | ||
| // PDFView behavior to preserve native actions (copy, share, lookup, etc.) | ||
| return super.target(forAction: action, withSender: sender) |
There was a problem hiding this comment.
target(forAction:withSender:) routes any selector that EditingActionsController.handlesAction returns true for. Since handlesAction includes native actions (copy/share/lookup/translate), enabling custom action routing will also reroute those native selectors up the responder chain, bypassing PDFDocumentView’s overrides (e.g. copy(_:)) and potentially breaking standard behavior. Consider routing only .custom editing actions (e.g. add a handlesCustomAction(_:) API or check the action kind) and let PDFView handle native actions as before.
| // Find the decoration and group that owns this annotation | ||
| var foundDecoration: Decoration? | ||
| var foundGroup: String? | ||
|
|
||
| for (group, decorationList) in decorations { | ||
| if let diffableDecoration = decorationList.first(where: { $0.decoration.id == decorationId }) { | ||
| foundDecoration = diffableDecoration.decoration | ||
| foundGroup = group | ||
| break | ||
| } | ||
| } | ||
|
|
||
| guard let decoration = foundDecoration, let group = foundGroup, let pdfView = pdfView else { |
There was a problem hiding this comment.
annotationWasHit identifies the decoration/group by searching decorations for a matching decorationId, but PDFAnnotation only stores the id (.name). If the same decoration id is used in multiple groups (which DecorationKey explicitly anticipates), this can dispatch the activation event to the wrong group/decoration. Consider storing a composite identifier (eg group|id) in the annotation, or resolving via annotationsByKey by finding which DecorationKey owns the tapped annotation instance.
| // Find the decoration and group that owns this annotation | |
| var foundDecoration: Decoration? | |
| var foundGroup: String? | |
| for (group, decorationList) in decorations { | |
| if let diffableDecoration = decorationList.first(where: { $0.decoration.id == decorationId }) { | |
| foundDecoration = diffableDecoration.decoration | |
| foundGroup = group | |
| break | |
| } | |
| } | |
| guard let decoration = foundDecoration, let group = foundGroup, let pdfView = pdfView else { | |
| // Find the decoration and group that owns this annotation. | |
| // If multiple groups contain a decoration with the same ID, treat this as ambiguous | |
| // and do not dispatch an activation event to avoid mis-routing. | |
| var foundDecoration: Decoration? | |
| var foundGroup: String? | |
| var conflictFound = false | |
| for (group, decorationList) in decorations { | |
| if let diffableDecoration = decorationList.first(where: { $0.decoration.id == decorationId }) { | |
| if foundDecoration == nil && foundGroup == nil { | |
| // First match: tentatively record it. | |
| foundDecoration = diffableDecoration.decoration | |
| foundGroup = group | |
| } else if foundGroup != group || foundDecoration?.id != diffableDecoration.decoration.id { | |
| // A second, distinct match was found for the same ID; this is ambiguous. | |
| conflictFound = true | |
| foundDecoration = nil | |
| foundGroup = nil | |
| break | |
| } | |
| } | |
| } | |
| guard !conflictFound, let decoration = foundDecoration, let group = foundGroup, let pdfView = pdfView else { |
1 participant
This PR brings DecorableNavigator support to PDF publications, enabling highlights and underlines. It's the first of two PRs split from #668 as requested—this one focuses solely on decorations, with the ContentIterator (TTS) to follow separately. Unfortunately, this PR was not small/atomic as I hoped it would be.
The implementation extracts rich anchor data when a user makes a selection: coordinate quads, character ranges, and surrounding text context, then stores it in
Locator.Locations.otherLocations["pdfAnchor"]. This uses the Locator's designated extension point rather than adding custom top-level properties, maintaining interoperability with other platforms (and hopefully addressing the feedback from the first review).When rendering highlights, a priority-based resolver tries the most precise method first (stored coordinates),falls back to character ranges, and finally attempts context-aware text search if all else fails. This graceful degradation means highlights stay accurate when possible, but still render something even in edge cases.
I also had to wrangle iOS 16's UIEditMenuInteraction system to get custom actions (like "Highlight") working properly in the context menu since PDFKit doesn't route these through the responder chain correctly by default.
Unit tests are included for the anchor extraction and resolution logic, covering: