diff --git a/Docs/editor-investigation.md b/Docs/editor-investigation.md index 8d3004c..2990438 100644 --- a/Docs/editor-investigation.md +++ b/Docs/editor-investigation.md @@ -1559,6 +1559,7 @@ Root cause: - Source code blocks reveal the whole fenced block. - `applyHybridAttributes` restored the previous `NSClipView.bounds.origin`, but the edited glyph could move relative to that origin after attributes changed. - Programmatic selection also called `scrollRangeToVisible` even when the selection was already visible, which could pre-scroll before the presentation transition ran. +- The first implementation applied anchoring too broadly from `applyHybridAttributes`, so ordinary selection changes and keyboard navigation could also receive custom scroll correction after AppKit had already handled caret visibility. Corrected stabilization model: @@ -1572,11 +1573,44 @@ flowchart LR The anchor is source-based, not element-specific. `ViewportPresentationAnchor` captures a character location and its viewport-relative Y position before a presentation transition. After styling, the coordinator asks TextKit for the same source location's new glyph position and scrolls the clip view by the delta. This applies equally to headings, task lists, code blocks, tables, and future rendered widgets because the mechanism does not inspect the rendered element type during restoration. +Anchoring policy: + +```mermaid +flowchart TD + Event["Editor event"] --> Keyboard{"Keyboard navigation?"} + Keyboard -- Yes --> Native["Let NSTextView own scrolling"] + Keyboard -- No --> Selection{"Programmatic selection?"} + Selection -- Yes --> Native + Selection -- No --> Transition{"Editable region changed?"} + Transition -- No --> Native + Transition -- Yes --> Presentation{"Mouse or focus presentation transition?"} + Presentation -- Yes --> Anchor["Capture and restore presentation anchor"] + Presentation -- No --> Native +``` + +Viewport stabilization now runs only when all of these are true: + +- A styling pass is required. +- The dirty reason is an active/editable-region transition rather than source text editing or initial render. +- The editable region actually changed. +- The selection is collapsed; range selections stay native. +- The trigger is mouse presentation entry or focus-driven presentation exit. + +Viewport stabilization explicitly does not run for: + +- Arrow-key navigation. +- Page up/down and other key-driven selection movement. +- Mouse drag or range selection. +- Programmatic selection updates. +- Source edits. +- View updates that do not change the editable region. + Anchor selection: - The selected source location is the default anchor. - If selection is on hidden or structural syntax for headings, lists, blockquotes, or tasks, the anchor moves to the visible content range for that line. - Programmatic selection changes only scroll when the target point is outside the visible viewport; visible targets preserve the existing origin. +- `ViewportStabilityEvent` records the event cause, render reason, stabilization decision, whether an anchor was captured, and whether a scroll restoration ran. This is intentionally test-facing instrumentation so regressions show which system tried to own scroll behavior. Geometry audit: @@ -1591,6 +1625,7 @@ Validation: - `testHeadingTransitionKeepsEditedContentVisuallyAnchored` reproduced the heading jump and now verifies the heading text keeps the same viewport-relative Y position when entering and leaving source mode. - `testCodeBlockTransitionKeepsEditedContentVisuallyAnchored` verifies the same contract for entering and leaving a rendered code block. +- `testKeyboardNavigationDoesNotRunViewportStabilization` verifies arrow-key-style selection movement is logged as native keyboard navigation and does not run custom scroll restoration. - The tests use `HybridMarkdownLiveEditorHarness`, so they exercise the same `NSTextView`, coordinator, TextKit layout, and scroll-view path as the app. Performance impact: diff --git a/Sources/SaplingEditor/HybridMarkdownEditor.swift b/Sources/SaplingEditor/HybridMarkdownEditor.swift index 7e72321..c5db35a 100644 --- a/Sources/SaplingEditor/HybridMarkdownEditor.swift +++ b/Sources/SaplingEditor/HybridMarkdownEditor.swift @@ -206,10 +206,10 @@ private struct NativeMarkdownTextView: NSViewRepresentable { textView.string = text textView.delegate = context.coordinator textView.onFocusStateChange = { [weak coordinator = context.coordinator] textView in - coordinator?.applyHybridAttributes(to: textView) + coordinator?.applyHybridAttributes(to: textView, cause: .focusChange) } - textView.onUserEditingInteraction = { [weak coordinator = context.coordinator] textView in - coordinator?.activateEditingPresentation(in: textView) + textView.onUserEditingInteraction = { [weak coordinator = context.coordinator] textView, interactionKind in + coordinator?.activateEditingPresentation(in: textView, interactionKind: interactionKind) } textView.isRichText = false textView.isEditable = true @@ -255,7 +255,7 @@ private struct NativeMarkdownTextView: NSViewRepresentable { let selectedRange = selection.range if textView.selectedRange() != selectedRange, selectedRange.location <= textView.string.utf16.count { - context.coordinator.setSelection(selectedRange, in: textView) + context.coordinator.setSelection(selectedRange, in: textView, interactionKind: .programmatic) } context.coordinator.applyHybridAttributes(to: textView) @@ -271,6 +271,9 @@ private struct NativeMarkdownTextView: NSViewRepresentable { private var pendingEdit: DocumentLineIndexEdit? private var hasUserActivatedEditing = false private var checklistButtons: [Int: ChecklistOverlayButton] = [:] + fileprivate var viewportStabilityEvents: [ViewportStabilityEvent] = [] + private var lastUserInteractionKind: EditorInteractionKind = .programmatic + private var pendingSelectionInteractionKind: EditorInteractionKind? init(_ parent: NativeMarkdownTextView) { self.parent = parent @@ -303,7 +306,7 @@ private struct NativeMarkdownTextView: NSViewRepresentable { } parent.onTextEdit(textView.string, edit, selection) parent.selection = selection - applyHybridAttributes(to: textView) + applyHybridAttributes(to: textView, cause: .sourceChange) pendingEdit = nil } @@ -312,11 +315,16 @@ private struct NativeMarkdownTextView: NSViewRepresentable { guard let textView = notification.object as? NSTextView else { return } let newSelection = EditorSelection(range: textView.selectedRange()) guard parent.selection != newSelection else { return } - applyHybridAttributes(to: textView) + let interactionKind = pendingSelectionInteractionKind ?? lastUserInteractionKind + pendingSelectionInteractionKind = nil + applyHybridAttributes(to: textView, cause: .selectionChange(interactionKind)) parent.selection = newSelection } - func applyHybridAttributes(to textView: NSTextView) { + func applyHybridAttributes( + to textView: NSTextView, + cause: PresentationUpdateCause = .viewUpdate + ) { guard let textStorage = textView.textStorage else { return } let editableRegion = presentationEditableRegion(in: textView) let activeLineIndex = editableRegion.primaryLineIndex @@ -324,11 +332,19 @@ private struct NativeMarkdownTextView: NSViewRepresentable { guard invalidationPlan.requiresStyling else { return } let selectedRange = textView.selectedRange() - let viewportAnchor = ViewportPresentationAnchor.capture( - in: textView, - selectedRange: selectedRange, - lineIndex: currentLineIndex + let stabilizationDecision = viewportStabilizationDecision( + cause: cause, + invalidationPlan: invalidationPlan, + editableRegion: editableRegion, + selectedRange: selectedRange ) + let viewportAnchor = stabilizationDecision.shouldStabilize + ? ViewportPresentationAnchor.capture( + in: textView, + selectedRange: selectedRange, + lineIndex: currentLineIndex + ) + : nil let start = Date() var stylingResult = MarkdownTextStylingResult.empty var didRestoreVisibleOrigin = false @@ -351,11 +367,20 @@ private struct NativeMarkdownTextView: NSViewRepresentable { textView.setSelectedRange(selectedRange) } } - didRestoreVisibleOrigin = restoreViewportAnchor(viewportAnchor, in: textView) + if stabilizationDecision.shouldStabilize { + didRestoreVisibleOrigin = restoreViewportAnchor(viewportAnchor, in: textView) + } lastStyledText = textView.string lastStyledActiveLineIndex = activeLineIndex lastStyledEditableRegion = editableRegion + recordViewportStabilityEvent( + cause: cause, + invalidationPlan: invalidationPlan, + decision: stabilizationDecision, + capturedAnchor: viewportAnchor != nil, + restored: didRestoreVisibleOrigin + ) syncChecklistControls( in: textView, stylingResult: stylingResult, @@ -382,17 +407,26 @@ private struct NativeMarkdownTextView: NSViewRepresentable { return EditableRegion.selection(textView.selectedRange(), in: currentLineIndex) } - func activateEditingPresentation(in textView: NSTextView) { + func activateEditingPresentation( + in textView: NSTextView, + interactionKind: EditorInteractionKind = .programmatic + ) { + lastUserInteractionKind = interactionKind guard !hasUserActivatedEditing else { return } hasUserActivatedEditing = true - applyHybridAttributes(to: textView) + applyHybridAttributes(to: textView, cause: .editingActivation(interactionKind)) } - func setSelection(_ range: NSRange, in textView: NSTextView) { + func setSelection( + _ range: NSRange, + in textView: NSTextView, + interactionKind: EditorInteractionKind = .programmatic + ) { guard textView.selectedRange() != range else { return } performProgrammaticUpdate { let visibleOrigin = textView.enclosingScrollView?.contentView.bounds.origin let shouldScroll = selectionNeedsScroll(range, in: textView) + pendingSelectionInteractionKind = interactionKind textView.setSelectedRange(range) if shouldScroll { textView.scrollRangeToVisible(range) @@ -447,6 +481,55 @@ private struct NativeMarkdownTextView: NSViewRepresentable { ) } + private func viewportStabilizationDecision( + cause: PresentationUpdateCause, + invalidationPlan: EditorDirtyLineInvalidationPlan, + editableRegion: EditableRegion, + selectedRange: NSRange + ) -> ViewportStabilizationDecision { + guard invalidationPlan.reason == .activeLineChange else { + return ViewportStabilizationDecision(shouldStabilize: false, reason: "not-active-line-change") + } + + guard lastStyledEditableRegion != editableRegion else { + return ViewportStabilizationDecision(shouldStabilize: false, reason: "no-editable-region-transition") + } + + guard selectedRange.length == 0 else { + return ViewportStabilizationDecision(shouldStabilize: false, reason: "range-selection") + } + + switch cause { + case .selectionChange(.keyboard), .editingActivation(.keyboard): + return ViewportStabilizationDecision(shouldStabilize: false, reason: "native-keyboard-navigation") + case .selectionChange(.programmatic), .editingActivation(.programmatic): + return ViewportStabilizationDecision(shouldStabilize: false, reason: "programmatic-selection") + case .selectionChange(.mouse), .editingActivation(.mouse), .focusChange: + return ViewportStabilizationDecision(shouldStabilize: true, reason: "presentation-transition") + case .selectionChange(.paste), .editingActivation(.paste), .sourceChange, .viewUpdate: + return ViewportStabilizationDecision(shouldStabilize: false, reason: "non-presentation-navigation") + } + } + + private func recordViewportStabilityEvent( + cause: PresentationUpdateCause, + invalidationPlan: EditorDirtyLineInvalidationPlan, + decision: ViewportStabilizationDecision, + capturedAnchor: Bool, + restored: Bool + ) { + viewportStabilityEvents.append(ViewportStabilityEvent( + cause: cause.description, + renderReason: "\(invalidationPlan.reason)", + decision: decision.reason, + capturedAnchor: capturedAnchor, + restored: restored + )) + if viewportStabilityEvents.count > 200 { + viewportStabilityEvents.removeFirst(viewportStabilityEvents.count - 200) + } + } + private func syncChecklistControls( in textView: NSTextView, stylingResult: MarkdownTextStylingResult, @@ -630,6 +713,49 @@ private struct NativeMarkdownTextView: NSViewRepresentable { } } +private enum EditorInteractionKind: String { + case mouse + case keyboard + case paste + case programmatic +} + +private enum PresentationUpdateCause: CustomStringConvertible { + case editingActivation(EditorInteractionKind) + case selectionChange(EditorInteractionKind) + case focusChange + case sourceChange + case viewUpdate + + var description: String { + switch self { + case .editingActivation(let kind): + return "editingActivation:\(kind.rawValue)" + case .selectionChange(let kind): + return "selectionChange:\(kind.rawValue)" + case .focusChange: + return "focusChange" + case .sourceChange: + return "sourceChange" + case .viewUpdate: + return "viewUpdate" + } + } +} + +private struct ViewportStabilizationDecision { + var shouldStabilize: Bool + var reason: String +} + +private struct ViewportStabilityEvent: Hashable, Sendable { + var cause: String + var renderReason: String + var decision: String + var capturedAnchor: Bool + var restored: Bool +} + private struct CodeBlockContainerPresentation: Equatable { var codeBlock: RenderedCodeBlockElement var languageLabel: String @@ -693,7 +819,7 @@ private struct ViewportPresentationAnchor { private final class EditorTextView: NSTextView { var onFocusStateChange: ((NSTextView) -> Void)? - var onUserEditingInteraction: ((NSTextView) -> Void)? + var onUserEditingInteraction: ((NSTextView, EditorInteractionKind) -> Void)? var codeBlockContainers: [CodeBlockContainerPresentation] = [] { didSet { guard oldValue != codeBlockContainers else { return } @@ -722,17 +848,17 @@ private final class EditorTextView: NSTextView { } override func mouseDown(with event: NSEvent) { - onUserEditingInteraction?(self) + onUserEditingInteraction?(self, .mouse) super.mouseDown(with: event) } override func keyDown(with event: NSEvent) { - onUserEditingInteraction?(self) + onUserEditingInteraction?(self, .keyboard) super.keyDown(with: event) } override func paste(_ sender: Any?) { - onUserEditingInteraction?(self) + onUserEditingInteraction?(self, .paste) super.paste(sender) } @@ -987,10 +1113,10 @@ public final class HybridMarkdownLiveEditorHarness { self.textView.isVerticallyResizable = true self.textView.isHorizontallyResizable = false self.textView.onFocusStateChange = { [weak coordinator] textView in - coordinator?.applyHybridAttributes(to: textView) + coordinator?.applyHybridAttributes(to: textView, cause: .focusChange) } - self.textView.onUserEditingInteraction = { [weak coordinator] textView in - coordinator?.activateEditingPresentation(in: textView) + self.textView.onUserEditingInteraction = { [weak coordinator] textView, interactionKind in + coordinator?.activateEditingPresentation(in: textView, interactionKind: interactionKind) } self.textView.string = source self.textView.delegate = coordinator @@ -1041,8 +1167,20 @@ public final class HybridMarkdownLiveEditorHarness { } public func setSelection(_ range: NSRange) { - coordinator.activateEditingPresentation(in: textView) - coordinator.setSelection(range, in: textView) + setSelection(range, interactionKind: .programmatic) + } + + public func setSelectionByKeyboard(_ range: NSRange) { + setSelection(range, interactionKind: .keyboard) + } + + public func setSelectionByMouse(_ range: NSRange) { + setSelection(range, interactionKind: .mouse) + } + + private func setSelection(_ range: NSRange, interactionKind: EditorInteractionKind) { + coordinator.activateEditingPresentation(in: textView, interactionKind: interactionKind) + coordinator.setSelection(range, in: textView, interactionKind: interactionKind) coordinator.textViewDidChangeSelection(Notification(name: NSTextView.didChangeSelectionNotification, object: textView)) syncState() } @@ -1109,6 +1247,18 @@ public final class HybridMarkdownLiveEditorHarness { return CGPoint(x: point.x - origin.x, y: point.y - origin.y) } + public func viewportStabilityEventDescriptions() -> [String] { + coordinator.viewportStabilityEvents.map { + [ + "cause=\($0.cause)", + "reason=\($0.renderReason)", + "decision=\($0.decision)", + "captured=\($0.capturedAnchor)", + "restored=\($0.restored)" + ].joined(separator: "|") + } + } + public func presentationSignature() -> String { guard let storage = textView.textStorage else { return "" } return MarkdownPresentationSnapshot.make( diff --git a/Tests/SaplingEditorTests/HybridMarkdownLiveEditorHarnessTests.swift b/Tests/SaplingEditorTests/HybridMarkdownLiveEditorHarnessTests.swift index ca8f9fc..685e433 100644 --- a/Tests/SaplingEditorTests/HybridMarkdownLiveEditorHarnessTests.swift +++ b/Tests/SaplingEditorTests/HybridMarkdownLiveEditorHarnessTests.swift @@ -186,7 +186,7 @@ final class HybridMarkdownLiveEditorHarnessTests: XCTestCase { harness.scrollViewport(toY: headingPoint.y - 160) let renderedViewportPoint = try XCTUnwrap(harness.viewportPoint(for: "Navigation")) - harness.setSelection(NSRange(location: nsSource.range(of: "Navigation").location, length: 0)) + harness.setSelectionByMouse(NSRange(location: nsSource.range(of: "Navigation").location, length: 0)) let sourceViewportPoint = try XCTUnwrap(harness.viewportPoint(for: "Navigation")) XCTAssertEqual(sourceViewportPoint.y, renderedViewportPoint.y, accuracy: 0.5) @@ -216,7 +216,7 @@ final class HybridMarkdownLiveEditorHarnessTests: XCTestCase { harness.scrollViewport(toY: codePoint.y - 180) let renderedViewportPoint = try XCTUnwrap(harness.viewportPoint(for: "value")) - harness.setSelection(NSRange(location: nsSource.range(of: "value").location, length: 0)) + harness.setSelectionByMouse(NSRange(location: nsSource.range(of: "value").location, length: 0)) let sourceViewportPoint = try XCTUnwrap(harness.viewportPoint(for: "value")) XCTAssertEqual(sourceViewportPoint.y, renderedViewportPoint.y, accuracy: 0.5) @@ -226,6 +226,30 @@ final class HybridMarkdownLiveEditorHarnessTests: XCTestCase { let restoredViewportPoint = try XCTUnwrap(harness.viewportPoint(for: "value")) XCTAssertEqual(restoredViewportPoint.y, renderedViewportPoint.y, accuracy: 0.5) } + + func testKeyboardNavigationDoesNotRunViewportStabilization() throws { + let intro = (0..<24).map { "Intro paragraph \($0)" }.joined(separator: "\n") + let outro = (0..<24).map { "Outro paragraph \($0)" }.joined(separator: "\n") + let source = """ + \(intro) + # Navigation Checklist + Body text below the heading. + \(outro) + """ + let nsSource = source as NSString + let harness = HybridMarkdownLiveEditorHarness(source: source, initialWidth: 700) + harness.simulateLaunchFirstResponder() + let headingPoint = try XCTUnwrap(harness.point(for: "Navigation")) + harness.scrollViewport(toY: headingPoint.y - 160) + + harness.setSelectionByKeyboard(NSRange(location: nsSource.range(of: "Navigation").location, length: 0)) + + XCTAssertTrue(harness.viewportStabilityEventDescriptions().contains { + $0.contains("cause=selectionChange:keyboard") + && $0.contains("decision=native-keyboard-navigation") + && $0.contains("restored=false") + }) + } } #endif