Conversation
VladaHarbour
requested review from
afn,
caio-pizzol,
chittolinag,
harbournick and
luccas-harbour
![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: d55da94c54
ℹ️ 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".
| if (tablesForPara) { | ||
| const state = paginator.ensurePage(); |
There was a problem hiding this comment.
With getTableAnchorBaseY returning page-top/margin-top anchors, registering tables only in this post-paragraph block means earlier text on the same page has already been laid out without the table exclusion zone. If a table uses anchor.vRelativeFrom = 'page' or 'margin' and its anchor paragraph appears later on the page, the table fragment can be placed near the top and overlap previously laid-out content, because wrapping was applied too late.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed it since it require more position calculations. Now the default case will be paragraph anchored tables
| const bottom = anchorY + (tableMeasure.totalHeight ?? 0); | ||
| if (bottom > tableBottomY) tableBottomY = bottom; | ||
| } | ||
| state.cursorY = tableBottomY; |
There was a problem hiding this comment.
This unconditional cursor update makes anchored tables consume document flow even when they are non-wrapping (wrap.type: 'None' or missing wrap), but registerTable explicitly treats that mode as absolute positioning with no exclusion zone. For such inputs, pushing state.cursorY to the table bottom adds unintended whitespace and can change pagination compared to expected absolute overlay behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
+1
state.cursorY = tableBottomY runs regardless of wrap.type. for wrap.type='None' (absolute-positioned), Word doesn't advance the flow cursor.
should this be gated on wrap type?
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@VladaHarbour this is great!
One thing to note here: since this touches rendering of documents, I think it's worth uploading a sample file or the file that caused the issue to our visual testing suite
(let me know if you want to sync on how to do that)
| for (const { block: tableBlock, measure: tableMeasure } of tablesForPara) { | ||
| if (placedAnchoredTableIds.has(tableBlock.id)) continue; | ||
| const totalWidth = tableMeasure.totalWidth ?? 0; | ||
| if (columnWidthForTable > 0 && totalWidth >= columnWidthForTable * 0.99) continue; |
There was a problem hiding this comment.
0.99 threshold is in two files — extract to a shared constant so they stay in sync.
| if (block.anchor?.isAnchored) { | ||
| return; | ||
| const totalWidth = measure.totalWidth ?? 0; | ||
| treatAsInline = columnWidth > 0 && totalWidth >= columnWidth * 0.99; |
| const bottom = anchorY + (tableMeasure.totalHeight ?? 0); | ||
| if (bottom > tableBottomY) tableBottomY = bottom; | ||
| } | ||
| state.cursorY = tableBottomY; |
There was a problem hiding this comment.
+1
state.cursorY = tableBottomY runs regardless of wrap.type. for wrap.type='None' (absolute-positioned), Word doesn't advance the flow cursor.
should this be gated on wrap type?
| * Compute the base Y coordinate for an anchored table based on vRelativeFrom. | ||
| * Ignores tblpYSpec (alignV) by design. | ||
| */ | ||
| function getTableAnchorBaseY(block: TableBlock, state: PageState): number { |
There was a problem hiding this comment.
state.cursorY here is after layoutParagraphBlock, so vRelativeFrom='paragraph' anchors to paragraph bottom, not top. OOXML says paragraph start.
is this intentional to prevent overlap?
There was a problem hiding this comment.
I did it intentionally in order to be sure that table doesn't overlap text. Most likely Word anchors to paragraph top but has some offset calculation to be sure that table goes after the text.
| const tableProps = block.attrs?.tableProperties as Record<string, unknown> | undefined; | ||
| const floatingProps = tableProps?.floatingTableProperties as Record<string, unknown> | undefined; | ||
| if (floatingProps && Object.keys(floatingProps).length > 0) { | ||
| if (floatingProps && Object.keys(floatingProps).length > 0 && !treatAsInline) { |
There was a problem hiding this comment.
full-width anchored tables also skip layoutMonolithicTable. if the table has floatingTableProperties but is full-width, it falls through to paginated table layout.
is that the right path for a floating table?
4 participants
Tables that have w:tblpPr devined are treated as anchor tables and can overlap the text
Before:
After: