fix(super-editor): import <w:temporary/> and lock appearance default contract (SD-3111)#3295
Merged
caio-pizzol merged 3 commits intoMay 14, 2026
Conversation
…contract (SD-3111)
Two narrow gaps, both about the import contract for content-control
properties:
1. <w:temporary/> was being dropped on import.
Word emits it (ECMA-376 §17.5.2.43), our exporter preserves it on
round-trip, the editor's node-attrs typedef carries it, the
sdt-info-builder maps it to ContentControlInfo.properties.temporary
— but handle-structured-content-node.js never read it from sdtPr.
So properties.temporary was always undefined, even for parts where
the source XML said true.
Fix: new extractTemporary helper applies Word's toggle conventions
(empty element = true; w:val "true"/"1" = true; "false"/"0" = false).
When the element is absent, the attr is NOT stamped — so consumers
can distinguish "absent from source" from "explicit false."
2. The appearance default contract was implicit.
ECMA-376 lets the source XML omit <w15:appearance>, and Word's
effective default is boundingBox. SuperDoc correctly returns
undefined when the source omits the element (resolveAppearance
validates against the three spec values). But the public type
JSDoc said nothing about this, so consumers building UI on top
of appearance could not tell whether undefined meant "data
missing" or "Word default applies."
Fix: JSDoc on ContentControlProperties.appearance now spells out
all four states explicitly, including 'undefined → treat as
boundingBox.' Same treatment for temporary's 'undefined → treat
as false.' No runtime behavior change — pure contract lock.
Tests:
- 7 new w:temporary parsing cases (empty toggle, w:val true/1/false/0,
absent twice, absent-doesn't-stamp).
- Existing info-builder tests already cover the attr-to-properties
mapping; they still pass.
Verified:
- super-editor v3/w/sdt suite: 134/134
- @superdoc/document-api: 1398/1398
Out of scope:
- The exporter side already round-trips <w:temporary/> (per ticket
description). Not modified.
- The appearance default behavior is unchanged — only the JSDoc that
documents it.
superdoc-bot
Bot
added
review: thorough
review: quick
and removed
review: thorough
labels
Contributor
|
The MCP tools haven't been permitted in this session, so I'll review against my knowledge of ECMA-376. Status: PASS The
The namespace prefix ( Tests cover all five CT_OnOff value forms plus the absent case — no spec violations found. See https://ooxml.dev/spec?q=temporary for reference. |
![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: 5218b52d82
ℹ️ 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".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The previous wording 'Word removes the SDT once the user fills it in' paraphrases user-visible behavior but isn't quite the spec's framing. Updated to 'When enabled, Word treats the content control as temporary and may remove the SDT wrapper after the user edits/fills the control.' No runtime change.
…ser (SD-3111) Codex bot review caught a real bug: my hand-rolled ST_OnOff check in extractTemporary only recognized '0' and 'false' as off tokens, but the project's shared ST_OnOff convention (token-sets.ts:27) also accepts 'off'. So <w:temporary w:val="off"/> incorrectly returned true, contradicting the source XML's explicit "not temporary". Replaced with parseStrictStOnOff from the same utils.js other ST_OnOff importers use. Gets: 'true'/'1'/'on' → true, 'false'/'0'/'off' → false, invalid tokens → undefined (with the import-diagnostic side effect for consistency with other ST_OnOff properties). Strict ECMA-376 §22.9.2.7 lists only 1|0|true|false, but the project deliberately accepts on/off for Word-in-the-wild robustness — same convention bold/italic/strike already follow. Three new tests: - <w:temporary w:val="on"/> → true - <w:temporary w:val="off"/> → false (the bug) - invalid token → undefined Verified: - 32/32 in handle-structured-content-node.test.js - 137/137 across the SDT handler suite
caio-pizzol
deleted the
caio-pizzol/SD-3111-import-temporary-and-document-appearance
branch
Contributor
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.94 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.138 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in vscode-ext v2.3.0-next.140 |
Contributor
|
🎉 This PR is included in superdoc-cli v0.8.0-next.109 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc-sdk v1.8.0-next.92 |
Contributor
|
🎉 This PR is included in superdoc v1.30.0-next.89 The release is available on GitHub release |
msviderok
pushed a commit
to msviderok/superdoc
that referenced
this pull request
May 16, 2026
…contract (SD-3111) (superdoc-dev#3295) * fix(super-editor): import <w:temporary/> and lock appearance default contract (SD-3111) Two narrow gaps, both about the import contract for content-control properties: 1. <w:temporary/> was being dropped on import. Word emits it (ECMA-376 §17.5.2.43), our exporter preserves it on round-trip, the editor's node-attrs typedef carries it, the sdt-info-builder maps it to ContentControlInfo.properties.temporary — but handle-structured-content-node.js never read it from sdtPr. So properties.temporary was always undefined, even for parts where the source XML said true. Fix: new extractTemporary helper applies Word's toggle conventions (empty element = true; w:val "true"/"1" = true; "false"/"0" = false). When the element is absent, the attr is NOT stamped — so consumers can distinguish "absent from source" from "explicit false." 2. The appearance default contract was implicit. ECMA-376 lets the source XML omit <w15:appearance>, and Word's effective default is boundingBox. SuperDoc correctly returns undefined when the source omits the element (resolveAppearance validates against the three spec values). But the public type JSDoc said nothing about this, so consumers building UI on top of appearance could not tell whether undefined meant "data missing" or "Word default applies." Fix: JSDoc on ContentControlProperties.appearance now spells out all four states explicitly, including 'undefined → treat as boundingBox.' Same treatment for temporary's 'undefined → treat as false.' No runtime behavior change — pure contract lock. Tests: - 7 new w:temporary parsing cases (empty toggle, w:val true/1/false/0, absent twice, absent-doesn't-stamp). - Existing info-builder tests already cover the attr-to-properties mapping; they still pass. Verified: - super-editor v3/w/sdt suite: 134/134 - @superdoc/document-api: 1398/1398 Out of scope: - The exporter side already round-trips <w:temporary/> (per ticket description). Not modified. - The appearance default behavior is unchanged — only the JSDoc that documents it. * wip(document-api): tighten temporary JSDoc wording per review (SD-3111) The previous wording 'Word removes the SDT once the user fills it in' paraphrases user-visible behavior but isn't quite the spec's framing. Updated to 'When enabled, Word treats the content control as temporary and may remove the SDT wrapper after the user edits/fills the control.' No runtime change. * fix(super-editor): route extractTemporary through shared ST_OnOff parser (SD-3111) Codex bot review caught a real bug: my hand-rolled ST_OnOff check in extractTemporary only recognized '0' and 'false' as off tokens, but the project's shared ST_OnOff convention (token-sets.ts:27) also accepts 'off'. So <w:temporary w:val="off"/> incorrectly returned true, contradicting the source XML's explicit "not temporary". Replaced with parseStrictStOnOff from the same utils.js other ST_OnOff importers use. Gets: 'true'/'1'/'on' → true, 'false'/'0'/'off' → false, invalid tokens → undefined (with the import-diagnostic side effect for consistency with other ST_OnOff properties). Strict ECMA-376 §22.9.2.7 lists only 1|0|true|false, but the project deliberately accepts on/off for Word-in-the-wild robustness — same convention bold/italic/strike already follow. Three new tests: - <w:temporary w:val="on"/> → true - <w:temporary w:val="off"/> → false (the bug) - invalid token → undefined Verified: - 32/32 in handle-structured-content-node.test.js - 137/137 across the SDT handler suite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two narrow gaps in the content-control import contract.
`<w:temporary/>` (ECMA-376 §17.5.2.43) was being dropped on import. The exporter preserves it, the editor's node-attrs typedef carries it, the info-builder maps it to `ContentControlInfo.properties.temporary` — but the SDT importer never read it from `sdtPr`. So `properties.temporary` was always undefined regardless of source XML. New `extractTemporary` applies Word's toggle conventions (empty element = true; `w:val` true/1 = true; false/0 = false). Absent in source = attr not stamped, so consumers can tell "absent" from "explicit false."
`appearance` already returned `undefined` for absent source XML, but the public type JSDoc said nothing about how to interpret that. Locked the contract: `undefined` means "treat as Word's effective default (boundingBox)" — the API does not fabricate the default. Same treatment for `temporary`'s effective default (false). No runtime behavior change — pure documentation/contract lock.
Verified: `pnpm --filter @superdoc/super-editor exec vitest run src/editors/v1/core/super-converter/v3/handlers/w/sdt` → 134/134; `pnpm --filter @superdoc/document-api test` → 1398/1398.
Review: confirm we don't want the API to silently fill in spec defaults for similar properties — keeping appearance/temporary as raw-imported is the call here, but if another reviewed surface returns computed defaults, this is the moment to bring that up.