fix: normalize bookmarks in tables #1892
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: 1f6ac31816
ℹ️ 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/super-editor/src/core/super-converter/v2/importer/docxImporter.js
Outdated
Show resolved
Hide resolved
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
looks good overall! just a minor comment
packages/super-editor/src/core/super-converter/v2/importer/docxImporter.js
Outdated
Show resolved
Hide resolved
packages/super-editor/src/core/super-converter/v2/importer/docxImporter.test.js
Show resolved
Hide resolved
… sd-1681_block-id-crash
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@VladaHarbour just a few more comments around ECMA spec - worth double checking before merging
| }, []); | ||
|
|
||
| updatedRows.forEach((row, index) => { | ||
| if (rowStartInlines[index]?.length) { |
There was a problem hiding this comment.
So I checked the spec (§17.13.6.2) actually shows bookmarkEnd as a sibling of table cells, not inside them:
<w:tr>
<w:tc><w:p/></w:tc>
<w:bookmarkEnd w:id="0" /> <!-- sits here, outside the cell -->
</w:tr>
But we're putting it inside a paragraph. This is probably fine given ProseMirror's schema constraints - just worth double-checking and documenting that we're intentionally normalizing the structure.
There was a problem hiding this comment.
The issue here is that Prosemirror schema doesn't accept anything but tableCell inside tableRow. We can intentionally change this but the compatibility with table plugins may be broken
| * @returns {Array} | ||
| */ | ||
| export function normalizeTableBookmarksInContent(content = [], editor) { | ||
| if (!Array.isArray(content) || content.length === 0) return content; |
There was a problem hiding this comment.
Table bookmarks can have colFirst and colLast attributes that specify which columns they cover:
<w:bookMarkStart w:colFirst="0" w:colLast="1" w:id="0" w:name="table"/>
I didn't see these being preserved when we relocate the bookmarks. If someone has a column-range bookmark, we might lose that info. Worth checking if this matters for our use cases, or at least adding a comment about it.
There was a problem hiding this comment.
@caio-pizzol aded support for colFirst/colLast attributes when calculating the cell for normalisation
| return result; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
ECMA spec doesn't actually show bookmarks as valid direct children of <w:tbl>. So this whole normalization is handling documents that aren't strictly conformant.
Would be nice to add a comment explaining that - something like:
/**
* Some non-conformant DOCX producers place bookmarks as direct table children.
* Per ECMA-376 §17.13.6.2, they should be inside cells (bookmarkStart) or
* as children of rows (bookmarkEnd). We relocate them for PM compatibility.
*/
|
@caio-pizzol please let me know If I have to do something with the last spec review. https://ooxml.dev/ page 900 tells that colFirst/colLast are valid attributes for bookmarkStart |
No description provided.