Apply italic correction per TeX Rule 14/17 - #272
Conversation
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
Every failing assertion is reconciled to a sum of the italic corrections introduced by items 1-6, propagated through unchanged composition/layout logic (composite ink rollup, accent centering, table column width, the mathit companion-overhang correction, and CoreText ligature shaping) - never blind-rebaselined. Each updated constant carries a comment naming its source. Also registers MTItalicCorrectionTest.m with the iosMathTests Xcode target (4 entries, following MTInkWidthTest.m's pattern).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMath typesetting now preserves existing kerning while applying font-specific italic corrections across atoms, scripts, styles, fused runs, and companion fonts. New tests cover correction rules and rendering modes. Existing width, sizing, positioning, and ligature expectations were updated. ChangesItalic correction rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MathInput
participant MTTypesetter
participant CoreTextFace
participant MATHTable
participant AttributedLine
MathInput->>MTTypesetter: create attributed math line
MTTypesetter->>CoreTextFace: resolve stamped font face
CoreTextFace->>MATHTable: read math-font correction metrics
MTTypesetter->>AttributedLine: compose glyphs and preserve existing kern
MTTypesetter->>AttributedLine: add applicable italic corrections
AttributedLine-->>MathInput: return corrected layout
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
iosMathTests/MTItalicCorrectionTest.m (1)
294-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that the script displays were found.
superscriptandsubscriptstay nil when the loop matches nothing.nil.position.xthen returns 0, and the assertions at lines 301-302 fail with a numeric difference instead of the real cause. Add nil checks before the comparisons.♻️ Proposed check
} + XCTAssertNotNil(superscript); + XCTAssertNotNil(subscript); XCTAssertEqualWithAccuracy(superscript.position.x - subscript.position.x, f, 0.001);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iosMathTests/MTItalicCorrectionTest.m` around lines 294 - 302, In the test around the superscript/subscript scan, add assertions that both `superscript` and `subscript` are non-nil immediately after the loop and before accessing their positions; keep the existing positional accuracy assertions unchanged.iosMath/render/internal/MTTypesetter.m (1)
1089-1108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an explicit terminal return after the switch.
The switch covers every enumerator, so
-Wswitchstill reports a new style. A value outside the enum range, for example from a cast through the public API, falls off the end of a non-void function. That is undefined behavior. A terminalreturn YES;keeps the-Wswitchguarantee and removes the undefined path.♻️ Proposed terminal return
case kMTFontStyleBlackboard: // msbm10 return YES; } + // Unreachable for any declared style; keeps out-of-range casts defined. + return YES; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iosMath/render/internal/MTTypesetter.m` around lines 1089 - 1108, Add a terminal return YES after the switch in MTStyleSuppressesInteriorItalicCorrection, preserving the existing per-style results while providing a defined fallback for unknown or future MTFontStyle values and retaining -Wswitch coverage.iosMathTests/MTInkWidthTest.m (1)
249-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keeping coverage for a composite that still overhangs.
The equality assertion is correct for a trailing
V. It no longer proves that a composite propagates a trailing child's ink past its own advance, which was the original property ofassertComposite:bare:shifted:. A trailing glyph whose ink still exceeds advance plus correction would restore that coverage. The change is otherwise consistent with the new correction behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iosMathTests/MTInkWidthTest.m` around lines 249 - 254, Keep the corrected trailing-V equality assertion, but add or retain a separate composite case in assertComposite:bare:shifted: using a trailing glyph whose ink exceeds its advance plus correction. Assert that this composite’s ink width still overhangs its width, preserving coverage of trailing-child overhang propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@iosMath/render/internal/MTTypesetter.m`:
- Around line 1138-1146: Update faceInCurrentLineAtIndex: and its callers,
including italicCorrectionInCurrentLineAtIndex: and
applyItalicCorrectionsInRange:forAtom:, to handle a NULL CTFontRef after
retaining the existing NSAssert. In release builds, skip italic correction or
glyph processing when no face is available, ensuring NULL is never passed to
CFEqual or CTFontGetGlyphsForCharacters while preserving normal behavior for
valid fonts.
- Around line 1172-1175: Guard the glyph measurement logic before calculating
the kern from CTFontGetBoundingRectsForGlyphs: detect CGRectNull and any
non-finite bounding-rect values, and return a finite fallback instead of passing
infinity through MAX. Keep the existing advance-based calculation for valid
rectangles.
---
Nitpick comments:
In `@iosMath/render/internal/MTTypesetter.m`:
- Around line 1089-1108: Add a terminal return YES after the switch in
MTStyleSuppressesInteriorItalicCorrection, preserving the existing per-style
results while providing a defined fallback for unknown or future MTFontStyle
values and retaining -Wswitch coverage.
In `@iosMathTests/MTInkWidthTest.m`:
- Around line 249-254: Keep the corrected trailing-V equality assertion, but add
or retain a separate composite case in assertComposite:bare:shifted: using a
trailing glyph whose ink exceeds its advance plus correction. Assert that this
composite’s ink width still overhangs its width, preserving coverage of
trailing-child overhang propagation.
In `@iosMathTests/MTItalicCorrectionTest.m`:
- Around line 294-302: In the test around the superscript/subscript scan, add
assertions that both `superscript` and `subscript` are non-nil immediately after
the loop and before accessing their positions; keep the existing positional
accuracy assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce753e81-530b-41cb-bc14-9ec7890fc5ed
📒 Files selected for processing (6)
iosMath.xcodeproj/project.pbxprojiosMath/render/internal/MTTypesetter.miosMathTests/MTInkWidthTest.miosMathTests/MTItalicCorrectionTest.miosMathTests/MTMathUILabelSizingTest.miosMathTests/MTTypesetterTest.m
kostub
left a comment
kostub
left a comment
There was a problem hiding this comment.
Reviewed by reading the diff against docs/lld/2026-07-25-italic-correction.md and docs/plans/2026-08-12-italic-correction.md. I did not build or run anything — CI (Build & Test) is still pending on this head at the time of writing, so the pass/fail claim in the description is the run to trust, not me.
Strengths
- The face-keyed metric is the right shape. Resolving the glyph against the same
CTFontRefthe attribute stamp names (italicCorrectionInCurrentLineAtIndex:, MTTypesetter.m:1146-1176) closes the real defect — a companion glyph id read against the math font's MATH table — and repointing the script path'sdeltaat the same helper means the scripted and scriptless cases can't drift apart. MTStyleSuppressesInteriorItalicCorrectionearns its 20 lines: the exhaustive switch with nodefault:is what makes-Wswitchforce a hand classification when a style is added, and the comment records the TFM each case stands for. The one-line!(a||b||c||d)version would be smaller and worse.- Making the inter-element kern additive rather than assigning (MTTypesetter.m:993-1001) is the minimum change that composes, and
testCorrectionAndInterElementSpaceComposeasserts the two contributions separately rather than only their sum, which is the assertion that actually catches a silent replacement. - Item 8's baselines are reconciled with arithmetic in the comment, not re-recorded.
testMathTable's note tracing +1.16 through centre-alignment into a column offset is the standard to hold.
I looked for machinery to delete and didn't find any — the four new units are each load-bearing for a case the tests pin (drop the face-seam branch and \mathit{f\alpha} loses f's correction; drop the CFEqual(face, _styleFont.ctFont) interior test and companion interiors stop being suppressed).
Issues
1. The trailing ε leaks into every nested sub-box, where TeX deletes it
applyItalicCorrectionsInRange:forAtom: attaches the trailing correction whenever the atom has no script, including when the atom is the last one in a sublist. Every sublist is typeset by createLineForMathList: — numerator/denominator (MTTypesetter.m:1523-1524), radicand (:1683), accentee (:2151, :2181), over/under inner (:2043, :2056), and both script lists (:1337, :1351) — and the trailing kern lands in that display's width via CTLineGetTypographicBounds, which testTrailingCorrectionReachesLineWidth proves.
TeX removes it. clean_box (TeX82 §720) simplifies a box whose list is exactly one char node followed by one kern node by freeing the kern — that kern is precisely Rule 17's italic correction, and clean_box is what make_fraction, make_radical, make_math_accent, make_over and make_scripts all wrap their sub-lists in. So in pdfTeX:
x^V— the superscript box isclean_box(supscr), ε dropped, thenscript_spaceadded. Here the superscript display carries V's ε (~3pt in script style at 20pt), so anything following the script —x^V y— sits that much further right.\frac{f}{2}— the numerator is a clean box, so TeX centres the baref. Here the numerator is ε wider, shiftingfleft by ε/2 and widening the whole fraction.\hat{f},\sqrt{f},\overline{f}— same mechanism: the accent centres over a box that is ε too wide, the radical rule extends ε too far.
The existing baselines didn't catch this because the sublists in MTTypesetterTest use x, 1, 2, 3, 5, whose corrections are zero — so this is untested in either direction.
I can't tell from reading whether you consider this in scope. It isn't in the LLD: §6's "Handled" list covers seams, scripts on the outer atom and flush boundaries, but nothing says what happens when the corrected glyph is the last atom of a nested list. Either the trailing kern gets dropped at the end of a sublist that reduces to one character, or this goes in §6 as a named divergence — but leaving it undecided means \frac{f}{2} and x^V y are now wrong against pdflatex in a way the manual verification pass (which the description says is still outstanding) will surface as a mystery.
2. The ligature loss is broader than the LLD's ligature note, which assumes the opposite
testMathitAppliesCompanionLigature was inverted from "one run, one glyph" to "two runs, one glyph each" (MTTypesetterTest.m:3859-3876). That test was written to pin that \mathit{fi} forms f_i; it now pins that it doesn't.
The cited justification — LLD §6, GSUB out of scope — doesn't cover this. §6's ligature bullet reads "CoreText substitutes f_f when it shapes \mathit{ff}, but the helper resolves the character f and measures that glyph … the trailing ε is right by coincidence". That limitation is about which glyph gets measured, and it presupposes the ligature still forms. The new behaviour is that it doesn't form at all, which is a visible rendering change for \mathit{fi}, \mathit{ff}, \mathit{fl}, \mathit{ffi} — all of which pdfTeX ligates in cmti10. That's a different, larger claim than the one §6 signed off, and it deserves its own decision rather than inheriting one.
Related, and worth checking rather than assuming: the same attribute boundary sits between the last two characters of every corrected atom, so it also splits the shaping run for GPOS pair kerning. testMathitAppliesCompanionKerning (\mathit{AV}, MTTypesetterTest.m:3849-3857) asserts pair.width < a.width + v.width - 0.05em. With the trailing kern in place, a.width now includes A's own ε while pair.width doesn't, so that inequality can be satisfied by the correction alone even if the A/V pair kern has been lost to the run split. If that's what's happening, the test still passes but no longer tests companion kerning. I can't establish which from reading — it needs the run count and positions inspected at runtime.
I'm raising both once; the call is yours.
3. New test file drops the repo's MTDisplay* cast convention, producing pointer-type warnings
MTItalicCorrectionTest.m assigns subDisplays elements — declared NSArray<MTDisplay*>* (MTMathListDisplay.h:117) — straight into subclass locals, which is -Wincompatible-pointer-types at each site: lines 49, 95, 105, 255, 263, 281, 287, 293, 310, 347, 371, 387, 450, 454, 455, plus the for (MTMathListDisplay* d in both.subDisplays) loop variable at 296.
MTTypesetterTest.m and MTInkWidthTest.m are consistent about this — either MTDisplay* sub0 = display.subDisplays[0]; (MTTypesetterTest.m:68, 100, 133, …) or an explicit (MTCTLineDisplay*) cast (MTInkWidthTest.m:42, MTTypesetterTest.m:3868). The fix is the cast at each site. Nothing is broken at runtime; it's warning noise in every build of the test target, and it diverges from a convention the neighbouring files hold uniformly. (Noting this came from the plan's prescribed test code — the plan is where it should be fixed too, if it's going to be reused.)
Assessment: with fixes. The core mechanism — face-keyed metric, additive kern, per-style gate — is right and well pinned. #1 is the one I'd want resolved before merge: it is a real divergence from the rule this PR is implementing, on common input, and currently untested in either direction. #2 needs a decision recorded, not necessarily a code change. #3 is mechanical.
…rage
Cast subDisplays elements at their 16 use sites in MTItalicCorrectionTest.
subDisplays is NSArray<MTDisplay*>*, so assigning an element straight into a
subclass local is -Wincompatible-pointer-types; MTTypesetterTest and
MTInkWidthTest already either declare MTDisplay* or cast explicitly.
Add testCompositeInkTracksOverhangingChild. Every assertComposite: case trails
a V, which no longer overhangs its corrected advance, so the composed-ink
assertion holds trivially and deleting the MAX over children from a composite
inkWidth getter would fail nothing. No plain glyph overhangs any more -- a
sweep of ~150 characters across the four bundled fonts finds none -- so the
collapse assertion stays and the new test nests \vec{f}, whose accent glyph
still overhangs, to force the child to drive the getter. \sum and
\overrightarrow are excluded: their own glyph always covers the base, so they
cannot overhang at all.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01TmCWfSMYeLRJmUEvSd5XoT
|
Responses to the three nitpicks in the review body.
Two measurements shaped the fix. First, no plain glyph is a candidate any more: sweeping ~150 characters (Latin, Greek, digits, the Second, the only overhang source left is an accent glyph, whose advance is near zero. Nesting
|
1 participant
Applies each glyph's italic correction (ε) wherever TeX Appendix G Rule 17 requires it — interior of a fused run, cross-atom and cross-face seams, and the trailing glyph — sourced from the face that actually drew the glyph.
Today iosMath inserts ε only when an atom carries a sub/superscript, so no correction is ever applied between scriptless glyphs; default-style math is 21.9% too tight and in the worst cases glyph ink visibly overlaps the next character.
Plan:
docs/plans/2026-08-12-italic-correction.mdLLD:
docs/lld/2026-07-25-italic-correction.md(+ measurement appendix)Goal
Every glyph with a nonzero italic correction and an empty subscript gets a kern of that correction, read from the face that drew it, suppressed on the interior of a text-font run exactly as TeX suppresses it.
One file changes in the render path:
iosMath/render/internal/MTTypesetter.m. Two private helpers read thekCTFontAttributeNamestamp back off_currentLineand return the correction from that face — the math font'sMATHtable, or measured ink overhang for the\mathitcompanion, which has noMATHtable. A third helper attacheskCTKernAttributeNameper character at the nucleus append site, gated by face and by the LaTeX-equivalent TFM'sSPACEfor the atom's style. The script path'sdeltais repointed at the same helper, so a scripted and a scriptless glyph shift by the same number.No data-model, API, font-resolution or code-point changes.
Commits
[item 1]Read italic correction from the face that drew the glyph[item 2]Make the inter-element kern additive[item 3]Apply italic corrections at the nucleus append site[item 4]Pin flush-boundary and script behaviour[item 5]Pin companion metric and cross-font behaviour[item 6]Pin kern composition and zero-correction cases[item 7]Pin that the text path takes no correction[item 8]Update geometry baselines for italic correctionTesting
iosMathTests/MTItalicCorrectionTest.mis new — 23 tests covering the per-style gate on both the math-font and companion sides, face seams inside one nucleus, all 19 flush boundaries, script arithmetic, kern composition with inter-element space, and cross-font behaviour across all 8 bundled fonts.swift test— 490/490 passxcodebuild test -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 16'— 442 tests, TEST SUCCEEDEDItem 8 reconciled every shifted baseline in
MTTypesetterTest,MTInkWidthTestandMTMathUILabelSizingTestcase by case against a computed sum of corrections, per the plan's no-blind-rebaselining rule. Two shifts are worth a reviewer's attention specifically:inkWidthcollapsing ontowidthfor trailing italics (predicted by LLD §5): ε now meets or exceeds the protruding ink for every letter and digit in Latin Modern Math, so the ink-aware widening becomes a no-op there. This affects thePcase the plan names plus the sevenassertCompositefamilies.\mathit{fi}no longer forms thef_iligature: the trailing correction onicreates an attribute boundary CoreText's shaper will not ligature across. GSUB-aware correction placement is explicitly out of scope (LLD §6 known limitations), so this was accepted rather than worked around.Manual side-by-side verification against pdflatex —
V],V),V\mathrm{l},\mathcal{…}and\mathit{f}^2across all 8 fonts — is called for by the LLD and has not been done yet.Summary by CodeRabbit
Bug Fixes
Tests