Skip to content

Fix Bug 66263 — Add support for reading SDT row in tables #971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hostedbygnome wants to merge 7 commits into
base: trunk
Choose a base branch
from
Open

Fix Bug 66263 — Add support for reading SDT row in tables #971

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
85f4837
Fix Bug 66263 — Add support for SDT row in tables
Dec 16, 2025
e0f3da1
remove repeated if
Dec 16, 2025
5c75ebe
add missing imports
Dec 16, 2025
9ba7239
add tests
Apr 6, 2026
34f3fa6
add tests
Apr 6, 2026
fe8e05f
add tests with file + fix paragraph duplication
Apr 6, 2026
d992752
fix indentation
Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFParagraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,26 @@ private void buildRunsInOrderFromXml(XmlObject object) {
if (o instanceof CTSdtBlock) {
XWPFSDT cc = new XWPFSDT((CTSdtBlock) o, part);
iruns.add(cc);
CTSdtContentBlock content = ((CTSdtBlock)o).getSdtContent();
if (content != null) {
for (CTP ctp : content.getPList()) {
processCTRs(ctp.getRList());
}
}
}
if (o instanceof CTSdtRun) {
XWPFSDT cc = new XWPFSDT((CTSdtRun) o, part);
iruns.add(cc);
if (o instanceof CTSdtRun) {
Comment thread
pjfanning marked this conversation as resolved.
Outdated
XWPFSDT cc = new XWPFSDT((CTSdtRun)o, part);
Comment thread
pjfanning marked this conversation as resolved.
Outdated
iruns.add(cc);

CTSdtContentRun sdtContent = ((CTSdtRun)o).getSdtContent();
if (sdtContent != null)
{
processCTRs(sdtContent.getRList());
}

processSdtRuns();
}
}
if (o instanceof CTRunTrackChange) {
final CTRunTrackChange parentRecord = (CTRunTrackChange) o;
Expand All @@ -164,6 +180,33 @@ private void buildRunsInOrderFromXml(XmlObject object) {
}
}

private void processCTRs(List<CTR> ctrs) {
if (ctrs == null) {
return;
}
for (CTR ctr : ctrs) {
if (ctr.getRPr() != null) {
Comment on lines +179 to +184
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

processCTRs adds XWPFRun objects for CTRs inside SDT content into the paragraph’s runs list. Many editing APIs (insertNewRun, insertNewHyperlinkRun, etc.) assume every entry in runs corresponds to a CTR whose cursor is in the paragraph (or hyperlink/field) container; for CTRs nested under CTSdtContentRun, insertNewProvidedRun will fail the isCursorInParagraph check and return null for positions that point at these SDT-derived runs. Either avoid adding nested SDT CTRs into runs (keep SDT content accessible via XWPFSDT/iruns), or update the insertion/removal logic and cursor positioning to support runs whose parent is SDT content so that the runs list remains a valid edit surface.

Suggested change
private void processCTRs(List<CTR> ctrs) {
if (ctrs == null) {
return;
}
for (CTR ctr : ctrs) {
if (ctr.getRPr() != null) {
private boolean isRunInParagraphEditContainer(CTR ctr) {
try (XmlCursor cursor = ctr.newCursor()) {
if (!cursor.toParent()) {
return false;
}
XmlObject parent = cursor.getObject();
return parent instanceof CTP
|| parent instanceof CTHyperlink
|| parent instanceof CTSimpleField;
}
}
private void processCTRs(List<CTR> ctrs) {
if (ctrs == null) {
return;
}
for (CTR ctr : ctrs) {
if (ctr.getRPr() != null && isRunInParagraphEditContainer(ctr)) {

Copilot uses AI. Check for mistakes.
runs.add(new XWPFRun(ctr, (IRunBody)this));
}
}
}

private void processSdtRuns() {
try (XmlCursor cursor = getCTP().newCursor()) {
cursor.selectPath("child::*");

while (cursor.toNextSelection()) {
XmlObject xmlObject = cursor.getObject();
if (xmlObject instanceof CTSdtRun) {
CTSdtContentRun content = ((CTSdtRun)xmlObject).getSdtContent();
if (content != null) {
processCTRs(content.getRList());
}
}
}
}
}

@Internal
public CTP getCTP() {
return paragraph;
Expand Down
63 changes: 50 additions & 13 deletions poi-ooxml/src/main/java/org/apache/poi/xwpf/usermodel/XWPFTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ Licensed to the Apache Software Foundation (ASF) under one or more
import org.apache.poi.ooxml.util.POIXMLUnits;
import org.apache.poi.util.Internal;
import org.apache.poi.util.Units;
import org.apache.xmlbeans.XmlCursor;
import org.apache.xmlbeans.XmlObject;
import org.openxmlformats.schemas.wordprocessingml.x2006.main.CTBorder;
import org.openxmlformats.schemas.wordprocessingml.x2006.main.CTDecimalNumber;
import org.openxmlformats.schemas.wordprocessingml.x2006.main.CTJcTable;
Expand Down Expand Up @@ -168,24 +170,59 @@ public XWPFTable(CTTbl table, IBody part, boolean initRow) {
createEmptyTable(table);
}
Comment on lines 170 to 173
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

The "empty table" check only looks at sizeOfTrArray() == 0. A table that contains only w:sdt row wrappers has no top-level w:tr, but it isn’t actually empty; createEmptyTable(table) will inject an extra blank row when using the default constructor (initRow=true). Consider extending the condition to also check for the presence of SDT rows (e.g., sizeOfSdtArray()==0/getSdtList().isEmpty()), or otherwise detect whether the table already contains any row-like content before creating a default row.

Copilot uses AI. Check for mistakes.

for (CTRow row : table.getTrList()) {
StringBuilder rowText = new StringBuilder();
XWPFTableRow tabRow = new XWPFTableRow(row, this);
tableRows.add(tabRow);
for (CTTc cell : row.getTcList()) {
for (CTP ctp : cell.getPList()) {
XWPFParagraph p = new XWPFParagraph(ctp, part);
if (rowText.length() > 0) {
rowText.append('\t');
try (XmlCursor cursor = table.newCursor()) {
cursor.selectPath("./*");
while (cursor.toNextSelection()) {
XmlObject xmlObject = cursor.getObject();
if (xmlObject instanceof CTRow) {
processCTRow((CTRow)xmlObject);
}
else if (xmlObject instanceof CTSdtRow) {
List<CTRow> rows = new ArrayList<>();
collectCTRowsInnerSdtRow((CTSdtRow)xmlObject, rows);
for (CTRow row : rows)
{
processCTRow(row);
}
rowText.append(p.getText());
}
}
Comment on lines +175 to 190
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

The constructor now adds CTRow instances extracted from CTSdtRow into tableRows. Many mutation/access methods (e.g., removeRow, insertNewTableRow, addRow, getRow(int), getNumberOfRows) assume tableRows indexes map 1:1 to ctTbl’s top-level w:tr array. With SDT-derived rows included, removeRow(pos) will remove the wrong underlying w:tr (or throw) and getRow(int)/getNumberOfRows() become inconsistent with getRows(). Consider either (a) keeping tableRows aligned to top-level w:tr only and handling SDT rows separately for text extraction, or (b) refactoring row operations to remove/insert via the actual row element’s cursor (or SDT wrapper) rather than ctTbl.removeTr(pos) and updating getRow/getNumberOfRows semantics accordingly.

Copilot uses AI. Check for mistakes.
if (rowText.length() > 0) {
this.text.append(rowText);
this.text.append('\n');
}
}

private void processCTRow(CTRow row) {
StringBuilder rowText = new StringBuilder();
XWPFTableRow tableRow = new XWPFTableRow(row, this);
tableRows.add(tableRow);
for (CTTc cell : row.getTcList()) {
for (CTP ctp : cell.getPList()) {
XWPFParagraph p = new XWPFParagraph(ctp, part);
if (rowText.length() > 0) {
rowText.append('\t');
}
rowText.append(p.getText());
}
}
if (rowText.length() > 0) {
this.text.append(rowText);
this.text.append('\n');
}
}

private void collectCTRowsInnerSdtRow(CTSdtRow sdtRow, List<CTRow> rows) {
CTSdtContentRow sdtContent = sdtRow.getSdtContent();
if (sdtContent == null) {
return;
}

List<CTRow> rowsInnerSdtContent = sdtContent.getTrList();
if (!rowsInnerSdtContent.isEmpty()) {
rows.addAll(rowsInnerSdtContent);
return;
}

for (CTSdtRow innerSdt : sdtContent.getSdtList()) {
collectCTRowsInnerSdtRow(innerSdt, rows);
Comment on lines +219 to +226
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

collectCTRowsInnerSdtRow returns immediately when sdtContent.getTrList() is non-empty, which skips any nested w:sdt elements that may appear alongside w:tr within the same CTSdtContentRow. This will miss rows in mixed-content SDTs and also loses document order between tr and nested sdt. Iterate through the SDT content children in order (e.g., via an XmlCursor over sdtContent) and collect both CTRow and nested CTSdtRow recursively without the early return.

Suggested change
List<CTRow> rowsInnerSdtContent = sdtContent.getTrList();
if (!rowsInnerSdtContent.isEmpty()) {
rows.addAll(rowsInnerSdtContent);
return;
}
for (CTSdtRow innerSdt : sdtContent.getSdtList()) {
collectCTRowsInnerSdtRow(innerSdt, rows);
XmlCursor cursor = sdtContent.newCursor();
try {
if (!cursor.toFirstChild()) {
return;
}
do {
XmlObject child = cursor.getObject();
if (child instanceof CTRow) {
rows.add((CTRow) child);
} else if (child instanceof CTSdtRow) {
collectCTRowsInnerSdtRow((CTSdtRow) child, rows);
}
} while (cursor.toNextSibling());
} finally {
cursor.dispose();

Copilot uses AI. Check for mistakes.
}
}

private void createEmptyTable(CTTbl table) {
Expand Down