Review round: keep the caret visible to a keyboard, and stop a comment overstating its API
Three findings from the whole-branch review, none of them blocking, two of them worth the branch not landing without. **The focus ring's replacement has to be visible on every path.** Dropping the ring rests on the caret saying more than a box around the paper does, and that holds — except when arriving by keyboard. Tab in from the sign-out button on a document scrolled away from wherever the selection was last left and ProseMirror restores that selection without scrolling to it (its focus() does not scroll, on purpose), so focus landed with no visible indicator anywhere in the viewport. The ring's own edges used to cover that. AGENTS.md's rule is WCAG 2.2 AA with no exemptions, so this is not a trade to make quietly: an empty transaction now carries a scrollIntoView on focus. Only for focus that did not arrive by pointer, which is the whole subtlety. A focus event fires before ProseMirror has set the selection the click asked for, so scrolling on every focus scrolls to the stale one instead — click into the third page and the view yanks back to the first, a worse bug than the one being closed. :focus-visible cannot tell the difference (a contenteditable takes keyboard input, so it matches whatever the modality was), hence the flag a pointer press sets and the next macrotask clears. Verified both ways in Chromium on a 5,528-character document: clicking at the top with the selection left at the end holds scrollY at 0, and tabbing in with the selection off-screen scrolls it on screen. **TextSelection.near is not an API.** TextSelection defines no static near of its own (prosemirror-state 1.4.4), so it silently resolved to the inherited Selection.near while reading like a promise that the result is a TextSelection — which near does not make, falling back to a NodeSelection or an AllSelection when no text position is reachable. Same function, spelled honestly now. **And one comment was reasoning from something untrue**: an empty document does not lack a line to be near, it holds an empty paragraph that resolves. The posAtCoords null fallback is right; its stated why was not. The remaining review findings are left alone deliberately: shift-click in the margin collapses rather than extends, and a drag starting in the margin does not grow a selection. Both are refinements to margin behaviour that never worked at all before this branch, neither regresses the reported bug, and inventing selection semantics from coordinates is its own piece of work. go test ./internal/... ./cmd/... and node --test fixtures/*.test.mjs pass. Bundle rebuilt: editor.98af6ed37bba8906.js.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017F61AU8zx7FcpF1zEPvMpp
fixtures/editor-entry.js+61 −4internal/docs/static/{editor.2c8c082596237a4c.js => editor.98af6ed37bba8906.js}+11 −11
CI passed — run details
Intent
merge and deploy
<cross-session-message from="uds:/run/user/1001/cc-socks/1010444.sock" from-name="checklist formatting and export" from-mode="bypass"> Coordinating scope in oficina/docs — I'm the session Paul asked to build the "maximally viable" checklist overnight: a formatting toolbar, font options, and PDF + Markdown export. I see a local branch worktree-click-to-place-caret which looks like yours. Which files are you touching (fixtures/editor-entry.js, the editor JS modules, internal/docs/views/show.html, core/src/fragment.rs)? I'll branch off main in my own worktree and stay out of whatever you name. </cross-session-message>
<cross-session-message from="uds:/run/user/1001/cc-socks/1010444.sock" from-name="checklist formatting and export" from-mode="bypass"> Thanks — that's exactly the information I needed, and I'll sequence around you rather than race you. My order: (1) start now on core/src/fragment.rs + the wasm ABI + Go handlers for Markdown and PDF export, which touch none of your three files; (2) once your squash lands on main, rebase onto it and only then write the toolbar into fixtures/editor-entry.js and show.html; (3) run fixtures/build-editor.mjs exactly once at the end, so the single committed bundle is built on top of yours and there is nothing to hand-merge. Ping me when the merge sha is on main and I'll pull it. Noted on maxRetainedUpdates=4096 — a toolbar spends it faster, so I'll keep each toolbar command to a single coalesced transaction and I won't touch compaction on this branch. </cross-session-message>
<task-notification> <task-id>a28ca5f808f0d5073</task-id> <tool-use-id>toolu_01QinEpu7wRHxWzDSBmGvh6N</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/dda3c5a8-1e06-4d4c-86ca-35135ab3464f/tasks/a28ca5f808f0d5073.output</output-file> <status>completed</status> <summary>Agent "Fable whole-branch review" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>Review complete. I read the branch as one diff (`git diff main...HEAD`), the full `fixtures/editor-entry.js` and `internal/docs/views/show.html`, AGENTS.md, the CI gate, and the compiled bundle itself. Verdict up front: **safe to merge**. No blocking or high findings; everything below is LOW or informational. CI is green at `3aaf231` (`amadan ci status` exit 0), the branch is pushed, and `go test ./internal/docs` passes locally. **What I verified holds (the hard questions in the brief):** - **Resync survival** — correct. The listener is registered once on `[docs-page]` (editor-entry.js:297-298), which outlives every EditorView; the handler reads `currentView` at event time (line 151), never a captured view. `attemptResync`'s destroy→remount (lines 259-261) is synchronous with no await between them, so there is no window where `currentView` points at a destroyed view; during the resync's fetch awaits the old view is still live and dispatching into it is harmless. With `{mount: el}`, destroy leaves `el` in the DOM and the new view remounts into the same node, so `view.dom.contains(event.target)` stays coherent across resyncs. No leak: one listener, ever. - **Save-queue interaction** — none, correctly. The dispatched transaction is selection-only; y-prosemirror emits no Y update for it, so `ydoc.on('update')` never fires and nothing is enqueued. If the initial mount fails, the listener is never added (registration is after `await attemptResync()`), leaving the read view fully inert as before. - **Bundle** — genuine: the filename hash `2c8c082596237a4c` matches sha256-16 of the content, the compiled handler is present in it, no Go/test/template pins a bundle hash, and CI's own gate rebuilds and diffs it anyway. - **Read view / long documents** — the flex change is safe. `[docs-text]` is transparent with no margins of its own, so growing it in the scriptless read view changes nothing visible; `flex: 1 0 auto` (basis auto, shrink 0) lets a document taller than 9in of text push `[docs-page]` past its `min-block-size: 11in` rather than being squeezed. Nothing else in the stylesheet depends on `[docs-page]` being block. - **Outline scope** — correctly confined to `[docs-editor]` only; the header button and save-status chrome keep rastrillo's focus styling. In the scriptless view the div isn't focusable, so the rule is inert there. - **Unbroken by design**: drag-select inside the editor (handler returns early for in-editor targets), right-click (button 2), middle-click paste/autoscroll (button 1, no preventDefault so defaults proceed). `Selection.near`'s `AllSelection` fallback and the `doc.content.size` fallback mean every path yields a valid selection, including an empty document. **Findings, most severe first:** 1. **LOW (accessibility)** — `internal/docs/views/show.html:191-194`. With the ring gone, a keyboard user Tabbing into the editor has only the caret as focus indicator, and PM's `focus()` deliberately does not scroll the restored selection into view. Scenario: in a 3-page document scrolled so the caret is off-screen, Tab from the sign-out button gives focus with *no visible indicator anywhere in the viewport* — the old ring's left/right edges would have been visible. The comment's argument is sound for the common case, and every shipping PM editor does this, but under this repo's "WCAG 2.2 AA, no exemptions" rule (AGENTS.md:323) this is a conscious trade-off Paul should sign off, not a free move. A cheap mitigation later: dispatch `scrollIntoView` on the editor's focus event. 2. **LOW (behavior vs. the comment's claim)** — `fixtures/editor-entry.js:150-172`. Two gaps against "the way every word processor does": shift-click in the margin collapses an existing selection instead of extending it, and a drag that *starts* in the margin places a caret but never grows a selection as the pointer moves (word processors select line-by-line). The second also suppresses the browser's previous native drag-from-padding selection. Neither regresses the reported bug; both are refinements, not fixes. 3. **INFO (comment accuracy — relevant given this repo's why-comment culture)** — `fixtures/editor-entry.js:163`: `TextSelection.near` is not a real API; `TextSelection` defines no static `near` (verified in the bundled prosemirror-state 1.4.4), so this inherits `Selection.near`, which can return a NodeSelection or AllSelection. Harmless with this schema and clamped in-editor coordinates, but the name overstates a guarantee the code doesn't have. Also line 142-144: the "(an empty document has no line to be near)" rationale for a null `posAtCoords` is speculative — an empty PM doc still holds an empty paragraph that resolves; the fallback is right, the stated why is dubious. 4. **INFO** — On touch devices the compatibility mousedown (button 0) fires on a margin tap, so tapping the margin focuses the editor and raises the keyboard — almost certainly the intended behavior, just noting it is now reachable from the margin. macOS ctrl+click in the margin both moves the caret and opens the context menu; negligible. One housekeeping note: the worktree has an untracked `.deploy-build.sh` that is not part of the branch — it won't merge, but it shouldn't be forgotten there. Conformance: comments are in-house style and substantive, no user-visible strings added (catalog rule respected), `docs-*` attribute vocabulary and the bare-attribute grammar are followed, and the merge-gate preconditions (pushed branch, CI green) are met — the branch discussion posting of this review remains the remaining AGENTS.md step before squash-merge.</result> <usage><subagent_tokens>76084</subagent_tokens><tool_uses>16</tool_uses><duration_ms>371004</duration_ms></usage> </task-notification>
landed from worktree-click-to-place-caret
fixtures/editor-entry.jsLoading diff…
internal/docs/static/{editor.2c8c082596237a4c.js => editor.98af6ed37bba8906.js}Loading diff…