feat(math): implement m:sSub subscript converter (SD-2373)#2635
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: 375438e41c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| msub.appendChild(convertChildren(base?.elements ?? [])); | ||
| msub.appendChild(convertChildren(sub?.elements ?? [])); |
There was a problem hiding this comment.
convertChildren(base?.elements ?? []) and convertChildren(sub?.elements ?? []) append raw operand fragments directly into <msub>, so any multi-token base/subscript (for example (a+b)_n) produces more than two direct children. MathML script elements are binary and expect exactly base+subscript nodes, so these cases become structurally invalid and can render with dropped/mis-associated tokens. Wrap each operand in an <mrow> (or convert m:e/m:sub as units) before appending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@gpardhivvarma confirmed — we'll fix this across all converters in a follow-up.
There was a problem hiding this comment.
Fixed — operands are now wrapped in <mrow> before appending, matching the pattern in bar.ts. Multi-part expressions like x_{n+1} now produce exactly 2 children in <msub>. Added a test for this case too.
There was a problem hiding this comment.
@gpardhivvarma nice work — follows the existing pattern well and the spec reference is correct.
simple subscripts like a₁ render correctly. when the base or subscript has multiple parts (like x_{n+1}), they don't get grouped properly and the rendering breaks. fraction.ts has the same issue — bar.ts already handles it. we'll fix all converters together in a follow-up.
tests look good.
i made a .docx with 10 subscript variations:
sd-2373-subscript-edge-cases-v2.docx
| # | expression | works? |
|---|---|---|
| 1 | a₁ | yes |
| 2 | x_{n+1} | no — 4 children instead of 2 |
| 3 | (a+b)_n | no — 4 children instead of 2 |
| 4 | (a+b)_{n+1} | no — 6 children instead of 2 |
| 5 | x_{i+j+k+1} | no — 8 children instead of 2 |
| 6 | a_{b_c} (nested) | yes |
| 7 | x_{a/b} (fraction) | yes |
| 8 | y₀ (with properties) | yes |
| 9 | a₁+b₂ (multiple) | yes |
| 10 | x_{} (empty) | no — 1 child instead of 2 |
Word:
SuperDoc:
simple cases (1, 6-9) look the same. multi-part cases (2-5) render incorrectly in SuperDoc.
left an inline comment.
Add OMML m:sSub → MathML <msub> converter following the fraction.ts pattern. Also move m:f from the "not yet implemented" registry section to "implemented" where it belongs. Closes superdoc-dev#2596
Converters for msub, mfrac were appending raw DocumentFragments directly, causing multi-token expressions (e.g. n+1) to produce too many direct children. MathML script/fraction elements require exactly 2 children. Wrap each operand in <mrow> to group them, matching the pattern already used by bar.ts.
gpardhivvarma
force-pushed
the
feat/math-ssub-subscript-converter
branch
from
71c5170 to
feab86a
Compare
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@gpardhivvarma the grouping fix works — tested 15 edge cases and everything renders correctly now. you also fixed the same issue in fraction.ts which is a nice bonus.
uploaded the edge case test file to our test corpus so we catch regressions going forward.
approving.
|
🎉 This PR is included in template-builder v1.3.0-next.10 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v1.1.0-next.53 |
|
🎉 This PR is included in esign v2.2.0-next.11 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.5.0-next.50 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.3.0-next.51 |
|
🎉 This PR is included in superdoc v1.24.0-next.50 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.0.0-next.11 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.4.0 |
|
🎉 This PR is included in superdoc v1.25.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.6.0 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.1 |
|
🎉 This PR is included in esign v2.3.0-next.1 The release is available on GitHub release |
|
🎉 This PR is included in template-builder v1.5.0-next.1 The release is available on GitHub release |
2 participants
Summary
m:sSub→ MathML<msub>converter following thefraction.tspatternm:ffrom the "not yet implemented" registry section to "implemented" where it belongsm:subgraceful degradationTest plan
pnpm --filter @superdoc/painter-dom test—omml-to-mathml.test.tspasses (17 tests)Closes #2596