PT-4313: Compensate every indented paragraph marker in the gutter view - #10
Conversation
Review follow-up on #10 (mirrors the paranext-core review of the same change). usj-nodes.css: - Remove the tr/tr1/tr2 gutter entries. A table row is a <tr> (ImmutableTableRowNode), never a .para, so the glyph rule cannot match them; the test excludes them from the derivation instead. - Scope the compensation comment to margins set in this file and name what it does not cover (host-injected generateUsjCss output, consumer commentary stylesheets); fix ::before -> ::after. usj-nodes.css.gutter-coverage.test.ts: - Resolve the LTR-over-agnostic cascade before filtering zero values, so a directional zero override can beat an agnostic non-zero margin. - Classify direction per comma-separated selector, not per selector list. - Fail loudly on a margin shorthand or margin-inline-* on a marker rule, on nested margin-left/text-indent setters, and on an LTR/RTL margin mismatch (one --para-indent serves both directions). - Add a small usfm.sty oracle so a base value drifting from the spec is caught rather than compensated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
katherinejensen00
left a comment
There was a problem hiding this comment.
Review — PR #10: PT-4313 Compensate every indented paragraph marker in the gutter view
For: Ira (PR author) · Reviewed at head a65fc748 against base 6920a2ae
Files: packages/platform/src/usj-nodes.css, packages/platform/src/usj-nodes.css.gutter-coverage.test.ts
Verdict: the fix is correct and worth merging. All 16 added --para-indent values equal their
paragraph's margin-left, every one has a symmetric RTL margin-right, and all 62 indented markers
in the stylesheet match usfm.sty exactly. The new test genuinely fails on main's stylesheet on
exactly the 16 markers claimed, and three of its guards (re-added table row, margin-inline-*
spelling, @media-nested rule) were each demonstrated to fail loudly.
Findings below are one Medium, three Low, six Nit. Only the Medium one asks for a decision before
merge; the rest are cleanup or comment accuracy.
Severity: Medium = wrong rendering in a real configuration · Low = latent gap or inaccurate
claim, nothing broken today · Nit = clarity/consistency.
1. Auto-direction projects get the compensation without the margin — Medium
Summary: For a project whose text direction is auto, the editor root gets no dir attribute,
so the [dir]-qualified base margins never apply — but this compensation is direction-agnostic and
applies anyway, pushing the marker glyph out by a margin the paragraph doesn't have.
Attach to: packages/platform/src/usj-nodes.css:2844 (.psc-gutter-markers.text-spacing .usfm_p2 {)
Detail: TextDirectionPlugin.tsx:22 returns early when textDirection === "auto", so
rootElement.dir is never set. The base rules are compound selectors that need dir on the same
element as .text-spacing (e.g. .text-spacing[dir="ltr"] .usfm_iq1, css:559), so under auto
they don't match and the paragraph has no indent. The gutter rules carry no [dir], so
--para-indent still resolves and the glyph moves by up to 20vw (~384px at a 1920px viewport) —
further out than the 4em gutter, i.e. clipped or off-element.
12 of the 16 markers are affected — p2 qd iq iq1 iq2 iq3 qm2 qm3 ph ph1 ph2 ph3 — because their
base margins are [dir]-qualified (css:544, 559, 911, 1317, 1356, 1705, …). The other four (ipq imq ipr psi, css:505/515/527/2045) set margins in direction-agnostic rules and are fine.
For these 12 markers this is a behavior change: on main they had no compensation, which under
auto happened to be right. The mismatch is pre-existing for the already-compensated markers (q*,
li*, lim*, io*, ili*, pi2, pi3), so the question is whether to keep extending it.
Worth noting: directionQualifiedGutterRules (test:270-286) actively forbids the most direct fix
(qualifying the compensation by [dir], which would correctly no-op when dir is absent), and
unreadableMarginSpellings (test:252-267) forbids the other one (logical margin-inline-start on
the base rules, which would resolve correctly under auto). Both bans are reasonable on their own
terms; together they lock in the physical-margin + agnostic-compensation shape. A deliberate "auto
is out of scope, see the known gap in ArrowNavigationPlugin.tsx:292" note would also settle it.
2. --verse-text-start no longer affects rendering — Low
Summary: Now that --para-indent coverage is complete, the gutter focus box always picks the
glyph column over --verse-text-start, so the four new ph entries change nothing on screen and a
whole test case exists to enforce values nothing reads.
Attach to: packages/platform/src/usj-nodes.css:2963 (.psc-gutter-markers.text-spacing .usfm_ph,)
Detail: The only readers are css:2993 and css:3007-3010. In gutter mode the box starts at
min(var(--verse-text-start), calc(-1 * var(--psc-gutter-width) - var(--para-indent))). For all 30
markers that set --verse-text-start, |text-indent| < margin-left (ph -5/10, iq -15/20, q
-10/15, li -7.5/10, q4 -2.5/15, …), so the second term is always more negative and always wins —
0 of 30 can win the min(), even before the 4em gutter term. Outside gutter mode (css:2993) every
setter is .psc-gutter-markers-scoped, so the variable is unset and falls back to 0px. Nothing in
TypeScript reads it, in this repo or in paranext-core.
This predates the PR (it became inert as soon as the same markers gained --para-indent), but the
PR extends it by four entries and by test:352-361. Either drop the --verse-text-start half, or
note in the comment at css:2911-2914 that it's a fallback for markers whose hanging indent exceeds
their margin — because as written ("::after must start at text-indent to align with the text") it
describes an alignment that never happens.
3. A margin on the RTL side alone is never required — Low
Summary: Expectations are derived only from LTR margin-left, so a marker indented only in RTL
would pass all four tests while its glyph sits in the text.
Attach to: packages/platform/src/usj-nodes.css.gutter-coverage.test.ts:305 (const EXPECTED_PARA_INDENT = needingCompensation()
Detail: EXPECTED_PARA_INDENT comes from resolveBaseValues("margin-left", "ltr"), and
directionAsymmetries (test:293) only iterates markers already in that set, so it can only catch a
marker whose LTR margin exists and whose RTL margin differs — never one that exists in RTL alone.
Demonstrated: adding .text-spacing[dir="rtl"] .usfm_zzr { margin-right: 5vw; } with no gutter entry
passes 4/4, though the RTL glyph rule (css:2780) reads the same --para-indent.
Related, same fix site: the nesting guard runs for margin-left and text-indent (test:326-327)
but never for margin-right, which directionAsymmetries reads through the same flat parser — an
RTL margin inside an @media block is silently invisible to it.
Deriving from the union of LTR margin-left and RTL margin-right markers closes both. Nothing in
the current stylesheet is affected.
4. The usfm.sty oracle is a 6-marker spot check, not drift protection — Low
Summary: The comment claims spec drift is caught, but only 6 of 52 markers are sampled, and a
drift in any of the other 46 passes as long as its compensation drifts with it.
Attach to: packages/platform/src/usj-nodes.css.gutter-coverage.test.ts:36 (the oracle comment block)
Detail: "One marker per distinct value is enough to catch a base rule drifting from the spec;
completeness comes from the derivation" (test:37-38) doesn't hold: the derivation guarantees
base↔gutter consistency, not base↔spec. Demonstrated: changing iq2's base margin from 20vw to 15vw
in both directions and moving its gutter entry to match passes 4/4. The PR description's "a small
usfm.sty oracle so a base value drifting from the spec is caught rather than compensated" reads
the same way.
Nothing is wrong today — all 62 indented markers match usfm.sty. Either reword as a spot check, or
extend the oracle to every marker if drift protection is actually wanted.
5. Tautological assertion — Nit
Summary: This assertion can never fail, because the set it checks is built by a filter that
already removes exactly these markers.
Attach to: packages/platform/src/usj-nodes.css.gutter-coverage.test.ts:338
Detail: EXPECTED_PARA_INDENT comes from needingCompensation (test:211), which drops every
NOT_COMPENSATED marker. Asserting they're absent restates the filter. The useful invariant is that
the exclusion is still load-bearing — expect(resolveBaseValues("margin-left", "ltr").has(marker)).toBe(true)
would fail if a tr* marker ever loses its margin and the exclusion goes stale.
6. Unexplained p assertion — Nit
Summary: A reader can't tell what this line is pinning.
Attach to: packages/platform/src/usj-nodes.css.gutter-coverage.test.ts:337
Detail: expect(EXPECTED_VERSE_TEXT_START.has("p")).toBe(false) checks that a positive
text-indent (p is 2.5vw, css:637) isn't mistaken for a hanging indent — i.e. it's a test of the
isNegativeLength filter. Four words of comment, or a name like p → "positive-indent marker",
would carry it.
7. declarationValue doc explains the wrong guard — Nit
Summary: The stated reason doesn't match the example it's given for.
Attach to: packages/platform/src/usj-nodes.css.gutter-coverage.test.ts:134
Detail: The prefix guard (start / ; / whitespace) is what rejects scroll-margin-left; it's
the trailing \s*: that rejects margin-left-foo, and also what makes var(--para-indent) not
count as a setter.
8. The compensation comment is long, and its last sentence is now incomplete — Nit
Summary: Twelve lines that partly restate the test's own header, ending on a special case the
test now enforces generally — and it names only two of the four markers it applies to.
Attach to: packages/platform/src/usj-nodes.css:2832
Detail: css:2842-2843 calls out pi2/pi3 as same-value-in-both-directions, but lim/lim1
have the identical LTR 15/5 → RTL 5/15 swap (css:1240-1261) and aren't mentioned;
directionAsymmetries now covers the whole class, so the sentence can go. The scope paragraph
(css:2836-2839) repeats test:21-23. Also "every marker its text-spacing rules give a margin-left" is
hard to parse. Something like five lines carries the same content:
/* Indent compensation: each marker's --para-indent must equal its text-spacing margin-left above
(the same value serves RTL margin-right), or the gutter glyph lands in the text.
usj-nodes.css.gutter-coverage.test.ts enforces this for margins set in THIS file; margins a host
injects later (generateUsjCss, consumer commentary sheets) are not compensated.
Table rows (tr, tr1, tr2) need no entry: a real row is a <tr>, not a .para. */9. Test header repeats the stylesheet comment — Nit
Summary: A 22-line, five-paragraph header where the middle section duplicates the CSS comment and
the rationale belongs in the PR description.
Attach to: packages/platform/src/usj-nodes.css.gutter-coverage.test.ts:21 (the Scope: paragraph)
Detail: The Scope paragraph restates css:2836-2839, and "A hand-typed expected list cannot do
that: it silently encodes whatever gap existed when it was written" is PR-description material. The
mechanism paragraph (test:8-12) and the mirroring note (test:25-27) are the parts that earn their
place. Roughly eight lines would cover it.
10. Helpers sit above the describe — Nit
Summary: The file opens with ~250 lines of parser helpers, so the reader meets the machinery
before the thing being tested; the repo convention is top-down.
Attach to: packages/platform/src/usj-nodes.css.gutter-coverage.test.ts:319 (describe()
Detail: Moving the describe block above the helpers was tried and still passes 4/4; function
declarations hoist and the module-level consts are only read inside it callbacks. The consts must
keep their relative order (MARKER_CLASS → css → blocks → EXPECTED_*). The sibling
usj-nodes.css.test.ts keeps everything inside its describe.
Verified good
- All 16
--para-indentvalues match their base rule in both directions; all four newph
--verse-text-startvalues matchtext-indent: -5vw. - No marker appears in two gutter groups with conflicting values; the new selectors (0,3,0) beat the
.psc-gutter-markers .paradefault (0,2,0) at css:2724. - The test fails on
main's stylesheet on exactly the 16 markers, and passes at head (4/4). - Every indented marker in the stylesheet matches
usfm.styLeftMargin/FirstLineIndent × 20 = vw. - Guards demonstrated working: re-added
trentry,margin-inline-startspelling,@media-nested
base rule. - Dropping the
tr/tr1/tr2entries is right:ImmutableTableRowNode.createDOMemits
<tr class="table-row usfm_tr">with noparaclass, and every USJ producer buildstable:row
for\troutside notes. - The
::before→::aftercomment fix is correct — the focus box pseudo is::after(css:2990). - No tracker IDs in code; index-signature style matches the ESLint rule; the
// @vitest-environment nodeheader matches the sibling test. - PR description claims check out: 11 tests (7 existing + 4 new), and "every one of the 16 has a
symmetric RTL margin-right".
Considered and dropped
- Margins outside
.text-spacingaren't required. No marker rule in the file sets a margin
outside.text-spacing, and the compensation is explicitly scoped that way. - A
.parawith markertr. The platform adaptor only warns on an unknown para marker, but no
producer emits{type: "para", marker: "tr"}outside note content. - Parser approximations that today's file can't reach:
:dir()/:not([dir])classification,
first-.usfm_-class attribution, first-vs-last duplicate declaration,!important. Worth knowing,
not worth changing. - Moving the values to a single source of truth (setting
--para-indentwhere the margin is
set). It works only with care — the.psc-gutter-markers .paradefault would have to become a
var(..., 0px)fallback or a:where()rule — and moving--verse-text-startwould change the
non-gutter focus box. It also touches both core SCSS copies and the generators, which is what
PT-4624 already owns.
@katherinejensen00 reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on irahopkinson).
packages/platform/src/usj-nodes.css line 2844 at r1 (raw file):
pi2 and pi3 use the same primary-indent value in both LTR and RTL (10vw and 15vw respectively), so no direction restriction is needed here. */ .psc-gutter-markers.text-spacing .usfm_p2 {
1. Auto-direction projects get the compensation without the margin — Medium
Summary: For a project whose text direction is auto, the editor root gets no dir attribute,
so the [dir]-qualified base margins never apply — but this compensation is direction-agnostic and
applies anyway, pushing the marker glyph out by a margin the paragraph doesn't have.
Attach to: packages/platform/src/usj-nodes.css:2844 (.psc-gutter-markers.text-spacing .usfm_p2 {)
Detail: TextDirectionPlugin.tsx:22 returns early when textDirection === "auto", so
rootElement.dir is never set. The base rules are compound selectors that need dir on the same
element as .text-spacing (e.g. .text-spacing[dir="ltr"] .usfm_iq1, css:559), so under auto
they don't match and the paragraph has no indent. The gutter rules carry no [dir], so
--para-indent still resolves and the glyph moves by up to 20vw (~384px at a 1920px viewport) —
further out than the 4em gutter, i.e. clipped or off-element.
12 of the 16 markers are affected — p2 qd iq iq1 iq2 iq3 qm2 qm3 ph ph1 ph2 ph3 — because their
base margins are [dir]-qualified (css:544, 559, 911, 1317, 1356, 1705, …). The other four (ipq imq ipr psi, css:505/515/527/2045) set margins in direction-agnostic rules and are fine.
For these 12 markers this is a behavior change: on main they had no compensation, which under
auto happened to be right. The mismatch is pre-existing for the already-compensated markers (q*,
li*, lim*, io*, ili*, pi2, pi3), so the question is whether to keep extending it.
Worth noting: directionQualifiedGutterRules (test:270-286) actively forbids the most direct fix
(qualifying the compensation by [dir], which would correctly no-op when dir is absent), and
unreadableMarginSpellings (test:252-267) forbids the other one (logical margin-inline-start on
the base rules, which would resolve correctly under auto). Both bans are reasonable on their own
terms; together they lock in the physical-margin + agnostic-compensation shape. A deliberate "auto
is out of scope, see the known gap in ArrowNavigationPlugin.tsx:292" note would also settle it.
Review follow-up on #10. usj-nodes.css.gutter-coverage.test.ts: - Derive expected --para-indent from the union of LTR margin-left and RTL margin-right, so a marker indented only in RTL is still required to have an entry; directionAsymmetries reports the one-sided margin. - Run the nesting guard for margin-right as well. - Replace the tautological table-row assertion with one that fails if the exclusion goes stale (the tr markers must still carry a margin). - Call the usfm.sty oracle a spot check; the derivation guarantees base-to-gutter consistency, not base-to-spec. - Explain the `p` assertion, correct the declarationValue doc, trim the header, and put the suite above the parser helpers. usj-nodes.css: - Shorter compensation comment; notes that an "auto" text direction is out of scope (TextDirectionPlugin sets no dir attribute for it, see the KNOWN GAP in ArrowNavigationPlugin). - --verse-text-start comment says what it does today: a fallback the focus box reads through min(), never the winner for any marker here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
irahopkinson
left a comment
There was a problem hiding this comment.
Pushed 2ccefd64 addressing all ten. The core twin gets the same changes in paranext/paranext-core#2827, since #2807 has already merged.
- #1 Out-of-scope note for
autoin the compensation comment; replied in the thread with why the shape stays. - #2 Agreed it is inert today. Kept the half rather than deleting it from three copies, and the comment now says what it does: a fallback the focus box reads through
min(), never the winner for any marker in this file. Whether the generators keep emitting it is a PT-4624 question. - #3
resolveInlineStartMarginsderives from the union of LTRmargin-leftand RTLmargin-right;directionAsymmetriescompares both sides for every expected marker and reports a one-sided margin.nestingProblemsalso runs formargin-right. Yourzzrmutation now fails with ".usfm_zzr: LTR margin-left none but RTL margin-right 5vw". - #4 Reworded as a spot check, in the comment, the test name and the PR description.
- #5 Replaced with the load-bearing assertion: each
NOT_COMPENSATEDmarker must still have a basemargin-left. - #6 One-line comment on the
passertion. - #7
declarationValuedoc now credits the prefix guard withscroll-margin-leftand the trailing\s*:withmargin-left-fooandvar(--x). - #8 Your five-line comment, plus the
autonote. - #9 Header trimmed to the mechanism and the mirroring note.
- #10
describesits above the helpers; the consts keep their relative order.
@irahopkinson+AI made 2 comments.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on katherinejensen00).
packages/platform/src/usj-nodes.css line 2844 at r1 (raw file):
Previously, katherinejensen00 wrote…
1. Auto-direction projects get the compensation without the margin — Medium
Summary: For a project whose text direction is
auto, the editor root gets nodirattribute,
so the[dir]-qualified base margins never apply — but this compensation is direction-agnostic and
applies anyway, pushing the marker glyph out by a margin the paragraph doesn't have.Attach to:
packages/platform/src/usj-nodes.css:2844(.psc-gutter-markers.text-spacing .usfm_p2 {)Detail:
TextDirectionPlugin.tsx:22returns early whentextDirection === "auto", so
rootElement.diris never set. The base rules are compound selectors that needdiron the same
element as.text-spacing(e.g..text-spacing[dir="ltr"] .usfm_iq1, css:559), so underauto
they don't match and the paragraph has no indent. The gutter rules carry no[dir], so
--para-indentstill resolves and the glyph moves by up to 20vw (~384px at a 1920px viewport) —
further out than the 4em gutter, i.e. clipped or off-element.12 of the 16 markers are affected —
p2 qd iq iq1 iq2 iq3 qm2 qm3 ph ph1 ph2 ph3— because their
base margins are[dir]-qualified (css:544, 559, 911, 1317, 1356, 1705, …). The other four (ipq imq ipr psi, css:505/515/527/2045) set margins in direction-agnostic rules and are fine.For these 12 markers this is a behavior change: on
mainthey had no compensation, which under
autohappened to be right. The mismatch is pre-existing for the already-compensated markers (q*,
li*,lim*,io*,ili*,pi2,pi3), so the question is whether to keep extending it.Worth noting:
directionQualifiedGutterRules(test:270-286) actively forbids the most direct fix
(qualifying the compensation by[dir], which would correctly no-op whendiris absent), and
unreadableMarginSpellings(test:252-267) forbids the other one (logicalmargin-inline-starton
the base rules, which would resolve correctly underauto). Both bans are reasonable on their own
terms; together they lock in the physical-margin + agnostic-compensation shape. A deliberate "auto
is out of scope, see the known gap inArrowNavigationPlugin.tsx:292" note would also settle it.
Took the third option: a deliberate out-of-scope note in the compensation comment, pointing at the KNOWN GAP in ArrowNavigationPlugin, with no shape change.
Two qualifiers. Platform.Bible cannot reach this case: its platform.textDirection setting is typed 'ltr' | 'rtl' | '' and an empty value falls back to the default, so the host never forwards auto. And the mismatch already applies to the 30 markers compensated before this PR, so keeping the shape consistent is the smaller change. Qualifying the gutter rules by [dir] would fix auto at the cost of two selectors per marker and a second place for the values to drift; if auto becomes a real configuration the right fix is in TextDirectionPlugin (resolve auto to a concrete dir on the root), which makes both the margins and the compensation apply together.
katherinejensen00
left a comment
There was a problem hiding this comment.
Re-review — PR #10: PT-4313 Compensate every indented paragraph marker in the gutter view
For: Ira (PR author) · Re-reviewed at head 2ccefd64 against the prior pass at a65fc748
(see pr-10-review.md)
Files: packages/platform/src/usj-nodes.css, packages/platform/src/usj-nodes.css.gutter-coverage.test.ts
Verdict: one blocker, then merge. The tr1/tr2 exclusion added in a65fc748 is a regression
inside the PR — it leaves two markers uncompensated with exactly the bug the PR fixes. Everything
else is latent or cleanup: nothing in the current stylesheet trips it today.
Baseline re-verified at this head: 11/11 tests pass, Prettier and ESLint clean, dist/index.css
carries no gutter rules (the "no rebuild needed" claim holds), all 16 --para-indent values agree
with the base rules, usfm.sty, and defaultStyleInfo, and no marker appears in two gutter groups.
The companion core branches exist — pr-2807 carries the CSS to both core copies and
origin/pt-4313-coverage-test-review-followup carries the derived test — so the test header's
paranext-core claim is accurate for the post-merge state.
Severity: Blocker = wrong rendering introduced by this PR · High = wrong rendering reachable
today, or a comment that hides a real failure mode · Latent = a guard that cannot fire, provable
only with a probe · Cleanup = reuse/clarity.
Blocker
1. tr1 and tr2 are excluded from compensation on a premise that is false for them
Attach to: packages/platform/src/usj-nodes.css:2836
d3186610 added --para-indent and --verse-text-start entries for tr, tr1 and tr2;
a65fc748 removed all three, on the grounds that "a real row is a <tr>, not a .para". Verified
by running the converter: that holds for \tr only.
usfmFragmentToUsj.ts:69/:1223 matches token.marker === "tr" exactly. So:
\tr \tc1 a→{type:"table:row"}→ a real<tr>, correctly excluded.\tr1 \tc1 a→[{type:"para",marker:"tr1"}, …]→ParaNode.createDOMemits
<p class="para usfm_tr1">.
That element matches the glyph rule at usj-nodes.css:2751 and carries margin-left: 10vw
(css:2016; tr2 is 15vw at css:2029), while NOT_COMPENSATED (test:71) keeps --para-indent at
the 0px default. The glyph lands ~192px inside the text at a 1920px viewport — the bug this PR
fixes.
Fix: restore the tr1/tr2 entries; narrow NOT_COMPENSATED to tr alone.
High
2. var(--para-indent) in the focus-box min() has no fallback
Attach to: packages/platform/src/usj-nodes.css:3006
One line above, var(--verse-text-start, 0px) has one. --para-indent is declared only on
.psc-gutter-markers .para, .psc-gutter-markers .book (css:2724-2729). ImmutableTableNode.createDOM
emits <table class="table"> — no para, no book — and a table is a top-level block root
(usj-editor.adaptor.ts:1279), which is what ActiveTextPlugin applies psc-active-text to
(ActiveTextPlugin.tsx:186).
Put the caret in a table cell with the gutter and focus box on: the var substitution is
guaranteed-invalid, so inset-inline-start is invalid at computed-value time and takes its initial
auto — it does not fall back to the css:2990 declaration. The focus box collapses to a sliver at
the inline-end.
Pre-existing, but this is the rule the PR extends, and the fix is var(--para-indent, 0px).
3. The host-injected-CSS warning was weakened, and now understates the failure
Attach to: packages/platform/src/usj-nodes.css:2834
The delta changed "can move a marker's margin away from the value compensated here" to "are not
compensated". The second reads as benign absence; what actually happens is an override.
generateUsjCss.ts:152-153 emits margin-${rtl ? "right" : "left"}: leftMargin * 20 * zoom vw at
.editor-input.usfm .usfm_<marker> (specificity 0,3,0, DEFAULT_CONTAINER_SELECTOR at :102),
injected after the static sheet — so it beats every base rule (the direction-agnostic ones are 0,2,0;
the [dir]-qualified ones tie at 0,3,0 and lose on order). It never emits --para-indent.
A project whose StyleInfo sets \ph LeftMargin to 0.75" gets margin-left: 15vw while
--para-indent stays 10vw — glyph ~96px into the text at 1920px. Zoom is a second leg: any
zoom ≠ 1 desyncs all 52 compensated markers at once.
"Overridden, so the compensation goes stale by the delta" is the sentence that belongs there.
4. The auto-direction note understates the gap
Attach to: packages/platform/src/usj-nodes.css:2837
The note added for prior finding #1 is accurate as far as it goes, and the
ArrowNavigationPlugin.tsx:292 pointer resolves. But under textDirection="auto" the problem is
larger than a wrong indent.
TextDirectionPlugin.tsx:22 returns before writing rootElement.dir (verified), and Lexical's own
dir writes land on paragraphs, not the .text-spacing ancestor the selectors need. Four gutter
rules are themselves [dir]-qualified — css:2777, 2778, 2806, 2827 — so
.psc-gutter-markers[dir="rtl"] .para > .marker never matches and RTL content keeps the LTR
gutter entirely. Meanwhile 12 of the 16 new markers (p2 qd iq iq1 iq2 iq3 qm2 qm3 ph ph1 ph2 ph3)
still get --para-indent, pushing the glyph up to 20vw + 3.5em past the paragraph edge — off the
editor at a 4em gutter.
Reachable in this repo: scribe's Editor.tsx:230 hard-codes textDirection="auto". A one-line fix
exists at TextDirectionPlugin.tsx:22 (resolve auto to a literal before writing dir), which the
ArrowNavigationPlugin note already identifies as the one-place change. Fine to leave out of scope —
but the note should say the gutter flips sides, not just that the indent is wrong.
5. The compensation comment states a narrower invariant than the test now enforces
Attach to: packages/platform/src/usj-nodes.css:2832
The comment presents margin-left as the trigger, but the delta made the test derive from the
LTR/RTL union and require symmetry. A maintainer who follows the comment and adds
.text-spacing[dir="rtl"] .usfm_zzr { margin-right: 5vw } with no LTR rule gets a red build —
one --para-indent cannot serve both directions (verified) — with nothing in the stylesheet stating
the rule they broke. One clause closes it: "a marker needs the same margin in both directions".
Latent — guards that cannot fire
Each of these was demonstrated with a probe that passes 4/4 while the glyph is visibly wrong. None
is reachable in today's stylesheet.
6. An explicit margin-left: 0 masks a non-zero RTL margin
…gutter-coverage.test.ts:239 — new Map([...rtl, ...ltr]) lets the LTR value win, so
margin-left: 0 + margin-right: 5vw yields "0", isNonZeroLength drops the marker, and
directionAsymmetries (test:344) iterates only expected.keys() so it never inspects it. The RTL
glyph rule at css:2780 then reads the 0px default. Control (same rule without the LTR line) correctly
fails. Fix: run directionAsymmetries over new Set([...ltr.keys(), ...rtl.keys()]). This is the
unfixed half of prior finding #3.
7. A mixed gutter+base selector group hides its base margin
…gutter-coverage.test.ts:185 — isGutterBlock/isBaseBlock .includes() over the whole
comma-separated list, so
.psc-gutter-markers.text-spacing .usfm_p2, .text-spacing .usfm_zzm { margin-left: 5vw; } classifies
entirely as gutter and zzm never enters EXPECTED_PARA_INDENT. The blocks comment at :130-132
promises per-selector classification, but that only holds for directionOf. Every other parser blind
spot has a loud guard; this one does not. Fix: classify per selector the way directionOf does.
8. MARKER_CLASS truncates at hyphens and capitals
…gutter-coverage.test.ts:109 — /\.usfm_([a-z0-9]+)/ turns .usfm_qt-s → qt, both .usfm_ts-s
and .usfm_ts-e → ts (collision, last wins), all four .usfm_zpa-* → zpa, and
.usfm_xtSee/.usfm_xtSeeAlso → xt, which is itself a real class at css:1649. 20 such classes
already exist; harmless only because they all sit under .formatted-font, which isBaseBlock skips.
Add a .text-spacing .usfm_xtSee margin and the test demands compensation on the wrong class, then
reports green. Fix: /\.usfm_([A-Za-z0-9-]+)/ plus a guard that each .usfm_ token matched to
its end.
9. --verse-text-start is derived from LTR text-indent only
…gutter-coverage.test.ts:361 — no RTL union and no asymmetry guard: the exact gap the delta just
closed for margins. It is consumed via inset-inline-start (css:2990, css:3005), which resolves to
right in RTL, so the correct value there is the RTL text-indent. Two probes pass 4/4: an RTL-only
text-indent is never required to have an entry, and an RTL text-indent disagreeing with LTR's is
never reported. Clean today (all 49 setters are direction-agnostic), but the comment at css:2911
claims the test keeps this in step "regardless", which is true only for LTR.
10. The test steers the next edit into breaking the focus box
…gutter-coverage.test.ts:360 — a negative text-indent with no margin makes the test demand a
--verse-text-start entry; supplying exactly that passes 4/4 while --para-indent falls back to 0px,
so min(-10vw, calc(-1 * 4em - 0px)) picks -10vw = 192px against a 64px gutter and starts the focus
box 128px outside the editor. Any |text-indent| ≥ ~3.5vw with no margin wins. The comment at
css:2907-2911 says no marker can win the min() — true today, unenforced, and the test pushes
against it.
11. calc() negatives are invisible; keywords are treated as lengths
…gutter-coverage.test.ts:248 — isNegativeLength/isNonZeroLength string-match raw values, so
text-indent: calc(-1 * 10vw) needs no --verse-text-start — and calc(-1 * …) is this file's own
house style (css:2756, 2780, 3006). In the other direction margin-left: auto makes the test demand
--para-indent: auto, and satisfying it produces an invalid calc(-1 * (4em - 0.5em) - auto). Same
for inherit/unset/revert; uppercase 0VW reads as non-zero.
12. The margin shorthand falls through the gap between two guards
…gutter-coverage.test.ts:26 — nestingProblems runs for margin-left, margin-right and
text-indent only, and unreadableMarginSpellings only inspects blocks that parsed as top-level. So
@media (min-width: 600px) { .text-spacing[dir="ltr"] .usfm_zz { margin: 0 0 0 10vw; } } passes 4/4:
the flat block regex swallows the nested selector, so neither guard sees it. Each guard was proven
individually in the prior review; their intersection is the hole. Fix: add
nestingProblems("margin") and nestingProblems("margin-inline").
13. :dir() bypasses directionQualifiedGutterRules
…gutter-coverage.test.ts:314 — the guard tests for the literal substring [dir=, so
.psc-gutter-markers.text-spacing:dir(ltr) .usfm_zz { --para-indent: 10vw } passes 4/4 while RTL
readers get the 0px default — precisely what the guard's own doc (:302-306) forbids. :dir() is
supported in the shipping Chromium. Note directionOf (:116) and this function use two different
definitions of "direction-qualified".
14. The replacement for the prior tautology still cannot fire
…gutter-coverage.test.ts:41 — prior finding #5's replacement uses Map.has(), which is true for a
zero margin, and resolveBaseValues deliberately keeps zeros. Set .usfm_tr's margin-left to 0
(css:1798) and the test passes 4/4 while the NOT_COMPENSATED exclusion has gone stale and its
comment has become false. Second mismatch: it reads resolveBaseValues("margin-left","ltr") while
EXPECTED_PARA_INDENT now derives from resolveInlineStartMargins(). Fix:
expect(isNonZeroLength(resolveInlineStartMargins().get(marker) ?? "0")).toBe(true).
15. Prettier-wrapped base values produce an unfixable red build
…gutter-coverage.test.ts:180 — selectors are whitespace-collapsed at :135 but declarations never
are, and valueMismatches compares exact strings. Prettier already wraps long values in this file
(css:3004-3007 is a three-line min(...)), so a base margin-left written across three lines can
only be satisfied by a --para-indent carrying the same internal newlines. !important on a base
value fails the same way.
Cleanup
16. The usfm.sty oracle hand-copies data the repo already generates
…gutter-coverage.test.ts:73-91 — USFM_LEFT_MARGIN/USFM_FIRST_LINE_INDENT re-type 11 values with
inch comments, while libs/shared/src/utils/usfm/defaultStyleInfo.ts is generated from that exact
usfm.sty and carries leftMargin/firstLineIndent for 63 markers; generateUsjCss.ts:147-153
already carries the ×20 → vw conversion, and platform already imports defaultStyleInfo from
"shared" elsewhere. Verified: deriving the oracle matches 52/52 EXPECTED_PARA_INDENT and
30/30 EXPECTED_VERSE_TEXT_START with zero diffs — so the six-marker spot check (prior finding
#4) becomes full coverage for free, and pick() plus both tables (19 lines) delete.
Two measured caveats: compare CSS→sty only (phi has a sty leftMargin but no rule here, so the
reverse direction fails); and importing shared into this node-env file took a standalone run from
0.44 s → 17.4 s. If that cost isn't wanted, parsing
tools/usfm-markers/src/generators/markers-data/data/usfm.sty directly is ~8 lines and no import.
17. valueMismatches + unexpectedMarkers re-implement toEqual on a Map
…gutter-coverage.test.ts:270-300 — two ~15-line documented helpers and four call sites exist to
report "expected has a key actual lacks / differs" and the converse, which is what Map equality does
in both directions at once. Verified: expect(ACTUAL_PARA_INDENT).toEqual(EXPECTED_PARA_INDENT)
passes at head, and on a deliberate break Vitest prints the per-key diff naming exactly iq2 and
qd. Replacing them deletes ~35 lines, the two single-use ACTUAL_* consts, and the
prettier-wrapped four-line assertions at :50-52 and :61-63.
18. The base maps are re-resolved three to six times, and one documented branch is dead
…gutter-coverage.test.ts:236-240 — resolveBaseValues walks the whole stylesheet and is called six
times, three of them the identical ("margin-left","ltr"). And resolveInlineStartMargins's ten-line
doc explains a preference whose else-branch can never survive: any marker reaching it has
ltr === undefined !== rtl, which directionAsymmetries fails first. Hoisting BASE_MARGIN_LTR /
BASE_MARGIN_RTL consts and inlining the union deletes one function, its doc block, and three walks.
19. The two halves of the gutter block group differently
packages/platform/src/usj-nodes.css:2912-2965 — the --para-indent half is one rule per distinct
value, and the PR correctly inserted ph* into the existing groups. The --verse-text-start half
is 11 rules for 5 values, and the PR added a 12th (ph* → -5vw at :2960-2965) directly below
iq3 → -5vw at :2957-2959 — adjacent, identical value, two rules. Duplicates: -15vw (:2925, :2950),
-10vw (:2912, :2929, :2954), -7.5vw (:2916, :2935), -5vw (:2919, :2932, :2957, :2960). Merging
by value takes 11 → 5 and makes both halves read the same way. Related: the --para-indent 5vw
group sits last (:2890-2905), so the section reads 2.5, 10, 15, 20, 25, 30, 5 — and the PR touched
both ends of that ordering.
20. The vacuity thresholds are invented numbers
…gutter-coverage.test.ts:23-24 — toBeGreaterThan(40)/(20) guard against "the parser read
nothing" while the real sizes are 52 and 30. The invariant is emptiness, so toBeGreaterThan(0) says
what is meant; if the counts are meant to be pinned, toBe(52)/toBe(30) says that instead.
21. The stylesheet-read preamble is duplicated with the sibling test
…gutter-coverage.test.ts:1-5, :122-125 vs usj-nodes.css.test.ts:1-5, :18 — the
// @vitest-environment node / jsdom-import.meta.url comment is byte-identical, as is the
readFileSync(new URL(...)) line. These are the only two files in the workspace that read a
stylesheet from disk. Platform already has the *.test-helpers.ts convention and already excludes
!src/**/*.test-helpers.* from published files.
22. Tests 3 and 4 are the same four-assertion shape
…gutter-coverage.test.ts:44-64 — both run nestingProblems, directionQualifiedGutterRules, then
the two comparison helpers, differing only in the (property, actual, expected) triple — while test 1
already exists as the "can the parser see the file" test and holds the other three nestingProblems
calls. Moving all five nestingProblems and both directionQualifiedGutterRules calls into test 1
leaves tests 3 and 4 as one comparison assertion each, with no it.each table needed.
One call for the author
The two halves of this review point opposite ways on the parser. The correctness pass demonstrated
nine blind spots that a real parser would close at a stroke — postcss, css-tree and lightningcss are
all already resolved in node_modules (postcss is a direct dep of scribe and of vite), so adding one
to platform's devDependencies costs no new lockfile entry, and root.walkRules/rule.parent would
delete the comment strip, the flat block regex and its caveat, braceDepthAt, nestingProblems, and
the declarationValue prefix subtleties. Against that: paranext-core's mirror
(extensions/src/platform-scripture-editor/src/usj-nodes-scss-coverage.test.ts) is the same
hand-rolled parser near-verbatim — braceDepthAt and nestingProblems especially — so divergence has
a real cross-repo cost, and dependency-freedom here looks deliberate. Your call, not the reviewer's.
Status of the prior review's findings
| Prior | Status |
|---|---|
| #1 auto-direction (Medium) | Settled by decision — note added, accurate, but understates the gap (see #4 above) |
#2 --verse-text-start inert |
Addressed — comment rewritten; claim verified true at every viewport |
| #3 RTL-only margin | Half-fixed — RTL-only now fails loudly; margin-left: 0 variant still silent (see #6) |
| #4 oracle is a spot check | Addressed — reworded (and #16 offers full coverage for free) |
| #5 tautological assertion | Half-fixed — replacement still cannot fire (see #14) |
#6 unexplained p assertion |
Addressed — commented |
#7 declarationValue doc |
Addressed — correctly attributed |
| #8 / #9 long comments | Addressed — shortened |
#10 helpers above describe |
Addressed — describe moved up; TDZ verified safe empirically |
Verified good at this head
- 11/11 tests pass; Prettier and ESLint clean.
dist/index.csshas no gutter rules — the "no rebuild needed" claim holds.- All 16
--para-indentvalues check out three ways: against the base rules, againstusfm.sty, and
againstdefaultStyleInfo. - No duplicate or conflicting gutter entries; every entry has a matching base rule.
- Companion core branches exist (
pr-2807,origin/pt-4313-coverage-test-review-followup), so the
test header's paranext-core claim is accurate post-merge. - No dead values left behind in the gutter block.
Below the cut
…gutter-coverage.test.ts:40puts a setupconstbetween assertions; the convention here is
setup / blank / SUT / blank / checks.- The
usfm.styoracle comment says "a drift in one of these six" while 11 values across 8 markers
are pinned. braceDepthAtis the one superlinear term (≈246 full-file passes, 4.5 ms of a 12 ms suite) and is
duplicated verbatim in paranext-core.- No existing CSS-parsing helper in
libs/shared,libs/shared-reactorpackages/utilitiesto
reuse;generateUsjCssruns the opposite direction and cannot supply the static sheet's values.
@katherinejensen00 reviewed 2 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on irahopkinson).
packages/platform/src/usj-nodes.css line 2836 at r2 (raw file):
usj-nodes.css.gutter-coverage.test.ts enforces this for margins set in THIS file; margins a host injects later (generateUsjCss, consumer commentary sheets) are not compensated. Table rows (tr, tr1, tr2) need no entry: a real row is a <tr>, not a .para.
Blocker
1. tr1 and tr2 are excluded from compensation on a premise that is false for them
Attach to: packages/platform/src/usj-nodes.css:2836
d3186610 added --para-indent and --verse-text-start entries for tr, tr1 and tr2;
a65fc748 removed all three, on the grounds that "a real row is a <tr>, not a .para". Verified
by running the converter: that holds for \tr only.
usfmFragmentToUsj.ts:69/:1223 matches token.marker === "tr" exactly. So:
\tr \tc1 a→{type:"table:row"}→ a real<tr>, correctly excluded.\tr1 \tc1 a→[{type:"para",marker:"tr1"}, …]→ParaNode.createDOMemits
<p class="para usfm_tr1">.
That element matches the glyph rule at usj-nodes.css:2751 and carries margin-left: 10vw
(css:2016; tr2 is 15vw at css:2029), while NOT_COMPENSATED (test:71) keeps --para-indent at
the 0px default. The glyph lands ~192px inside the text at a 1920px viewport — the bug this PR
fixes.
Fix: restore the tr1/tr2 entries; narrow NOT_COMPENSATED to tr alone.
Add --para-indent (and --verse-text-start where the indent is hanging) for the 19 paragraph markers whose text-spacing margin had no gutter compensation: ipq imq ipr psi qd p2 iq iq1 iq2 iq3 qm2 qm3 ph ph1 ph2 ph3 tr tr1 tr2. Without it the marker glyph sat at the paragraph's margin edge instead of in the gutter and, for the hanging-indent markers, was painted over the first line of text at every column width. The tr/tr1/tr2 entries are inert for real tables, which render as <tr> rather than .para; they are kept so the block stays complete. Add usj-nodes.css.gutter-coverage.test.ts, which derives the expected gutter values from the base text-spacing rules in the same file, so a marker that gains a margin without compensation fails by construction rather than depending on a hand-maintained list. The committed dist is unchanged: usj-nodes.css is not part of the bundle, consumers copy the source file. paranext-core carries the same change on its copies. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review follow-up on #10 (mirrors the paranext-core review of the same change). usj-nodes.css: - Remove the tr/tr1/tr2 gutter entries. A table row is a <tr> (ImmutableTableRowNode), never a .para, so the glyph rule cannot match them; the test excludes them from the derivation instead. - Scope the compensation comment to margins set in this file and name what it does not cover (host-injected generateUsjCss output, consumer commentary stylesheets); fix ::before -> ::after. usj-nodes.css.gutter-coverage.test.ts: - Resolve the LTR-over-agnostic cascade before filtering zero values, so a directional zero override can beat an agnostic non-zero margin. - Classify direction per comma-separated selector, not per selector list. - Fail loudly on a margin shorthand or margin-inline-* on a marker rule, on nested margin-left/text-indent setters, and on an LTR/RTL margin mismatch (one --para-indent serves both directions). - Add a small usfm.sty oracle so a base value drifting from the spec is caught rather than compensated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review follow-up on #10. usj-nodes.css.gutter-coverage.test.ts: - Derive expected --para-indent from the union of LTR margin-left and RTL margin-right, so a marker indented only in RTL is still required to have an entry; directionAsymmetries reports the one-sided margin. - Run the nesting guard for margin-right as well. - Replace the tautological table-row assertion with one that fails if the exclusion goes stale (the tr markers must still carry a margin). - Call the usfm.sty oracle a spot check; the derivation guarantees base-to-gutter consistency, not base-to-spec. - Explain the `p` assertion, correct the declarationValue doc, trim the header, and put the suite above the parser helpers. usj-nodes.css: - Shorter compensation comment; notes that an "auto" text direction is out of scope (TextDirectionPlugin sets no dir attribute for it, see the KNOWN GAP in ArrowNavigationPlugin). - --verse-text-start comment says what it does today: a fallback the focus box reads through min(), never the winner for any marker here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Second review round on #10. usj-nodes.css: - Restore the tr1 and tr2 gutter entries. The converter matches the table-row marker as exactly "tr", so the obsolete tr1 and tr2 become plain paragraphs with a 10vw/15vw margin and need compensation; only tr renders as a <tr>. - Give var(--para-indent) a 0px fallback in the focus-box min(), so a caret in a table cell (a block root with no .para ancestor) does not invalidate inset-inline-start. - Comment now says host-injected margins override these and leave the compensation stale by the difference, that a marker needs the same margin in both directions, and that under an "auto" text direction RTL content keeps the LTR gutter. usj-nodes.css.gutter-coverage.test.ts: - Parse with postcss instead of a flat regex. Rules are classified per selector, nesting is read from the tree, and declarations come from the parser, which closes the mixed-selector-group, nested-shorthand, wrapped-value and !important blind spots the review probed. - Derive from the union of LTR and RTL margins and compare both sides for every marker, so a one-sided or zero-masked margin is reported. - Marker classes may carry hyphens and capitals; a class the regex cannot read whole throws instead of matching a shorter marker. - Report keyword and calc() values as unreadable rather than deriving from them; treat :dir() as direction-qualified like [dir=]. - A hanging indent on a marker with no margin fails, since it would win the focus-box min() and start the box outside the editor. - Map equality replaces the two hand-written diff helpers; the table-row exclusion is checked against a non-zero margin. postcss is added as a platform devDependency at the version already resolved in the workspace. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2ccefd6 to
e34511f
Compare
irahopkinson
left a comment
There was a problem hiding this comment.
Committed locally as e34511f1, on the branch rebased to main. The core twin gets the same changes on paranext/paranext-core#2827.
The call on the parser: postcss. The nine latent probes made the case. postcss is added as a platform devDependency at the version already resolved in the workspace (one importer entry in the lockfile, no new package), and the core twin uses postcss plus postcss-scss, both already in that repo. The comment strip, the flat block regex, braceDepthAt, nestingProblems and the declarationValue prefix logic are gone; rules are classified per selector and nesting is read from the tree.
Blocker
- #1
tr1/tr2restored, exclusion narrowed totr. Replied in the thread; the markers are OBSOLETE inusfm.sty, which is why the earlier removal looked right, but the stylesheet styles them so they are compensated.
High
- #2
var(--para-indent, 0px)in the focus-boxmin(). - #3 "override these and leave the compensation stale by the difference".
- #4 The note now says the
[dir]-qualified gutter rules never match underauto, so RTL content keeps the LTR gutter. - #5 "a marker needs the same margin in both directions".
Latent
- #6
directionAsymmetriesruns over the union of LTR and RTL keys and treats a missing side as0, somargin-left: 0againstmargin-right: 5vwis reported. - #7 Closed by the parser: each selector in a list is classified on its own.
- #8
markerOfreads[A-Za-z0-9-]+to the end of the class and throws on a.usfm_it cannot read whole. - #9
--verse-text-startnow has the same LTR/RTL asymmetry check as margins; RTL-only or disagreeingtext-indentis reported. - #10 A hanging indent on a marker with no margin fails: every
EXPECTED_VERSE_TEXT_STARTkey must be inEXPECTED_PARA_INDENT. - #11 Keyword and
calc()values are reported as unreadable rather than derived from (unreadableDeclarations), and zero matching is case-insensitive. Evaluatingcalc()is a stated limit in the header. - #12 Closed by the parser: any tracked or shorthand property inside a nested rule is reported, whatever the property.
- #13
:dir(ltr)/:dir(rtl)classify like[dir=…], and a gutter rule qualified either way is reported. - #14 The exclusion check is
isNonZeroLength(BASE_INLINE_START_MARGIN.get(marker)). - #15 Declaration values are whitespace-collapsed from the parser, and
!importanton a tracked property is reported.
Cleanup
- #17
expect(ACTUAL).toEqual(EXPECTED)on the Maps;valueMismatchesandunexpectedMarkersare gone. - #18
resolveInlineStartMarginsis gone; the union is built once asBASE_INLINE_START_MARGIN. - #20
toBeGreaterThan(0). - #22 All the parser-readability checks sit in test 1; tests 3 and 4 are asymmetry plus one Map comparison each.
- #16 Declined. Importing
sharedtook the file from under a second to 17 seconds by your measurement, and core has nousfm.styto parse, so the twins would diverge. The spot check stays and its comment says what it is. - #19, #21 Declined as out of scope for a reviewed PR: regrouping the
--verse-text-starthalf and extracting a shared read helper are both fine follow-ups.
Mutation probes rerun at this head, each failing as intended: RTL-only margin, margin-left: 0 masking an RTL margin, a mixed gutter+base selector group, a hyphenated marker margin, a margin shorthand inside @media, a :dir() gutter rule, a calc() text-indent, and a removed tr1 entry. 13/13 tests across both stylesheet suites, Prettier and ESLint clean.
@irahopkinson+AI made 2 comments.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on katherinejensen00).
packages/platform/src/usj-nodes.css line 2836 at r2 (raw file):
Previously, katherinejensen00 wrote…
Blocker
1.
tr1andtr2are excluded from compensation on a premise that is false for themAttach to:
packages/platform/src/usj-nodes.css:2836
d3186610added--para-indentand--verse-text-startentries fortr,tr1andtr2;
a65fc748removed all three, on the grounds that "a real row is a<tr>, not a.para". Verified
by running the converter: that holds for\tronly.
usfmFragmentToUsj.ts:69/:1223matchestoken.marker === "tr"exactly. So:
\tr \tc1 a→{type:"table:row"}→ a real<tr>, correctly excluded.\tr1 \tc1 a→[{type:"para",marker:"tr1"}, …]→ParaNode.createDOMemits
<p class="para usfm_tr1">.That element matches the glyph rule at
usj-nodes.css:2751and carriesmargin-left: 10vw
(css:2016;tr2is 15vw at css:2029), whileNOT_COMPENSATED(test:71) keeps--para-indentat
the 0px default. The glyph lands ~192px inside the text at a 1920px viewport — the bug this PR
fixes.Fix: restore the
tr1/tr2entries; narrowNOT_COMPENSATEDtotralone.
Confirmed: usfmFragmentToUsj matches the table-row marker as exactly "tr", so \tr1 and \tr2 come through as para nodes. One more fact worth recording, since it is why the earlier removal looked right: both markers are flagged OBSOLETE in usfm.sty and are absent from USFM 3, so this is a legacy-input case rather than a live one. usfm.sty still gives them 0.5in and 0.75in left margins and defaultStyleInfo carries the same, so Paratext accepts them and we style them; that settles it.
Restored in e34511f1: tr1 in the 10vw group, tr2 in the 15vw group, both in the -5vw --verse-text-start group; NOT_COMPENSATED is tr alone, and the comment says why the obsolete pair is compensated while \tr is not. This reverses #2807 finding 5 for those two markers; the core copies get the same change on paranext/paranext-core#2827.
katherinejensen00
left a comment
There was a problem hiding this comment.
Looks good. Thanks, Ira!
@katherinejensen00 reviewed 4 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on irahopkinson).
Summary
In the gutter view (
.psc-gutter-markers), each paragraph's marker glyph is positioned atleft: calc(-(gutter width) + 0.5em - var(--para-indent)), so--para-indentmust equal the paragraph's text-spacing margin, and the active focus box starts atvar(--verse-text-start), which must equal a hanging paragraph's negativetext-indent.Eighteen paragraph markers had a text-spacing margin but no compensation:
ipq imq ipr psi qd p2 iq iq1 iq2 iq3 qm2 qm3 ph ph1 ph2 ph3 tr1 tr2. The last two are the obsolete table-row levels fromusfm.sty; the converter turns only\trinto a real<tr>, so\tr1and\tr2render as plain paragraphs and are compensated like any other. Their glyph sat at the paragraph's margin edge instead of in the gutter and, for the ten hanging-indent markers, was painted over the first line of text at every column width. This is the same gap that was closed forli/limin #518, for the rest of the indented markers.Changes
packages/platform/src/usj-nodes.css: add--para-indentfor the 18 markers (2.5vw for p2; 5vw for ipq imq ipr psi qd; 10vw for ph ph1 tr1; 15vw for ph2 tr2; 20vw for iq iq1 iq2 iq3 qm2 qm3 ph3) and--verse-text-start: -5vwforph ph1 ph2 ph3 tr1 tr2. Every value equals the marker's base LTRmargin-left/text-indent; every one of the 16 has a symmetric RTLmargin-right, so the direction-agnostic variables are correct in both directions. Only\trhas no entry: a real row is a<tr>, not a.para, so the glyph rule never matches it, and the test excludes that one marker from the derivation.packages/platform/src/usj-nodes.css.gutter-coverage.test.ts(new): derives the expected--para-indentand--verse-text-startfor every marker from the base text-spacing rules in the same file and checks the gutter block against it, both directions (missing and stale entries). Againstmain's stylesheet it fails on exactly these 18 markers; on this branch it passes. It also guards its parser: a vacuity check on derived-list sizes, a brace-depth check for nested rules, a check that no gutter rule is direction-qualified, a loud failure on amarginshorthand or logicalmargin-inline-*on a marker rule, an LTR/RTL margin symmetry check, and a smallusfm.styspot check on six markers.Review follow-ups
a65fc748mirrors the Reviewable pass on paranext/paranext-core#2807: thetr,tr1,tr2entries were removed (a table row is a<tr>, never a.para, so the glyph rule cannot match them), the compensation comment is scoped to margins set in this file, and the test gained the cascade, per-selector direction, shorthand, symmetry, and oracle checks listed above. Margins a host injects later in source order (generateUsjCssoutput, consumer commentary stylesheets) are not compensated by this file; that follow-up is paranext-core PT-4624, which moves the compensation into the generators.2ccefd64answers the review of this PR: expected--para-indentis derived from the union of LTRmargin-leftand RTLmargin-right(a marker indented only in RTL now fails), the nesting guard coversmargin-right, the table-row assertion checks the exclusion is still load-bearing, theusfm.styoracle is described as the spot check it is, and the compensation comment notes that anautotext direction is out of scope. paranext-core carries the same test changes in a separate follow-up PR.e34511f1answers the second round. The test now parses withpostcss(added as a platform devDependency at the version already resolved in the workspace) instead of a flat regex, which closes the parser blind spots the review probed: selector lists mixing gutter and base selectors, amarginshorthand inside an at-rule, wrapped values and!important. It also derives from the union of LTR and RTL margins for both--para-indentand--verse-text-start, reads hyphenated and capitalised marker classes whole, reports keyword andcalc()values as unreadable, treats:dir()as direction-qualified, and fails a hanging indent on a marker with no margin. Thetr1/tr2entries removed ina65fc748are restored (see above), and the focus-boxmin()gains a0pxfallback for--para-indentso a caret in a table cell does not invalidate it.Committed build output
Unchanged on purpose:
usj-nodes.cssis not part of the Vite bundle (dist/index.csscontains no gutter rules); consumers copy the source file.pnpm rebuild-committed-outputwas run and produced no diff.Verification
pnpm vitest run usj-nodesinpackages/platform: 13/13 across both stylesheet tests. Eight mutation probes (RTL-only margin, zero-masked margin, mixed selector group, hyphenated class, nested shorthand,:dir()gutter rule,calc()indent, removedtr1entry) each fail as intended.\iq1gap went from -38.5px to +20.8px at the 297px column floor; RTL confirmed on zzzz14 Psalm 1 (\q1, gutter on the right, gaps +72.5 / +53.9px).Screenshots
Before, LTR, 297px column floor, P_HBKENG Job introduction. The
\iq1and\iq2glyphs are painted over the first line of text.After, same view. The glyphs sit in the gutter (x 26 to 62) with gaps of +20.8px and +35.7px.
RTL confirmation, zzzz14 (Aramaic, Syriac script) Psalm 1, 958px. Gutter on the right,
\q1glyphs at x 896 to 932, gaps +72.5px and +53.9px. This project has none of the 16 fixed markers, so this shows the mechanism for a hanging-indent marker in RTL rather than the fix.Companion PR
paranext/paranext-core#2807 (paranext/paranext-core#2807) carries the same change on both of core's hand-maintained copies, with the same derived test run over both. After this merges,
platform-yalcneeds moving to include it for core consumption.Fixes PT-4313.
AI-assisted — Claude Fable 5.1, local Claude Code session (no session URL)
🤖 Generated with Claude Code
This change is