Re-fix of reopened T-1099: keyboard and View-menu scroll commands were scrolling the document behind coordinator-owned modals because the T-1542 WebKit cutover dropped the binding that mirrored coordinatorOwnsModalPresentation into KeyboardScrollController.suspended. This branch restores the binding in both layout hosts and pins the wiring with a source-structural regression test.
.onChange(of: coordinator.coordinatorOwnsModalPresentation, initial: true) in CompactDocumentLayout and RegularDocumentLayout, each suspending both renderedScroll and rawSourceScroll together.suspended gates were fully unit-tested for three months while nothing in production ever set the flag — same shape as T-1943's dead crash-recovery.ModalPresentationSuspendsScrollHostTests string-matches both layout sources (the FootnotePresentationHostTests/T-1893 pattern — no view-hosting harness exists in this project).DocumentScrollContent owns neither controller nor focus state; the two new blocks are the only production suspended assignments; DocumentActions routes View-menu commands to the coordinator-owned controllers; no ungated JS key handling exists in the web renderer.make test-quick could not run (machine at load average 94 during review; earlier attempts hit the known “test runner hung before establishing connection” failure). Targeted classes KeyboardScrollControllerTests, DocumentLayoutCoordinatorModalPresentationTests, ModalPresentationSuspendsScrollHostTests, FootnotePresentationHostTests pass; make lint: 0 violations in 524 files. GitHub Actions is billing-blocked; the green CI on PR #353 predates the block.suspended with a coordinator-injected isSuspended closure (the WebCommands pattern already in the same class), which would survive future view restructuring by construction and turn the structural test into a behavioral one.Ready to push
The fix is correct, minimal, and verified: every scroll entry point (handleKey, all six scroll methods) guards on suspended, the two new .onChange(initial: true) bindings are the only production assignments to it, both sit on always-mounted modifier chains, and the structural test pins the wiring in both layouts. Targeted test classes and lint pass. One major-severity architectural finding was raised — the view-mediated state mirroring re-installs the exact shape that regressed in the cutover, and a coordinator-injected closure would be sturdier — but it is explicitly a follow-up, not a defect: the shipped fix works and is now regression-pinned. Nothing must change before pushing.
fc213df Fix T-1099: Document scroll keys still route behind modal presentations Prism lets you scroll a document with the keyboard (arrows, Page Up/Down, Space) and with View-menu commands. When a small panel is open on top of the document — a note, a footnote, the add-note sheet — those scroll commands are supposed to be ignored so the document doesn't move underneath the panel.
That protection existed once, but a big internal rewrite of how documents are drawn (the “WebKit cutover”) accidentally removed the wire connecting “a modal is open” to “stop scrolling”. The safety switch itself (suspended) still existed and still worked — nothing ever flipped it. This change reconnects the wire in both of the app's layouts (iPhone, and iPad/Mac) and adds a test that fails if the wire is ever removed again.
Without the fix, pressing Space or Page Down while reading a note silently scrolled the document behind it — when you closed the note you'd find yourself somewhere else, as if the app lost your place.
suspended flag: a boolean on the scroll controller; when true, every scroll command does nothing. A “do not disturb” sign — this fix makes sure someone actually hangs it on the door..onChange(initial: true): SwiftUI for “run this whenever the value changes — and also once right now”. The “right now” matters when a modal is already open at the moment the view appears.prism/Views/CompactDocumentLayout.swift, prism/Views/RegularDocumentLayout.swift: each gains one .onChange(of: coordinator.coordinatorOwnsModalPresentation, initial: true) mirroring the flag into both coordinator.renderedScroll.suspended and coordinator.rawSourceScroll.suspended.prismTests/KeyboardScrollControllerTests.swift: new ModalPresentationSuspendsScrollHostTests — reads both layout sources and asserts the binding (with initial: true and both assignments) is present.The original fix (PR #231) bound suspended from DocumentScrollContent, which then owned the controller. The T-1542 cutover moved the controllers to DocumentLayoutCoordinator (renderedScroll/rawSourceScroll) and T-1719 routed View-menu commands through DocumentActions straight to them — the binding was never rebuilt, so suspended sat permanently false while its unit tests stayed green. The re-fix binds at the layout level because the layout is always mounted while a document is open (raw-source mode unmounts DocumentScrollContent). Both controllers are suspended together because a modal (e.g. document-note Cmd+Shift+N) can open over either surface and showRawSource can flip while a modal is up.
.onChange vs centralising: matches the sibling focus-restore watchers, but the codebase's own answer to two-host drift (T-1893) was a shared modifier — and a coordinator-level derivation would be sturdier still. Flagged as follow-up.FootnotePresentationHostTests already made for the identical failure mode.The regression class is “handler correct, wiring absent”: handleKey (line 253) and all six scroll methods (lines 143-177) guard on suspended, and every one of those guards was covered by direct-invocation tests throughout the three months the bug was live — no test asserted anything sets the flag. Same shape as T-1943's never-armed startNavigationObservation().
initial: true is load-bearing: a raw-source toggle rebuilds the layout body, and without the initial pass a modal already open at (re)mount leaves the gate open until the next presentation transition. Suspending both controllers unconditionally removes an ordering hazard: DocumentReaderView.makeDocumentActions picks the target controller from showRawSource at invocation time, so gating only the active controller would leak commands if the surface flips mid-modal. The .ignored return in handleKeyPress is deliberate — it lets SwiftUI route the key to the actually-focused responder (the modal) instead of swallowing it.
The binding's location has moved twice (content view → nowhere → layouts); the agent-note and the structural test both pin the current location, so any future refactor must move the test in the same commit — which is the point. No new state or API surface: pure wiring of existing observable state into an existing flag.
String.contains); today no false-pass exists because the production comments word-wrap so the matched strings appear only in live code — luck, not design.DocumentReaderView selects one via size class over a single shared coordinator); a size-class flip mid-modal is re-asserted by initial: true, and even transient overlap would be idempotent MainActor writes.resetForNewSession() does not clear suspended, but session change dismisses coordinator modals (watcher fires false) and remounts hit initial: true — no stale path found.prism/Views/CompactDocumentLayout.swift
Why it matters. The production half of the fix for the iPhone layout: without it, View-menu and keyboard scroll commands act on the document behind any coordinator-owned modal.
What to look at. CompactDocumentLayout.swift:305-320 (.onChange on coordinator.coordinatorOwnsModalPresentation)
prism/Views/RegularDocumentLayout.swift
Why it matters. The two layouts are separate presentation hosts; T-1893 already demonstrated this exact failure mode (a modifier silently missing from one layout). The binding coexists with the layout's separate focus-restore watcher on the same value — independent modifiers, disjoint state, no ordering hazard.
What to look at. RegularDocumentLayout.swift:191-205
prismTests/KeyboardScrollControllerTests.swift
Why it matters. The controller-level suspension tests stayed green through the entire three-month regression because they test the gate, not that anything engages it. This test fails if either layout drops the binding, or reintroduces it without initial: true.
What to look at. KeyboardScrollControllerTests.swift:405-484 (new suite at end of file)
specs/bugfixes/document-scroll-keys-route-behind-modals/report.md
Why it matters. The report now records how a fully-tested fix died silently (T-1542 restructured the host view; T-1719 rerouted the commands; nothing re-created the binding) — the causal chain a future refactorer needs before moving this binding a third time.
What to look at. report.md Reopening section; docs/agent-notes/keyboard-scrolling.md suspension section rewrite
The original home (DocumentScrollContent) no longer owns a controller and is unmounted in raw-source mode; the layouts are the stable, always-mounted owners. Stated in the code comments and report. The reviewed alternative — deriving suspension inside the coordinator via an injected closure (the WebCommands pattern already in KeyboardScrollController) — would remove the view from the loop entirely and is recorded as the recommended follow-up.
DocumentActions selects rawSourceScroll vs renderedScroll from showRawSource at invocation time (DocumentReaderView.swift:426-428), and a modal such as the document-note Cmd+Shift+N shortcut can open over either surface — gating only the active controller would leak commands if the surface flips while a modal is up. Stated in the code comments and report.
Covers a modal already presented when a layout (re)mounts — e.g. immediately after a raw-source toggle rebuilds the layout body. Without it the gate stays open until presentation state next changes. Stated in the code comments; the structural test asserts the parameter specifically.
No view-hosting harness exists in this project (verified — no ViewInspector, NSHostingController, or UIHostingController anywhere in prismTests), so a real .onChange cannot be driven from a unit test. The test copies the FootnotePresentationHostTests (T-1893) pattern verbatim, including the #filePath-relative source loading. Stated in the test doc comment and report, with WebContentTerminationWiringTests (T-1943) cited as the 'pin the wiring' reference.
| Severity | Area | Finding | Resolution |
|---|---|---|---|
| major | Architecture: mirrored vs derived state | The fix re-installs the exact shape that regressed: KeyboardScrollController.suspended is a stored mirror of the computed coordinator.coordinatorOwnsModalPresentation, synchronised only by view-layer .onChange — the mechanism whose silent death in the T-1542 cutover caused the three-month regression. A coordinator-injected closure (`@ObservationIgnored var isSuspended: () -> Bool`, set at DocumentLayoutCoordinator init with `[unowned self]` — the WebCommands pattern already in the same class) would derive the state with no view in the loop, survive any future view restructuring by construction, and allow a real behavioral test (set showDocumentNoteSheet, assert handleKey returns false) in place of the source-string test. | Deferred as a follow-up, not fixed in this PR: the refactor would require changing existing controller tests (suspended = true → isSuspended = { true }) and replacing the new structural test — the pre-push constraint says refactors requiring test changes should be reconsidered — and the full suite cannot validate a deeper refactor under current machine contention (load average 94). The shipped fix is functionally correct and regression-pinned; the reviewer explicitly rated this non-blocking. Recommend filing a Transit ticket for the closure-injection refactor. |
| minor | Copy-paste: duplicated .onChange across layouts | The two .onChange blocks plus ~13-line comments are near-identical in CompactDocumentLayout.swift:305-320 and RegularDocumentLayout.swift:191-205. Per-layout duplication matches the sibling focus-restore watchers, but the codebase's explicit remedy for this exact two-host drift problem was a shared modifier: .footnotePresentation(...) exists precisely 'so the two presentation hosts cannot drift apart (T-1893)'. A .modalScrollSuspension(coordinator:) ViewModifier would hold the logic once. | Skipped: extracting the modifier would relocate the strings the new structural test matches, requiring test changes for a pure refactor; and it is moot if the major finding's closure-injection follow-up is taken, which deletes both blocks entirely. Fold into the same follow-up ticket. |
| nit | Structural test: comment-blindness | source.contains(...) cannot distinguish live code from commented-out code — if the .onChange block were commented out with //, the test would still pass. Today there is no false-pass (the production comments word-wrap so the matched strings appear only in live code), but that is luck, not design. Weakness shared with the FootnotePresentationHostTests precedent. | Skipped (test-file change, not a bug; the test is not wrong today). Cheap hardening if ever touched: filter lines whose trimmed prefix is // before matching. |
| nit | Test helper duplication | The #filePath-relative layoutSource(_:) helper is duplicated verbatim from FootnotePresentationHostTests. A shared test-support helper would be tidy if a third structural test appears. | Skipped: two occurrences do not yet justify extraction, and test-file refactoring is out of scope for a pre-push review. |
Click to expand.
diff --git a/prism/Views/CompactDocumentLayout.swift b/prism/Views/CompactDocumentLayout.swiftindex 2b95073..8011dac 100644--- a/prism/Views/CompactDocumentLayout.swift+++ b/prism/Views/CompactDocumentLayout.swift@@ -302,6 +302,22 @@ struct CompactDocumentLayout: View { coordinatorOwnsModalPresentation: coordinator.coordinatorOwnsModalPresentation, action: restoreBodyFocusIfIdle ))+ // Gate keyboard/menu scroll commands while a coordinator-owned modal+ // (note popover, add-note / reply / document-note sheet, footnote+ // popover) is presented over the document (T-1099). The WebKit+ // cutover dropped this binding entirely — `suspended` gates every+ // entry point on `KeyboardScrollController` (View-menu Page/Top/+ // Bottom commands via `DocumentActions`, and raw source's own+ // `.onKeyPress`), but nothing in production ever set it. `initial:+ // true` covers a modal already up when this view (re)mounts — e.g.+ // after a raw-source toggle. Both controllers are suspended together+ // since either can be the active `DocumentActions` target and a+ // modal (like the document-note shortcut) can open regardless of+ // which one is currently driving.+ .onChange(of: coordinator.coordinatorOwnsModalPresentation, initial: true) { _, present in+ coordinator.renderedScroll.suspended = present+ coordinator.rawSourceScroll.suspended = present+ } #if os(iOS) .onChange(of: scenePhase) { _, phase in if phase == .active { restoreBodyFocusIfIdle() }diff --git a/prism/Views/RegularDocumentLayout.swift b/prism/Views/RegularDocumentLayout.swiftindex 8877671..5598489 100644--- a/prism/Views/RegularDocumentLayout.swift+++ b/prism/Views/RegularDocumentLayout.swift@@ -188,6 +188,21 @@ struct RegularDocumentLayout: View { .onChange(of: coordinator.coordinatorOwnsModalPresentation) { _, present in if !present { restoreBodyFocusIfIdle() } }+ // Gate keyboard/menu scroll commands while a coordinator-owned modal+ // is presented over the document (T-1099). The WebKit cutover+ // dropped this binding entirely — `suspended` gates every entry+ // point on `KeyboardScrollController` (View-menu Page/Top/Bottom+ // commands via `DocumentActions`, and raw source's own+ // `.onKeyPress`), but nothing in production ever set it. `initial:+ // true` covers a modal already up when this view (re)mounts. Both+ // controllers are suspended together since either can be the+ // active `DocumentActions` target and a modal (like the+ // document-note shortcut) can open regardless of which one is+ // currently driving.+ .onChange(of: coordinator.coordinatorOwnsModalPresentation, initial: true) { _, present in+ coordinator.renderedScroll.suspended = present+ coordinator.rawSourceScroll.suspended = present+ } #if os(macOS) .onChange(of: controlActiveState) { _, state in if state == .key { restoreBodyFocusIfIdle() }diff --git a/prismTests/KeyboardScrollControllerTests.swift b/prismTests/KeyboardScrollControllerTests.swiftindex d131c3e..afe334a 100644--- a/prismTests/KeyboardScrollControllerTests.swift+++ b/prismTests/KeyboardScrollControllerTests.swift@@ -22,6 +22,7 @@ // offset failed. Both symptoms are the same bug (T-1984). // +import Foundation import Testing import SwiftUI @testable import prism@@ -403,3 +404,83 @@ struct DocumentLayoutCoordinatorModalPresentationTests { #expect(!coordinator.coordinatorOwnsModalPresentation) } }++// MARK: - Modal presentation is actually wired to `suspended` (T-1099, host-level)++/// The T-1099 regression, twice over. Fixed once in PR #231 by mirroring+/// `coordinator.coordinatorOwnsModalPresentation` into+/// `KeyboardScrollController.suspended` from an `.onChange` in+/// `DocumentScrollContent`. The WebKit rendering cutover (T-1542) restructured+/// that view — `DocumentScrollContent` no longer owns a `KeyboardScrollController`+/// or `bodyHasFocus` at all, both moved to the layout files — and the binding was+/// never re-created anywhere. T-1719 then rebuilt View-menu Page/Top/Bottom+/// commands to route through `DocumentActions` straight to+/// `coordinator.renderedScroll` / `coordinator.rawSourceScroll`, so those commands+/// kept working — just with `suspended` permanently `false`, silently reopening+/// the bug the guard clauses in `KeyboardScrollController` (covered above) were+/// never actually protecting production from.+///+/// `DocumentLayoutCoordinatorModalPresentationTests` above pins that+/// `coordinatorOwnsModalPresentation` itself tracks presentation state+/// correctly — and stayed green through the entire regression, because nothing+/// reads that flag except a view's `.onChange`. That is exactly the shape+/// `WebContentTerminationWiringTests` documents: a handler that is correct once+/// invoked proves nothing about whether anything invokes it. There is no SwiftUI+/// view-hosting harness in this project to drive a real `.onChange` from a unit+/// test (no ViewInspector, no NSHostingController pump), so — mirroring+/// `FootnotePresentationHostTests`, which pins the T-1893 sibling of this exact+/// failure mode (a modifier silently missing from one layout) the same way —+/// this reads the two layout sources directly and fails if the binding is ever+/// removed, or reintroduced without `initial: true` (which would leave a modal+/// already open at mount time unsuspended until it changes state at least once).+@Suite("Modal presentation suspends keyboard scroll (T-1099)")+struct ModalPresentationSuspendsScrollHostTests {++ /// Reads a layout source file relative to this test file, the same+ /// `#filePath`-relative approach `FootnotePresentationHostTests` uses so the+ /// check needs no bundle resource wiring.+ private static func layoutSource(_ fileName: String) throws -> String {+ let viewsDirectory = URL(fileURLWithPath: #filePath)+ .deletingLastPathComponent() // prismTests+ .deletingLastPathComponent() // repo root+ .appendingPathComponent("prism")+ .appendingPathComponent("Views")+ return try String(+ contentsOf: viewsDirectory.appendingPathComponent(fileName),+ encoding: .utf8+ )+ }++ @Test(+ "Both document layouts bind coordinatorOwnsModalPresentation into an initial + live suspended gate",+ arguments: ["CompactDocumentLayout.swift", "RegularDocumentLayout.swift"]+ )+ func bothLayoutsSuspendScrollControllersOnModalPresentation(fileName: String) throws {+ let source = try Self.layoutSource(fileName)+ #expect(+ source.contains(+ ".onChange(of: coordinator.coordinatorOwnsModalPresentation, initial: true)"+ ),+ """+ \(fileName) must mirror `coordinator.coordinatorOwnsModalPresentation` into \+ `KeyboardScrollController.suspended` via an `.onChange` with `initial: true`. \+ Without `initial: true`, a modal already presented when this layout (re)mounts \+ (e.g. after a raw-source toggle) leaves scroll commands live until presentation \+ state next changes. Without the binding at all, View > Page Down/Top/Bottom \+ keep scrolling the document underneath a note/footnote/add-note/reply/document-note \+ modal (T-1099 — reopened after the WebKit rendering cutover dropped this binding).+ """+ )+ #expect(+ source.contains("coordinator.renderedScroll.suspended = present"),+ "\(fileName) must suspend the rendered-document scroll controller."+ )+ #expect(+ source.contains("coordinator.rawSourceScroll.suspended = present"),+ """+ \(fileName) must suspend the raw-source scroll controller too — a modal (e.g. \+ the document-note Cmd+Shift+N shortcut) can open while raw source is displayed.+ """+ )+ }+}diff --git a/specs/bugfixes/document-scroll-keys-route-behind-modals/report.md b/specs/bugfixes/document-scroll-keys-route-behind-modals/report.mdindex 274e8d4..c2001c8 100644--- a/specs/bugfixes/document-scroll-keys-route-behind-modals/report.md+++ b/specs/bugfixes/document-scroll-keys-route-behind-modals/report.md@@ -1,9 +1,75 @@ # Bugfix Report: Document scroll keys still route behind modal presentations -**Date:** 2026-05-04+**Date:** 2026-05-04 (original fix); reopened and re-fixed 2026-08-10 **Status:** Fixed **Ticket:** T-1099 +## Reopening (2026-08-10)++The original fix (PR #231, described below) mirrored+`coordinator.coordinatorOwnsModalPresentation` into+`KeyboardScrollController.suspended` from an `.onChange` on+`DocumentScrollContent`. The WebKit rendering cutover (T-1542) restructured+that view: `DocumentScrollContent` no longer owns a `KeyboardScrollController`+or a `bodyHasFocus` `@FocusState` at all — both moved out to+`CompactDocumentLayout` and `RegularDocumentLayout` — and the suspension+binding was never re-created anywhere. T-1719 then rebuilt the View-menu+Page/Top/Bottom commands to route through `DocumentActions` straight to+`coordinator.renderedScroll` / `coordinator.rawSourceScroll`, so those+commands kept working functionally — just with `suspended` permanently+`false`. On main prior to this fix there were no production assignments to+`suspended` at all; the controller's suspension gates (still fully unit+tested — see the Regression Test section below) were dead code in+production.++**Repro (unchanged):** present a note/footnote/add-note/reply/document-note+modal in a long document, then invoke View > Page Down/Top/Bottom (or Arrow/+Page/Space/Cmd+Arrow with keyboard focus on the document). The underlying+document scrolls behind the modal.++**Re-fix:** restored an initial + live binding in both layout hosts —+`CompactDocumentLayout.swift` and `RegularDocumentLayout.swift` — each with+its own+`.onChange(of: coordinator.coordinatorOwnsModalPresentation, initial: true)`+that sets `coordinator.renderedScroll.suspended` and+`coordinator.rawSourceScroll.suspended` together. Both controllers are+suspended regardless of which one is currently active (`showRawSource`),+because a modal such as the document-note Cmd+Shift+N shortcut can open+while either surface is displayed, and because the active surface can change+while a modal is up. `initial: true` covers a modal already open when a+layout (re)mounts (e.g. after toggling raw source).++Added a host-level regression test,+`ModalPresentationSuspendsScrollHostTests` in+`prismTests/KeyboardScrollControllerTests.swift`, following the same+source-structural pattern `FootnotePresentationHostTests` uses for the T-1893+sibling of this exact failure mode (a SwiftUI modifier silently missing from+a layout). There is no SwiftUI view-hosting harness in this project (no+ViewInspector, no `NSHostingController` test pump) to drive a real+`.onChange` from a unit test, so the test reads both layout sources directly+and fails if the `.onChange(..., initial: true)` binding or either+`.suspended = present` assignment is removed. `WebContentTerminationWiringTests`+(T-1943) was the reference for "pin the wiring, not just the handler", though+that case could pin a live async stream subscription directly; this one+cannot, for the same reason `FootnotePresentationHostTests` couldn't.++**Files changed in the re-fix:**+- `prism/Views/CompactDocumentLayout.swift` — added the suspension `.onChange`.+- `prism/Views/RegularDocumentLayout.swift` — added the suspension `.onChange`.+- `prismTests/KeyboardScrollControllerTests.swift` — added+ `ModalPresentationSuspendsScrollHostTests`.++**Verification:** targeted run of `KeyboardScrollControllerTests`,+`DocumentLayoutCoordinatorModalPresentationTests`,+`ModalPresentationSuspendsScrollHostTests`, and `FootnotePresentationHostTests`+passed (`** TEST SUCCEEDED **`). `make lint` passed. A full `make test-quick`+run could not complete in this environment — it failed twice with "The test+runner hung before establishing connection", an environment-level xcodebuild/+simulator connection issue unrelated to this change (no assertion failures,+no crash report tied to the changed files).++## Original Fix (2026-05-04)+ ## Description of the Issue Keyboard scrolling (introduced in T-1083) leaves the rendered document body focusable behind coordinator-owned popovers and sheets. While a note popover, footnote popover/sheet, add-note sheet, reply sheet, or document-note sheet is presented, SwiftUI may keep the body's focus state active. Arrow keys, Page Up/Down, Space/Shift+Space, and Cmd+Up/Cmd+Down then continue to scroll the document underneath the modal presentation.@@ -75,14 +141,22 @@ The keyboard scroll controller has no concept of being suspended while a modal o | File | Change | |------|--------|-| `prism/Services/KeyboardScrollController.swift` | Added `suspended` flag and gating in `handleKeyPress` and all scroll command methods. |-| `prism/Views/DocumentScrollContent.swift` | Bound `suspended` to `coordinatorOwnsModalPresentation`; clear `bodyHasFocus` when modal becomes presented. |-| `prismTests/KeyboardScrollControllerTests.swift` | Regression coverage for suspension behaviour and coordinator presentation flag. |-| `docs/agent-notes/keyboard-scrolling.md` | Documented the suspension gate and removed the T-1099 follow-up entry. |+| `prism/Services/KeyboardScrollController.swift` | Added `suspended` flag and gating in `handleKeyPress` and all scroll command methods (original fix; unchanged by the reopening). |+| `prism/Views/DocumentScrollContent.swift` | Original fix bound `suspended` here; removed by the T-1542 WebKit cutover along with `DocumentScrollContent`'s own `KeyboardScrollController`/`bodyHasFocus` ownership. No longer where the binding lives. |+| `prism/Views/CompactDocumentLayout.swift` | **(2026-08-10)** Added `.onChange(of: coordinator.coordinatorOwnsModalPresentation, initial: true)` mirroring presentation state into both `renderedScroll.suspended` and `rawSourceScroll.suspended`. |+| `prism/Views/RegularDocumentLayout.swift` | **(2026-08-10)** Same binding as the compact layout. |+| `prismTests/KeyboardScrollControllerTests.swift` | Regression coverage for suspension behaviour and coordinator presentation flag (original fix); **(2026-08-10)** added `ModalPresentationSuspendsScrollHostTests`, a host-level structural test pinning that both layouts apply the binding. |+| `docs/agent-notes/keyboard-scrolling.md` | Documented the suspension gate and removed the T-1099 follow-up entry (original fix). | ## Verification -**Automated:**+**Automated (2026-08-10 re-fix):**+- [x] Targeted regression tests pass: `KeyboardScrollControllerTests`, `DocumentLayoutCoordinatorModalPresentationTests`, `ModalPresentationSuspendsScrollHostTests`, `FootnotePresentationHostTests` (`** TEST SUCCEEDED **`)+- [x] Linters/validators pass (`make lint`)+- [ ] Full test suite (`make test-quick`) — could not complete; failed twice with an environment-level "test runner hung before establishing connection" error unrelated to the changed files+- [ ] Both platform builds (`make build-ios`, `make build-macos`) — not run in this pass; covered by CI / pre-push review++**Automated (2026-05-04 original fix):** - [x] Regression tests pass - [x] Full test suite passes (`make test-quick`) - [x] Linters/validators pass (`make lint`)diff --git a/docs/agent-notes/keyboard-scrolling.md b/docs/agent-notes/keyboard-scrolling.mdindex e92e73b..f79e3b5 100644--- a/docs/agent-notes/keyboard-scrolling.md+++ b/docs/agent-notes/keyboard-scrolling.md@@ -80,21 +80,26 @@ Mutation-checked: disabling the `ResizeObserver` branch fails only `resizeObserv ## Suspension behind coordinator-owned modals (T-1099) -`KeyboardScrollController.suspended` gates every scroll entry point. The host view (`DocumentScrollContent`) binds `keyboardScroll.suspended = coordinator.coordinatorOwnsModalPresentation` via `.onChange(initial: true)` so the gate is correct on first appearance and on every transition. While suspended:+`KeyboardScrollController.suspended` gates every scroll entry point on both `renderedScroll` and `rawSourceScroll`: `handleKeyPress` returns `.ignored` (letting SwiftUI route the key to the actually focused responder — the popover / sheet — instead of consuming it on the body), and every public scroll method (`arrowUp/Down`, `pageUp/Down`, `scrollToTop/Bottom`) early-returns so explicit callers (heading-row Space forwarding, `DocumentActions`' View-menu commands) no-op without each call site having to check coordinator state. -- `handleKeyPress` returns `.ignored` immediately, letting SwiftUI route the key to the actually focused responder (the popover / sheet) instead of consuming it on the body.-- All public scroll methods (`arrowUp/Down`, `pageUp/Down`, `scrollToTop/Bottom`) early-return so explicit callers — heading-row Space forwarding, future menu integrations — also no-op without each call site having to check coordinator state.+**Where the binding lives has moved twice.** The original (pre-WebKit-cutover) fix bound `keyboardScroll.suspended = coordinator.coordinatorOwnsModalPresentation` inside `DocumentScrollContent`, which at the time owned both a `KeyboardScrollController` and a `bodyHasFocus` `@FocusState`. The T-1542 WebKit cutover restructured `DocumentScrollContent` into a bridge-driven view with neither — the rendered document has no SwiftUI scroll geometry or `.onKeyPress` at all; `DocumentActions`' Page/Top/Bottom commands call straight into `coordinator.renderedScroll` / `coordinator.rawSourceScroll` (selected by `coordinator.showRawSource`). The suspension binding was not re-created anywhere, and `suspended` sat permanently `false` in production for three months (T-1099 reopened, 2026-08-10) even though every controller-level guard above stayed fully unit-tested — the tests exercised a gate nothing in production ever engaged. -The same `.onChange` clears `bodyHasFocus` on entering modal state and re-asserts it on dismissal (unless a search field already holds focus). The dismiss-side restore was the only path before T-1099; without the suspend-side gate the body kept consuming scroll keys behind popovers and sheets.+The binding now lives at the **layout level**, one per host, each independent:+- `CompactDocumentLayout` and `RegularDocumentLayout` each carry their own `.onChange(of: coordinator.coordinatorOwnsModalPresentation, initial: true)` that sets `coordinator.renderedScroll.suspended = present` and `coordinator.rawSourceScroll.suspended = present` together.+- Both controllers are suspended regardless of which one `DocumentActions` currently targets: a modal (e.g. the document-note Cmd+Shift+N shortcut) can open while either surface is displayed, and `showRawSource` can flip while a modal is up.+- `initial: true` is load-bearing, not decorative — it covers a modal already open when a layout (re)mounts, e.g. immediately after a raw-source toggle.+- There is no SwiftUI view-hosting harness in this project (no ViewInspector, no `NSHostingController` test pump) to drive a real `.onChange` from a unit test. `ModalPresentationSuspendsScrollHostTests` (`prismTests/KeyboardScrollControllerTests.swift`) instead reads both layout sources and fails if either the `initial: true` binding or either `.suspended = present` assignment goes missing — the same source-structural shape `FootnotePresentationHostTests` uses for the T-1893 sibling of this failure mode.++If this binding is ever refactored again (e.g. centralised back onto the coordinator), keep the host-level test in step, or the fix can regress silently a third time. ## Layout-level focus restore (T-1103) -`bodyHasFocus` is owned at the layout level (`CompactDocumentLayout` / `RegularDocumentLayout`) and passed to both `DocumentScrollContent` and `RawSourceView` as `@FocusState.Binding`. Whichever content view is mounted picks up `.focused($bodyHasFocus)`, so any layout-level write to the state lands on the active scroll surface.+`bodyHasFocus` is owned at the layout level (`CompactDocumentLayout` / `RegularDocumentLayout`) as an `@FocusState` and passed to `RawSourceView` as `@FocusState.Binding`. The rendered path (`DocumentScrollContent`) does not take `bodyHasFocus` at all post-cutover — it has no `.onKeyPress` or focusable surface of its own — so this state and its restore triggers matter only while raw source is mounted. Layout-level `.onChange` triggers that re-assert `bodyHasFocus` via `restoreBodyFocusIfIdle()`: - Compact: `showTOCSheet`, `showNotesSheet`, `showSearchOverlay`, `coordinator.coordinatorOwnsModalPresentation`, and `scenePhase` (iOS). - Regular: `session.isSearchActive`, `coordinator.coordinatorOwnsModalPresentation`, `controlActiveState` (macOS), and `scenePhase` (iOS). -The `coordinatorOwnsModalPresentation` watcher (T-1103) is required at the layout level because raw source replaces `DocumentScrollContent` rather than augmenting it — the equivalent watcher inside `DocumentScrollContent` is unmounted while raw source is shown, so the layout-level version is what keeps focus restore working uniformly across the two content views. Do not re-add a duplicate `.onChange(of: coordinator.coordinatorOwnsModalPresentation)` inside `DocumentScrollContent` for focus restore — the T-1099 binding there already handles the rendered-body suspend/resume; the layout watcher covers raw source.+The `coordinatorOwnsModalPresentation` watcher (T-1103) is required at the layout level because raw source replaces `DocumentScrollContent` rather than augmenting it, so a watcher scoped inside `DocumentScrollContent` would be unmounted while raw source is shown. This focus-restore `.onChange` is a separate modifier from the T-1099 suspension `.onChange` above — both are attached to each layout's body and fire independently off the same `coordinatorOwnsModalPresentation` value. Known follow-ups:diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 5224d5a..e5ed971 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Arrow keys, Page Up/Down, Space, and the View menu's **Page Down**/**Page Up**/**Scroll to Top**/**Scroll to Bottom** no longer scroll the document behind an open note, footnote, add-note, reply, or document-note modal (T-1099, reopened). This was fixed once before; the WebKit rendering cutover restructured the views that carried the fix and the binding was never rebuilt, so scroll commands kept reaching the document underneath a modal that should have blocked them. The gate is restored on both the iPhone and iPad/Mac layouts, taking effect immediately when a modal is already open and staying live across every presentation and dismissal. - Choosing where to go while a document reloads now takes you there (T-1975). If a file changed on disk — or a URL document was refreshed — while you had it open, and during the moment the app spends preparing the new version you picked a table-of-contents entry, tapped a note, followed a link to a heading, or stepped to a search match, the reloaded document appeared at your saved reading position instead. What you chose was handed to the copy still on screen, which was about to be replaced, so nothing was left to say where you had asked to go and restoring your place won — and on a large document, where preparing the new version takes longest, that window is at its widest. The document on screen is now treated as superseded from the moment a reload starts rather than from the moment the new version is ready, so anything you choose in between is held for the version that is coming and takes precedence over your saved place, exactly as it already did when you chose a moment later. This holds when a file changes twice in quick succession, so a second reload beginning before the first has finished preparing still takes you where you asked rather than back to your saved place. Reloads you did not navigate during still return you to where you were reading, and once the reloaded document has taken you where you asked, the next reload restores your place normally. Scrolling while a reload prepares still counts too, however you do it — dragging, a trackpad or wheel, **Page Up** and **Page Down**, or **Scroll to Top** and **Scroll to Bottom** — because the document stays in front of you and stays scrollable the whole time: the place you scroll to is the place you are returned to. - Changing the reading font or text size no longer moves you somewhere else in the document (T-1965). Both settings already applied without reloading, but they reflow the whole document and nothing put you back afterwards: raising **Larger Text** to an accessibility size makes every block roughly three times as tall, so the text you were reading slid off the bottom of the screen and left you looking at something you had already been through. The app then recorded that new spot as where you were reading, so closing and reopening the document returned you to it as well. Your place is now kept across the change — including how far into a paragraph you were, so the same words stay in front of you rather than merely the same paragraph starting at the top — and a place you were never reading can no longer be saved while the document settles. Jumping somewhere while the change is settling wins: a table-of-contents entry, a link, a note, or a search match all take you where you asked, and the re-anchoring steps aside. Collapsing the section you were reading during the change leaves you at its heading rather than at content that is no longer shown. - A document that goes blank because its rendering process stopped now restores itself (T-1943). The app has always been able to recover from this — it reloads the document and puts back your theme, your reading position, your note markers, and any active search highlights — but nothing was ever watching for the rendering process to stop, so the recovery never actually ran. A large or image-heavy document whose renderer was shut down under memory pressure therefore showed an empty page, with no error and no way back except closing the file and opening it again. The app now watches for it and recovers on the spot. An ordinary failure to load — a link that goes nowhere, an image that cannot be fetched — is told apart from a stopped renderer, so it neither causes a needless reload nor stops the app watching for a real one afterwards. The recovery also covers its own failure: if the reload it starts cannot itself load the document, that counts as the recovery failing and is tried again, instead of leaving the page blank with nothing running. A reload that neither succeeds nor fails — one that simply never finishes — is covered too: it is given a generous time limit, well beyond what even a large document takes to appear, and is then treated as a failed recovery and tried again rather than leaving the page blank indefinitely. If reloading repeatedly fails to bring the document back, the app stops retrying rather than reloading over and over — and says so, with a banner offering to reload. Taking that reload also restores the document's ability to recover on its own again, so giving up is never permanent while the file stays open.The machine was under heavy contention throughout (load average 94, 15 concurrent build processes), and earlier full-suite attempts on this branch hit the known environment-level “test runner hung before establishing connection” failure. Verification therefore rests on: the targeted classes (KeyboardScrollControllerTests, DocumentLayoutCoordinatorModalPresentationTests, ModalPresentationSuspendsScrollHostTests, FootnotePresentationHostTests — all passing), make lint (0 violations, re-run during this review), and the green CI on PR #353 (which predates the current GitHub Actions billing block). The change surface is two view-modifier additions plus a test, so targeted coverage maps well onto the risk — but run make test-quick once the machine is quiet if you want belt-and-braces.
Open a long document, present a note popover, and try View > Page Down and the Space/arrow keys — then toggle raw source with the modal still up and try again. The initial: true path (modal already open at layout remount) is exactly the case a unit test cannot drive here.
Phase 6 wrote specs/bugfixes/document-scroll-keys-route-behind-modals/implementation.md (three-level explanation + completeness assessment) into the working tree. It is uncommitted — commit it with the branch or drop it, as you prefer.