From e881e4085f86cb38a0dc7f6ed5b9238b6ebea4da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dark=C3=B8=20Tasevski?= Date: Mon, 26 Jan 2026 10:56:08 +0100 Subject: [PATCH 1/5] feat(pdf): add highlight support with anchor extraction and resolution 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. --- .../Navigator/PDF/PDFAnchorExtractor.swift | 197 +++++++++ Sources/Navigator/PDF/PDFAnchorResolver.swift | 394 ++++++++++++++++++ Sources/Navigator/PDF/PDFDocumentView.swift | 95 ++++- .../PDF/PDFNavigatorViewController.swift | 374 ++++++++++++++++- .../Reader/PDF/PDFViewController.swift | 12 +- 5 files changed, 1061 insertions(+), 11 deletions(-) create mode 100644 Sources/Navigator/PDF/PDFAnchorExtractor.swift create mode 100644 Sources/Navigator/PDF/PDFAnchorResolver.swift diff --git a/Sources/Navigator/PDF/PDFAnchorExtractor.swift b/Sources/Navigator/PDF/PDFAnchorExtractor.swift new file mode 100644 index 000000000..59fa1cb75 --- /dev/null +++ b/Sources/Navigator/PDF/PDFAnchorExtractor.swift @@ -0,0 +1,197 @@ +// +// Copyright 2026 Readium Foundation. All rights reserved. +// Use of this source code is governed by the BSD-style license +// available in the top-level LICENSE file of the project. +// + +import Foundation +import PDFKit +import ReadiumShared + +/// Extracts precise anchor data from a PDF selection for highlight persistence. +/// +/// The extractor captures multiple forms of positioning data: +/// - Coordinate quads for pixel-perfect rendering +/// - Character ranges for text-based lookup +/// - Surrounding text context for disambiguation +public struct PDFAnchorExtractor: Loggable { + + /// Number of characters to capture before/after the selection for context. + public static let contextCharacterCount = 20 + + /// Extracts anchor data from a PDF selection. + /// + /// - Parameters: + /// - selection: The PDF selection to extract anchor data from. + /// - page: The page containing the selection. + /// - Returns: A dictionary suitable for storage in Locator.Locations.otherLocations, + /// or nil if extraction fails. + public static func extractAnchor( + from selection: PDFSelection, + on page: PDFPage + ) -> [String: Any]? { + guard let pageIndex = page.document?.index(for: page), + let selectedText = selection.string, + !selectedText.isEmpty + else { + log(.debug, "PDF anchor extraction skipped: invalid selection") + return nil + } + + var anchor: [String: Any] = [ + "pageIndex": pageIndex, + "text": selectedText + ] + + // Extract coordinate quads + if let quads = extractQuads(from: selection, on: page) { + anchor["quads"] = quads + } + + // Extract character range + if let pageText = page.string, + let range = extractCharacterRange(for: selectedText, in: pageText, selection: selection, page: page) { + anchor["characterStart"] = range.lowerBound + anchor["characterEnd"] = range.upperBound + + // Extract context + let (before, after) = extractContext( + around: range, + in: pageText, + contextLength: contextCharacterCount + ) + if let before = before { + anchor["textBefore"] = before + } + if let after = after { + anchor["textAfter"] = after + } + } + + log(.debug, "PDF anchor extracted: page=\(pageIndex), chars=\(anchor["characterStart"] ?? "nil")-\(anchor["characterEnd"] ?? "nil"), quads=\(anchor["quads"] != nil)") + + return anchor + } + + /// Extracts quadrilateral bounds for each line of the selection. + private static func extractQuads( + from selection: PDFSelection, + on page: PDFPage + ) -> [[[String: Double]]]? { + let lineSelections = selection.selectionsByLine() + guard !lineSelections.isEmpty else { return nil } + + var quads: [[[String: Double]]] = [] + + for lineSelection in lineSelections { + let bounds = lineSelection.bounds(for: page) + guard !bounds.isNull, !bounds.isEmpty else { continue } + + // Convert CGRect to quad (4 corner points) + let quad: [[String: Double]] = [ + ["x": Double(bounds.minX), "y": Double(bounds.minY)], // bottomLeft + ["x": Double(bounds.maxX), "y": Double(bounds.minY)], // bottomRight + ["x": Double(bounds.maxX), "y": Double(bounds.maxY)], // topRight + ["x": Double(bounds.minX), "y": Double(bounds.maxY)] // topLeft + ] + quads.append(quad) + } + + return quads.isEmpty ? nil : quads + } + + /// Extracts the character range of the selection within the page text. + private static func extractCharacterRange( + for selectedText: String, + in pageText: String, + selection: PDFSelection, + page: PDFPage + ) -> Range? { + // Try to get the range from PDFKit's selection + // PDFSelection doesn't expose character range directly, so we search + + // Find all occurrences of the selected text + var ranges: [Range] = [] + var searchStart = pageText.startIndex + + while let range = pageText.range(of: selectedText, range: searchStart.., + in text: String, + contextLength: Int + ) -> (before: String?, after: String?) { + let startIndex = text.index(text.startIndex, offsetBy: range.lowerBound) + let endIndex = text.index(text.startIndex, offsetBy: range.upperBound) + + // Extract before context + let beforeStart = text.index( + startIndex, + offsetBy: -contextLength, + limitedBy: text.startIndex + ) ?? text.startIndex + let before = String(text[beforeStart.. Bool { + abs(a.minX - b.minX) <= tolerance && + abs(a.minY - b.minY) <= tolerance && + abs(a.maxX - b.maxX) <= tolerance && + abs(a.maxY - b.maxY) <= tolerance + } +} diff --git a/Sources/Navigator/PDF/PDFAnchorResolver.swift b/Sources/Navigator/PDF/PDFAnchorResolver.swift new file mode 100644 index 000000000..e789ff26c --- /dev/null +++ b/Sources/Navigator/PDF/PDFAnchorResolver.swift @@ -0,0 +1,394 @@ +// +// Copyright 2026 Readium Foundation. All rights reserved. +// Use of this source code is governed by the BSD-style license +// available in the top-level LICENSE file of the project. +// + +import Foundation +import PDFKit +import ReadiumShared + +/// Resolves PDF anchor data back to coordinate bounds for rendering highlights. +/// +/// Uses a priority-based resolution strategy: +/// 1. Quads (pixel-perfect) - directly use stored coordinates +/// 2. Character range (precise) - create selection from stored offsets +/// 3. Context-aware text search (fallback) - find text with surrounding context +public struct PDFAnchorResolver: Loggable { + + /// Resolves anchor data from a locator to renderable bounds. + /// + /// - Parameters: + /// - locator: The locator containing anchor data in otherLocations. + /// - page: The PDF page to resolve bounds on. + /// - Returns: Array of CGRect bounds for each line, or empty if resolution fails. + public static func resolveBounds( + from locator: Locator, + on page: PDFPage + ) -> [CGRect] { + // Try to extract anchor from otherLocations + guard let anchorData = locator.locations.otherLocations["pdfAnchor"] else { + // No anchor data - fall back to legacy text search + return legacyTextSearch(locator: locator, page: page) + } + + // Parse anchor data + guard let anchor = parseAnchor(anchorData) else { + log(.warning, "Failed to parse PDF anchor data") + return legacyTextSearch(locator: locator, page: page) + } + + // Strategy 1: Try quads first (pixel-perfect) + if let bounds = resolveFromQuads(anchor) { + log(.debug, "Resolved PDF highlight from quads") + return bounds + } + + // Strategy 2: Try character range (precise) + if let bounds = resolveFromCharacterRange(anchor, page: page) { + log(.debug, "Resolved PDF highlight from character range") + return bounds + } + + // Strategy 3: Context-aware text search (fallback) + if let bounds = resolveFromContextSearch(anchor, page: page) { + log(.debug, "Resolved PDF highlight from context search") + return bounds + } + + log(.warning, "All PDF anchor resolution strategies failed") + return [] + } + + // MARK: - Private Resolution Methods + + private static func parseAnchor(_ data: Any) -> ParsedAnchor? { + // Handle both dictionary and JSON string formats + let dict: [String: Any] + if let d = data as? [String: Any] { + dict = d + } else if let jsonString = data as? String, + let jsonData = jsonString.data(using: .utf8), + let d = try? JSONSerialization.jsonObject(with: jsonData) as? [String: Any] { + dict = d + } else { + return nil + } + + guard let text = dict["text"] as? String else { + return nil + } + + return ParsedAnchor( + pageIndex: dict["pageIndex"] as? Int, + quads: parseQuads(dict["quads"]), + characterStart: dict["characterStart"] as? Int, + characterEnd: dict["characterEnd"] as? Int, + text: text, + textBefore: dict["textBefore"] as? String, + textAfter: dict["textAfter"] as? String + ) + } + + private 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 } + return quad.compactMap { point -> CGPoint? in + guard let x = point["x"], let y = point["y"] else { return nil } + return CGPoint(x: x, y: y) + } + } + } + + private static func resolveFromQuads(_ anchor: ParsedAnchor) -> [CGRect]? { + guard let quads = anchor.quads, !quads.isEmpty else { + return nil + } + + return quads.compactMap { quad -> CGRect? in + guard quad.count == 4 else { return nil } + + // Convert quad points to bounding rect + let minX = quad.map(\.x).min() ?? 0 + let maxX = quad.map(\.x).max() ?? 0 + let minY = quad.map(\.y).min() ?? 0 + let maxY = quad.map(\.y).max() ?? 0 + + let rect = CGRect(x: minX, y: minY, width: maxX - minX, height: maxY - minY) + guard !rect.isEmpty else { return nil } + return rect + } + } + + private static func resolveFromCharacterRange( + _ anchor: ParsedAnchor, + page: PDFPage + ) -> [CGRect]? { + guard let start = anchor.characterStart, + let end = anchor.characterEnd, + start < end + else { + return nil + } + + let nsRange = NSRange(location: start, length: end - start) + guard let selection = page.selection(for: nsRange) else { + return nil + } + + return boundsFromSelection(selection, page: page) + } + + private static func resolveFromContextSearch( + _ anchor: ParsedAnchor, + page: PDFPage + ) -> [CGRect]? { + guard let pageText = page.string else { + return nil + } + + // Find all occurrences of the text + var ranges: [Range] = [] + var searchStart = pageText.startIndex + + while let range = pageText.range(of: anchor.text, range: searchStart.., + anchor: ParsedAnchor, + in text: String + ) -> Int { + var score = 0 + + if let before = anchor.textBefore { + let contextLength = before.count + let contextStart = text.index( + range.lowerBound, + offsetBy: -contextLength, + limitedBy: text.startIndex + ) ?? text.startIndex + let actualBefore = String(text[contextStart.. [CGRect] { + let lineSelections = selection.selectionsByLine() + + var bounds: [CGRect] = [] + for lineSelection in lineSelections { + let lineBounds = lineSelection.bounds(for: page) + guard !lineBounds.isNull, !lineBounds.isEmpty else { continue } + bounds.append(lineBounds) + } + + // Fallback to full selection bounds + if bounds.isEmpty { + let fullBounds = selection.bounds(for: page) + if !fullBounds.isNull, !fullBounds.isEmpty { + bounds.append(fullBounds) + } + } + + return bounds + } + + private static func legacyTextSearch(locator: Locator, page: PDFPage) -> [CGRect] { + guard let highlightedText = locator.text.highlight, + !highlightedText.isEmpty + else { + return [] + } + + guard let pageText = page.string else { + return [] + } + + // Strategy 1: Try exact match first + if let range = pageText.range(of: highlightedText, options: .caseInsensitive) { + let nsRange = NSRange(range, in: pageText) + if let selection = page.selection(for: nsRange) { + return boundsFromSelection(selection, page: page) + } + } + + // Strategy 2: Normalize whitespace and try again + // TTS may combine text with spaces while PDF has newlines + let normalizedSearch = normalizeWhitespace(highlightedText) + let normalizedPage = normalizeWhitespace(pageText) + + if let normalizedRange = normalizedPage.range(of: normalizedSearch, options: .caseInsensitive) { + // Map back to original page text range + // Count characters up to the match in normalized text + let normalizedPrefix = String(normalizedPage[..= 3, let range = pageText.range(of: firstWords, options: .caseInsensitive) { + let nsRange = NSRange(range, in: pageText) + if let selection = page.selection(for: nsRange) { + return boundsFromSelection(selection, page: page) + } + } + + return [] + } + + /// Normalizes whitespace by collapsing multiple spaces/newlines into single spaces + private static func normalizeWhitespace(_ text: String) -> String { + let components = text.components(separatedBy: .whitespacesAndNewlines) + return components.filter { !$0.isEmpty }.joined(separator: " ") + } + + /// Extracts the first N words from text + private static func extractFirstWords(from text: String, count: Int) -> String { + let words = text.split(separator: " ", omittingEmptySubsequences: true) + let firstWords = words.prefix(count) + return firstWords.joined(separator: " ") + } + + /// Maps a position from normalized text back to original text + private static func findOriginalRange( + in originalText: String, + normalizedPrefix: String, + matchLength: Int + ) -> Range? { + // 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 + } + + if originalStart != nil && normalizedPosition >= targetEnd { + originalEnd = originalText.index(after: i) + break + } + + i = originalText.index(after: i) + } + + guard let start = originalStart, let end = originalEnd else { + return nil + } + + return start.. UIEdgeInsets? @@ -15,13 +16,41 @@ public final class PDFDocumentView: PDFView { var editingActions: EditingActionsController private weak var documentViewDelegate: PDFDocumentViewDelegate? + /// When `true`, routes custom editing actions up the responder chain instead of + /// letting PDFView handle them. Required for custom actions like "Highlight" to work. + private let enableCustomActionRouting: Bool + + /// When `true`, prevents PDFKit's default annotation context menu from appearing. + /// + /// - Warning: This removes **all** `UIEditMenuInteraction` instances from the PDF view, + /// which may affect text selection menus (Copy, Look Up) and accessibility features. + /// Only enable if you're providing a complete custom menu implementation. + /// + /// If you experience issues with text selection or VoiceOver, consider disabling this + /// option and using `buildMenu(with:)` to selectively remove unwanted menu items instead. + private let preventDefaultAnnotationMenu: Bool + + override public var document: PDFDocument? { + didSet { + // Remove annotation menu interactions after document is set, as PDFKit + // may add them during document loading + if preventDefaultAnnotationMenu, #available(iOS 16.0, *) { + removeAnnotationMenuInteractions() + } + } + } + init( frame: CGRect, editingActions: EditingActionsController, - documentViewDelegate: PDFDocumentViewDelegate + documentViewDelegate: PDFDocumentViewDelegate, + enableCustomActionRouting: Bool = true, + preventDefaultAnnotationMenu: Bool = false ) { self.editingActions = editingActions self.documentViewDelegate = documentViewDelegate + self.enableCustomActionRouting = enableCustomActionRouting + self.preventDefaultAnnotationMenu = preventDefaultAnnotationMenu super.init(frame: frame) @@ -32,6 +61,37 @@ public final class PDFDocumentView: PDFView { // Thefore, we will handle the adjustement manually by only taking the notch area into // account. firstScrollView?.contentInsetAdjustmentBehavior = .never + + // Optionally prevent the default annotation context menu from appearing. + // Only applies when preventDefaultAnnotationMenu is true. + if preventDefaultAnnotationMenu, #available(iOS 16.0, *) { + removeAnnotationMenuInteractions() + } + } + + /// Removes all `UIEditMenuInteraction` instances from the view. + /// + /// This is an aggressive approach that may affect more than just annotation menus. + /// See `preventDefaultAnnotationMenu` documentation for limitations and alternatives. + @available(iOS 16.0, *) + private func removeAnnotationMenuInteractions() { + for interaction in interactions where interaction is UIEditMenuInteraction { + removeInteraction(interaction) + } + } + + /// Intercepts interaction additions to block `UIEditMenuInteraction` when + /// `preventDefaultAnnotationMenu` is enabled. + /// + /// This prevents PDFKit from re-adding edit menu interactions after document + /// changes or view updates. See `preventDefaultAnnotationMenu` for limitations. + override public func addInteraction(_ interaction: UIInteraction) { + if preventDefaultAnnotationMenu, #available(iOS 16.0, *) { + guard !(interaction is UIEditMenuInteraction) else { + return + } + } + super.addInteraction(interaction) } @available(*, unavailable) @@ -76,7 +136,14 @@ public final class PDFDocumentView: PDFView { } override public func canPerformAction(_ action: Selector, withSender sender: Any?) -> Bool { - super.canPerformAction(action, withSender: sender) && editingActions.canPerformAction(action) + // 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?) { @@ -119,7 +186,7 @@ public final class PDFDocumentView: PDFView { // // - Visual snap: There is no API to pre-set the zoom scale for the next // page. PDFView resets the scale per page, causing a visible snap - // when swiping. We don’t see the issue with edge taps. + // when swiping. We don't see the issue with edge taps. // - Incorrect anchoring: When zooming larger than the page fit, the // viewport centers vertically instead of showing the top. The API to // fix this works in scroll mode but is ignored in paginated mode. @@ -295,4 +362,26 @@ public final class PDFDocumentView: PDFView { // Use the smaller scale to ensure both dimensions fit return min(widthScale, heightScale) } + + 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 + } + + return nil + } } diff --git a/Sources/Navigator/PDF/PDFNavigatorViewController.swift b/Sources/Navigator/PDF/PDFNavigatorViewController.swift index 527c54999..027274841 100644 --- a/Sources/Navigator/PDF/PDFNavigatorViewController.swift +++ b/Sources/Navigator/PDF/PDFNavigatorViewController.swift @@ -4,6 +4,7 @@ // available in the top-level LICENSE file of the project. // +import Combine import Foundation import PDFKit import ReadiumShared @@ -37,14 +38,54 @@ open class PDFNavigatorViewController: /// The default set of editing actions is `EditingAction.defaultActions`. public var editingActions: [EditingAction] + /// 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 + + /// 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 + public init( preferences: PDFPreferences = PDFPreferences(), defaults: PDFDefaults = PDFDefaults(), - editingActions: [EditingAction] = EditingAction.defaultActions + editingActions: [EditingAction] = EditingAction.defaultActions, + enableCustomActionRouting: Bool? = nil, + preventDefaultAnnotationMenu: Bool = false ) { self.preferences = preferences self.defaults = defaults self.editingActions = editingActions + + // Default to true if there are custom actions, false otherwise + if let enableCustomActionRouting = enableCustomActionRouting { + self.enableCustomActionRouting = enableCustomActionRouting + } else { + self.enableCustomActionRouting = editingActions.contains { action in + guard case .custom = action.kind else { return false } + return true + } + } + + self.preventDefaultAnnotationMenu = preventDefaultAnnotationMenu } } @@ -246,7 +287,9 @@ open class PDFNavigatorViewController: let pdfView = PDFDocumentView( frame: view.bounds, editingActions: editingActions, - documentViewDelegate: self + documentViewDelegate: self, + enableCustomActionRouting: config.enableCustomActionRouting, + preventDefaultAnnotationMenu: config.preventDefaultAnnotationMenu ) self.pdfView = pdfView pdfView.delegate = self @@ -274,6 +317,7 @@ open class PDFNavigatorViewController: NotificationCenter.default.addObserver(self, selector: #selector(pageDidChange), name: .PDFViewPageChanged, object: pdfView) NotificationCenter.default.addObserver(self, selector: #selector(visiblePagesDidChange), name: .PDFViewVisiblePagesChanged, object: pdfView) NotificationCenter.default.addObserver(self, selector: #selector(selectionDidChange), name: .PDFViewSelectionChanged, object: pdfView) + NotificationCenter.default.addObserver(self, selector: #selector(annotationWasHit), name: .PDFViewAnnotationHit, object: pdfView) if let locator = locator { await go(to: locator, isJump: false) @@ -520,15 +564,17 @@ open class PDFNavigatorViewController: } } - guard - let positions = positionsByReadingOrder, - var position = locator.locations.position - else { + guard var position = locator.locations.position else { return nil } + // For multi-resource publications, adjust position relative to the resource's first page. + // This requires positionsByReadingOrder to be loaded. For single-resource publications + // or when positions aren't loaded yet, we can use the position directly since it + // represents the absolute page number within that resource. if publication.readingOrder.count > 1, + let positions = positionsByReadingOrder, let index = publication.readingOrder.firstIndexWithHREF(locator.href), let firstPosition = positions[index].first?.locations.position { @@ -603,14 +649,70 @@ open class PDFNavigatorViewController: return } + // Build locator with anchor data + var updatedLocator = locator.copy(text: { $0.highlight = text }) + + // Extract and attach PDF anchor for precise repositioning + if let anchorData = PDFAnchorExtractor.extractAnchor(from: selection, on: page) { + updatedLocator = updatedLocator.copy(locations: { locations in + var otherLocations = locations.otherLocations + otherLocations["pdfAnchor"] = anchorData + locations.otherLocations = otherLocations + }) + } + editingActions.selection = Selection( - locator: locator.copy(text: { $0.highlight = text }), + locator: updatedLocator, frame: pdfView.convert(selection.bounds(for: page), from: page) // Makes it slightly bigger to have more room when displaying a popover. .insetBy(dx: -8, dy: -8) ) } + @MainActor @objc private func annotationWasHit(_ notification: Notification) { + guard let annotation = notification.userInfo?["PDFAnnotationHit"] as? PDFAnnotation else { + return + } + + // Get the decoration ID from the annotation + guard let decorationId = annotation.value(forAnnotationKey: .name) as? String else { + return + } + + // 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 { + return + } + + // Get the bounds of the annotation in the view's coordinate space + guard let page = annotation.page else { + return + } + + let annotationBounds = annotation.bounds + let viewBounds = pdfView.convert(annotationBounds, from: page) + + let event = OnDecorationActivatedEvent( + decoration: decoration, + group: group, + rect: viewBounds, + point: nil + ) + + notifyDecorationActivated(event) + } + /// From iOS 13 to 15, the Share menu action is impossible to remove without /// resorting to complex method swizzling in the subviews of ``PDFView``. /// (https://stackoverflow.com/a/61361294) @@ -743,6 +845,264 @@ extension PDFNavigatorViewController: UIGestureRecognizerDelegate { } } +// MARK: - DecorableNavigator + +extension PDFNavigatorViewController: DecorableNavigator { + // MARK: - Highlight Appearance Constants + + /// Default tint color for highlights when no custom color is specified. + private static var defaultHighlightTint: UIColor { .yellow } + + /// Alpha value for active (selected) highlights. + private static var activeHighlightAlpha: CGFloat { 0.5 } + + /// Alpha value for inactive highlights. + private static var inactiveHighlightAlpha: CGFloat { 0.3 } + + // MARK: - Associated Object Storage + + /// Storage for decorations by group name + private var decorations: [String: [DiffableDecoration]] { + get { objc_getAssociatedObject(self, &decorationsKey) as? [String: [DiffableDecoration]] ?? [:] } + set { objc_setAssociatedObject(self, &decorationsKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } + } + + /// Storage for PDF annotations mapped by decoration ID + private var annotationsByDecorationId: [Decoration.Id: [PDFKit.PDFAnnotation]] { + get { objc_getAssociatedObject(self, &annotationsKey) as? [Decoration.Id: [PDFKit.PDFAnnotation]] ?? [:] } + set { objc_setAssociatedObject(self, &annotationsKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } + } + + /// Storage for decoration interaction callbacks by group, with tokens for removal + private var decorationCallbacks: [String: [(token: UUID, callback: OnActivatedCallback)]] { + get { objc_getAssociatedObject(self, &callbacksKey) as? [String: [(token: UUID, callback: OnActivatedCallback)]] ?? [:] } + set { objc_setAssociatedObject(self, &callbacksKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } + } + + public func supports(decorationStyle style: Decoration.Style.Id) -> Bool { + // PDF supports highlight and underline decoration styles + return style == .highlight || style == .underline + } + + public func apply(decorations newDecorations: [Decoration], in group: String) { + // Thread safety: Associated object storage uses OBJC_ASSOCIATION_RETAIN_NONATOMIC, + // which is not thread-safe. PDFKit also requires main thread access. + // TODO: Consider adding @MainActor isolation when migrating to Swift 6. + dispatchPrecondition(condition: .onQueue(.main)) + + log(.debug, "PDF DecorableNavigator.apply called: \(newDecorations.count) decorations in group '\(group)'") + + guard let pdfView = pdfView, let document = pdfView.document else { + log(.warning, "PDF DecorableNavigator.apply: pdfView or document is nil") + return + } + + // 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) + case .remove(let id): + removeAnnotation(withId: id, from: document) + case .update(let decoration): + removeAnnotation(withId: decoration.id, from: document) + addAnnotation(for: decoration, in: document) + } + } + } + } + + /// Protocol conformance - registers a callback for decoration interactions. + /// + /// For cancellation support, use `observeDecorationInteractionsCancellable(inGroup:onActivated:)` instead. + /// + /// - Parameters: + /// - group: The decoration group to observe. + /// - onActivated: Callback invoked when a decoration in this group is tapped. + /// + /// - Important: Callers should use `[weak self]` in callbacks to avoid retain cycles. + public func observeDecorationInteractions( + inGroup group: String, + onActivated: @escaping OnActivatedCallback + ) { + _ = observeDecorationInteractionsCancellable(inGroup: group, onActivated: onActivated) + } + + /// Observes decoration interactions with cancellation support. + /// + /// Returns an `AnyCancellable` that removes the callback when cancelled or deallocated. + /// Store the cancellable to maintain the observation, or call `cancel()` to stop observing. + /// + /// - Parameters: + /// - group: The decoration group to observe. + /// - onActivated: Callback invoked when a decoration in this group is tapped. + /// - Returns: A cancellable that removes the callback when deallocated or cancelled. + /// + /// ## Example + /// ```swift + /// private var cancellables = Set() + /// + /// func setupHighlights() { + /// navigator.observeDecorationInteractionsCancellable(inGroup: "highlights") { [weak self] event in + /// self?.handleHighlightTapped(event) + /// } + /// .store(in: &cancellables) + /// } + /// ``` + /// + /// - Important: Callers should still use `[weak self]` in callbacks to avoid retain cycles + /// if the cancellable is stored on the same object that holds the navigator. + @discardableResult + public func observeDecorationInteractionsCancellable( + inGroup group: String, + onActivated: @escaping OnActivatedCallback + ) -> AnyCancellable { + let token = UUID() + var callbacks = decorationCallbacks[group] ?? [] + callbacks.append((token: token, callback: onActivated)) + decorationCallbacks[group] = callbacks + + return AnyCancellable { [weak self] in + // Must be called on main thread to safely modify decorationCallbacks + dispatchPrecondition(condition: .onQueue(.main)) + guard let self = self else { return } + var callbacks = self.decorationCallbacks[group] ?? [] + callbacks.removeAll { $0.token == token } + self.decorationCallbacks[group] = callbacks.isEmpty ? nil : callbacks + } + } + + // MARK: - Private Helpers + + private func addAnnotation(for decoration: Decoration, in document: PDFKit.PDFDocument) { + log(.debug, "addAnnotation: id=\(decoration.id), position=\(decoration.locator.locations.position ?? -1), text=\(decoration.locator.text.highlight?.prefix(30) ?? "nil")") + + guard let page = findPage(for: decoration.locator, in: document) else { + log(.warning, "Could not find page for decoration \(decoration.id) - position: \(decoration.locator.locations.position ?? -1)") + return + } + + let boundsArray = self.boundsForLines(for: decoration.locator, on: page) + guard !boundsArray.isEmpty else { + log(.warning, "Could not find bounds for decoration \(decoration.id) - text: '\(decoration.locator.text.highlight?.prefix(50) ?? "nil")'") + return + } + + log(.debug, "Creating \(boundsArray.count) PDF annotations for TTS highlight") + var createdAnnotations: [PDFKit.PDFAnnotation] = [] + for bounds in boundsArray { + let annotation = createAnnotation(for: decoration.style, bounds: bounds, decorationId: decoration.id) + page.addAnnotation(annotation) + createdAnnotations.append(annotation) + } + + annotationsByDecorationId[decoration.id] = createdAnnotations + } + + private func removeAnnotation(withId id: Decoration.Id, from document: PDFKit.PDFDocument) { + guard let annotations = annotationsByDecorationId[id] else { + return + } + + for annotation in annotations { + guard let page = annotation.page else { continue } + page.removeAnnotation(annotation) + } + + annotationsByDecorationId[id] = nil + } + + private func createAnnotation(for style: Decoration.Style, bounds: CGRect, decorationId: Decoration.Id) -> PDFKit.PDFAnnotation { + let annotation: PDFKit.PDFAnnotation + + // Extract highlight config if available + let config = style.config as? Decoration.Style.HighlightConfig + let tint = config?.tint ?? Self.defaultHighlightTint + let isActive = config?.isActive ?? false + let alpha = isActive ? Self.activeHighlightAlpha : Self.inactiveHighlightAlpha + + switch style.id { + case .highlight: + annotation = PDFKit.PDFAnnotation(bounds: bounds, forType: .highlight, withProperties: nil) + annotation.color = tint.withAlphaComponent(alpha) + + case .underline: + annotation = PDFKit.PDFAnnotation(bounds: bounds, forType: .underline, withProperties: nil) + annotation.color = tint + + default: + // Fallback to highlight for unknown styles + annotation = PDFKit.PDFAnnotation(bounds: bounds, forType: .highlight, withProperties: nil) + annotation.color = tint.withAlphaComponent(Self.inactiveHighlightAlpha) + } + + // Store decoration ID for later lookup + annotation.setValue(decorationId, forAnnotationKey: .name) + + return annotation + } + + private func findPage(for locator: Locator, in document: PDFKit.PDFDocument) -> PDFKit.PDFPage? { + guard let pageNumber = pageNumber(for: locator) else { + return nil + } + + // PDFKit uses 0-based indexing + let pageIndex = pageNumber - 1 + return document.page(at: pageIndex) + } + + /// Returns an array of CGRect bounds, one for each line of the highlighted text. + /// This ensures precise highlighting that follows text flow across multiple lines. + /// + /// Uses PDFAnchorResolver for priority-based resolution: + /// 1. Quads (pixel-perfect coordinates) + /// 2. Character range (text offset based) + /// 3. Context-aware text search (fallback) + private func boundsForLines(for locator: Locator, on page: PDFKit.PDFPage) -> [CGRect] { + // Use the anchor resolver for precise positioning + let bounds = PDFAnchorResolver.resolveBounds(from: locator, on: page) + + if !bounds.isEmpty { + return bounds + } + + // Final fallback: return empty (don't show misleading default rectangle) + log(.warning, "Could not resolve bounds for PDF highlight") + return [] + } + + /// Notifies all registered callbacks for the decoration's group that the decoration was activated. + private func notifyDecorationActivated(_ event: OnDecorationActivatedEvent) { + guard let callbacks = decorationCallbacks[event.group] else { + return + } + + for (_, callback) in callbacks { + callback(event) + } + } +} + +// Associated object keys for decoration storage +private var decorationsKey: UInt8 = 0 +private var annotationsKey: UInt8 = 0 +private var callbacksKey: UInt8 = 0 + private extension Axis { var displayDirection: PDFDisplayDirection { switch self { diff --git a/TestApp/Sources/Reader/PDF/PDFViewController.swift b/TestApp/Sources/Reader/PDF/PDFViewController.swift index 1bffcd829..4691736fa 100644 --- a/TestApp/Sources/Reader/PDF/PDFViewController.swift +++ b/TestApp/Sources/Reader/PDF/PDFViewController.swift @@ -26,11 +26,21 @@ final class PDFViewController: VisualReaderViewController Date: Mon, 26 Jan 2026 10:55:49 +0100 Subject: [PATCH 2/5] fix(navigator): route custom editing actions via UIEditMenuInteraction 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. --- Sources/Navigator/EditingAction.swift | 79 +++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/Sources/Navigator/EditingAction.swift b/Sources/Navigator/EditingAction.swift index 7203c9b9a..285df30b7 100644 --- a/Sources/Navigator/EditingAction.swift +++ b/Sources/Navigator/EditingAction.swift @@ -135,6 +135,18 @@ final class EditingActionsController { return delegate?.editingActions(self, canPerformAction: action, for: selection) ?? true } + /// Checks whether a given selector is handled by any of the configured editing actions. + /// + /// This is useful for custom responder chain handling, particularly in PDF views + /// where you need to distinguish between actions managed by this controller + /// and system actions. + /// + /// - Parameter selector: The selector to check. + /// - Returns: `true` if any editing action handles this selector, `false` otherwise. + func handlesAction(_ selector: Selector) -> Bool { + actions.contains { $0.actions.contains(selector) } + } + /// Verifies that the user has the rights to use the given `action`. private func isActionAllowed(_ action: EditingAction) -> Bool { switch action { @@ -158,9 +170,76 @@ final class EditingActionsController { // Expansion setting which allows to copy the selection. // To reproduce, comment out and select Japanese text on a PDF. builder.remove(menu: .learn) + + // iOS 16+ enhancement: Add custom actions as UICommand items to the edit menu. + // This ensures custom actions (like "Highlight") appear properly in the modern + // UIEditMenuInteraction on iOS 16+, fixing the issue where custom actions + // wouldn't show up or wouldn't route correctly to the responder chain. + if #available(iOS 16.0, *) { + addCustomActionsToMenu(builder) + } + } + + /// Adds custom editing actions to the menu builder for iOS 16+. + /// + /// This method converts custom `UIMenuItem` actions into `UICommand` items, + /// 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) { + // Extract custom actions and convert them to UICommand + let customElements: [UIMenuElement] = actions.compactMap { action in + switch action.kind { + case let .custom(menuItem): + return UICommand( + title: menuItem.title, + image: nil, + action: menuItem.action, + propertyList: nil + ) + case .native: + return nil + } + } + + guard !customElements.isEmpty else { + return + } + + // Check if we have any native actions (copy, lookup, translate, share) + let hasNativeActions = actions.contains { + if case .native = $0.kind { + return true + } + return false + } + + // Get existing menu items + var standardChildren = builder.menu(for: .standardEdit)?.children ?? [] + + if hasNativeActions { + // If we have native actions, prepend custom actions to preserve + // standard system actions (copy, lookup, etc.) + standardChildren.insert(contentsOf: customElements, at: 0) + } else { + // If only custom actions, replace the menu to avoid clutter + // This gives integrators full control when they don't use native actions + standardChildren = customElements + } + + builder.replaceChildren(ofMenu: .standardEdit) { _ in standardChildren } } func updateSharedMenuController() { + if #available(iOS 16.0, *) { + // On iOS 16+, UIMenuController is deprecated in favor of UIEditMenuInteraction. + // Clear the shared menu items to avoid conflicts between the old and new systems. + // Custom actions are now handled via buildMenu(with:) and UICommand. + UIMenuController.shared.menuItems = nil + return + } + + // iOS 15 and earlier: Use legacy UIMenuController system var items: [UIMenuItem] = [] if isEnabled, let selection = selection { items = actions From 9dfa6e6cbc288c811b141a6ec2e1c27e7374885b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dark=C3=B8=20Tasevski?= Date: Mon, 26 Jan 2026 13:55:41 +0100 Subject: [PATCH 3/5] test(pdf): add unit tests for PDF anchor extraction and resolution --- .../Navigator/PDF/PDFAnchorExtractor.swift | 6 +- Sources/Navigator/PDF/PDFAnchorResolver.swift | 47 ++- Tests/NavigatorTests/PDF/PDFAnchorTests.swift | 367 ++++++++++++++++++ 3 files changed, 401 insertions(+), 19 deletions(-) create mode 100644 Tests/NavigatorTests/PDF/PDFAnchorTests.swift diff --git a/Sources/Navigator/PDF/PDFAnchorExtractor.swift b/Sources/Navigator/PDF/PDFAnchorExtractor.swift index 59fa1cb75..0f6ddc09f 100644 --- a/Sources/Navigator/PDF/PDFAnchorExtractor.swift +++ b/Sources/Navigator/PDF/PDFAnchorExtractor.swift @@ -153,7 +153,8 @@ public struct PDFAnchorExtractor: Loggable { } /// Extracts text context around the given range. - private static func extractContext( + /// - Note: Internal for testing. + static func extractContext( around range: Range, in text: String, contextLength: Int @@ -184,7 +185,8 @@ public struct PDFAnchorExtractor: Loggable { } /// Checks if two bounds are approximately equal within a tolerance. - private static func boundsApproximatelyEqual( + /// - Note: Internal for testing. + static func boundsApproximatelyEqual( _ a: CGRect, _ b: CGRect, tolerance: CGFloat diff --git a/Sources/Navigator/PDF/PDFAnchorResolver.swift b/Sources/Navigator/PDF/PDFAnchorResolver.swift index e789ff26c..a9b953421 100644 --- a/Sources/Navigator/PDF/PDFAnchorResolver.swift +++ b/Sources/Navigator/PDF/PDFAnchorResolver.swift @@ -39,7 +39,7 @@ public struct PDFAnchorResolver: Loggable { } // Strategy 1: Try quads first (pixel-perfect) - if let bounds = resolveFromQuads(anchor) { + if let bounds = resolveFromQuads(anchor.quads) { log(.debug, "Resolved PDF highlight from quads") return bounds } @@ -60,9 +60,11 @@ public struct PDFAnchorResolver: Loggable { return [] } - // MARK: - Private Resolution Methods + // MARK: - Resolution Methods (internal for testing) - private static func parseAnchor(_ data: Any) -> ParsedAnchor? { + /// Parses anchor data from dictionary or JSON string format. + /// - Note: Internal for testing. + static func parseAnchor(_ data: Any) -> ParsedAnchor? { // Handle both dictionary and JSON string formats let dict: [String: Any] if let d = data as? [String: Any] { @@ -90,7 +92,9 @@ public struct PDFAnchorResolver: Loggable { ) } - private static func parseQuads(_ data: Any?) -> [[CGPoint]]? { + /// Parses quad coordinate data. + /// - Note: Internal for testing. + static func parseQuads(_ data: Any?) -> [[CGPoint]]? { guard let quadsArray = data as? [[[String: Double]]] else { return nil } @@ -104,8 +108,10 @@ public struct PDFAnchorResolver: Loggable { } } - private static func resolveFromQuads(_ anchor: ParsedAnchor) -> [CGRect]? { - guard let quads = anchor.quads, !quads.isEmpty else { + /// Resolves bounds from quad coordinates. + /// - Note: Internal for testing. + static func resolveFromQuads(_ quads: [[CGPoint]]?) -> [CGRect]? { + guard let quads = quads, !quads.isEmpty else { return nil } @@ -182,8 +188,8 @@ public struct PDFAnchorResolver: Loggable { // Multiple occurrences: score by context match let bestRange = ranges.max { range1, range2 in - contextScore(for: range1, anchor: anchor, in: pageText) < - contextScore(for: range2, anchor: anchor, in: pageText) + contextScore(for: range1, textBefore: anchor.textBefore, textAfter: anchor.textAfter, in: pageText) < + contextScore(for: range2, textBefore: anchor.textBefore, textAfter: anchor.textAfter, in: pageText) } guard let best = bestRange else { return nil } @@ -196,14 +202,17 @@ public struct PDFAnchorResolver: Loggable { return boundsFromSelection(selection, page: page) } - private static func contextScore( + /// Calculates a score for how well a range matches the expected context. + /// - Note: Internal for testing. + static func contextScore( for range: Range, - anchor: ParsedAnchor, + textBefore: String?, + textAfter: String?, in text: String ) -> Int { var score = 0 - if let before = anchor.textBefore { + if let before = textBefore { let contextLength = before.count let contextStart = text.index( range.lowerBound, @@ -221,7 +230,7 @@ public struct PDFAnchorResolver: Loggable { } } - if let after = anchor.textAfter { + if let after = textAfter { let contextLength = after.count let contextEnd = text.index( range.upperBound, @@ -318,14 +327,16 @@ public struct PDFAnchorResolver: Loggable { return [] } - /// Normalizes whitespace by collapsing multiple spaces/newlines into single spaces - private static func normalizeWhitespace(_ text: String) -> String { + /// Normalizes whitespace by collapsing multiple spaces/newlines into single spaces. + /// - Note: Internal for testing. + static func normalizeWhitespace(_ text: String) -> String { let components = text.components(separatedBy: .whitespacesAndNewlines) return components.filter { !$0.isEmpty }.joined(separator: " ") } - /// Extracts the first N words from text - private static func extractFirstWords(from text: String, count: Int) -> String { + /// Extracts the first N words from text. + /// - Note: Internal for testing. + static func extractFirstWords(from text: String, count: Int) -> String { let words = text.split(separator: " ", omittingEmptySubsequences: true) let firstWords = words.prefix(count) return firstWords.joined(separator: " ") @@ -382,7 +393,9 @@ public struct PDFAnchorResolver: Loggable { // MARK: - Internal Types - private struct ParsedAnchor { + /// Parsed anchor data structure. + /// - Note: Internal for testing. + struct ParsedAnchor { let pageIndex: Int? let quads: [[CGPoint]]? let characterStart: Int? diff --git a/Tests/NavigatorTests/PDF/PDFAnchorTests.swift b/Tests/NavigatorTests/PDF/PDFAnchorTests.swift new file mode 100644 index 000000000..f6f27502b --- /dev/null +++ b/Tests/NavigatorTests/PDF/PDFAnchorTests.swift @@ -0,0 +1,367 @@ +// +// Copyright 2026 Readium Foundation. All rights reserved. +// Use of this source code is governed by the BSD-style license +// available in the top-level LICENSE file of the project. +// + +@testable import ReadiumNavigator +import ReadiumShared +import XCTest + +class PDFAnchorExtractorTests: XCTestCase { + + // MARK: - Context Extraction Tests + + func testExtractContextAroundMiddleOfText() { + let text = "The quick brown fox jumps over the lazy dog." + let range = 16..<19 // "fox" + + let (before, after) = PDFAnchorExtractor.extractContext( + around: range, + in: text, + contextLength: 10 + ) + + XCTAssertEqual(before, "ick brown ") // 10 chars before "fox": indices 6-15 + XCTAssertEqual(after, " jumps ove") // 10 chars after "fox": indices 19-28 + } + + func testExtractContextAtStartOfText() { + let text = "The quick brown fox jumps over the lazy dog." + let range = 0..<3 // "The" + + let (before, after) = PDFAnchorExtractor.extractContext( + around: range, + in: text, + contextLength: 10 + ) + + XCTAssertNil(before) // No text before + XCTAssertEqual(after, " quick bro") + } + + func testExtractContextAtEndOfText() { + let text = "The quick brown fox jumps over the lazy dog." + let range = 40..<44 // "dog." + + let (before, after) = PDFAnchorExtractor.extractContext( + around: range, + in: text, + contextLength: 10 + ) + + XCTAssertEqual(before, " the lazy ") + XCTAssertNil(after) // No text after + } + + func testExtractContextWithShortText() { + let text = "Hello" + let range = 0..<5 + + let (before, after) = PDFAnchorExtractor.extractContext( + around: range, + in: text, + contextLength: 20 + ) + + XCTAssertNil(before) + XCTAssertNil(after) + } + + // MARK: - Bounds Comparison Tests + + func testBoundsApproximatelyEqualWithExactMatch() { + let a = CGRect(x: 10, y: 20, width: 100, height: 50) + let b = CGRect(x: 10, y: 20, width: 100, height: 50) + + XCTAssertTrue(PDFAnchorExtractor.boundsApproximatelyEqual(a, b, tolerance: 1.0)) + } + + func testBoundsApproximatelyEqualWithinTolerance() { + let a = CGRect(x: 10, y: 20, width: 100, height: 50) + let b = CGRect(x: 12, y: 22, width: 102, height: 52) + + XCTAssertTrue(PDFAnchorExtractor.boundsApproximatelyEqual(a, b, tolerance: 5.0)) + } + + func testBoundsApproximatelyEqualOutsideTolerance() { + let a = CGRect(x: 10, y: 20, width: 100, height: 50) + let b = CGRect(x: 20, y: 30, width: 110, height: 60) + + XCTAssertFalse(PDFAnchorExtractor.boundsApproximatelyEqual(a, b, tolerance: 5.0)) + } +} + +class PDFAnchorResolverTests: XCTestCase { + + // MARK: - Anchor Parsing Tests + + func testParseAnchorFromDictionary() { + let data: [String: Any] = [ + "pageIndex": 0, + "text": "Hello World", + "characterStart": 10, + "characterEnd": 21, + "textBefore": "Say ", + "textAfter": " to everyone" + ] + + let anchor = PDFAnchorResolver.parseAnchor(data) + + XCTAssertNotNil(anchor) + XCTAssertEqual(anchor?.pageIndex, 0) + XCTAssertEqual(anchor?.text, "Hello World") + XCTAssertEqual(anchor?.characterStart, 10) + XCTAssertEqual(anchor?.characterEnd, 21) + XCTAssertEqual(anchor?.textBefore, "Say ") + XCTAssertEqual(anchor?.textAfter, " to everyone") + } + + func testParseAnchorFromJSONString() { + let jsonString = """ + {"pageIndex":1,"text":"Test text","characterStart":5,"characterEnd":14} + """ + + let anchor = PDFAnchorResolver.parseAnchor(jsonString) + + XCTAssertNotNil(anchor) + XCTAssertEqual(anchor?.pageIndex, 1) + XCTAssertEqual(anchor?.text, "Test text") + XCTAssertEqual(anchor?.characterStart, 5) + XCTAssertEqual(anchor?.characterEnd, 14) + } + + func testParseAnchorWithQuads() { + let data: [String: Any] = [ + "text": "Highlighted", + "quads": [ + [ + ["x": 10.0, "y": 20.0], + ["x": 110.0, "y": 20.0], + ["x": 110.0, "y": 40.0], + ["x": 10.0, "y": 40.0] + ] + ] + ] + + let anchor = PDFAnchorResolver.parseAnchor(data) + + XCTAssertNotNil(anchor) + XCTAssertNotNil(anchor?.quads) + XCTAssertEqual(anchor?.quads?.count, 1) + XCTAssertEqual(anchor?.quads?.first?.count, 4) + XCTAssertEqual(anchor?.quads?.first?.first, CGPoint(x: 10, y: 20)) + } + + func testParseAnchorWithMissingTextReturnsNil() { + let data: [String: Any] = [ + "pageIndex": 0, + "characterStart": 10 + ] + + let anchor = PDFAnchorResolver.parseAnchor(data) + + XCTAssertNil(anchor) + } + + func testParseAnchorWithInvalidDataReturnsNil() { + let anchor = PDFAnchorResolver.parseAnchor(12345) + XCTAssertNil(anchor) + } + + // MARK: - Quads Parsing Tests + + func testParseQuadsWithValidData() { + let quadsData: [[[String: Double]]] = [ + [ + ["x": 0.0, "y": 0.0], + ["x": 100.0, "y": 0.0], + ["x": 100.0, "y": 20.0], + ["x": 0.0, "y": 20.0] + ], + [ + ["x": 0.0, "y": 25.0], + ["x": 80.0, "y": 25.0], + ["x": 80.0, "y": 45.0], + ["x": 0.0, "y": 45.0] + ] + ] + + let quads = PDFAnchorResolver.parseQuads(quadsData) + + XCTAssertNotNil(quads) + XCTAssertEqual(quads?.count, 2) + XCTAssertEqual(quads?[0].count, 4) + XCTAssertEqual(quads?[1].count, 4) + } + + func testParseQuadsWithNilReturnsNil() { + let quads = PDFAnchorResolver.parseQuads(nil) + XCTAssertNil(quads) + } + + func testParseQuadsWithInvalidFormatReturnsNil() { + let quads = PDFAnchorResolver.parseQuads("invalid") + XCTAssertNil(quads) + } + + // MARK: - Quads to Bounds Resolution Tests + + func testResolveFromQuads() { + let quads: [[CGPoint]] = [ + [ + CGPoint(x: 10, y: 20), + CGPoint(x: 110, y: 20), + CGPoint(x: 110, y: 40), + CGPoint(x: 10, y: 40) + ] + ] + + let bounds = PDFAnchorResolver.resolveFromQuads(quads) + + XCTAssertNotNil(bounds) + XCTAssertEqual(bounds?.count, 1) + XCTAssertEqual(bounds?.first, CGRect(x: 10, y: 20, width: 100, height: 20)) + } + + func testResolveFromQuadsWithMultipleLines() { + let quads: [[CGPoint]] = [ + [ + CGPoint(x: 10, y: 100), + CGPoint(x: 200, y: 100), + CGPoint(x: 200, y: 120), + CGPoint(x: 10, y: 120) + ], + [ + CGPoint(x: 10, y: 75), + CGPoint(x: 150, y: 75), + CGPoint(x: 150, y: 95), + CGPoint(x: 10, y: 95) + ] + ] + + let bounds = PDFAnchorResolver.resolveFromQuads(quads) + + XCTAssertNotNil(bounds) + XCTAssertEqual(bounds?.count, 2) + XCTAssertEqual(bounds?[0], CGRect(x: 10, y: 100, width: 190, height: 20)) + XCTAssertEqual(bounds?[1], CGRect(x: 10, y: 75, width: 140, height: 20)) + } + + func testResolveFromQuadsWithEmptyArrayReturnsNil() { + let bounds = PDFAnchorResolver.resolveFromQuads([]) + XCTAssertNil(bounds) + } + + // MARK: - Context Score Tests + + func testContextScoreWithExactMatch() { + let text = "prefix Hello World suffix" + let range = text.range(of: "Hello World")! + + let score = PDFAnchorResolver.contextScore( + for: range, + textBefore: "prefix ", + textAfter: " suffix", + in: text + ) + + XCTAssertEqual(score, 40) // 20 for exact before + 20 for exact after + } + + func testContextScoreWithPartialMatch() { + // In this text, the actual context is "prefix " before and " suffix" after + // which exactly matches the expected context, so it scores 40 + let text = "Some prefix Hello World suffix here" + let range = text.range(of: "Hello World")! + + let score = PDFAnchorResolver.contextScore( + for: range, + textBefore: "prefix ", + textAfter: " suffix", + in: text + ) + + XCTAssertEqual(score, 40) // 20 for exact before + 20 for exact after + } + + func testContextScoreWithNoMatch() { + let text = "Different Hello World content" + let range = text.range(of: "Hello World")! + + let score = PDFAnchorResolver.contextScore( + for: range, + textBefore: "nomatch ", + textAfter: " nomatch", + in: text + ) + + XCTAssertEqual(score, 0) + } + + func testContextScoreWithNilContext() { + let text = "Hello World" + let range = text.range(of: "Hello World")! + + let score = PDFAnchorResolver.contextScore( + for: range, + textBefore: nil, + textAfter: nil, + in: text + ) + + XCTAssertEqual(score, 0) + } + + // MARK: - Whitespace Normalization Tests + + func testNormalizeWhitespaceCollapsesSpaces() { + let text = "Hello World" + let normalized = PDFAnchorResolver.normalizeWhitespace(text) + XCTAssertEqual(normalized, "Hello World") + } + + func testNormalizeWhitespaceConvertsNewlines() { + let text = "Hello\n\nWorld" + let normalized = PDFAnchorResolver.normalizeWhitespace(text) + XCTAssertEqual(normalized, "Hello World") + } + + func testNormalizeWhitespaceHandlesMixedWhitespace() { + let text = "Hello \t\n World\n\n\tTest" + let normalized = PDFAnchorResolver.normalizeWhitespace(text) + XCTAssertEqual(normalized, "Hello World Test") + } + + func testNormalizeWhitespaceTrimsEdges() { + let text = " Hello World " + let normalized = PDFAnchorResolver.normalizeWhitespace(text) + XCTAssertEqual(normalized, "Hello World") + } + + // MARK: - First Words Extraction Tests + + func testExtractFirstWordsWithEnoughWords() { + let text = "The quick brown fox jumps over the lazy dog" + let firstWords = PDFAnchorResolver.extractFirstWords(from: text, count: 5) + XCTAssertEqual(firstWords, "The quick brown fox jumps") + } + + func testExtractFirstWordsWithFewerWords() { + let text = "Hello World" + let firstWords = PDFAnchorResolver.extractFirstWords(from: text, count: 5) + XCTAssertEqual(firstWords, "Hello World") + } + + func testExtractFirstWordsWithEmptyString() { + let text = "" + let firstWords = PDFAnchorResolver.extractFirstWords(from: text, count: 5) + XCTAssertEqual(firstWords, "") + } + + func testExtractFirstWordsHandlesExtraSpaces() { + let text = " The quick brown " + let firstWords = PDFAnchorResolver.extractFirstWords(from: text, count: 2) + XCTAssertEqual(firstWords, "The quick") + } +} From 11ed9c5b17f317ab811d06bcfaed58976af4cb8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dark=C3=B8=20Tasevski?= Date: Tue, 27 Jan 2026 00:19:37 +0100 Subject: [PATCH 4/5] fix(pdf): address Copilot review feedback - 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 --- Sources/Navigator/PDF/PDFAnchorResolver.swift | 18 +++++- Sources/Navigator/PDF/PDFDocumentView.swift | 4 +- .../PDF/PDFNavigatorViewController.swift | 63 ++++++++++++------- 3 files changed, 59 insertions(+), 26 deletions(-) diff --git a/Sources/Navigator/PDF/PDFAnchorResolver.swift b/Sources/Navigator/PDF/PDFAnchorResolver.swift index a9b953421..2c5c321d1 100644 --- a/Sources/Navigator/PDF/PDFAnchorResolver.swift +++ b/Sources/Navigator/PDF/PDFAnchorResolver.swift @@ -101,10 +101,15 @@ public struct PDFAnchorResolver: Loggable { return quadsArray.compactMap { quad -> [CGPoint]? in guard quad.count == 4 else { return nil } - return quad.compactMap { point -> CGPoint? in + + let points = quad.compactMap { point -> CGPoint? in guard let x = point["x"], let y = point["y"] else { return nil } return CGPoint(x: x, y: y) } + + // Require exactly 4 valid points + guard points.count == 4 else { return nil } + return points } } @@ -115,7 +120,7 @@ public struct PDFAnchorResolver: Loggable { return nil } - return quads.compactMap { quad -> CGRect? in + let bounds = quads.compactMap { quad -> CGRect? in guard quad.count == 4 else { return nil } // Convert quad points to bounding rect @@ -128,6 +133,10 @@ public struct PDFAnchorResolver: Loggable { guard !rect.isEmpty else { return nil } return rect } + + // Return nil if no valid bounds were produced + guard !bounds.isEmpty else { return nil } + return bounds } private static func resolveFromCharacterRange( @@ -348,6 +357,11 @@ public struct PDFAnchorResolver: Loggable { normalizedPrefix: String, matchLength: Int ) -> Range? { + // Early return for degenerate cases + guard matchLength > 0, !originalText.isEmpty else { + return nil + } + // Walk through original text, tracking position in normalized space var normalizedPosition = 0 var originalStart: String.Index? diff --git a/Sources/Navigator/PDF/PDFDocumentView.swift b/Sources/Navigator/PDF/PDFDocumentView.swift index f9685aa0d..5c8e90277 100644 --- a/Sources/Navigator/PDF/PDFDocumentView.swift +++ b/Sources/Navigator/PDF/PDFDocumentView.swift @@ -382,6 +382,8 @@ public final class PDFDocumentView: PDFView { responder = currentResponder.next } - return nil + // 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) } } diff --git a/Sources/Navigator/PDF/PDFNavigatorViewController.swift b/Sources/Navigator/PDF/PDFNavigatorViewController.swift index 5eea7023d..33f726aa7 100644 --- a/Sources/Navigator/PDF/PDFNavigatorViewController.swift +++ b/Sources/Navigator/PDF/PDFNavigatorViewController.swift @@ -874,9 +874,9 @@ extension PDFNavigatorViewController: DecorableNavigator { set { objc_setAssociatedObject(self, &decorationsKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } } - /// Storage for PDF annotations mapped by decoration ID - private var annotationsByDecorationId: [Decoration.Id: [PDFKit.PDFAnnotation]] { - get { objc_getAssociatedObject(self, &annotationsKey) as? [Decoration.Id: [PDFKit.PDFAnnotation]] ?? [:] } + /// Storage for PDF annotations mapped by (group, decoration ID) composite key + private var annotationsByKey: [DecorationKey: [PDFKit.PDFAnnotation]] { + get { objc_getAssociatedObject(self, &annotationsKey) as? [DecorationKey: [PDFKit.PDFAnnotation]] ?? [:] } set { objc_setAssociatedObject(self, &annotationsKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } } @@ -895,7 +895,7 @@ extension PDFNavigatorViewController: DecorableNavigator { // Thread safety: Associated object storage uses OBJC_ASSOCIATION_RETAIN_NONATOMIC, // which is not thread-safe. PDFKit also requires main thread access. // TODO: Consider adding @MainActor isolation when migrating to Swift 6. - dispatchPrecondition(condition: .onQueue(.main)) + assert(Thread.isMainThread, "apply(decorations:in:) must be called on main thread") log(.debug, "PDF DecorableNavigator.apply called: \(newDecorations.count) decorations in group '\(group)'") @@ -922,12 +922,12 @@ extension PDFNavigatorViewController: DecorableNavigator { for change in changeList { switch change { case .add(let decoration): - addAnnotation(for: decoration, in: document) + addAnnotation(for: decoration, in: document, group: group) case .remove(let id): - removeAnnotation(withId: id, from: document) + removeAnnotation(withId: id, from: document, group: group) case .update(let decoration): - removeAnnotation(withId: decoration.id, from: document) - addAnnotation(for: decoration, in: document) + removeAnnotation(withId: decoration.id, from: document, group: group) + addAnnotation(for: decoration, in: document, group: group) } } } @@ -984,32 +984,40 @@ extension PDFNavigatorViewController: DecorableNavigator { decorationCallbacks[group] = callbacks return AnyCancellable { [weak self] in - // Must be called on main thread to safely modify decorationCallbacks - dispatchPrecondition(condition: .onQueue(.main)) - guard let self = self else { return } - var callbacks = self.decorationCallbacks[group] ?? [] - callbacks.removeAll { $0.token == token } - self.decorationCallbacks[group] = callbacks.isEmpty ? nil : callbacks + guard let self else { return } + + if Thread.isMainThread { + var callbacks = self.decorationCallbacks[group] ?? [] + callbacks.removeAll { $0.token == token } + self.decorationCallbacks[group] = callbacks.isEmpty ? nil : callbacks + } else { + DispatchQueue.main.async { [weak self] in + guard let self else { return } + var callbacks = self.decorationCallbacks[group] ?? [] + callbacks.removeAll { $0.token == token } + self.decorationCallbacks[group] = callbacks.isEmpty ? nil : callbacks + } + } } } // MARK: - Private Helpers - private func addAnnotation(for decoration: Decoration, in document: PDFKit.PDFDocument) { - log(.debug, "addAnnotation: id=\(decoration.id), position=\(decoration.locator.locations.position ?? -1), text=\(decoration.locator.text.highlight?.prefix(30) ?? "nil")") + private func addAnnotation(for decoration: Decoration, in document: PDFKit.PDFDocument, group: String) { + log(.debug, "addAnnotation: group='\(group)', id=\(decoration.id), position=\(decoration.locator.locations.position ?? -1), text=\(decoration.locator.text.highlight?.prefix(30) ?? "nil")") guard let page = findPage(for: decoration.locator, in: document) else { - log(.warning, "Could not find page for decoration \(decoration.id) - position: \(decoration.locator.locations.position ?? -1)") + log(.warning, "Could not find page for decoration \(decoration.id) in group '\(group)' - position: \(decoration.locator.locations.position ?? -1)") return } let boundsArray = self.boundsForLines(for: decoration.locator, on: page) guard !boundsArray.isEmpty else { - log(.warning, "Could not find bounds for decoration \(decoration.id) - text: '\(decoration.locator.text.highlight?.prefix(50) ?? "nil")'") + log(.warning, "Could not find bounds for decoration \(decoration.id) in group '\(group)' - text: '\(decoration.locator.text.highlight?.prefix(50) ?? "nil")'") return } - log(.debug, "Creating \(boundsArray.count) PDF annotations for TTS highlight") + log(.debug, "Creating \(boundsArray.count) PDF annotations for decoration \(decoration.id) in group '\(group)'") var createdAnnotations: [PDFKit.PDFAnnotation] = [] for bounds in boundsArray { let annotation = createAnnotation(for: decoration.style, bounds: bounds, decorationId: decoration.id) @@ -1017,11 +1025,13 @@ extension PDFNavigatorViewController: DecorableNavigator { createdAnnotations.append(annotation) } - annotationsByDecorationId[decoration.id] = createdAnnotations + let key = DecorationKey(group: group, id: decoration.id) + annotationsByKey[key] = createdAnnotations } - private func removeAnnotation(withId id: Decoration.Id, from document: PDFKit.PDFDocument) { - guard let annotations = annotationsByDecorationId[id] else { + private func removeAnnotation(withId id: Decoration.Id, from document: PDFKit.PDFDocument, group: String) { + let key = DecorationKey(group: group, id: id) + guard let annotations = annotationsByKey[key] else { return } @@ -1030,7 +1040,7 @@ extension PDFNavigatorViewController: DecorableNavigator { page.removeAnnotation(annotation) } - annotationsByDecorationId[id] = nil + annotationsByKey[key] = nil } private func createAnnotation(for style: Decoration.Style, bounds: CGRect, decorationId: Decoration.Id) -> PDFKit.PDFAnnotation { @@ -1105,6 +1115,13 @@ extension PDFNavigatorViewController: DecorableNavigator { } } +/// Composite key for annotation storage to handle decoration IDs that may +/// be duplicated across different groups. +private struct DecorationKey: Hashable { + let group: String + let id: Decoration.Id +} + // Associated object keys for decoration storage private var decorationsKey: UInt8 = 0 private var annotationsKey: UInt8 = 0 From 61bbf7978c6f826ed8fb27cc2481d07e0f0fe988 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dark=C3=B8=20Tasevski?= Date: Tue, 27 Jan 2026 09:54:56 +0100 Subject: [PATCH 5/5] fix(testapp): add missing highlightSelection method --- .../Sources/Reader/PDF/PDFViewController.swift | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/TestApp/Sources/Reader/PDF/PDFViewController.swift b/TestApp/Sources/Reader/PDF/PDFViewController.swift index ee27e8a2d..b0f237fa8 100644 --- a/TestApp/Sources/Reader/PDF/PDFViewController.swift +++ b/TestApp/Sources/Reader/PDF/PDFViewController.swift @@ -50,6 +50,21 @@ final class PDFViewController: VisualReaderViewController