feat(pdf): add highlight support with anchor extraction and resolution#705
feat(pdf): add highlight support with anchor extraction and resolution#705darktasevski wants to merge 6 commits intoreadium:developfrom
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 { |
mickael-menu
left a comment
mickael-menu
left a comment
There was a problem hiding this comment.
Thank you @darktasevski, this is great work!
Sorry for the delay. I should be able to respond faster to any follow-up changes.
| /// - Coordinate quads for pixel-perfect rendering | ||
| /// - Character ranges for text-based lookup | ||
| /// - Surrounding text context for disambiguation | ||
| public struct PDFAnchorExtractor: Loggable { |
There was a problem hiding this comment.
It doesn't look like this API needs to be public?
| if let before = before { | ||
| anchor["textBefore"] = before | ||
| } | ||
| if let after = after { | ||
| anchor["textAfter"] = after | ||
| } |
There was a problem hiding this comment.
These data should be part of the locator.text struct.
| /// Controls custom action routing behavior for PDF text selection menus. | ||
| /// | ||
| /// When `true`, custom editing actions (created via `EditingAction(title:action:)`) | ||
| /// will be routed up the responder chain to the parent view controller instead of | ||
| /// being handled by PDFKit's PDFView. This is necessary for custom actions like | ||
| /// "Highlight" to work properly, especially on iOS 16+ where they need to reach | ||
| /// the view controller implementing the action. | ||
| /// | ||
| /// **Default**: `true` when custom actions are present, `false` otherwise. | ||
| /// | ||
| /// Set to `false` if you want to handle actions within the PDFView itself or | ||
| /// need the legacy behavior. | ||
| public var enableCustomActionRouting: Bool |
There was a problem hiding this comment.
Is adding this new config property necessary if we infer this from editingActions already? As it's required for .custom actions, in which case would you set this to false when using custom actions?
Set to
falseif you want to handle actions within the PDFView itself or need the legacy behavior.
You mentioned that the legacy behavior doesn't work with custom actions anyways. But I'm not sure what you mean by "if you want to handle actions within the PDFView itself"? Is it something that integrators can do without forking the toolkit?
| /// Controls whether to prevent PDFKit's default annotation context menu on iOS 16+. | ||
| /// | ||
| /// When `true`, blocks `UIEditMenuInteraction` instances that PDFKit automatically | ||
| /// adds for showing annotation context menus (e.g., when tapping existing highlights). | ||
| /// This is useful when you want to provide your own custom annotation UI. | ||
| /// | ||
| /// **Default**: `false` (preserves PDFKit's default annotation menus). | ||
| /// | ||
| /// Set to `true` if you're implementing custom annotation management and want to | ||
| /// prevent PDFKit's built-in annotation menus from appearing. | ||
| public var preventDefaultAnnotationMenu: Bool |
There was a problem hiding this comment.
What's the default annotation context menu that PDFKit displays?
In the current state, running on an iPad (A16) with iOS 26, tapping on a highlight does nothing and I get this log:
The edit menu (configuration: (null)) can only be presented when the view is in a window scene: (null)
But I think in the case of Readium we just want to trigger the "decoration activated" event and not show anything.
| get { objc_getAssociatedObject(self, &decorationsKey) as? [String: [DiffableDecoration]] ?? [:] } | ||
| set { objc_setAssociatedObject(self, &decorationsKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } |
There was a problem hiding this comment.
Did you use this because it's in an extension and you can't add real properties? I'd prefer having regular properties in the PDFNavigatorViewController in this case. Maybe even extracting most of it in a dedicated PDFViewModel or PDFDecorationsController @MainActor, as the PDFNavigatorViewController is getting pretty busy.
| inGroup group: String, | ||
| onActivated: @escaping OnActivatedCallback | ||
| ) { | ||
| _ = observeDecorationInteractionsCancellable(inGroup: group, onActivated: onActivated) |
There was a problem hiding this comment.
Wouldn't this be cancelled immediately as the AnyCancellable is not stored anywhere? You could add it to a subscriptions property in the PDFNavigatorViewController.
| public func observeDecorationInteractionsCancellable( | ||
| inGroup group: String, | ||
| onActivated: @escaping OnActivatedCallback | ||
| ) -> AnyCancellable { |
There was a problem hiding this comment.
Do you have a use case where cancelling / removing a interaction callback was needed before the PDFNavigatorViewController is discarded?
If yes, I think it's worth modifying the observeDecorationInteractions() in DecorableNavigator directly to support it in all navigators. Otherwise I think we can stick with what's already there (no cancellation support). In any case we don't want to have a dedicated observeDecorationInteractionsCancellable only available in the PDF navigator.
| func handlesAction(_ selector: Selector) -> Bool { | ||
| actions.contains { $0.actions.contains(selector) } | ||
| } |
There was a problem hiding this comment.
Aren't you able to use canPerformAction() for this?
| /// which properly integrate with iOS 16's `UIEditMenuInteraction` system. | ||
| /// It maintains backward compatibility by only affecting iOS 16+ behavior. | ||
| @available(iOS 16.0, *) | ||
| private func addCustomActionsToMenu(_ builder: UIMenuBuilder) { |
There was a problem hiding this comment.
Nice, we will be able to remove the iOS 15 system entirely next year when we bump to iOS 16 👍
| // Try to extract anchor from otherLocations | ||
| guard let anchorData = locator.locations.otherLocations["pdfAnchor"] else { |
There was a problem hiding this comment.
Unfortunately this solution is still not interoperable. We added otherLocations for in-house third-party extensions, but the toolkit itself should stick to standardized locations so that other toolkits and reading systems can use it.
In the case of PDF, that means using PDF Fragment Identifiers instead, as the codebase already does for page=N.
| Custom field | Standard alternative |
|---|---|
pageIndex (0-based) |
page=N (1-based) in locator.locations.fragments, already used by PDFPositionsService and parsed in pageNumber(for:) |
text |
locator.text.highlight |
quads |
highlight=<left>,<right>,<top>,<bottom> in locator.locations.fragments - one per line, per RFC 8118 |
textBefore / textAfter |
locator.text.before / locator.text.after |
characterStart / characterEnd |
No direct spec equivalent |
For example:
locations: Locator.Locations(
fragments: [
"page=3",
"highlight=72,200,120,140", // line 1 bounding rect
"highlight=72,200,160,180" // line 2 bounding rect,
// Can also be in a combined form:
"page=3&highlight=72,200,120,140&highlight=72,200,160,180",
]
),
text: Locator.Text(
highlight: "the selected text",
before: "20 chars before ",
after: " 20 chars after"
)
I looked into it and there are no equivalent to character offsets in the PDF spec. While it should work fine if you use PDFKit to create and render such decorations, it might break when synchronizating the locators with a different reading system. So I think we should work with highlight= and use text as a fallback.
2 participants
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: