Skip to content

feat: implement m:rad radical/sqrt converter#2784

Merged
caio-pizzol merged 5 commits intosuperdoc-dev:mainfrom
Anuj52:feat/radical-converter
Apr 12, 2026
Merged

feat: implement m:rad radical/sqrt converter#2784
caio-pizzol merged 5 commits intosuperdoc-dev:mainfrom
Anuj52:feat/radical-converter

Conversation

@Anuj52
Copy link
Copy Markdown
Contributor

@Anuj52 Anuj52 commented Apr 12, 2026

Closes #2598

Summary

This PR implements the m:rad (radical) OMML to MathML converter, adding support for square roots and n-th roots in mathematical expressions.

Changes

  • Created radical.ts converter to handle radical expressions.
  • Implemented logic to switch between <msqrt> (for square roots with hidden/missing degrees) and <mroot> (for n-th roots).
  • Ensured correct MathML child ordering: radicand (base) first, followed by the degree (index).
  • Registered the converter in the OMML-to-MathML registry.

Verification

  • Verified against omml-to-mathml.test.ts patterns.
  • Handles m:degHide logic robustly according to ECMA-376 §22.1.2.86.

Closes #2598

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c680cca804

ℹ️ 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".


if (degreeHidden || !deg) {
// Use msqrt if degree is explicitly hidden OR if m:deg is missing/empty
if (degreeHidden || !deg || (deg.elements ?? []).length === 0) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Honor degHide=false when degree node is empty

The new || (deg.elements ?? []).length === 0 branch makes this converter emit <msqrt> even when m:degHide is absent or explicitly m:val="0" (degree should be visible). In documents with an intentionally visible but currently empty degree (for example, an unfinished n-th root placeholder), this now collapses the structure to a square root and drops the index slot semantics; <mroot> should still be preserved whenever degree visibility is on.

Useful? React with 👍 / 👎.

@caio-pizzol caio-pizzol reopened this Apr 12, 2026
caio-pizzol and others added 3 commits April 12, 2026 08:19
- Dedupe duplicate `m:rad converter` describe block in omml-to-mathml.test.ts
- Add unit test for empty <m:deg/> with no <m:degHide> — Word's round-trip
  canonical form for "no explicit degree". Without the empty-deg check this
  case produced an invalid <mroot> with an empty index.
- Add unit tests for ST_OnOff "on" / "off" variants on m:degHide
  (ECMA-376 §22.9.2.7) — Word normalizes these on save but other DOCX
  producers (Google Docs, LibreOffice, Pages) may emit them.
- Treat m:degHide val="off" as not hidden — previously interpreted as true.
- Restore correct spec citation §22.1.2.88 for m:rad (the prior change to
  §22.1.2.86 was incorrect; §22.1.2.88 is the rad section in ECMA-376).
- Add math-radical-tests.docx behavior fixture (3 cases: canonical sqrt,
  cube root, empty-deg-no-degHide).
- Add behavior tests asserting <msqrt> / <mroot> shape per case and that
  no <mroot> ever has an empty index.
@caio-pizzol
Copy link
Copy Markdown
Contributor

Quick update on my last comment, @Anuj52 — actually walking that back. We tested your empty-<m:deg/> fix against real Word output and confirmed it's correct and important: Word adds an empty <m:deg/> when round-tripping a sqrt that lacked one, which made the current code emit a broken <mroot> with an empty index. Your one-liner fixes it.

I just pushed a commit to your branch with:

  • Behavior + unit tests for the empty-deg case (so it stays fixed)
  • Tests for ST_OnOff "on" / "off" (Word strips these but other producers emit them) + a one-line tweak to handle val="off"
  • A small fixture docx for the behavior tests
  • Reverted just the spec citation back to §22.1.2.88 — that's the actual m:rad section in ECMA-376
  • Deduped a copy-paste of the m:rad converter describe block

Sorry for the back-and-forth on closing this. Thanks for the fix!

Add unit tests for m:val="1" (canonical Word output) and m:val="true"
(ST_OnOff true alias) so degHide handling for the common true values is
locked in, complementing the existing "no val" / "on" / "off" / "0" cases.
Copy link
Copy Markdown
Contributor

@caio-pizzol caio-pizzol left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally. Thanks @Anuj52!

@caio-pizzol caio-pizzol enabled auto-merge April 12, 2026 16:34
@caio-pizzol caio-pizzol added this pull request to the merge queue Apr 12, 2026
Merged via the queue into superdoc-dev:main with commit 1011a37 Apr 12, 2026
50 checks passed
@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot bot commented Apr 12, 2026

🎉 This PR is included in vscode-ext v2.3.0-next.4

@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot bot commented Apr 12, 2026

🎉 This PR is included in esign v2.3.0-next.4

The release is available on GitHub release

@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot bot commented Apr 12, 2026

🎉 This PR is included in @superdoc-dev/react v1.2.0-next.1

The release is available on GitHub release

@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot bot commented Apr 12, 2026

🎉 This PR is included in template-builder v1.5.0-next.4

The release is available on GitHub release

@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot bot commented Apr 12, 2026

🎉 This PR is included in superdoc v1.26.0-next.4

The release is available on GitHub release

@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot bot commented Apr 12, 2026

🎉 This PR is included in superdoc-cli v0.7.0-next.4

The release is available on GitHub release

@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot bot commented Apr 12, 2026

🎉 This PR is included in superdoc-sdk v1.6.0-next.1

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Math: implement m:rad radical/sqrt converter (community)

3 participants