PT-4189: EditorRef note index/key/highlight and keep DOM focus on unfocused applyUpdate - #15
Draft
tjcouch-sil wants to merge 21 commits into
Draft
tjcouch-sil wants to merge 21 commits into
tjcouch-sil wants to merge 21 commits into
Conversation
…ys and document-order indexes Adds $getNoteIndex to shared-react's note.utils (a DFS over note nodes, reused by ImmutableNoteCallerNode's existing getNoteIndex so both paths share one implementation) and exposes it plus its inverse as EditorRef.getNoteIndex/getNoteKey. This is the coordinate bridge a footnotes pane needs: the host addresses notes by the document-order index a USJ walk yields, while the editor identifies them by Lexical node key. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cexoio1HtVmuAp2zJQKK3g
…ete getNoteKey TSDoc MarginalRef extends EditorRef, and Marginal.tsx builds its useImperativeHandle object by hand, so the two new EditorRef members need matching delegations there, following selectNote/getNoteOps. Also adds the missing @PARAM noteIndex to getNoteKey's TSDoc. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cexoio1HtVmuAp2zJQKK3g
… border NoteCallerHighlightPlugin keeps `caller_highlight` on exactly one note's caller element, re-applying it after every commit because Lexical recreates the caller DOM on a collapse toggle. The class is set imperatively, never through node state, so a highlight cannot dirty the document. The highlighted note is tracked by key. A commit that destroys it keeps the highlight only for an in-place replacement — replaceEmbedUpdate re-keys the note it rewrites — recognized as a note created in the same commit at the destroyed note's document-order index. A note that left the document, or a wholesale load (EXTERNAL_USJ_MUTATION_TAG) whose fresh notes are a different chapter's, drops the highlight instead of sliding it onto an unrelated note. The caller is found among the note's children rather than as its first child: in editable marker mode a note opens with its marker glyph. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cexoio1HtVmuAp2zJQKK3g
The public TSDoc promised a highlight without naming the two paths where none appears and nothing is reported: a note BUILT expanded under editable marker mode has a plain-text caller instead of the immutable caller element the style attaches to, and the note is resolved eagerly with no retry, so a call made before the document has loaded or with a stale key or out-of-range index is discarded and clears any highlight already showing. Both sentences go on EditorRef.highlightNote and on the plugin handle's setHighlightedNote, where the behavior lives. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cexoio1HtVmuAp2zJQKK3g
…Key/highlightNote Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cexoio1HtVmuAp2zJQKK3g
…Update Lexical reconciles the DOM selection after every commit, and writing a selection inside a contenteditable focuses that element as a browser side effect. A programmatic applyUpdate therefore stole the caret from whatever the user was actually typing in - most visibly a host's inline note editor applying its live edits back into the Scripture text, which blurred one debounce interval after the first keystroke. Tag the commit with Lexical's SKIP_DOM_SELECTION_TAG when this editor's root does not contain the active element. A focused editor still reconciles, so a collaborator's op keeps the local caret in the right place. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cexoio1HtVmuAp2zJQKK3g
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cexoio1HtVmuAp2zJQKK3g
…ed-palette test Space commits the typed marker literal at once, but the char node it becomes settles afterwards; the assertion read it synchronously and failed under CI load (the palette itself reopens, filters, and commits correctly after any delay). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cexoio1HtVmuAp2zJQKK3g
…a note Two host-facing caret placements a footnotes pane needs. selectNoteTextOffset lands at an offset over a note's CONTENT text, skipping every display artifact the view adds around it - marker glyphs in either marker mode, attribute display runs, engine-owned NBSP spacers, an opening glyph's NBSP separator prefix, and an expanded editable note's caller. A host that captured a click position over its own rendering of the same note now resolves to the same character whatever markerMode this editor renders in, which a host-side DOM walk cannot do: under markerMode "editable" a glyph is real text inside the char span and the span's first text child carries the separator as a prefix, neither of which a per-node DOM filter can drop. selectAfterNote parks the caret immediately after a note - past its caller in a collapsed note. Like applyUpdate, it carries SKIP_DOM_SELECTION_TAG when this editor's root does not hold DOM focus, so a host can set it while the user keeps typing in a note editor elsewhere; that rule now has one name (holdsDomFocus) rather than two copies. Also polyfills Range.getBoundingClientRect for jsdom, which Lexical measures through whenever it scrolls a collapsed caret into view after a commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013PLV1rbFoxTNwRtwfoUTdk
markerMenuContext.utils.test.tsx asserts that a missing Range rect is exactly what leaves anchorRect undefined headlessly, so polyfilling Range.getBoundingClientRect globally made that behavior untestable. Install it only in the caret-placement suite that needs it, and restore the original afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013PLV1rbFoxTNwRtwfoUTdk
… module ImmutableNoteCallerNode needs $getNoteIndex, and the note machinery in note.utils needs ImmutableNoteCallerNode, so the two imported each other. That works only for as long as every use stays inside a function body; the first module-level use - or a CJS evaluation order that loads the caller node first - fails at load with an uninitialized binding. Move the index into note-index.utils, a leaf that imports neither, and re-export it from note.utils so callers keep finding it where they always have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/9e7049b7-4943-4d56-b83c-1f99d89866e7
$selectAfterNote parked the caret at offset 0 of whatever text node followed the note, but a glyph text node's bytes are a picture of its own state - a verse number, a marker's syntax - so offset 0 is a position INSIDE the picture, which the next keystroke splits. That is exactly what $normalizeSelectionOutOfGlyphText exists to prevent, and a note ending a verse is followed by precisely such a node. Skip glyph text so those notes take the parent-offset branch instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/9e7049b7-4943-4d56-b83c-1f99d89866e7
…et APIs This repository commits each published package's dist so paranext-core can copy it out of a checkout without running this toolchain, and the branch's editor changes had not been built into it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/d979f940-2a90-4191-8659-7d82e6558085
…ight APIs
Selecting an EXPANDED note that holds no content run at all (`\f + \f*`) was a
no-op: `$selectNote`'s expanded branch selects the end of the last char child,
and such a note has none, so the caret stayed outside the note and the next
keystroke landed in the surrounding text. It now lands at the child slot
content would occupy. `selectNoteTextOffset` documents that fallback, so the
promise it makes now holds.
Documentation that had drifted or was missing:
- `selectNoteTextOffset` takes DOM focus where its neighbour `selectAfterNote`
deliberately does not. That asymmetry is correct — it mirrors `selectNote`,
which it falls back to, and its job is placing the caret in an editor the
host is about to focus — but it was unstated, and the two neighbouring caret
APIs read as behaving oppositely for no reason. Also records that the caret
is placed, not revealed, while a note is collapsed.
- `getNoteIndex`, `getNoteKey` and `highlightNote` now carry the `EXPERIMENTAL:`
prefix every neighbouring note method has.
- The caller highlight is a yellow fill AND thin blue borders, not just the
borders; corrected in the model, the plugin and the changelog, with a note
that a host vendoring its own `usj-nodes.css` needs the rule.
- `holdsDomFocus` says why it is deliberately broader than `isFocused`.
- The `$getNoteIndex` re-export's comment claimed a backwards-compatibility
constraint that never existed; it now states the real reason (the barrel
exports this file, not the leaf). `NoteCallerHighlightPlugin` imports the
leaf directly rather than re-coupling to the heavy module.
Tests:
- The caret tests ran only under `noteMode: "collapsed"`, while the consumer's
note editor runs expanded — a materially different interior where the caller
is a plain TextNode and a `\cat` category becomes an attribute run. Both are
documented behaviors with no coverage anywhere. Added an expanded block
covering the caller skip, the `\cat` skip, a `\fp` paragraph-break crossing,
and the no-content fallback (which is what found the defect above).
- "does not mark the document dirty" asserted only that `onUsjChange` did not
fire, which a node-state highlight producing identical USJ would also pass,
and never checked that the highlight applied. It now counts commits and
asserts the highlight landed.
- The glyph-avoidance case asserted `not.toBe("verse")`, which passes for any
other wrong placement; it now asserts the caret's actual position.
- Added the missing positive pre-assertion to "clears when the note leaves".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/4c051464-cd5d-40f7-ab18-232f1c272d27
How many leading code units of a text node are an opening glyph's display separator was worked out independently in three places: the note caret offset (`$noteDataTextStart`), `$selectCharContentStart`, and the collab emit path's note-content strip. markerSeparators.utils.ts already owns the convention and the predicate that builds the prefix, so the reading now lives there too, as `$separatorPrefixLength`: plain host text directly after a char-span opening glyph that starts with the NBSP. - The note caret offset and `$selectCharContentStart` call it directly. `$selectCharContentStart` used to skip ANY leading NBSP; every caller hands it the text straight after a span's opener, where the two agree. - The collab emit path keeps its narrower positional rule on top (only the span's own opener, its first child), so the ops it sends are unchanged. Adds unit tests pinning each guard in the helper (each was verified to fail a test when removed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/e30d5c4e-6477-44a0-af88-da5bf2056809
…elper Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/e30d5c4e-6477-44a0-af88-da5bf2056809
`highlightNote` read the committed editor state, so an index resolved against the document as it was before any update still pending: a host that inserted or removed a note and then highlighted by index in the same turn marked the wrong note, or none. It now reads with `editor.read`, which commits pending updates first, as `getNoteIndex` and `getNoteKey` already do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/62270ca3-4092-4411-a74c-f2fa9943757f
…t fix Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/62270ca3-4092-4411-a74c-f2fa9943757f
Selecting an expanded note with no content run (`\f + \f*`) lands the caret in the slot just before the note's closing glyph. The closer was found by looking for a closing marker node, which is what `markerMode: "editable"` builds. `"visible"` (the Markers view) builds the closer as display-only typed text, so it was not found and the caret went after `\f*` instead. The lookup now recognizes both shapes, and a test pins both marker modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/131a9039-65c3-4bd1-9d12-987ea2fb1b8b
…g one A fresh editor moves its caret to the start of `scrRef`'s verse a few microtasks after the document loads. The caret-placement tests sometimes acted before that move landed, so it overwrote the caret under test. "stays out of a glyph when the note is followed by one" failed about 3 runs in 8 this way. The render helper now waits a macrotask for the mount-time move before returning. 12 consecutive runs pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/131a9039-65c3-4bd1-9d12-987ea2fb1b8b
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/131a9039-65c3-4bd1-9d12-987ea2fb1b8b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Editor-side half of Paratext 10's PT-4189 (footnotes edited in the footnotes pane in Standard view; core PR paranext/paranext-core#2805). Additive
EditorRefAPI plus one focus fix; nothing removed.Moved here from eten-tech-foundation/scripture-editors#556, which is closed with a pointer to this PR. The branch was rebased with
--ontoso it carries only its own commits; the old base (eten's #531, "apply annotations issued while a load is in flight") is not on this repository'smainand did not come across.Changes
EditorRef.getNoteIndex(noteKey)— document-order index of a note (the coordinate a USJ-built notes list addresses; the same indexnoteCallerOnClickreports).$getNoteIndexinshared-reactbacks it andImmutableNoteCallerNodereuses it.EditorRef.getNoteKey(noteIndex)— the inverse, so a host that addresses notes by index can handreplaceEmbedUpdatea key.EditorRef.highlightNote(noteKeyOrIndex | undefined)— applies PT9'scaller_highlightclass (already inusj-nodes.css) to one note's caller.NoteCallerHighlightPluginre-applies after every commit (Lexical recreates the caller DOM on collapse toggles andreplaceEmbedUpdatere-keys), follows an in-place replacement in the same commit, refuses to adopt a note across an external load, and never dirties the document. Two silent no-op modes are documented in the TSDoc (a note built expanded in editable marker mode has a plain text caller; the address is resolved eagerly, never retried).EditorRef.selectNoteTextOffset(noteKeyOrIndex, utf16Offset)— puts the caret at an offset within a note's own text, counting the note's content only. Every display artifact the activeViewOptionsadd around that content is skipped: marker glyphs in either marker mode, attribute display runs, engine-owned NBSP spacers, an opening glyph's NBSP separator prefix, and an expanded editable note's caller. See Why this belongs in the editor below.EditorRef.selectAfterNote(noteKeyOrIndex)— puts the caret immediately after a note (past its caller in a collapsed note), where PT9 leaves it once the user is done reading. LikeapplyUpdate, it carriesSKIP_DOM_SELECTION_TAGwhen this editor's root does not hold DOM focus, so a host can park the Scripture caret while the user keeps typing in a note editor elsewhere; the selection is still recorded, so a laterfocus()lands on it. It skips a following GLYPH text node: a glyph's bytes are a picture of its own state (a verse number, a marker's syntax), so offset 0 there is a position inside the picture that the next keystroke splits — exactly what$normalizeSelectionOutOfGlyphTextexists to prevent, and a note ending a verse is followed by precisely such a node. Those notes take the parent-offset branch instead.$getNoteIndexmoves to its own leaf module (note-index.utils), re-exported fromnote.utils.ImmutableNoteCallerNodeneeds the index andnote.utilsneedsImmutableNoteCallerNode, so the two imported each other — which works only while every use stays inside a function body; the first module-level use, or a CJS evaluation order that loads the caller node first, fails at load with an uninitialized binding. No API change.MarginalRefdelegates all five.applyUpdate/replaceEmbedUpdatetag the update withSKIP_DOM_SELECTION_TAGwhen the editor does not hold DOM focus, so a host editing a note in a separate editor (the pane row editor) keeps its caret; explicit caret APIs (focus,setSelection,selectNote) are unchanged. Test fails without the tag. That rule now has one name (holdsDomFocus) shared by both sites rather than two copies.$separatorPrefixLengthinmarkerSeparators.utils.ts, which already owns that convention. Three sites derived it independently: the note caret offset behindselectNoteTextOffset,$selectCharContentStart, and the collab emit path's note-content strip. The emit path keeps its narrower positional rule on top (only the span's own opener, its first child), so the ops it sends are unchanged.$selectCharContentStartnow skips a leading NBSP only in the separator slot. Every caller already hands it the text straight after a span's opener, so their results do not change. New unit tests pin each guard in the helper; removing any one of them fails a test.packages/platform/distis rebuilt in its own commit. This repository commits each published package's build output so paranext-core can copy it out of a checkout without running this toolchain; eten-tech-foundation's did not, so the branch arrived without that rebuild.Why
selectNoteTextOffsetbelongs in the editorThe host captures a caret position over its own read-only rendering of a note (a footnotes pane row) and needs it resolved inside a live editor showing the same note. It was doing that by walking the editor's DOM behind a text-node filter — which worked only for the marker mode that filter had been verified against. Under
markerMode: "editable"an opening glyph is a real editable text node inside the char span, and the span's first text child carries the NBSP display separator as a prefix; the walk counted both as note text, so every run shifted the caret, compounding per run. A prefix of a node cannot be dropped by a per-node filter at all.Only the editor can tell its own display artifacts from content, by node type, so that is where the resolution now lives. The offset origin becomes the note's USJ text — stable across marker modes — which is what makes the two renderings agree.
Known gap,
TODO(PT-4322): text a source wrote directly inside a note rather than inside an\ft-style run is not counted, because the walk requires a char-span ancestor (which is also what excludes the caller). A host that renders such text inline still drifts by its length.Testing
Full local run of what CI runs, at the rebased tip:
nx run-many -t test— all 9 projects green:@eten-tech-foundation/platform-editor1714,shared-react1654 (1 skipped),shared675,utilities56, plus the demos. After the separator-prefix refactor:platform-editor1718,shared-react1654 (1 skipped),shared684,utilities56, plus typecheck and lint, andverify-committed-distis clean at6ad4a091. IncludesnoteIndexAndHighlight.test.tsx,applyUpdateDomSelection.test.tsx, andnoteCaretPlacement.test.tsx— the last renders underSTANDARD_VIEW_MODE, i.e.markerMode: "editable", deliberately (that is the mode the caret bug lived in), and — since the review pass — under the expanded note mode a host's own note editor actually uses.nx run-many -t typecheck lint,prettier --check .,nx format:checknode scripts/rebuild-committed-output.mjsfollowed bynode scripts/verify-committed-dist.mjs— the committed output is current and reproduces byte-for-byte on a second rebuildAI-assisted — session 1, session 2, session 3, session 4, session 5, session 6; the human directed scope and reviewed.
🤖 Generated with Claude Code
Code Review Summary — automated pass, 2026-09-16
Review model: Claude Opus 5 (1M context) · Base:
origin/main· Files changed: 17This PR was reviewed together with its consumer, paranext-core#2805 — six parallel
analysis passes in total, two of them (API/correctness, style/tests) on this repo. The full
cross-repo summary, including the core half's findings and the open decisions for the review
meeting, is in the same block on core#2805. Below is what applied here.
Fixed in
5a639930selectNoteTextOffset's documented no-content fallback landed nowhere.$selectNote's expanded branch isgetChildren().reverse().find($isCharNode)?.selectEnd(), whichfor a note with no content run at all (
\f + \f*) is a complete no-op — the caret stayed outsidethe note and the next keystroke landed in the surrounding text. It now lands at the child slot
content would occupy, just before the closing glyph. Found by a test written during the review.
CHANGELOGentry added under Fixed.noteCaretPlacement.test.tsxrendered under
STANDARD_VIEW_MODE, which isnoteMode: "collapsed", while the consumer's noteeditor runs
noteMode: 'expanded'— a materially different interior, where the adaptor builds thecaller as a plain
TextNodeand folds a\catcategory into an attribute display run. Both ofthose skips are promised in
editor.model.tsand had zero coverage in either repo. Anexpandeddescribe block now covers the caller skip, the\catskip, a\fpparagraph-breakcrossing, and the no-content fallback.
onUsjChangefires only when thedeserialized USJ differs, so a highlight held in node state would dirty the editor and still pass;
and the test never asserted the highlight applied, so a no-op
highlightNotepassed it twiceover. It now counts commits via
registerUpdateListenerand asserts the highlight landed.not.toBe("verse"), which also passes for a caret leftbefore the note or back in the preceding text. It now asserts the caret's actual position (the
para-anchored slot immediately after the note). Added the missing positive pre-assertion to"clears when the highlighted note leaves the document" too.
selectNoteTextOffset's focus behavior (seebelow) and its collapsed-note constraint; the
EXPERIMENTAL:prefix ongetNoteIndex,getNoteKeyandhighlightNote; the caller highlight described as "a thin top-and-bottom border"in three places when
.caller_highlightalso paints#ffffb5(plus a warning that a hostvendoring its own
usj-nodes.cssneeds the rule, or the API is a silent visual no-op — verifiedcore's copies do carry it); and why
holdsDomFocusis deliberately broader thanisFocused.$getNoteIndexre-export was annotated "re-exported here because this is where callers have always found it" —
the symbol was created by this branch's own
a88a0dc3and moved out three commits later, sopost-merge that reads as a backwards-compatibility constraint nobody would dare remove. It now
states the real reason (the barrel exports
note.utils, not the leaf), andNoteCallerHighlightPluginimports$getNoteIndexfrom the leaf rather than re-coupling to theheavy module.
markerMenuHarness.test.tsxread as self-contradictory; reworded toname the engine's deferred settle clock, matching its sibling 40 lines above. No behavior change.
Refuted
selectNoteTextOffsetis missingselectAfterNote'sSKIP_DOM_SELECTION_TAGfocus guardwas raised as a Critical. The asymmetry is correct, not an omission:
selectNoteTextOffsetfallsback to
$selectNote, which has never carried the guard, so adding it would make the fallbackpath behave differently from
selectNotewithin the same call. Its job is "put the caret intoa note this host is about to focus" — the only consumer,
placeInitialCaretin core'sfootnote-editor.component.tsx, pairs it withfocus()unconditionally.selectAfterNoteis theopposite case: driving the Scripture caret from the pane, where focus must not move. Fixed as a
documentation gap instead: the TSDoc now states the asymmetry and tells hosts to pair it with
focus().Dismissed, with reasons
$noteDataTextStartre-derives the NBSP separator-prefix offset thatcharStack.utils.tsandeditor-delta.adaptor.tseach derive independently. Centralizing it into one$separatorPrefixLengthinmarkerSeparators.utils.tsis right — that module declares itself theowner of the representation, and any divergence is silent caret drift — but it touches the emit
path's stricter variant and belongs in its own change. Flagged for follow-up. (Done
2026-09-18 in
1097b210. The emit path keeps its stricter rule on top of the shared helper.)holdsDomFocus/isFocusedunification: documented instead, since changingisFocused's meaningaffects existing consumers.
tagsOfUpdatesDuring,renderEditor, the zero-DOMRectRangepolyfill — now on its 7th copy in the package): a package-wide cleanup, not this PR's.
getNoteIndex/getNoteKeyeach walking the whole document: one chapter's worth of notes.applyUpdateDomSelection.test.tsxnever exercisingapplyUpdatedirectly or itssource: "remote"branch: a worthwhile addition, but the collab path needs more setup than a review passshould improvise. Flagged for follow-up.
Gates after the fixes
nx run-many -t test --projects=platform-editor,shared-react— 1717 passed (98 files)nx run-many -t typecheck lint— pass (2 pre-existingno-consolewarnings indemos/)rebuild-committed-outputthenverify-committed-dist— clean.etc/platform-editor.api.mdis unchanged: no public type signature moved.
This change is