feat(math): implement m:d delimiter converter (SD-2380)#2710
feat(math): implement m:d delimiter converter (SD-2380)#2710caio-pizzol merged 9 commits intosuperdoc-dev:mainfrom
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: f2079a7a82
ℹ️ 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".
packages/layout-engine/painters/dom/src/features/math/converters/delimiter.ts
Outdated
Show resolved
Hide resolved
andrewsrigom
force-pushed
the
feat/math-delimiter-converter
branch
from
f2079a7 to
804c4d3
Compare
There was a problem hiding this comment.
@andrewsrigom clean implementation, follows the existing converter pattern nicely.
two issues to fix — both confirmed by comparing SuperDoc rendering against Word (test file + screenshots below).
1. element present without m:val should mean "no character" — right now <m:begChr/> (no attribute) behaves the same as the element being absent, so it renders (. the spec says it should render nothing. same applies to endChr and sepChr.
2. wrong default separator — should be \u2502 (box drawing vertical) not ASCII |, per §22.1.2.95.
tests look good. you'll want to add one for the no-val case after fixing #1. left inline comments with fixes.
💡 tip: you can look up any ECMA-376 section directly using the ooxml.dev MCP server — handy for checking defaults and edge cases in the spec.
test file: attached .docx with 21 cases covering every m:dPr property. cases 5, 8, 9, 12 show the bugs:
SD-2380-delimiter-spec-tests.docx
Case 5 — <m:begChr/> should hide the opening delimiter:
SuperDoc:
Word:
Case 8 — <m:endChr/> should hide the closing delimiter:
SuperDoc:
Word:
Case 9 — both present without val, should show no delimiters:
SuperDoc:
Word:
Case 12 — <m:sepChr/> should hide the separator:
SuperDoc:
Word:
packages/layout-engine/painters/dom/src/features/math/converters/delimiter.ts
Outdated
Show resolved
Hide resolved
packages/layout-engine/painters/dom/src/features/math/converters/delimiter.ts
Outdated
Show resolved
Hide resolved
|
@andrewsrigom let me know if you have any questions here! thanks for you work ❤️ |
…onverter # Conflicts: # packages/layout-engine/painters/dom/src/features/math/converters/index.ts # packages/layout-engine/painters/dom/src/features/math/omml-to-mathml.ts
|
Hi @caio-pizzol , this should be ready for another review now. Sorry for the delay, I got caught up with some work on my side. I rechecked the delimiter cases and they now match the expected output I've noticed a timeout a issue in the pipeline, can you check |
Add 5 behavior tests exercising the delimiter converter with a dedicated test document covering all 21 ECMA-376 §22.1.2.24 compliance cases: default parentheses, U+2502 separator, suppressed delimiters (chr without m:val), custom Unicode delimiters (floor, ceiling, abs), and nesting. Fix stale comment that still referred to delimiter as unimplemented.
Replace map + filter + forEach chain with a single for-loop that builds expression groups directly, removing the single-use helper.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@andrewsrigom all three items from last round are fixed, looks good.
i pushed two commits — behavior tests with a test document covering all 21 spec cases, and a small refactor inlining createExpressionGroup into a single loop. also added the test doc to our rendering corpus for automatic regression checks.
approving.
|
🎉 This PR is included in @superdoc-dev/react v1.0.0-next.30 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v1.1.0-next.76 |
|
🎉 This PR is included in template-builder v1.3.0-next.36 The release is available on GitHub release |
|
🎉 This PR is included in esign v2.2.0-next.34 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.24.0-next.73 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.5.0-next.74 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.3.0-next.75 |
2 participants
Summary
m:dOMML -> MathML converterTesting
pnpm exec vitest run packages/layout-engine/painters/dom/src/features/math/omml-to-mathml.test.tsCloses #2611