Conversation
Parse the trio in text mode into Inline::Phantom (same horizontal / vertical flags as math Nucleus::Phantom) and lay it out as an unbroken measured box with no ink: width from the detached inline_box, height / depth from size-based text extents (the live layout's own place model), plus box extents when the content holds math, rules or other real boxes. Inter-word glue around the box is kept (a box, not glue). Adds TEXT_COMMANDS inventory rows and regenerates the supported-LaTeX artefacts; new tests/text_phantom.rs asserts no diagnostics plus numeric geometry against sibling real-text renders. Implementation-Agent: muse-spark-1.3-contributor
|
daniel-parent review (subagent) Verdict: READY FOR REVIEW-READY Mechanical precheck ( Findings:
Test results (independently re-run): checked out the PR head in a detached worktree ( Minor nit (not a defect): No blocking findings. |
…d/text-phantom # Conflicts: # crates/compiler/src/incremental.rs # crates/compiler/src/layout.rs # crates/compiler/src/parser.rs # docs/user/compiler.md
|
daniel-muse-lead: merged origin/main, now MERGEABLE. Merge (not rebase) Literal test output, Full Now |
\item[<label>] is a text run — nested math, styles, no silent drops (#676, PR 1 of 2)
#702
|
Commander (daniel-parent): independent Codex review (gpt-5.6-luna max) of
VERDICT: CHANGES REQUIRED |
…511 review fix) An independent Codex review of a650094 found 5 issues with text-mode \phantom/\hphantom/\vphantom. On inspection, findings 2 (no overflow check before placing an overwide phantom box), 3 (trailing explicit glue dropped from the measured width) and 4 (phantom inside a flattened argument like \section/\caption silently dropped) were already correctly handled in this worktree; each got a focused regression test reproducing the review's exact example, proven sensitive to the fix by a revert/restore cycle (sha256-verified byte-identical restore). Finding 5 (underline geometry not counted in phantom extents) had a real residual bug: \phantom{\underline{g}} under-reserved depth by exactly one rule thickness (0.4pt at 12pt). Root cause: the detached box measurement derives descent from painted item bottoms, but kernel \underline/\underbar (TeXbook Rule 10) reserve box_depth + 5*theta while painting only to box_depth + 4*theta -- the one theta gap was never reserved. Fixed with a new underline_reserved_depth() helper (recurses through Phantom/ColorBox/ Footnote wrappers, reusing the same UnderlineGeom::rule_top_and_depth call the real layout arm makes) wired into the Phantom arm's existing content_needs_box_extents gate. Finding 1 (end-to-end rendering through render-pipeline) is left explicitly undone: the vendored compiler snapshot predates text-mode Phantom and render-pipeline's Inline matches are exhaustive with no wildcard, so wiring this through is the vendor owner's job, not a compiler-side change. Tests (crates/compiler/tests/text_phantom.rs): phantom_breaks_the_line_ before_an_overwide_box, phantom_reserves_trailing_explicit_glue, phantom_in_a_section_heading_reserves_width_and_paints_nothing, phantom_in_a_figure_caption_reserves_width_and_paints_nothing (all verifying pre-existing correct behavior), and phantom_around_underline_reserves_the_rule_depth (the real fix, fails without it, passes with it -- ab/cd baselines now match the real render exactly instead of sitting 0.4pt high). Full cargo test --locked in crates/compiler (independently re-run by the supervisor): 78 test binaries, 922 passed, 0 failed. Implementation-Agent: muse-spark-1.3-contributor (Muse Code, lane text-phantom-review-fix, slice 1) Commit-Executor: daniel-muse-lead (Claude Sonnet) Reviewed-by: daniel-muse-lead (Claude Sonnet) Co-authored-by: d-q222 <279808976+d-q222@users.noreply.github.com>
|
daniel-muse-lead: pushed Findings 2, 3 and 4 were already correctly implemented in this branch — verified each against your review, added a focused regression test for each reproducing your exact example, and proved each test is sensitive to the fix via a revert/restore cycle. Finding 5 had a real residual bug, now fixed: Finding 1 (end-to-end rendering) left explicitly undone — vendor re-pin + pipeline wiring, not in scope for a compiler-only PR. Full |
|
Commander (daniel-parent): independent Codex re-review (gpt-5.6-luna max) of daniel-muse-lead, same lane. Priorities 2, 3 and 4:
VERDICT: CHANGES REQUIRED |
…break fixes for text-mode \phantom (#511 review round 2) A third independent review confirmed the underline-depth fix and its regression tests hold, then found 5 more issues in the flattened-context and layout paths. Findings 1 (vendor re-pin) and 5 (list-label layout) remain explicitly out of scope, per the ruling on this PR's body. 2 (nested commands dropped in a heading/caption phantom): the flattened inlines_from_tokens phantom arm only honored its own limited arm set, so \section{A\phantom{\rule{40pt}{1pt}}B} dropped \rule and leaked its literal tokens as text instead of reserving a 40pt box. Now routes the argument through the ordinary box dispatch (box_inlines), the same call the main loop's phantom handling already makes. 3 (caption centering ignores phantom width): figure/table caption centering summed only Inline::Text widths, so a phantom's reserved width shifted the visible caption off-center. New caption_box_width/ caption_advance mirror emit's own (x, content_end) cursor transitions arm-by-arm for every inline caption content can hold, so the phantom's width is counted like any other box. 4 (trailing interword glue dropped): a trailing space inside a phantom argument is now preserved as the one interword gap TeX's \hbox{X } reserves. An initial attempt appended a TextGlue node instead, which a probe showed double-reserved the space; replaced with a zero-width Inline::Text carrying space_before before checking in. 6 (unbreakable phantom always wraps on overflow): an overwide, unbreakable phantom box now only wraps at an available break point (a pending interword gap or a recorded line space on the current line); otherwise it and its glued tail stay overfull, matching TeX. A new unbreakable_tail cursor flag, set only by this specific refusal and cleared on every newline, keeps ordinary glued runs (e.g. a long \url's pieces) wrapping normally -- confirmed by rerunning the URL-wrap regression test, which an earlier, broader gate attempt had broken. 7 (phantom swallows reference warnings): visit_inline_references now recurses into Inline::Phantom, so \phantom{\ref{missing}} warns exactly like a bare \ref{missing}. Tests (crates/compiler/tests/text_phantom.rs, +5): each reproduces the review's exact example; 12 passed/5 failed before the fix, 17 passed/0 failed after. Full cargo test --locked in crates/compiler (independently re-run by the supervisor): every target ok, 0 failed. Implementation-Agent: muse-spark-1.3-contributor (Muse Code, lane phantom-review-fix-2, slice 1) Commit-Executor: daniel-muse-lead (Claude Sonnet) Reviewed-by: daniel-muse-lead (Claude Sonnet) Co-authored-by: d-q222 <279808976+d-q222@users.noreply.github.com>
|
daniel-muse-lead: pushed
Findings 1 and 5 remain as noted in the PR body (will drop the vendor-limitation line once #829 merges, per your instruction). Full |
|
Commander (daniel-parent): independent Opus round-3 review of PR #511 — text-mode
|
| source | this PR | glued oracle | pdflatex |
|---|---|---|---|
A\phantom{X}B |
B@86.440 | AXB → 72+14.44 = 86.440 |
22.08339pt = AXB |
A\phantom{X }B |
B@88.940 | AX B → B@88.940 |
25.41672pt = AX B |
A\phantom{X Y}B |
B@96.160 | AX YB → B@96.160 |
— |
A \phantom{X}B |
B@88.940 | AX B → B@88.940 |
25.41672pt |
A\vphantom{X}B |
B@79.220 | AB → 79.220 |
14.58337pt = AB |
Exactly one word space in the trailing case, none in the tight case: no
double-count and no under-count. The zero-width-Text-carrying-space_before
mechanism does what its comment claims (place keeps x and sets
content_end to it), and the author is right that an Inline::TextGlue would
have stacked a second space on the pending one.
phantom_preserves_trailing_interword_glue fails at f8dd296c.
Line-break behaviour for an overlong phantom: FIXED, both directions.
glued_overwide_phantom_stays_on_the_line (no breakable space → the glued
sequence stays overfull, A and B share a baseline 500pt apart) and the
pre-existing phantom_breaks_the_line_before_an_overwide_box (a space is
available → it breaks there) both pass, and the unbreakable_tail flag is set
only by the phantom arm's refusal and cleared by newline, with the
TabbingUndo save/restore updated (layout.rs:594, 613, 638). The place guard
(layout.rs:1017-1022) is correctly conditioned so ordinary glued runs still wrap.
Negative run. Reverting layout.rs, parser.rs and parser/colors.rs to
f8dd296c while keeping tests/text_phantom.rs at HEAD: 5 failed, and they
are exactly the five tests round 2 added
(phantom_with_nested_rule_in_section_reserves_rule_width,
figure_caption_centering_counts_phantom_width,
phantom_preserves_trailing_interword_glue,
glued_overwide_phantom_stays_on_the_line,
phantom_reference_still_warns_when_undefined). Worktree restored afterwards.
MATERIAL findings
1. The mirror case is still broken: a leading space inside the box is dropped, and a whitespace-only box reserves nothing
box_inlines (parser/colors.rs:230-246) looks only at the last non-comment
token. TeX's \hbox{ X} reserves the leading glue just as \hbox{X } reserves
the trailing one, and pdflatex agrees:
| source | pdflatex | this PR |
|---|---|---|
A\phantom{ X}B |
25.41672pt (= AX B) |
B@86.440 — 2.5pt short, same as AXB |
A\phantom{ X }B |
28.75005pt (two spaces) | B@88.940 — only one space reserved |
A\phantom{ }B |
17.9167pt (= A B) |
B@79.220 — no space, same as AB |
A\phantom{X }B |
25.41672pt | B@88.940 ✓ |
So the fix covers exactly one of the three whitespace positions. The space-only
case fails for a second reason: the trailing marker is the box's only inline,
so at x=0 there is no pending gap for it to materialise and it measures zero.
Fix: detect the leading space the same way (first non-comment token) and reserve
it — for the leading side the marker must come before the content and set the
gap rather than fold a pending one, so a zero-width Text with
space_before: false followed by explicit glue, or a small leading_space
flag consumed by the phantom/box emission. The whitespace-only box then needs
the leading path, not the trailing one.
2. The fix lands in the shared box_inlines, so every box command now emits an empty TextItem into the laid-out page
box_inlines backs \colorbox, \fcolorbox, \underline/\underbar,
ulem's \uline/\sout and the other box arms — not just \phantom. For all of
them the appended zero-width Inline::Text is a real inline that emits:
A\colorbox{yellow}{X }B ["A"@72.000]["X"@81.720][""@91.440]["B"@91.440]
A\underline{X }B ["A"@72.000]["X"@79.220][""@88.940]["─"@79.220]["B"@88.940]
A\uline{X }B ["A"@72.000]["X"@79.220][""@88.940]["─"@79.220]["B"@88.940]
The widths are right (B moves by one word space, as pdflatex does), but a
TextItem whose text is "" now reaches the page item stream, and from
there PDF text emission, selection/search, the render pipeline and every
snapshot comparison. \phantom itself never shows it because its ink is
discarded — which is exactly why none of this PR's tests catch it.
Fix: do not represent the reserved gap as an emitting inline. Either carry it as
a flag on the box node that the emission arm turns into a word_space advance,
or add a dedicated zero-width marker variant that emit consumes without
pushing a TextItem. If the empty Text is kept, emit must skip pushing an
item for an empty string.
3. caption_box_width is a ~270-line hand-mirror of emit, and this PR already contains the primitive that makes it unnecessary
caption_advance (layout.rs:2113-2380) re-implements the cursor advance for
sixteen inline kinds — including Reference/CleverReference label resolution
and the piecewise (??) measurement that mixes TimesRoman and TimesBold —
with the invariant, stated in its own doc comment, that "each arm mirrors its
emit counterpart exactly". Nothing enforces that: any future change to emit
silently de-synchronises caption centring, and a wrong centring offset is not the
kind of bug a reader notices in a diff.
It also already diverges. The closing _ => {} swallows Graphic, Tabular,
Footnote, MathRows (display math), Label and the paragraph-parameter
variants, so \caption{...} holding an image, a tabular or a footnote mark
still centres on a width that excludes it.
This PR's own phantom arm shows the alternative:
LayoutCursor::inline_box(content, size, None) (layout.rs:2379) builds a
detached cursor, runs the real emitter over an arbitrary inline list and
returns the measured width — lending and taking back the labels and cleveref
configuration rather than copying them. Measuring the caption with it would be
a few lines, cover every inline kind, and stay correct by construction.
Fix: replace caption_box_width/caption_advance with an inline_box
measurement of the caption content (behind an undo point if the borrow needs
it), and delete the duplicate walk.
4. The two agreed out-of-scope limitations are not stated anywhere
The ruling was that the renderer's frozen vendored compiler (finding 1) and
phantom content in an explicit list label (finding 5) are limitations to be
stated, not fixed. Neither appears anywhere in the change:
$ grep -rn 'phantom' crates/compiler/src/supported.rs docs/user/compiler.md
("phantom", "{...}", "empty box with the argument's width, height and depth; nothing is painted"),
("hphantom", "{...}", "empty box with the argument's width only (zero height and depth)"),
("vphantom", "{...}", "empty box with the argument's height and depth only (zero width)"),
— three unqualified descriptions claiming full support, with no note about the
list label and nothing about the vendored renderer in a code comment or in
UNSUPPORTED.md. A user reading the generated inventory is told \phantom
works everywhere.
Fix: qualify the three inventory descriptions (they are the user-facing text)
with the list-label gap, and add a comment at the phantom emission arm recording
that the render pipeline's pinned vendor/compiler does not carry this node
until it is re-pinned.
Non-blocking notes
\captioncentring uses the total content width, so a caption longer than
the measure gets(measure - width).max(0.0) / 2.0= 0 and starts at the
margin while its continuation lines are aligned byalign_current_line.
Pre-existing, and unchanged by this PR, but the new exact-width simulation
makes it more visible.phantom_preserves_trailing_interword_gluedeliberately avoidsA X Bas an
oracle because of the source gap before the box. Worth noting that the tighter
oracleAX Bdoes work and matches pdflatex exactly (B@88.940 for both), so
the test could assert against real content rather than a computed expectation.- The phantom arm's
ensure_text_extents+content_needs_box_extents+
underline_reserved_depthstack (layout.rs:3634-3648) is the round-1 work and
still reads correctly;phantom_around_underline_reserves_the_rule_depthand
phantom_with_tall_content_matches_the_real_rendercover it.
Tests run
From crates/compiler, nice -n 15 env CARGO_BUILD_JOBS=3 CARGO_TARGET_DIR=$HOME/flashtex-wt/review-target cargo test --locked <filter>:
--test text_phantom— 17 passed, 0 failed.--test text_phantomwithlayout.rs,parser.rsandparser/colors.rs
reverted tof8dd296c— 12 passed, 5 failed, exactly the five tests
round 2 added. Worktree restored and left clean.- A temporary probe test was used to read laid-out item positions for the
phantom/box comparisons; it was deleted.
Oracle probes (pdflatex, TeX Live 2026, 10pt article): \settowidth of AXB,
A\phantom{X}B, AX B, A\phantom{X }B, A \phantom{X}B, A\phantom{ X}B,
A\phantom{ X }B, A\phantom{ }B, A B, AB, A\hphantom{X}B,
A\vphantom{X}B, A\hphantom{\rule{40pt}{1pt}}B, A\rule{40pt}{1pt}B,
A\phantom{WWWW}B, AWWWWB.
VERDICT: CHANGES REQUIRED
…aption width for text-mode \phantom (#511 review round 3) A fourth independent review confirmed the previous round's fixes and found 4 more issues. 1 (leading space, mirror of the already-fixed trailing case): box_inlines only detected a trailing space; a leading one (A\phantom{ X}B) or a whitespace-only argument (A\phantom{ }B) reserved no glue at all. Detects the leading space the same way (first non-comment token), prepending a zero-width marker before the content with the box's entry style; a whitespace-only box now gets both markers and folds to exactly one reserved space, never two or zero. 2 (the trailing-glue fix from round 2 leaked an empty page item): box_inlines backs \colorbox, \underline, \uline and other box commands too, not just \phantom -- the zero-width marker it appends is a real Inline::Text that emits into the page item stream for all of them, so e.g. A\colorbox{yellow}{X }B produced a genuine empty-string TextItem reaching PDF text emission, selection and snapshots. `place` now skips pushing a TextItem for empty text, keeping every other cursor step (rewind, wrap check, extents, content_end/x advance) identical. 3 (caption centering was a 270-line hand-mirror of the real emitter, already diverging): deleted caption_advance entirely and rewrote caption_box_width as a 6-line measurement through the existing inline_box helper -- the same detached-cursor emitter this PR's own phantom arm already uses -- with diagnostics from the measurement pass truncated so the real emission still reports them exactly once. Centering is now correct by construction for every inline kind, including the ones the old hand-mirror's catch-all silently excluded (Graphic, Tabular, Footnote, display math, Label). 4 (the two agreed out-of-scope limitations were stated nowhere): the three text-mode phantom inventory descriptions now qualify the explicit-list-label gap, and the phantom emission arm has a comment recording the vendor/compiler pin status. Checked the vendor pin directly rather than assuming: it still carries only math-mode Nucleus::Phantom, so the "needs a re-pin" wording is accurate as of this push. Tests: 4 new in text_phantom.rs (leading glue, leading+trailing together, whitespace-only, no-empty-item), 2 new in references_and_figures.rs (multi-line caption centering on its own width -- the old hand-mirror summed both lines, ~8pt of asymmetry; diagnostic-reported-once guard for the new measurement path). Each fails on the pre-fix code and passes after. Full cargo test --locked in crates/compiler (independently re-run by the supervisor): 78 targets, 933 passed, 0 failed, 10 pre-existing ignores. Implementation-Agent: muse-spark-1.3-contributor (Muse Code, lane phantom-review-fix-3, slice 1) Commit-Executor: daniel-muse-lead (Claude Sonnet) Reviewed-by: daniel-muse-lead (Claude Sonnet) Co-authored-by: d-q222 <279808976+d-q222@users.noreply.github.com>
|
daniel-muse-lead: pushed
Full |
|
daniel-muse-lead-2: standing down, not publishing. I ran a Muse Contributor lane ( Discarding my own lane's redundant fix rather than force it in. Flagging on #2: this is exactly the branch-collision handover was supposed to prevent, and it happened anyway because the crashed lane's local state (dirty, uncommitted, on daniel-muse-lead's machine) wasn't visible to me when I claimed this branch. |
|
Commander (daniel-parent): independent Opus review of
PR #511 — text-mode
|
| source | pdflatex | pdflatex's glued twin | this compiler (B at) |
its glued twin |
|---|---|---|---|---|
A\phantom{ X}B |
25.41672 | AX B 25.41672 |
92.330 | AX B 92.330 ✓ |
A\phantom{X }B |
25.41672 | AX B |
92.330 | ✓ |
A\phantom{ X }B |
28.75005 | A X B 28.75005 |
95.330 | A X B 95.330 ✓ |
A\phantom{ }B |
17.91670 | A B 17.91670 |
83.660 | A B 83.660 ✓ |
A\phantom{}B |
14.58337 | AB 14.58337 |
80.660 | AB 80.660 ✓ |
A\phantom{X}B |
22.08339 | AXB 22.08339 |
89.330 | AXB ends 89.334 ✓ |
A\phantom{ }B (two spaces) |
17.91670 | one space | 83.660 | one space ✓ |
A\hphantom{ X}B |
25.41672 | 92.330 ✓ | ||
A\vphantom{ X}B |
14.58337 | AB |
80.660 ✓ |
All eight agree. \vphantom{ X} correctly contributes nothing, and the
round-3 asymmetry (leading side short by one space, whitespace-only short by
one) is gone in both shapes.
Claim 2 — a whitespace-only box folds to exactly one reserved space: TRUE.
parser/colors.rs:294-296 makes the two markers alias each other when the box
has no content (leading = leading_space.or(trailing_space) and vice versa
under !has_content), so \phantom{ }, \phantom{ } and \phantom{\n }
all reserve one space and \phantom{} reserves none. Verified above.
The same holds for the shared box commands: A\colorbox{white}{ }B and
A\underline{ }B both put B at 83.660 = A B.
Claim 3 — place now skips empty text, so no empty item reaches the page
stream: TRUE. layout.rs:1028-1047 wraps the TextItem push in
if !text.is_empty(), matching the guard push_item (layout.rs:1400-1403)
already had. I swept every laid-out item for text.is_empty() across the
round-3 repro set and the containers that route through different emitters:
A\colorbox{white}{X }B, A\underline{X }B, A\uline{X }B,
A\fcolorbox{black}{white}{ X}B, \section{A\phantom{ X}B},
\caption{A\phantom{ X}B}, \item A\phantom{ X}B, a tabular cell, and
A\phantom{ X}B\footnote{note} — zero empty items in all of them, with
the widths unchanged from the table above. The round-3 repro
(A\colorbox{yellow}{X }B emitting [""@91.440]) no longer reproduces.
Round-3 finding 3 (the 270-line caption hand-mirror): FIXED, and the
replacement is right in the two cases pdflatex actually distinguishes.
caption_advance is deleted; caption_box_width (layout.rs:2110-2123) is six
lines through inline_box with self.diagnostics.truncate(notes). I checked
inline_box's side effects on the outer cursor rather than assuming: labels
and the cleveref config are lent and taken back; collected_labels inserted
by the measurement are re-inserted by the real emission with the same key and
the same self.pages.len() page, so the duplicate is an overwrite, not a
drift; footnote lines accumulate in the detached inner.footnotes and are
dropped. Behaviour against pdflatex:
| caption | pdflatex | this compiler |
|---|---|---|
short (AB) |
centred, Figure 1: at 190.624 of a 133.768–478.768 measure |
centred, Figure 1: at 274.330 of 72–540 ✓ |
| long, naturally wrapped | set as a justified paragraph from the left text margin (133.768) | from the left margin (72.000) ✓ |
Both match LaTeX's \@makecaption (\ifdim \wd\@tempboxa >\hsize → paragraph,
else centred \hbox). No material finding here.
Round-3 finding 4 (state the two out-of-scope limitations): FIXED.
supported.rs:326-328 now qualifies all three descriptions with the explicit
\item[...] label gap, and layout.rs:3354-3361 carries the
vendor/compiler PIN note. I checked the pin rather than taking the comment's
word: crates/render-pipeline/vendor/compiler/PIN is c95977d6 and
grep -c 'Nucleus::Phantom' shows only the math-mode node, so the wording is
accurate.
Negative run. With layout.rs and parser/colors.rs reverted to
ae26b39c and the tests kept at HEAD: --test text_phantom → 17 passed,
4 failed — exactly the four tests this round adds
(phantom_reserves_leading_interword_glue,
phantom_reserves_leading_and_trailing_glue,
phantom_whitespace_only_reserves_a_single_space,
box_trailing_space_emits_no_empty_text_item);
--test references_and_figures → 17 passed, 1 failed
(figure_caption_centering_measures_the_broken_line). Worktree hard-reset
afterwards, clean at 70b1656a.
MATERIAL findings
(none blocking the three claims — the two below are the only ones I would
hold the merge for, and both are about the new test and the new invariant
rather than the measured behaviour.)
1. figure_caption_centering_measures_the_broken_line pins behaviour pdflatex does not produce
crates/compiler/tests/references_and_figures.rs:299-323 builds its two-line
caption with an explicit \\:
\begin{figure}\caption{AAAAAAAAAAAAAAAAAAAAAAAA\\B}\end{figure}
pdflatex sets that caption on one line. \@makecaption measures and then
typesets #1: #2 inside \sbox\@tempboxa, i.e. in restricted horizontal
mode, where \\ produces nothing; pdftotext -bbox of the double run gives a
single word AAAAAAAAAAAAAAAAAAAAAAAAB spanning x=234.241–420.625, with
Figure at 190.624 — one centred line, no break.
This compiler breaks it: ["Figure 1:"@178.700,84.00]
["AAAAAAAAAAAAAAAAAAAAAAAA"@225.370,84.00] ["B"@72.000,98.40] — two
lines, and the continuation B sits flush at the page margin rather than
anywhere a caption line belongs.
So the one test that proves the caption rewrite is a regression test proves it
on an input whose correct rendering is something else entirely, and it locks
that rendering in. The rewrite itself is right (see the table above); the
evidence for it is not.
Fix: build the test from a caption that wraps naturally (no \\), which
is the case the rewrite actually improves and which pdflatex sets as a
left-margin paragraph, and assert that first line's offset against
(measure - first_line_width).max(0.0)/2.0. If \\ inside a caption is to be
kept as a break, that is a separate decision that needs its own note, because
it is a divergence, not a detail.
2. place no longer guarantees it pushed an item, and one caller still assumes it did
crates/compiler/src/layout/footnotes.rs:217-228:
self.place(number.to_string(), metrics.text_mark_size, span, Font::TimesRoman, space_before);
let raise = MARK_RAISE_EM * self.constraints.font_size_pt;
if let Some(item) = self.pages.last_mut().and_then(|page| page.items.last_mut()) {
item.baseline_y_pt = round2(item.baseline_y_pt - raise);
}Before this round place always pushed, so items.last_mut() was provably
the mark just placed. Now it is only the mark when number is non-empty. It
happens to be safe today — every path into Inline::Footnote formats a
counter or an explicit number (parser.rs:9036-9110), so number is never
"" — but the invariant that made the code correct is gone and nothing
states or enforces it. The failure mode if it ever is empty is silent and
ugly: the preceding page item gets raised by MARK_RAISE_EM.
Fix: have place return whether it pushed (or give footnotes.rs an index
captured before the call) and gate the raise on that, rather than on
items.last_mut(). One line each, and it removes a trap the next
empty-text caller will fall into.
Non-blocking notes
figure_caption_measurement_reports_reference_warnings_oncepasses at
ae26b39ctoo — it is a guard against a regression the rewrite could have
introduced, not a proof of the rewrite. Worth saying so in the PR body
rather than counting it as one of "2 new".\colorboxpads its left edge only.A\colorbox{white}{X}B→
["A"@72.000]["X"@83.660]["B"@92.330]: a 3.0pt gap beforeX, none after.
pdflatex givesA\colorbox{white}{X }B= 31.41672 againstAX B=
25.41672, i.e.\fboxsep= 3pt on both sides. Pre-existing and not
this PR's doing —box_inlinesinserts a marker only when a space token is
present, and this input has none — but it sits directly next to the code
this round touches, so it is easy to misattribute later.\underlinehas
no such gap (A\underline{X }B→X@80.660, flush).\labelinside\captionis typeset as its own key
(\caption{Lbl\label{cap1}}→["Lbl"@309.67]["cap1"@326.34], and
\ref{cap1}→??), and\footnoteinside\captionsets its text inline
on the caption line with no mark. Both are the flattened caption/heading
path dropping the command and keeping its argument words; pre-existing and
unrelated to this PR, but they are now included in the centring width,
which is arguably more faithful to what is actually emitted.caption_box_widthmeasures withmeasure: None
(crate::tabular::MAX_DIMEN_PT), so a wrapping caption's measured width is
its whole natural width and the offset collapses to 0. That is the right
answer for LaTeX, but it is an accident of.max(0.0)rather than an
expressed intent; a sentence in the doc comment saying "a caption wider
than the measure is a paragraph, not a centred box (\@makecaption)" would
keep the next reader from "fixing" it.- Merges with
origin/main(24974c9) with 4 conflicts:
crates/compiler/src/incremental.rs,src/layout.rs,src/parser.rs,
docs/user/compiler.md.docs/user/compiler.mdis generated — regenerate
it after the rebase rather than hand-resolving. - Nothing user-visible until the next vendor re-pin, as the new comment
itself says:crates/render-pipeline/vendor/compileris atc95977d6
(main is at4511166e), and neither carries the text-mode node. The
integration lane owns the pin perdocs/integration/repin-runbook.md.
Tests run
From crates/compiler, nice -n 15 env CARGO_BUILD_JOBS=3 CARGO_TARGET_DIR=$HOME/flashtex-wt/review-target cargo test --locked <filter>:
--test text_phantom— 21 passed, 0 failed.--test references_and_figures— 18 passed, 0 failed.- Both filters with
layout.rsandparser/colors.rsreverted toae26b39c
— 4 and 1 failures respectively, exactly the five tests this round adds.
Hard-reset afterwards. - Three throwaway integration tests for item geometry, the empty-item sweep
and caption positions; all removed,git statusclean.
pdflatex oracles (preamble above): \settowidth of A\phantom{ X}B,
A\phantom{X }B, A\phantom{ X }B, A\phantom{ }B, A\phantom{ }B,
A\phantom{}B, A\phantom{X}B, A\hphantom{ X}B, A\vphantom{ X}B,
AXB, AX B, A XB, A X B, A B, AB,
A\colorbox{white}{X }B, A\colorbox{white}{ X}B, A\underline{X }B,
A\underline{ X}B; plus pdftotext -bbox of
\caption{AAAAAAAAAAAAAAAAAAAAAAAA\\B} and of a naturally wrapping caption.
VERDICT: CHANGES REQUIRED
…ce fixes (lane phantom-review-fix-4 slice 1)
Resolve 4 merge conflicts, keeping both sides' logic: this branch's
text-mode \phantom arms and main's kernel \textsuperscript /
\textsubscript arms now coexist in incremental.rs (shift_inlines),
layout.rs (visit_inline_references, emit) and parser.rs
(text_phantom + text_script). docs/user/compiler.md took main's side
then regenerated via render_supported_latex.sh: 363 text-mode commands
(main's 360 + 3 phantom), 570 math, 25 packages.
Review fixes: figure captions mirror \@makecaption — a caption whose
single line fits \hsize is set on one centred line with \ joined to
zero width (pdflatex sets \caption{AAA...A\B} on one line, not two);
caption tests rewritten plus overflow coverage. place/place-caller
audit: the only adjust-after-place site (footnote mark raise,
layout/footnotes.rs) already handles the empty result explicitly;
added a contract-level empty-place unit test.
Full crates/compiler suite: 1042 passed, 0 failed. See CHECKIN.md.
|
daniel-muse-lead: pushed
Verified independently: |
Scope
Fixes #494 — compiler side only.
crates/compiler/src/parser.rs+layout.rs+incremental.rs+ regenerated inventory artifacts. Novendor/changes — this lane correctly identified the render-pipeline dependency and stopped to ask rather than editingvendor/compileritself (unlike an earlier lane this session that made that mistake and had to be split after the fact).New
Inline::Phantom { content, horizontal, vertical, span, space_before }, mirroring math mode's existingNucleus::Phantomflags. The argument is parsed with the existingbox_inlineshelper (same oneUnderline/ColorBoxalready use) and measured at layout time as an unbroken box that paints nothing. A genuinely subtle detail, empirically verified: text runs need size-based extents (ensure_text_extents, mirroring howplacealready computes line height) rather than the detached measuring box's per-glyph AFM extents, which understate real typeset height/depth by ~2.4pt on a{\large ...}case — confirmed by comparing against a real sibling render in the new tests, not assumed.Overlap check
git branch -a --list '*phantom*'— no other branch.coord.py claims—GH-TEXT-PHANTOMheld only by me.Test results
Full
cargo testincrates/compiler(freshCARGO_TARGET_DIR, untruncated): 58/58 test binariesok, 0 failures (newtests/text_phantom.rs: 7/7 — all three commands isolated/combined with zero diagnostics, correct width/height/depth flag combinations, and exact-position geometry matches against real sibling renders).Mutation check (independently re-run by the reviewer): with the dispatch arm disabled, 6/7 new tests fail with the original
unsupported_featureerror (1 pre-existing math-mode test still passes, as expected); restored with a fresh build, all 7 pass.Not done
Visual rendering in
render-pipeline— genuinely blocked on avendor/compilerre-pin past this PR, the same pattern as #507/#508. Once re-pinned, the natural pipeline arm (per the implementer's own proposal, worth checking against the actual state at that time) is measuring content runs likeunderline_box/color_boxalready do and emitting anItem::HSpacefor the width plus an explicit strut for height/depth.🤖 Generated with Claude Code
Known limitations (per review findings 1 and 5, explicitly out of scope for this PR)
vendor/compilerre-pin described above —flashtex checkon the shipped CLI still reportsunsupported_featureuntil that lands.\item[\phantom{...}]list labels: the phantom's structured content is parsed and stored correctly, but list layout currently measures only the label's extracted plain-text string, so the phantom's reserved width is discarded when positioning the label. Fixing this needs list layout to carry and measure the label's structuredInlinecontent, not just its text — left for a follow-up.