PT-4537: Keep a visible caret in an empty verse - #16
Open
mattgetgen wants to merge 2 commits into
Open
mattgetgen wants to merge 2 commits into
mattgetgen wants to merge 2 commits into
Conversation
Arrowing across an empty verse between two verses in one paragraph left the caret on the paragraph's element point, where Chromium returns zero client rects and nothing is drawn. Saroj loses her place and her typing appears to go somewhere else. EmptyVerseCaretGuardPlugin was already detecting the boundary and inserting its zero-width-space caret host correctly. TextSpacingPlugin then destroyed it in three steps: the host's next sibling is a verse marker, so the trailing-space transform appended a space; the host was no longer placeholder-only, so the guard's own strip-on-edit transform read that as the user typing and removed the zero-width space; and the lone space left behind matched the empty-verse clause, which cleared it to nothing. Lexical collected the empty node and the caret fell back to the element point. Exempting a bare cursor host from the trailing-space transform, alongside the exemptions already there for notes, chars, typed marks and attribute runs, is what makes the host survive. Two further gaps in the same behavior: A host is a one-character text node, so the browser offers a caret position on each side of its zero-width space and both paint in the same place. Crossing the verse therefore cost a press at which the caret did not appear to move, and only going forward, so the two directions disagreed. Arrow traversal now treats a bare host as the single insertion point it stands for, stepping over the marker it was materialized against, which makes the crossing two presses each way. An edit that empties the verse the caret is resting in strands it the same way, and announces nothing: Lexical skips its selection-change dispatch when the DOM selection already matches the one the edit applied, so the guard's SELECTION_CHANGE route never runs. That arrival is now repaired from the edit itself, as a node transform, so the host lands in the same commit and the caret is never committed to a state it cannot be seen in. It is deliberately untagged: CURSOR_CHANGE_TAG would suppress the whole commit for USJ-change consumers, and that commit is the user's edit. The host needs no tag to stay out of the document, since the USJ adaptor, the delta adaptor and the collab coordinates each exclude a placeholder-only text node by its content. Verified in Chromium against the platform demo, on an empty verse 3 between verses 2 and 4 in one paragraph. Crossing it reports one client rect and a caret height of 17 at every keystroke in both directions, where the middle stop previously reported zero client rects; deleting a verse's text leaves a visible caret in the emptied verse; the delete still emits one local USJ change carrying no zero-width space; and a single undo restores the deleted text cleanly. The new tests mount TextSpacingPlugin alongside the guard. Mounting the guard alone, which is what the existing tests did, cannot reproduce any of this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t meets The behavior fixes landed with tests that asserted less than they appeared to. Mutation testing found three decisions unguarded: tagging the repair's commit with CURSOR_CHANGE_TAG left both suites green, though it would suppress the user's own edit from USJ-change consumers; dropping ImpliedParaNode from the transform registration left both suites green; and the arrow test covering the "nothing follows the host" branch passed with that branch deleted, because an unmoved caret alone cannot tell "left to the browser" from "handled to no effect". Each is now pinned, and each was re-checked by re-applying its mutation: the stranded-caret test reads the tags of the commit that produces the host, an implied-paragraph case covers the second registration, and the decline case asserts the press was not claimed. Add a platform-level harness for the host, modelled on the one the trailing-note guard already has: real USX through the production adaptor, with CharNodePlugin, MarkerEditPlugin and TextSpacingPlugin mounted alongside the guard, in the marker-hidden view — the only kind where these hosts exist, since an editable verse marker is a TextNode and hosts the caret itself. Plugin-isolated tests could not have caught the original defect; this is the shape that can. Move PARA_KLASSES above the plugin's TSDoc. TSDoc binds to the next declaration, so the block was documenting the private constant, and the constant's one-liner was what reached dist/index.d.ts as the plugin's published documentation. The plugin's doc now also names both arrival routes, since it had gained a second entry point without saying so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Code Review Summary
Branch: pt-4537-make-caret-visible-in-empty-verse
Base: origin/main
Date: 2026-09-18
Review model: Claude Opus 5
Files changed: 6
Overview
An empty verse between two verses in one paragraph gave no visible caret. Arrowing across it left
the caret on the paragraph's element point, where Chromium returns zero client rects and nothing is
drawn, so the user loses their place and typing appears to go somewhere else.
The cause was not the rule that decides where a caret host is needed — that was already correct and
already firing.
TextSpacingPlugin's trailing-space transform destroyed the host it made, in threesteps: the host's next sibling is a verse marker, so a space was appended; the host was then no
longer placeholder-only, so the guard's own strip-on-edit transform read that as the user typing and
removed the zero-width space; and the lone space left behind matched the transform's empty-verse
clause, which cleared it to nothing. Lexical collected the empty node and the caret fell back to the
element point. Two further gaps in the same behavior were fixed alongside it: a caret host is a
one-character text node, so the browser offered a caret position on each side of its zero-width
space and crossing cost a press at which the caret did not appear to move; and an edit that empties
the verse the caret is resting in announces nothing, because Lexical skips its selection-change
dispatch when the DOM selection already matches the one the edit applied.
Verified in Chromium against the platform demo, not only in tests: crossing the verse reports one
client rect and a caret height of 17 at every keystroke in both directions, two presses each way;
deleting a verse's text leaves a visible caret in the emptied verse; the delete still emits one
local USJ change carrying no zero-width space; and a single
UNDO_COMMANDrestores the textcleanly.
API Changes
None. No exported symbol was added, removed, renamed or re-signatured — verified by diffing the
^exportdeclaration lists of all changed source files against the merge base. Three behaviorschanged behind unchanged signatures:
EmptyVerseCaretGuardPlugin— additionally registers node transforms onParaNodeandImpliedParaNodethat materialize the caret host inside the same commit as the edit, deliberatelywithout
CURSOR_CHANGE_TAG.ArrowNavigationPlugin— a new module-private$exitCaretHostForwardstep in the forwardunmodified-key chain consumes one press when the caret is in a bare cursor host.
TextSpacingPlugin—$textNodeTrailingSpaceTransformnow exempts placeholder-only text nodes.packages/platform/etc/platform-editor.api.mdcorrectly needs no update.Findings
Critical — Must address before merge
None.
Important — Should address before merge
$addUpdateTag(CURSOR_CHANGE_TAG)inside the new transform left both full suites green, yetthat mutation would suppress the user's own edit from USJ-change consumers.
(fixed during review: the stranded-caret test now records the tags of the commit that
produces the host and asserts
CURSOR_CHANGE_TAGis absent; mutation now fails exactly thatassertion and nothing else)
ImpliedParaNodehalf of the new transform was completely uncovered. ChangingPARA_KLASSESto[ParaNode]left both full suites green.(fixed during review: added "hosts the caret in an implied paragraph too"; the mutation now
fails exactly that test)
at the merge base and also passed with the
if (!following) return false;branch it guardsdeleted.
(fixed during review: it now asserts
event.defaultPrevented === false, which is whatdistinguishes "left to the browser" from "handled to no effect"; deleting the branch now fails
it)
PARA_KLASSESwas inserted between the plugin'sTSDoc block and
export function EmptyVerseCaretGuardPlugin. TSDoc binds to the nextdeclaration, so the plugin's doc documented a private constant and the constant's one-liner
became the doc published in
dist/index.d.ts. Flagged independently by two analysis passes.(fixed during review: constant moved above the doc block, given its own wrapped doc, and the
plugin's doc gained a paragraph naming both arrival routes, since a second entry point was
added)
tests raise the mounted set from one plugin to two; the real editor mounts ~30.
(fixed during review, partially: added
packages/platform/src/editor/emptyVerseCaretHost.test.tsx,modelled on the sibling
trailingNoteCaretHost.test.tsx— real USX through the productionadaptor,
CharNodePlugin+MarkerEditPlugin+TextSpacingPlugin+ the guard, in themarker-hidden view where these hosts exist. Both its tests fail without the fix. See
"Suggested Review Focus" for what it still does not cover.)
$exitCaretHostForwardis gated on!hasModifier, and the only shift-aware path,$extendOneVisibleStop, is gated onmarkerMode === "editable"— which is exactly the modewhere these hosts cannot exist, because an editable verse marker is a
VerseNode extends TextNodeand the rule declines. Measured in Chromium: shift+ArrowRight from inside a hostdoes not move the selection at all, across four presses, with anchor and focus both pinned at
the host's offset 0. Controls confirm shift+arrow works normally in ordinary text, from the
end of the preceding verse, and leftward from the host. Left open deliberately — see Interview
Notes.
Extending a selection out of a host removes the node the anchor sits in, and needs a(Author: that decision is already recordeddeliberate decision recorded at the sync site.
and tested in unchanged code —
EmptyVerseCaretGuardPlugin.test.tsx:151-155states the hostmust be removed when a range selection spans the verse, because the clipboard path serializes
the node tree and has no placeholder awareness. It is ratified behavior for copy/cut
correctness. The fair residue is that the new transform makes hosts appear from an ordinary
delete, so this path now sees more traffic; recorded here rather than changed.)
[Author response: four of the six were fixed during the review, each re-verified by mutation. One
was dismissed with a citation to the existing ratified behavior. The shift+arrow finding was
confirmed by measurement, found to be worse than reported, and deliberately left for its own ticket.]
Minor — Consider
$emptyVerseNeedingHost's TSDoc still says "Read-only: call insideeditor.getEditorState().read()", but the new node transform calls it from insideeditor.update(). The calling-context line is treated as load-bearing in this repo.PARA_KLASSES.filter((klass) => editor.hasNodes([klass]))diverges from the convention inthis directory, where every other plugin makes one
editor.hasNodes([...])check and throws.The filter also cannot fire in practice, since both para types are in
usjBaseNodes.$exitCaretHostForward'sif (!$isTextNode(following))re-answers a question$caretHostAtBoundaryowns;$placeCaretAtBoundaryon the next line calls it internally.$exitCaretHostForwardskips exactly one non-text node without asking whether it renders nocaret positions. Correct today only because the guard guarantees
followingis a versedecorator;
$isVisibleAtomwould make the invariant local.(
$isCursorPlaceholderOnlyText/isCursorPlaceholderOnly), not by tracked node key, whiletransientCaretHost.tspromises the opposite ("a zero-width space is legitimate content insome scripts (Thai/Khmer/Lao line breaks) and is never touched"). Matches existing precedent
in four other files, so it may be a deliberate consistency choice — flagged so it is one.
pass
"rtl"). The rule is direction-agnostic by construction — it sits in theisMovingForwardbranch — so the risk is low and this is coverage, not a defect.TextSpacingPluginexemption has no test inTextSpacingPlugin.test.tsx; its coverage isborrowed from the guard's test file.
document load — not only the user edit its comment describes.
packages/platform/dist/is tracked and is what paranext-core copies, and is not rebuilt onthis branch, so the fix is not yet in the consumable artifact. Repo history suggests this is a
dedicated follow-up commit (
6920a2ae).[Author response: left open. None change behavior; several are worth a follow-up sweep, and the
distrebuild is a scheduling question rather than a defect.]Template Propagation
Shared Regions Modified
None. No
#region shared withmarkers exist in this repository — that convention belongs toparanext-core.
Extension Config Changes
None — not applicable. This repo has no
extensions/directory and no extension-template lineage.Positive Observations
stage tags and change narration found none.
EmptyVerseCaretGuardPlugin's new body mirrors its siblingTrailingNoteCaretGuardPluginexactly,so the two guards still read as one pattern; the repair is a node transform rather than an
editor.updatefrom inside a listener, which the repo's rules call out.sharedhelpers rather than re-deriving placeholder logic.answer to a defect jsdom cannot reproduce: it re-evaluates the production rule on every commit, so
it fails on jsdom's one-commit window and on the browser's permanent one.
TextSpacingPluginwas checked and is clean:isCursorPlaceholderOnly("")is
false, so the new exemption cannot shadow the empty-verse cleanup below it or reintroduce thedocumented transform loop.
files, 113/113 each time.
Interview Notes
Stated purpose. Make the caret visible in an empty verse (PT-4537), so a user arrowing through a
passage can see where the insertion point is and have typing land there.
Root cause was found by measurement, not inspection. The ticket asserted that ZWSP placeholders
were present in the DOM and that the host mechanism therefore existed but yielded no caret. That
premise is wrong: those ZWSPs are inside the verse decorator's own span
(
ImmutableVerseNode.decorate()rendersZWSP + number + ZWSPfor double-click selection), andLexical forces
contentEditable='false'on decorator DOM, so they are not caret positions. Theticket also reported three intermediate keystrokes and a keystroke with no selection at all; Chromium
shows one invisible stop and
rangeCountnever 0. TheNO RANGEclaim could not be reproducedand should be treated as unconfirmed.
Design decision the reviewer should scrutinize. The new transform repairs inside the user's own
edit commit and deliberately does not tag it. Tagging would be the obvious defensive choice and is
wrong:
CURSOR_CHANGE_TAGis inblackListedChangeTags, andDeltaOnChangePluginis givenignoreTags={blackListedChangeTags}, so it short-circuits the whole commit — the user's edit wouldbe suppressed, not just the host. The host needs no tag to stay out of the document, because the USJ
adaptor, the delta adaptor and the collab coordinates each exclude a placeholder-only text node by
content. This was verified end to end in the browser: the delete emits one local USJ change carrying
no zero-width space.
A rejected approach worth knowing about. The delete-arrival repair was first attempted from an
editor.registerUpdateListener, in a separate tagged update. It worked in the browser and brokeTrailingNoteCaretGuardPlugin's cross-guard test: the lateSELECTION_CHANGEstill carries thepre-repair anchor, so the hook's stale-host pass deletes the host it has just created. That approach
was backed out rather than shipped.
transientCaretHost.ts's stale-host pass is the common factor in three separate problems seenduring this work: the race above, the shift+arrow stuck state, and the host being reaped under a
full-subtree transform pass in the new platform harness. It is unchanged by this branch. A reviewer
who wants one thing to look hardest at should look there.
Author does not understand / could not confirm: nothing was deferred to AI, but two things are
genuinely unverified rather than understood. (1) Whether the shift+arrow stuck state is pre-existing
or newly reachable — the mechanism lives in unchanged code and the trailing-note guard makes hosts
the same way, so it is probably pre-existing, but this was reasoned, not measured on the merge base.
(2) Whether a full-subtree transform pass of the kind the platform harness performs occurs in real
use; the browser showed the host surviving every real edit path exercised, so the synthetic pass may
be harsher than reality.
A wrong claim made and corrected during the review. An initial bisect concluded that
CharNodePluginandMarkerEditPlugineach destroy the caret host. That was cross-testcontamination from module-level adaptor singletons. Run in isolation, neither is involved; the
remover is the guard's own stale-host pass.
In-Review Quality Check
All checks run against the
scripture-editorsworktree (this repo uses nx/pnpm, not paranext-core'snpm scripts):
libs/shared-reacttests: 1660 passed, 1 skipped — clean, and stable across repeated runs.packages/platformtests: 1698 passed — clean.nx typecheck(shared-react, platform-editor): green.nx lint: 0 errors. 4 pre-existing warnings in shared-react and 2 in platform-editor, all infiles this branch does not touch.
prettier --checkon all changed files: clean.Two incidents worth recording. Running the nx
testtarget regenerates committed build artifacts(
packages/utilities/dist,packages/platform/dist) via a rawbuild, which strips declarations —the
extract-apitrap the repo's own CLAUDE.md documents; these were reverted each time and are notin the diff. Separately, an untracked
tsc --buildoutput (packages/platform/dist/Editorial.d.ts)was deleted during cleanup, which broke typecheck with TS6305; it was regenerated with
tsc --build --forceand the committed dists that rebuild touched were reverted.Suggested Review Focus
above; it is the decision most worth a second opinion, because getting it wrong silently drops
the user's edits rather than failing loudly.
blocks this branch or gets its own ticket. A Playwright reproduction with controls exists.
transientCaretHost.ts's stale-host pass — implicated in three distinct problems duringthis work, all from the same "a late selection-change carries a stale anchor" shape.
changes together, so the user story — arrow across an empty verse, see a caret, one press
crosses it — has no end-to-end assertion. An attempt was made and dropped: jsdom re-resolves
the DOM selection between a guard-made arrival and a key press, making it order-dependent.
isMovingForward, so it is direction-agnostic byconstruction, but there is no RTL test and none of this was exercised in an RTL project. Note
the pre-existing gap documented on
getEditorTextDirection: a project configureddir="auto"reads as
"ltr", so this change adds one more consumer of that bug.2 Kings 2:7 — were not opened; verification was done in the engine demo app only. The change
was also never checked against
isBlockVerseLayoutfrom engine #538, which the ticket askedfor.
packages/platform/dist/is not rebuilt, so paranext-core does not yet consume this fix.🤖 Generated with Claude Code
This change is