[SD-1706] Fix bug in round-trip of w:pict elements #2000
Conversation
Returns a new Editor instance using an unpacked docx directory. Also eliminate duplicate definitions of zipFolderToBuffer().
Include test files in tsconfig to allow for auto-imports and type checking in test files. Also add src/tests/** to exclusion in tsconfig.build.json, so that generated types don't include test-only files
This directory contains files that should be tracked in git. If the concern is that users will be unzipping docx files into this directory and accidentally committing them, we should instead change unzip.sh to use a specific destination directory that's gitignored.
Import and export the entire document and perform assertions on the resulting XML, rather than simply testing handleImageImport()
This changes `pictNodeImporter` to operate directly on the `w:pict` element rather than on the containing `w:p`. As a result, it no longer wraps generated nodes in new paragraphs. Since `pictNodeImporter` now just calls the v3 translator directly, we've eliminated pictNodeImporter.test.js and created a new pict-translator.test.js. The translator also includes passthrough nodes for additional children of the <w:pict> element besides the translated <w:shape>. **NOTE:** The passthrough nodes are currently only handled in `handle-shape-image-watermark-import.js`; this covers the usage in the example document in the original ticket, but it should generally handle passing through any unhandled content. Additionally, the passthrough elements are appended to the end when exporting; ideally their position would be maintained through an import/export roundtrip. Also: improved typing in various places, and converted a few JS files to TS.
afn
force-pushed
the
afn/SD-1706-pict-roundtrip-bug
branch
from
79d76d4 to
ef2ac12
Compare
![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: 79d76d4a60
ℹ️ 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".
packages/super-editor/src/core/super-converter/v3/handlers/w/pict/pict-translator.js
Show resolved
Hide resolved
Rather than deep-ending on fixing all of the downstream typing issues discovered by typescriptifying various files in this PR, selectively fix a few issues and tag the remainder with @ts-expect-error, with FIXME messages describing the reasons for the errors.
| */ | ||
| backgroundColor?: string; | ||
|
|
||
| numbering?: SuperConverter['numbering']; |
There was a problem hiding this comment.
This property isn't used by the pm-adapter anymore, it was superseded by translatedNumbering. The translatedNumbering property contains all the numbering information but uses a v3 translator for it. I had to keep the old numbering in the SuperConverter class just not to break existing code on the ProseMirror side of things but the pm-adapter and the layout-engine in general don't depend on it anymore.
I can see that removing this property causes an error in this line but you can safely delete that line.
| numbering; | ||
|
|
||
| /** @type {import('./types').Numbering} */ | ||
| translatedNumbering; |
There was a problem hiding this comment.
numbering and translatedNumbering actually have different types. Only translatedNumbering uses wAbstractNumTranslator and wNumTranslator.
numbering actually contains the raw XML elements, like this:
{
"abstracts": {
"0": {
"type": "element",
"name": "w:abstractNum",
"attributes": {
"w:abstractNumId": "0",
"w15:restartNumberingAfterBreak": "0"
},
"elements": [
{
"type": "element",
"name": "w:nsid",
"attributes": {
"w:val": "16126B07"
}
},
{
"type": "element",
"name": "w:multiLevelType",
"attributes": {
"w:val": "hybridMultilevel"
}
},
{
"type": "element",
"name": "w:tmpl",
"attributes": {
"w:val": "51EC4E08"
}
},
{
"type": "element",
"name": "w:lvl",
"attributes": {
"w:ilvl": "0",
"w:tplc": "04090001"
},
"elements": [
{
"type": "element",
"name": "w:start",
"attributes": {
"w:val": "1"
}
},
...
]
},
]
}
},
"definitions": {
"1": {
"type": "element",
"name": "w:num",
"attributes": {
"w:numId": "1",
"w16cid:durableId": "100147962"
},
"elements": [
{
"type": "element",
"name": "w:abstractNumId",
"attributes": {
"w:val": "0"
}
}
]
},
}
}2 participants
This changes
pictNodeImporterto operate directly on thew:pictelement rather than on the containingw:p. As a result, it no longer wraps generated nodes in new paragraphs.Since
pictNodeImporternow just calls the v3 translator directly, we've eliminated pictNodeImporter.test.js and created a new pict-translator.test.js.The translator also includes passthrough nodes for additional children of the
<w:pict>element besides the translated<w:shape>.NOTE: The passthrough nodes are currently only handled in
handle-shape-image-watermark-import.js; this covers the usage in the example document in the original ticket, but it should generally handle passing through any unhandled content. Additionally, the passthrough elements are appended to the end when exporting; ideally their position would be maintained through an import/export roundtrip.Also: improved typing in various places, and converted a few JS files to TS.