feat(math): implement m:d delimiter converter (SD-2380)#2710
feat(math): implement m:d delimiter converter (SD-2380)#2710andrewsrigom wants to merge 2 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:
| const property = properties?.elements?.find((element) => element.name === name); | ||
| return property?.attributes?.['m:val'] ?? fallback; |
There was a problem hiding this comment.
the spec treats "element missing" and "element present without m:val" differently. missing → use default char, present without val → no character. right now both return the default because property.attributes is undefined in both cases.
| const property = properties?.elements?.find((element) => element.name === name); | |
| return property?.attributes?.['m:val'] ?? fallback; | |
| const property = properties?.elements?.find((element) => element.name === name); | |
| if (!property) return fallback; | |
| return property.attributes?.['m:val'] ?? ''; |
|
|
||
| const DEFAULT_BEGIN_DELIMITER = '('; | ||
| const DEFAULT_END_DELIMITER = ')'; | ||
| const DEFAULT_SEPARATOR_DELIMITER = '|'; |
There was a problem hiding this comment.
spec says the default separator is U+2502, not ASCII | (§22.1.2.95). they look similar but are different characters.
| const DEFAULT_SEPARATOR_DELIMITER = '|'; | |
| const DEFAULT_SEPARATOR_DELIMITER = '\u2502'; // U+2502 BOX DRAWINGS LIGHT VERTICAL (ECMA-376 §22.1.2.95) |
|
@andrewsrigom let me know if you have any questions here! thanks for you work ❤️ |
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