Skip to content

fix(REN-3): \color and \colorbox atoms now receive inter-element spacing#231

Open
kostub wants to merge 2 commits into
masterfrom
em/2026-06-11-issues/t7
Open

fix(REN-3): \color and \colorbox atoms now receive inter-element spacing#231
kostub wants to merge 2 commits into
masterfrom
em/2026-06-11-issues/t7

Conversation

@kostub

@kostub kostub commented Jun 12, 2026

Copy link
Copy Markdown
Owner

Summary

  • kMTMathAtomColor and kMTMathAtomColorbox cases in -createDisplayAtoms: never called addInterElementSpace:, so colored groups lost their leading inter-element gap (e.g. the medium binary-operator space in x+\color{red}y was dropped, causing y to abut + with no gap).
  • Fix adds one [self addInterElementSpace:prevNode currentType:atom.type] call per case, after the line flush and before display.position = _currentPosition, mirroring the existing text-case and inner-case patterns. Both atom types already map to Ordinary (index 0) in getInterElementSpaceArrayIndexForType, so no spacing-table changes are required. Net change: +2 lines in MTTypesetter.m.
  • Three new tests in MTTypesetterTest: \color before binary-op gap (was RED, now GREEN), \colorbox before binary-op gap (was RED, now GREEN), regression guard that spacing after a \color group is preserved (was already GREEN).

Closes REN-3 from issues.md.

Test plan

  • swift build passes with no warnings
  • swift test passes all 295 tests (89 in MTTypesetterTest, 3 new)
  • testColorReceivesInterElementSpacingBeforeIt — asserts the medium binary-op gap (4 mu = 4 × muUnit) before \color
  • testColorboxReceivesInterElementSpacingBeforeIt — same for \colorbox
  • testSpacingAfterColorGroupIsPreserved — regression guard that the already-working post-color spacing still holds

🤖 Generated with Claude Code

Before this fix, `kMTMathAtomColor` and `kMTMathAtomColorbox` cases in
`-createDisplayAtoms:` never called `addInterElementSpace:prevNode
currentType:atom.type`, so a colored group lost its leading inter-element
gap. For example, the medium binary-operator space in `x+\color{red}y`
was dropped, causing the colored `y` to abut the `+` with no gap.

The fix adds the missing `addInterElementSpace:` call to each case (after
the line flush, before `display.position = _currentPosition`), mirroring
the text-case and inner-case patterns. Both atom types already map to index
0 (Ordinary) in `getInterElementSpaceArrayIndexForType`, so no table
changes are needed.

Three new typesetter tests cover:
- `\color` before a binary operator gap (RED → GREEN)
- `\colorbox` before a binary operator gap (RED → GREEN)
- regression guard that spacing after a `\color` group is preserved (already GREEN)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds inter-element spacing before color and colorbox atoms in MTTypesetter.m to ensure they are spaced correctly as ordinary atoms (e.g., leaving a medium gap after a binary operator). It also adds comprehensive unit tests in MTTypesetterTest.m to verify this spacing behavior and ensure that spacing after color groups remains preserved. There are no review comments to address, and I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@kostub

kostub commented Jun 12, 2026

Copy link
Copy Markdown
Owner Author

EM-REVIEW v1

Verdict: Approve-equivalent (not approving per instructions). Blocking issues: none.

Reviewed the REN-3 fix: adds [self addInterElementSpace:prevNode currentType:atom.type] to the kMTMathAtomColor and kMTMathAtomColorbox cases in -createDisplayAtoms: (MTTypesetter.m), plus 3 typesetter tests.

1. Call placement — correct. Placed after the if (_currentLine.length > 0) [self addDisplayLine]; flush and before display.position = _currentPosition, exactly mirroring the sibling kMTMathAtomText, kMTMathAtomFraction, kMTMathAtomInner, and kMTMathAtomLargeOperator cases. addInterElementSpace advances _currentPosition.x, so it must precede the display.position = assignment — and it does.

2. Spacing resolution — correct. getInterElementSpaceArrayIndexForType maps both kMTMathAtomColor and kMTMathAtomColorbox to index 0 (Ordinary) for both row and column. prevNode is the canonical previous atom assigned at the bottom of the loop (prevNode = atom), and atom.type is passed unchanged (color stays color → Ord index). For x+\color..., prevNode is + (BinaryOperator). In MTMathList finalized, a color atom hits the default branch and does NOT demote a preceding binary op (only relation/punct/close do), so + stays binary → BinOp(left,2)→Ord(right,0) = kMTSpaceNSMedium = 4mu at display style. Sound.

3. No double-spacing / off-by-one. The color case flushes _currentLine first, so prevNode's glyph line is already closed; the new space advances _currentPosition only (no kerning attribute added to the flushed line). The next atom after a color group sees _currentLine.length == 0 and takes the _currentPosition.x += interElementSpace branch, not the kerning branch — no double count on either side.

4. Test expectation — correct and robust. displayForLaTeX: uses kMTLineStyleDisplay + default font, so _style < kMTLineStyleScriptkMTSpaceNSMedium = 4mu, matching the test's 4.0 * self.font.mathTable.muUnit. At display style _styleFont muUnit == _font muUnit, so the formula matches getInterElementSpace exactly. Assertions use XCTAssertEqualWithAccuracy(..., 0.01) and verify sub-display kinds/colors before measuring — robust. localTextColor/localBackgroundColor are public on MTDisplay.

5. No regression to non-color atoms. Change is confined to the two color cases; all other cases untouched. The post-color side already worked via the standard table path and is now guarded by testSpacingAfterColorGroupIsPreserved. PR reports 295 tests pass.

Minor (non-blocking):

  • The comment block inside testSpacingAfterColorGroupIsPreserved is rambling/self-contradicting ("the binary-operator gap after the colored group" then re-derives it twice). The assertion itself is correct (Ord→BinOp = 4mu medium). Consider trimming the comment to one clear sentence.
  • The two new code comments say color "is spaced as Ord (see getInterElementSpaceArrayIndexForType)" — accurate, good cross-reference.

No blocking issues. The +2 functional lines are minimal, correctly placed, and the spacing derivation is sound on both sides of the color group.

🤖 Generated with Claude Code

Addresses non-blocking review nit. The previous comment block re-derived
the Ord->BinOp spacing three times with a self-contradicting "But because"
pivot. Replaced with one accurate sentence. No behavioral change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kostub

kostub commented Jun 12, 2026

Copy link
Copy Markdown
Owner Author

Addressed the non-blocking nit in f40c063: trimmed the rambling/self-contradicting comment in testSpacingAfterColorGroupIsPreserved to one accurate sentence (Ord->BinOp = medium 4mu). No behavioral change; all 295 tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant