feat(math): implement m:groupChr group-character converter#2751
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0be6b2d3d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'm:d': null, // Delimiter (parentheses, brackets, braces) | ||
| 'm:eqArr': null, // Equation array (vertical array of equations) | ||
| 'm:groupChr': null, // Group character (overbrace, underbrace) | ||
| 'm:groupChr': convertGroupCharacter, // Group character (overbrace, underbrace) |
There was a problem hiding this comment.
Enabling m:groupChr conversion here changes output from a flat fallback to real <munder>/<mover> layout, but run sizing still comes from estimateMathDimensions() (packages/layout-engine/pm-adapter/src/converters/math-constants.ts), where VERTICAL_ELEMENTS does not include m:groupChr. The DOM measurer then uses run.height for line height budgeting (packages/layout-engine/measuring/dom/src/index.ts, math run branch via maxImageHeight), so under/over-brace formulas can render taller than allocated space and visually collide with neighboring lines. This registration should be paired with a m:groupChr height multiplier update (and a sizing test) to avoid layout regressions.
Useful? React with 👍 / 👎.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@Abdeltoto solid contribution, tested all the common cases against Word and they match. one small edge case on the character fallback — left an inline comment.
| const chr = groupChrPr?.elements?.find((e) => e.name === 'm:chr'); | ||
| const pos = groupChrPr?.elements?.find((e) => e.name === 'm:pos'); | ||
|
|
||
| const groupChar = chr?.attributes?.['m:val'] ?? DEFAULT_GROUP_CHAR; |
There was a problem hiding this comment.
when <m:chr/> exists but has no value, the spec says the character should be hidden — Word shows nothing. right now this falls back to the default curly bracket instead. doesn't happen with Word-generated files (Word always writes an explicit value), but worth handling correctly.
| const groupChar = chr?.attributes?.['m:val'] ?? DEFAULT_GROUP_CHAR; | |
| const groupChar = chr ? (chr.attributes?.['m:val'] ?? '') : DEFAULT_GROUP_CHAR; |
| * Convert m:groupChr (group character) to MathML <munder> or <mover>. | ||
| * | ||
| * OMML structure: | ||
| * m:groupChr → m:groupChrPr (optional: m:chr@m:val, m:pos@m:val, m:vertJc@m:val), m:e |
There was a problem hiding this comment.
the spec also has a m:vertJc property that controls where the baseline sits — Word renders it differently for each combination. not blocking, just something to keep in mind for later.
Made-with: Cursor
- empty <m:chr/> now renders as a hidden character per ECMA-376 §22.1.2.20 - read m:vertJc, stamp data-vert-jc attribute, and shift the construct via position: relative when the value is non-natural for the given m:pos - add unit coverage for all four pos × vertJc combinations and empty-val vertJc - add behavior tests and a multi-variant fixture covering every §22.1.2.41 case
caio-pizzol
force-pushed
the
feat/math-group-character-converter
branch
from
958499a to
1a6bc26
Compare
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
follow-up on my earlier review — thanks again for the contribution, @Abdeltoto! went ahead and pushed the fixes for both points myself (empty <m:chr/> + m:vertJc). also added a few tests and a fixture, and rebased onto main. good to go from my end!
|
Hey @caio-pizzol, thanks a lot for the thorough review and for pushing the fixes directly — really appreciate you taking the time to handle the empty m:chr edge case and the m:vertJc support as well! Everything looks great. Happy to keep contributing, looking forward to tackling more converters! |
|
🎉 This PR is included in vscode-ext v2.3.0-next.12 |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.9 The release is available on GitHub release |
|
🎉 This PR is included in esign v2.3.0-next.12 The release is available on GitHub release |
|
🎉 This PR is included in template-builder v1.5.0-next.12 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.26.0-next.12 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.7.0-next.13 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.6.0-next.10 |
|
🎉 This PR is included in superdoc-cli v0.7.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.27.0 The release is available on GitHub release |
2 participants
Closes #2606
Summary
m:groupChrOMML-to-MathML converter (overbrace, underbrace, and other stretchy group characters)<munder>(default, bottom position) or<mover>(top position) with a stretchy<mo>characterm:chris absent, matching Word behaviorMATH_OBJECT_REGISTRYSpec reference
ECMA-376 Section 22.1.2.41
Test plan
vitest runpasses for omml-to-mathml.test.ts